Page MenuHomePhabricator

Parsoid REST API may not check access permissions of user
Open, HighPublic

Description

We were diagnosing an issue on officewiki and realized that the parsoid CLI interface can access content on officewiki, despite it being private. This isn't unexpected; the CLI doesn't have any permissions checks (as far as I know).

But I went looking for permission checks in the REST interface (ParsoidHandler) and couldn't immediately find anything which seemed to implement permission checking. In contract, ApiVisualEditor::execute in VisualEditor has pages and pages of permission checks sitting ahead of the parsoid request.

If the checks are missing, it isn't generally user-exploitable, since our network partition is (deliberately) such that Parsoid's REST API is not externally accessible. RESTBase sits in front of Parsoid -- so any protections in RESTBase cover Parsoid.

However: 1) wikitech exposes the Parsoid API for that wiki, and 2) the 1.35 LTS release has a zero-conf setup for VisualEditor which exposes the Parsoid API on third-party wikis.

It seems that we could use a speedy security review here to ensure that the Parsoid REST API doesn't:

  1. For private wikis (login-required), doesn't expose page content w/o proper authentication
  2. For wikis in general, doesn't expose content which should be subject to per-user restrictions (is that a thing?)
  3. For wikis in general, doesn't allow edits which should be subject to per-user restrictions

This could be a false alarm; I don't have enough familiarity with the REST API infrastructure to know where permission checks "ought" to be done, so they could be sitting there (or part of the infrastructure) and I'm just not seeing them. But I'd appreciate a second look.

Event Timeline

cscott created this task.Aug 11 2020, 10:39 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 11 2020, 10:39 PM

I tested on 1.35, the REST API checks if the wiki is private, and denies access in that case.

user@bullseye:~$ curl -I "http://localhost/mediawiki/rest.php/localhost/v3/page/html/Main_Page/1?redirect=false&stash=true"
HTTP/1.1 403 Forbidden
Date: Tue, 11 Aug 2020 23:29:06 GMT
Server: Apache/2.4.43 (Debian)
X-Content-Type-Options: nosniff
X-Request-Id: 953e3769c76eb3d75366505d
Content-Type: application/json
user@bullseye:~$ curl "http://localhost/mediawiki/rest.php/localhost/v3/page/html/Main_Page/1?redirect=false&stash=true" | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100    68    0    68    0     0   5230      0 --:--:-- --:--:-- --:--:--  5230
{
  "error": "rest-read-denied",
  "httpCode": 403,
  "httpReason": "Forbidden"
}

So seemingly no security problem. But, we're not forwarding cookies on private wikis, so zeroconf VE/Parsoid doesn't work.

I don't believe that Parsoid exports a 'save page' interface, so that access control is (still) being done by the Action API 'edit' action. So case 3 above shouldn't be an issue.

And I don't know that case 2 actually occurs (there was some user-specific permissions regarding deleted revisions, etc, but as I recall Parsoid solved this by removing that information from Parsoid output for all users).

So I *think* that we just need to double-check support for private wikis, where most pages are only viewable with a login.

@Legoktm Do you know where that permission check is being done? git grep 403 doesn't turn up anything in the Parsoid repo, so I assume this is being down by the REST infrastructure. I'd like to understand this better so I can be confident we don't accidentally break it...

Change 619593 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/extensions/VisualEditor@master] Support private wikis in Parsoid zero configuration mode

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

@Legoktm Do you know where that permission check is being done? git grep 403 doesn't turn up anything in the Parsoid repo, so I assume this is being down by the REST infrastructure. I'd like to understand this better so I can be confident we don't accidentally break it...

MWBasicRequestAuthorizer::isReadAllowed() does the underlying permission check.

Then BasicRequestAuthorizer::authorize() checks $this->handler->needsReadAccess(), which defaults to true (and Parsoid doesn't override).

Change 619593 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Support private wikis in Parsoid zero configuration mode

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

Change 619609 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/extensions/VisualEditor@REL1_35] Support private wikis in Parsoid zero configuration mode

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

Change 619609 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@REL1_35] Support private wikis in Parsoid zero configuration mode

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

Legoktm added a comment.EditedAug 13 2020, 8:40 AM

I would prefer if someone (@cscott?) could verify my analysis in T260201#6378238 to double-check that we're OK from a security perspective before closing this.

ssastry triaged this task as High priority.Aug 13 2020, 10:47 PM
ssastry moved this task from Needs Triage to Current & Upcoming Work on the Parsoid board.

Did a little more testing today.

It seems that we could use a speedy security review here to ensure that the Parsoid REST API doesn't:

  1. For private wikis (login-required), doesn't expose page content w/o proper authentication

Verified in T260201#6378204, enforced by rest.php.

  1. For wikis in general, doesn't expose content which should be subject to per-user restrictions (is that a thing?)

I revdeled a revision, and then tried to access it via rest.php, correctly received a 404:

$ curl "http://localhost/mediawiki/rest.php/localhost/v3/page/html/Test/2" | jq
{
  "message": "The specified revision is deleted or suppressed.",
  "httpCode": 404,
  "httpReason": "Not Found"
}

This is implemented in ParsoidHandler::respondToMissingRevisionContent.

  1. For wikis in general, doesn't allow edits which should be subject to per-user restrictions

As cscott noted, Parsoid doesn't have any save API functionality. I did basic testing and VE correctly enforced proper permisions checks for page protection.

sbassett moved this task from Incoming to Watching on the Security-Team board.Aug 17 2020, 3:19 PM

For wikis in general, doesn't expose content which should be subject to per-user restrictions (is that a thing?)

Yes, for instance if the Lockdown extension is used.
https://www.mediawiki.org/wiki/Extension:Lockdown

So seemingly no security problem. But, we're not forwarding cookies on private wikis, so zeroconf VE/Parsoid doesn't work.

I'm also quite sure this issue is currently not something RESTBase-related. Instead I'm quite sure we have found a VisualEditor Technical-Debt after an unhappy assumption in its current design.

(Just a moment to defend VisualEditor) anyway this design is somehow widespread, looking at this flow on the RESTbase documentation:


File:Restbase request flow.svg

Look at the schema and notice the first blue arrow in the flow. I mean the MediaWiki → REST API blue arrow. That is a server-side HTTP call to reach itself.

WARNING: The problem with server-side HTTP calls is that they work in the assumption that your wiki has data publicly available to everybody. If VisualEditor does a server-side HTTP call and if it acts like whatever unauthorized logged-out user from the Internet, whatever security check fail. They fail because you are not requesting your data but, from the MediaWiki perspective, someone else, unauthenticated and unauthorized, is requesting them for you and passing the result back to you. For private wikis, with something like the LockDown extension, this means inevitable HTTP 403 Unauthorized errors and inability to use VisualEditor, even if you are sysop, even if you are a wiki bureaucrat or if there is a full moon.

This is the reasons I would be not surprised to see more and more comments about "strange server-side HTTP 403 Unauthorized errors when involving VisualEditor and a private wiki" filed here:

Some hints about how to fix this issue:

  1. patch VisualEditor/includes/ApiParsoidTrait.php to pass user's cookies to its internal RESTBase HTTP request
  2. refactor VisualEditor/includes/ApiParsoidTrait.php to be never called and avoid any kind of server-side HTTP request and relying instead of clean business logic in PHP
    1. eventually refactor RESTBase to provide a clean business logic in PHP (if actually only available via HTTP)

The first point should be enough because:

the API responds to the presence of a logged-in user and returns content appropriate to that user's permissions.
API:REST API § Permissions and authorization

valerio.bozzolan added a subscriber: Ferdi2005.EditedDec 9 2020, 3:17 AM
NOTE: Don't take this too seriously. Check T260201#6678249 before.

I was arguing with @Ferdi2005 about this behavior but we had some difficulties in describing this issue to other friends. Our feedback was something like:

Macro zoidberg-heckle: Heey! We do NOT understand!

So we gave birth to this nice metaphor/meme:

Alice with VisualEditor joins your new bar and asks for her sandwich. You (Bob) have that sandwich in the fridge. But serving sandwiches it's a job for your new waiter Carlos. Now the fun part. Carlos knows what to do. Carlos have to find that sandwich. Carlos knows a good bar making sandwiches. Carlos leaves the building. Carlos turns around. «Uh!» Carlos notices a cute bar. Carlos rings its intercom. At the same time, Bob welcomes a new costumer who looks exactly like Carlos (spoiler: he is Carlos). Carlos asks for a specific sandwich. Carlos looks at Bob. Bob looks at Carlos. Bob screams to Carlos: «Uhm! Who the hell are you? This is NOT your sandwich! This is reserved for Alice!» (HTTP 403). Carlos retraces his steps to find Bob and tell him this terrible news. That's why Carlos re-exits the bar, he finds again its original bar, he re-enters and comes back to Bob who was waiting with Alice. Carlos tells Bob that a weird guy blocked his request. Now. Alice is still waiting her sandwich. Bob reports to Alice this bad news: «Apologies Alice. As you can see, my staff reported you are not authorized to receive this sandwich. Nothing personal.»
Alice watches Bob. Bob watches Carlos. Alice looks at the fridge, so close. Alice cries.
Some seconds later, another costumer using wikitext enters your bar. Your ugly waitress Deborah goes to the fridge, she takes a sandwich and she throws that sandwich into the costumer's mouth in 13 milliseconds «Good morning, you want the usual I guess?» «Sure! Thanks Debor.. *gnam*». After this nice job Deborah goes back to do a lot of other useful things, for example polishing the floor for Bob with the right hand and cleaning the intercom for Carlos with the left one and serving other sandwiches to everyone with her cannon as usual.

O:)

MBinder_WMF edited projects, added Parsoid (Tracking); removed Parsoid.

Some hints about how to fix this issue:

  1. patch VisualEditor/includes/ApiParsoidTrait.php to pass user's cookies to its internal RESTBase HTTP request

I already did this in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/619593 for private wikis. Is your problem that this isn't working for semi-private wikis that use Lockdown?

  1. refactor VisualEditor/includes/ApiParsoidTrait.php to be never called and avoid any kind of server-side HTTP request and relying instead of clean business logic in PHP
    1. eventually refactor RESTBase to provide a clean business logic in PHP (if actually only available via HTTP)

This is the long-term plan.

I already did this in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/619593 for private wikis. Is your problem that this isn't working for semi-private wikis that use Lockdown?

Yup.

First of all let me say I think the check you introduced there has much sense.

But... I noticed that PermissionManager#isEveryoneAllowed( 'read' ) has an unexpected behavior for semi-private wikis. For these wikis it always returns true even if you cannot edit whatever page.

In short it seems it always returns true even if you have some protected namespaces with $wgNamespacePermissionLockdown:

<?php

...

// semi-private wiki with Lockdown (e.g. protected namespaces)

$is_public =
  MediaWikiServices::getInstance()
    ->getPermissionManager()
      ->isEveryoneAllowed( 'read' )


// expected false for semi-private wikis - but always true

Now.

I don't know all the consequences of altering the result of PermissionManager#isEveryoneAllowed() but I think that semi-private wikis may consider this workaround, while waiting to discover if this sort of thing should be fixed in the extension Lockdown or in the MediaWiki's core. This is a workaround:

LocalSettings.php
<?php

...

// dirty fix for wikis with Lockdown + VisualEditor (REST)
// patch for VisualEditor/includes/ApiParsoidTrait.php
// https://phabricator.wikimedia.org/T260201
$wgHooks['OnUserIsEveryoneAllowed'][] = function( $right ) {
  return false;
};

This restores the expected behavior and will allow the propagation of user's cookies in the innermost layer of the REST API currently in use.

IMPORTANT: Now the weird part. This issue apart I've just found a security issue. This is not the place to discuss. Apologies for no spoiler.