API action=parse does not check-per title read permissions
Closed, ResolvedPublic

Description

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

Maniphest changed the visibility from "Public (No Login Required)" to "Custom Policy".Oct 13 2015, 3:13 PM
Maniphest changed the edit policy from "All Users" to "Custom Policy".
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 13 2015, 3:13 PM
Tobias changed the title from "" to "API leaks private information".Oct 13 2015, 3:13 PM
Tobias edited the task description. (Show Details)
Tobias triaged this task as "High" priority.
Tobias changed Security from None to Software security bug.
Tobias edited subscribers, added: Tobias; removed: Aklapper.
Bawolff changed the title from "API leaks private information" to "[E:Lockdown] API leaks private information".Oct 13 2015, 3:18 PM
MaxSem added a subscriber: MaxSem.Oct 13 2015, 5:18 PM

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!

"enabled by default"? Do we really still allow the API to be disabled..?

Anomie added a subscriber: Anomie.Oct 13 2015, 6:13 PM

$wgEnableAPI exists, so I guess yes. I don't know how much would break if someone did do so, though.

Apparently it breaks things like the visual editor, so that's really not a satisfying way of fixing this issue.

Krenair added a comment.EditedOct 13 2015, 8:26 PM

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.

(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.

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.

daniel added a comment.EditedJun 12 2016, 6:51 PM

@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.

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.

Patch follows in next comment

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'] ) ) {

Adding API tag, since this is a bug in ApiParse, not Lockdown.

daniel removed JanZerebecki as the assignee of this task.
daniel added a subscriber: JanZerebecki.

@JanZerebecki oops, meant to CC you, not assign you

See patch in comment above.

@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.

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.

Bawolff changed the title from "[E:Lockdown] API leaks private information" to "API does not check per title read permissions properly".Jun 13 2016, 7:40 AM
Bawolff added a comment.EditedJun 13 2016, 8:08 AM

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).

(I thought Daniel had been notified with T115333#1721791 since he's a member of MediaWiki-extensions-Lockdown; thanks for looking into this.)

This should probably be included in the next core security release.
It seems people need to be individually subscribed to an access restricted ticket.

The project list doesn't affect visibility policies. Most security tickets have a line allowing all subscribers to view.

@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.

Oh, a project subscriber, right. That might be restricted to watchers instead of members? Not sure.

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.

commit 674be21e062292f8b16cdda7c15b290741eeb8f3

This patch seems sane, but attaching it instead of pasting it in a comment would have made things a bit nicer.

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.

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.

Anomie moved this task from Unsorted to Needs Review on the MediaWiki-API board.Jun 13 2016, 2:15 PM

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}}".

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?

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.

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).

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}}".

That's why I introduced $wgNonincludableNamespaces when I wrote Lockdown.

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.

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 :)

matmarex changed the title from "API does not check per title read permissions properly" to "API action=parse does not check-per title read permissions".Jul 18 2016, 10:06 PM
Bawolff closed this task as "Resolved".
Bawolff claimed this task.

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]

Thanks. Looking forward to reenabling the API on wiki.wikimedia.it once 1.27.1 comes out. :)

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 23 2016, 1:22 AM
demon changed the edit policy from "Custom Policy" to "All Users".
demon changed Security from Software security bug to None.

Change 306086 merged by jenkins-bot:
SECURITY: Check read permission when loading page content in ApiParse.

https://gerrit.wikimedia.org/r/306086

Change 306114 merged by jenkins-bot:
SECURITY: Check read permission when loading page content in ApiParse.

https://gerrit.wikimedia.org/r/306114

Change 306105 merged by jenkins-bot:
SECURITY: Check read permission when loading page content in ApiParse.

https://gerrit.wikimedia.org/r/306105

Change 306124 had a related patch set uploaded (by Ejegg):
SECURITY: Check read permission when loading page content in ApiParse.

https://gerrit.wikimedia.org/r/306124

Change 306095 merged by jenkins-bot:
SECURITY: Check read permission when loading page content in ApiParse.

https://gerrit.wikimedia.org/r/306095

Change 306124 merged by jenkins-bot:
SECURITY: Check read permission when loading page content in ApiParse.

https://gerrit.wikimedia.org/r/306124

Thanks for the release and the report update!

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.