Patrol allows click catching and patrolling of any page
Closed, ResolvedPublic

Description

The following wikitext content allows to patrolling any other page:

<span class="patrollink" style="position:absolute;top:-1000px;left:-1000px;right:-1000px;bottom:-1000px;">[//example.com/?rcid=123456789 <span style="position:absolute;top:0;left:0;width:100%;height:100%;z-index:2;cursor:default"></span>]</span>
Maniphest changed the visibility from "Public (No Login Required)" to "Custom Policy".Jun 20 2015, 7:26 PM
Maniphest changed the edit policy from "All Users" to "Custom Policy".
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 20 2015, 7:26 PM
Fomafix changed the title from "" to "Patrol allows click catching and patrolling of any page".Jun 20 2015, 7:26 PM
Fomafix edited the task description. (Show Details)
Fomafix changed Security from None to Software security bug.
Fomafix edited subscribers, added: Fomafix; removed: Aklapper.

@Fomafix, sorry for the slow response on this.

The vector you're using to add the link on the page looks like a specific case of T40848, which is (still) not fixed.

I'm trying to figure out from your PoC how the patrolling is happening, because that seems like a separate csrf issue if it is. But every entry point into the patrolling looks like it's correctly protected with a csrf token. Are you seeing a working url that marks pages patrolled that doesn't include a token? Or is there a script/gadget adding the token to that link automatically?

csteipp added a subscriber: hoo.Jul 10 2015, 4:43 PM

I'm suspecting mediawiki.page.patrol.ajax.js

@hoo ^

Confirmed that yes, mediawiki.page.patrol.ajax.js is automatically adding the token.

@hoo, is there a reason you're getting the token on click, instead of using the token that already exists in the url when you add the onclick to those links?

csteipp triaged this task as "High" priority.Jul 10 2015, 4:51 PM
csteipp added a project: Vuln-CSRF.

The selector $( '.patrollink a' ) must exclude the content. This can be done by a filter or by an other position in the DOM tree. A better position of the patrol button can also fix T66176.

hoo added a comment.Jul 10 2015, 6:47 PM

Confirmed that yes, mediawiki.page.patrol.ajax.js is automatically adding the token.

@hoo, is there a reason you're getting the token on click, instead of using the token that already exists in the url when you add the onclick to those links?

Yes, we can't use that token in the URL for the patrol API, thus I had to get it by other means.

A possible fix would be to allow for the API to use the same tokens and make use of that.

Also we should adopt the selector, but that's not really nice… maybe we should set it to match a class/ id on the <a> as users can't set that via wikitext.

https://gerrit.wikimedia.org/r/242050 would introduce the same problem for the rollback action.

The vector you're using to add the link on the page looks like a specific case of T40848

Unrelated. that's just to make the link move inviting. The link works either way.

The same vector potential exists in page/patrol.js, page/watch.js and the upcoming page/rollback.js.

page/watch.js prevents this vector by restricting the event handling nodes to be outside the content area. We should do the same for page/patrol.js.

The filter in watch.js depends on the skin. It matches in skins Vector and Monobook but is does not match in skin Modern.

Using IDs or classes as selector and filter out user content is always a labile construct.

What about using a token as selector:

<html data-mw-token="fedcba9876543210">
<span class="patrollink fedcba9876543210">[<a>patrol</a>]</span>
$( '.patrollink.' + document.documentElement.dataset.mwToken + ' a' )

Setting the class on the <a> seems like an easy solution for right now.

In the longer term, personally I'd like to see us start blacklisting all attributes starting with data-mw, and then we could do something like data-mw-real-patrol="true", which seems like a more future proof way of dealing with these kinds of issues (In case one day we do decide to start allowing people to specify attributes on <a>'s somehow).

Proposed patch:

A class on <a> seams to mitigate the problem. Currently.

Bawolff moved this task from Backlog to In Progress on the Security-Team board.Oct 28 2015, 8:42 PM

In the longer term, personally I'd like to see us start blacklisting all attributes starting with data-mw, and then we could do something like data-mw-real-patrol="true", which seems like a more future proof way of dealing with these kinds of issues (In case one day we do decide to start allowing people to specify attributes on <a>'s somehow).

That's https://gerrit.wikimedia.org/r/#/c/252892/

Here's a better version of the patch that uses a data attribute

It works right now, as users cannot set anything on the <a>, but is also more future proof, as if at some point in the future we allow people to use <a> tags (for e.g. microformats or rfda or something), then it will still be secure (provided the gerrit patch for reserving data-mw gets merged)

Krinkle added a comment.EditedDec 3 2015, 12:37 PM

@Bawolff I like the approach. Let's generalise it. I proposed it for the diff hooks in js at https://gerrit.wikimedia.org/r/256665. That one isn't sensitive from a security point of view, but interface integrity overall is important as well.

Perhaps something generic like data-mw="interface".

Perhaps something generic like data-mw="interface".

@Krinkle, just so I understand your intent, you're thinking to use this exact value for elements that we generate in php and later search for in javascript? The general blacklisting of data-mw in Sanitizer would mean we could use any value for this problem, but I like the idea of standardization so we can document it, and hopefully there's less of a chance that someone removes it later, thinking they can save a few bytes of html.

I talked with Krinkle about this while he was in town. We're using data-mw="interface" in a couple of places to address this same issue. @Bawolff, any reason to not standardize in this patch too?

@Bawolff I like the approach. Let's generalise it. I proposed it for the diff hooks in js at https://gerrit.wikimedia.org/r/256665. That one isn't sensitive from a security point of view, but interface integrity overall is important as well.

Perhaps something generic like data-mw="interface".

This sounds like a good idea to me. I wonder if it should be data-mw-elmtype="interface" (or soemthing) to make it less likely to conflict with other things (e.g. Parsoid).

Looks good to me. I'll deploy this soon.

csteipp closed this task as "Resolved".Jan 27 2016, 6:01 PM
csteipp claimed this task.

Deployed

18:00 csteipp: deploy patch for T103239

For 1.23-1.26 branches, which predate I06585380bde3bc57b17ad76740c5acc2056d7c44, I'm going to use Bawolff's initial F2880385 patch, which puts the selector on the <a>, which can't be done by users in wikitext.

The patch correctly updates the two code paths in MediaWiki core (Article:: showPatrolFooter and DifferenceEngine::markPatrolledLink).

However, a quick search for markpatrolled in Wikimedia Git shows that Flow and MobileFrontend both produce their own action=markpatrolled urls.

  • Flow\UrlGenerator\markRevisionPatrolledAction()
  • MobileFrontend: InlineDifferenceEngine::getPatrolledLink()

Possibly LiquidThreads and PageTriage as well (unsure).

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".May 20 2016, 5:27 PM
demon changed the edit policy from "Custom Policy" to "All Users".
demon changed Security from Software security bug to None.
Restricted Application added a subscriber: Malyacko. · View Herald TranscriptMay 20 2016, 5:27 PM