| Fomafix | |
| Jun 20 2015, 7:26 PM |
| F3284031: Add data tag to prevent spoofing of patrol links. Using data-mw="interface" for consistency. | |
| Jan 26 2016, 11:40 PM |
| F2958692: Use a data attribute to better future proof | |
| Nov 13 2015, 4:47 AM |
| F2880385: patrol.patch | |
| Oct 27 2015, 9:28 AM |
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>
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | • demon | T124940 MediaWiki 1.26.3 security release | |||
| Resolved | • csteipp | T103239 Patrol allows click catching and patrolling of any page |
@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?
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.
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:
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)
@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?
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).
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.
Possibly LiquidThreads and PageTriage as well (unsure).