It seems that the API can be used to entirely circumvent the Lockdown extension. As it is enabled by default, this renders the entire Lockdown extension useless.
https://example.com/wiki/api.php?format=txtfm&action=parse&page=SecretNamespace:Page
Description
Description
Details
Details
Customize query in gerrit
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | • demon | T133070 MediaWiki 1.27.1 security release | |||
| Resolved | Bawolff | T115333 API action=parse does not check-per title read permissions |
Event Timeline
Comment Actions
Another bug: https://github.com/wikimedia/mediawiki-extensions-Lockdown/blob/master/Lockdown.php#L247
If you've filtered out all namespaces, just search in all searchable namespaces!
Comment Actions
$wgEnableAPI exists, so I guess yes. I don't know how much would break if someone did do so, though.
Comment Actions
Apparently it breaks things like the visual editor, so that's really not a satisfying way of fixing this issue.
Comment Actions
Yes, VE simply could not function without the API, along with probably a few core features... We should probably pull support for disabling the API out of core.
Comment Actions
(That was moved to T115414.) I set $wgEnableAPI = false on some Lockdown/SimpleSecurity wiki but that's... suboptimal. The Lockdown code should probably be patched so that it disables the API on its own, until it can handle the API.
Comment Actions
This security bug has been open > 3 months, and it appears that nobody intends to work on the issue. Unless someone begins working on this in the next 2 weeks, I am going to mark the extension with {{Security Alert}} and make this bug public.
Comment Actions
@Bawolff this is the first time I even hear of this bug.
As the original author of Lockdown, I'll give this issue a poke - even though I haven't looked at the code in years.
A thought before investigating: if this is true, something must be very wrong. Lockdown hooks into the generic mechanism for checking permissions. If the API doesn't use that mechanism for checking permissions, that's not good... But then, I haven't yet looked at what is actually happening.
Comment Actions
Ugh, wow. Confirmed. Extension:Lockdown isn't the problem here. ApiParse doesn't check userCan('read') at all on the title provided for the page parameter.
Comment Actions
commit 674be21e062292f8b16cdda7c15b290741eeb8f3
Author: daniel <daniel.kinzler@wikimedia.de>
Date: Sun Jun 12 21:22:17 2016 +0200
Check read permission when loading page content in ApiParse.
Bug: T115333
Change-Id: I4bb9d3a5cb9b18ae5697db472d4a64e069956c84
diff --git a/includes/api/ApiParse.php b/includes/api/ApiParse.php
index fe418e3..04e08f8 100644
--- a/includes/api/ApiParse.php
+++ b/includes/api/ApiParse.php
@@ -36,6 +36,12 @@ class ApiParse extends ApiBase {
/** @var Content $pstContent */
private $pstContent = null;
+ private function checkReadPermissions( Title $title ) {
+ if ( !$title->userCan( 'read', $this->getUser() ) ) {
+ $this->dieUsage( "You don't have permission to view this page", 'permissiondenied' );
+ }
+ }
+
public function execute() {
// The data is hot but user-dependent, like page views, so we set vary cookies
$this->getMain()->setCacheMode( 'anon-public-user-private' );
@@ -102,6 +108,8 @@ class ApiParse extends ApiBase {
if ( !$rev ) {
$this->dieUsage( "There is no revision ID $oldid", 'missingrev' );
}
+
+ $this->checkReadPermissions( $rev->getTitle() );
if ( !$rev->userCan( Revision::DELETED_TEXT, $this->getUser() ) ) {
$this->dieUsage( "You don't have permission to view deleted revisions", 'permissiondenied' );
}
@@ -161,6 +169,8 @@ class ApiParse extends ApiBase {
if ( !$titleObj || !$titleObj->exists() ) {
$this->dieUsage( "The page you specified doesn't exist", 'missingtitle' );
}
+
+ $this->checkReadPermissions( $titleObj );
$wgTitle = $titleObj;
if ( isset( $prop['revid'] ) ) {Comment Actions
So big whoops about you not being CC'd on this. I just wanted to note, my original comment about "This security bug has been open > 3 months..." was something I was spamming on most long open secret security bugs about things not maintained by WMF (On the principle that us keeping secret bugs about unmaintained extensions is not exactly responsible behaviour if nobody is fixing the extensions, at some point we have to simply mark the extension as insecure. There are some bugs about unmaintained extensions that have been open for many years). However clearly that message was inappropriate for this bug, since you couldn't possibly respond to something you didn't know about. So my apologies about that.
On top of that, this turned out to really be a core mediawiki bug too, so something the security team should be taking responsibility for.
Comment Actions
Re Daniel's patch.
I think for extra caution, we should maybe check if the user can read the page given before resolving redirects, lest we leak some info about the redirect if the user is not supposed to be able to read it.
Modified version of Daniel's patch that does that:
(Other than the checking the pre-redirect title, I give his patch a +1).
Comment Actions
(I thought Daniel had been notified with T115333#1721791 since he's a member of MediaWiki-extensions-Lockdown; thanks for looking into this.)
Comment Actions
This should probably be included in the next core security release.
It seems people need to be individually subscribed to an access restricted ticket.
Comment Actions
The project list doesn't affect visibility policies. Most security tickets have a line allowing all subscribers to view.
Comment Actions
@Krenair yes, but apparently, adding a project in CC does not add all the project members in CC, if they are not already allowed to view the ticket. At least, that's my understanding of why I didn't learn about this until yesterday.
Comment Actions
Oh, a project subscriber, right. That might be restricted to watchers instead of members? Not sure.
Comment Actions
I note that MediaWiki inconsistently applies title-based read permission checks in general, which is why we have https://www.mediawiki.org/wiki/Security_issues_with_authorization_extensions. For example, using the 'userCan' hook to block access to "Test" on my local wiki, I was still able to view the content of the page by editing a different page to contain "{{:Test}}". Links tables leak information all over the place too, e.g. a category page lists titles of pages you don't have permission to view, Special:WhatLinksHere tells you restricted pages that link to the target, and so on.
This patch seems sane, but attaching it instead of pasting it in a comment would have made things a bit nicer.
This seems a bit unnecessary at this point, considering that it gives the same information about the redirect that you might get by accessing Special:ListRedirects, the API action=query&titles=Foo&redirects=, etc.
Comment Actions
That's why I introduced $wgNonincludableNamespaces when I wrote Lockdown.
Links tables leak information all over the place too, e.g. a category page lists titles of pages you don't have permission to view
Special:WhatLinksHere tells you restricted pages that link to the target, and so on.
I don't think we have a right that governs the visibility of page existence and titles. I consider that a missing bug, rather than a flaw in the enforcement of the read permission. Link tables leaking info is a known issue, yes - but not nearly as bad as the full page text becoming visible.
This patch seems sane, but attaching it instead of pasting it in a comment would have made things a bit nicer.
Excuse the dumb question, but... how?
This seems a bit unnecessary at this point, considering that it gives the same information about the redirect that you might get by accessing Special:ListRedirects, the API action=query&titles=Foo&redirects=, etc.
Yea... we should decide whether we want to try and avoid leaking info via the links tables, and whether we want to try to hide titles. Currently, neither is attempted or supported by Lockdown (or core).
Comment Actions
Although that's not really the same thing.
This patch seems sane, but attaching it instead of pasting it in a comment would have made things a bit nicer.
Excuse the dumb question, but... how?
There's a button on the toolbar above the comment box, with a tooltip "Upload File", that was recently fixed to actually function. This sets appropriate privacy permissions on the newly-uploaded file.
I believe you can also still drag-and-drop the file into the comment box, but I've never used this method. This should also set appropriate privacy permissions.
Or you can go to https://phabricator.wikimedia.org/file/upload/ to upload it manually. In that case be sure to change the "Visible To" box to "No One" for security patches, since this method defaults to public visibility.
In all cases, when you "transclude" the file into the task it magically allows everyone who can view the task to also view the file.
Comment Actions
There's a button on the toolbar above the comment box, with a tooltip "Upload File", that was recently fixed to actually function. This sets appropriate privacy permissions on the newly-uploaded file.
Ah, I was not aware that this could be used for private uploads. I thought they were always public (at least if you guessed the URL). Good to know, thanks!
In all cases, when you "transclude" the file into the task it magically allows everyone who can view the task to also view the file.
That's basically the same issue we have with {{:Test}}, right?... But here's it's supposedly a feature :)
Comment Actions
dapatrick !log Deployed patch for T115333 to wmf.10
[Marking resolved. We generally mark things resolved when they are deployed to WMF. This bug will also be fixed in 1.27.1 whenever that happens]
Comment Actions
Thanks. Looking forward to reenabling the API on wiki.wikimedia.it once 1.27.1 comes out. :)
Comment Actions
Change 306086 merged by jenkins-bot:
SECURITY: Check read permission when loading page content in ApiParse.
Comment Actions
Change 306114 merged by jenkins-bot:
SECURITY: Check read permission when loading page content in ApiParse.
Comment Actions
Change 306105 merged by jenkins-bot:
SECURITY: Check read permission when loading page content in ApiParse.
Comment Actions
Change 306124 had a related patch set uploaded (by Ejegg):
SECURITY: Check read permission when loading page content in ApiParse.
Comment Actions
Change 306095 merged by jenkins-bot:
SECURITY: Check read permission when loading page content in ApiParse.
Comment Actions
Change 306124 merged by jenkins-bot:
SECURITY: Check read permission when loading page content in ApiParse.
Comment Actions
Is this bug really resolved?
I have tried with master/REL1_27 version of lockdown with master/1,28/1/27 version ApiParse.php under MW1.27.1.
Neither allow API edit in any combined case out of 6 possible combination.