Page MenuHomePhabricator

Special:ImportFile does not check permissions from own config FileImporterRequiredRight
Closed, ResolvedPublic2 Estimated Story PointsSecurity


The FileImporter extension provides a Special:ImportFile and allows to set the needed rights for the special page with config FileImporterRequiredRight

Setting $wgFileImporterRequiredRight = 'editinterface'; in LocalSettings.php does not shows a permission error on the special page when logged in as non-sysop.
Without $wgFileImporterShowInputScreen = true; is does not give a input form, but it handles actions given by request params.

The function SpecialPage::userCanExecute is not executed on the special page. The SpecialPage::__constructor mention to call it itself when override execute, which is not happen here.
The permission is only taken into account on Special:SpecialPages and the page is not listed there.
It needs a call to SpecialPage::checkPermissions to handle the user right from the option.

I have not tested if one of the action of that special page could be executed without the necessary rights, I have only seen that the permission error is missing when first open the page.

The special page correctly checks if the upload is enabled on the wiki and the default permission is upload. In the default config there is no security issue, because UploadBase::isAllowed already checks for the upload right. Only when using customized the needed rights it is executed.

Maybe there is no need for such a option and the option needs to be removed without further mention of the security issue.

wmf wikis does not change or use the config.

Event Timeline

This appears to be a legitimate issue, if I'm understanding the permissions model correctly. It was probably never noticed on the projects since $wgFileImporterRequiredRight defaults to upload and every non-anon user has that right. I just tested with a brand new account on commons and when I navigated to:

I didn't see the form, but when I navigated to:

I could view the import form and was able to import another example to commons (e.g. this image). This all makes sense since every non-anon has edit and upload. The relevant code for UploadBase::isAllowed hasn't changed in a long time, so I suppose SpecialImportFile::executeStandardChecks should be checking $wgFileImporterRequiredRight around here for this to be properly implemented. Maybe something like:

$permissionRequired = UploadBase::isAllowed( $user ) &&
    $user->isAllowed( $this->config->get( 'FileImporterRequiredRight' ) );
thiemowmde set the point value for this task to 2.
thiemowmde moved this task from Backlog to Tickets in sprint on the Move-Files-To-Commons board.
thiemowmde added subscribers: Lena_WMDE, WMDE-Fisch.

As written at this is not relevant on the Wikimedia cluster. The $wgFileImporterRequiredRight config is set to "upload" by default, but the upload right is checked anyway, independent from that config. The config was introduced for 2 reasons:

  • To be able to quickly change it in case there is a legitimate reason to limit the user group allowed to use the special page.
  • For 3rd party installations.

While this is a legitimate bug, it does not allow to bypass the upload restriction. Therefor I think this does not need to be market as a restricted security issue.

While this is a legitimate bug, it does not allow to bypass the upload restriction. Therefor I think this does not need to be market as a restricted security issue.

That's a fair assessment, I'll make this task public now.

sbassett triaged this task as Medium priority.Apr 30 2021, 2:25 PM
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".

Change 683860 merged by jenkins-bot:

[mediawiki/extensions/FileImporter@master] Fix incomplete check for $wgFileImporterRequiredRight