Page MenuHomePhabricator

User content can redirect the logout button to different URL (CVE-2020-10959)
Closed, ResolvedPublic

Description

Steps to reproduce

  • Put the following wiki text into a wiki page:
<div id="pt-logout">[https://www.example.com/ click]</div>
  • Log in the wiki.
  • Load the page with the rendered wiki text.
  • Click on the logout button.

-> The user get logged out.
-> The browser redirects to https://www.example.com/

Expected behavior

It should not possible to change the target of an interface button by user content.

Mitigation

https://gerrit.wikimedia.org/r/536725 (rMWd4a552e65bdf) by @Krinkle mitigates this issue.

The button in the user content still logs out. Mitigation for this is to add data-mw="interface" as HTML attribute to the logout button and add [data-mw="interface"] to the jQuery selector for selecting the button.

Details

Event Timeline

Fomafix created this task.Sep 14 2019, 7:43 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 14 2019, 7:44 PM
Fomafix updated the task description. (Show Details)Sep 14 2019, 7:47 PM
Fomafix added a subscriber: Krinkle.
Fomafix updated the task description. (Show Details)Sep 17 2019, 9:46 AM
Reedy added a subscriber: Reedy.EditedSep 21 2019, 9:20 PM

Does this only affect master? Or should we be back porting to REL1_33/REL1_32/REL1_31?

I'm aiming to get a security and maintenance release out next week (though there are no private security patches to go out)

It looks like there 1490e9dac828af097f0a15eca63eaa89e2d2bc50 and 97fffb3fd0b345db0a8999eda1ffa672311da9f1 might be needed as dependancies, just for REL1_33. But then there's some resources.php patch... Never mind any previous versions of MW...

Can someone take care of these backports where they're appropriate?

Krinkle added a comment.EditedSep 25 2019, 7:58 PM

Good catch. The link should be protected by [data-mw="interface"] which is illegal in user content (enforced, automatically filtered out).

The click handler for the logout button was introduced in rMW8f033911030d included in REL1_34. This has two vulnerabilities by user content:

  1. The target of the redirect of the logout button can manipulated. This is fixed in rMWd4a552e65bdf included in REL1_34. A backport to older releases is not needed.
  2. The user content can create an own logout button and do click catching. This is not fixed yet. This can be done by protecting the link with [data-mw="interface"].
chasemp triaged this task as Medium priority.Dec 9 2019, 3:57 PM
Krinkle added a project: Security-Team.EditedFeb 10 2020, 11:40 PM
Krinkle removed a subscriber: Krinkle.
Krinkle added a subscriber: Krinkle.

This seems important, tagging Security-Team directly to push, forward, or delagate accordingly.

Krinkle removed a subscriber: Krinkle.Feb 10 2020, 11:40 PM
  1. The user content can create an own logout button and do click catching. This is not fixed yet. This can be done by protecting the link with [data-mw="interface"].

Is the fix as simple as this? Seems to work locally for me, I think.

This is only the query part. The logout button doesn't have the attribute data-mw="interface" yet.

Second attempt. Not sure how hacky this is, though if personal_urls now require data-mw attribute support, this seems like the logical place to do it.

mitigates the second vulnerability. Just the trailing whitespace in BaseTemplate.php is wrong.

Ok, trailing whitespace removed.

If there are no objections to the rev3 patch above, I can plan to security-deploy it this afternoon (before SWAT and the train) or tomorrow (2020-02-14).

Can there a caching issue happen? The old selector also matches to the new HTML but the new selector doesn't match to the old HTML. If the deployment can't ensure that always a new HTML is loaded when the new JavaScript is delivered to the browsers then this patch have to split up.

Can there a caching issue happen? The old selector also matches to the new HTML but the new selector doesn't match to the old HTML. If the deployment can't ensure that always a new HTML is loaded when the new JavaScript is delivered to the browsers then this patch have to split up.

I'm wondering if we could get away with performing a conditional check for the new selector and if it wasn't found, the old one, within resources/src/mediawiki.page.ready/ready.js for some period of time (24 hrs, a week, something else) to allow for various caches to clear.

sbassett moved this task from Incoming to In Progress on the Security-Team board.Feb 19 2020, 4:35 PM

I guess there is no caching problem on deployment. And even when the new JavaScript is delivered to the browsers before the new HTML, then the fallback way of the logout button still works.

Logged in users bypass the varnish cache so I don't believe there are any caching issues here.

Ok, I'll plan to deploy the patch as-is (T232932#5877700) today and keep an eye on logstash just in case.

Patch deployed. Tested on testwiki (and oversighted) - everything worked fine for me. Just to note: this issue is being held for the next security release, so please keep this task private for now (adding PermanentlyPrivate to be safe, until release) and refrain from backporting in gerrit for the time being. Thanks.

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptFeb 21 2020, 10:12 PM
Restricted Application changed the edit policy from "All Users" to "Custom Policy". · View Herald Transcript
sbassett claimed this task.Mar 2 2020, 8:41 PM
sbassett added a subscriber: Krinkle.
Reedy added a comment.Tue, Mar 24, 5:15 PM

Patch applies cleanly to master and REL1_34.

Doesn't to REL1_33 and REL1_31. Time to poke further

Reedy added a comment.EditedTue, Mar 24, 5:18 PM
Changes to be committed:

	modified:   includes/skins/BaseTemplate.php
	modified:   includes/skins/SkinTemplate.php

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)

	deleted by us:   resources/src/mediawiki.page.ready/ready.js

As rMW8f033911030d26759c327145403f91ac6b1c5e66 in REL1_34 added "Turn logout to a POST action"... Can we just not apply the changes to mediawiki.page.ready.js but apply the two PHP ones (amend the commit summary too) and continue?

Changes to be committed:
	modified:   includes/skins/BaseTemplate.php
	modified:   includes/skins/SkinTemplate.php
Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	deleted by us:   resources/src/mediawiki.page.ready/ready.js

As rMW8f033911030d26759c327145403f91ac6b1c5e66 in REL1_34 added "Turn logout to a POST action"... Can we just not apply the changes to mediawiki.page.ready.js but apply the two PHP ones (amend the commit summary too) and continue?

Yeah, this security issue has been introduced in by turning the log out button to a POST action which is rather recent (around a year now).

Reedy closed this task as Resolved.Tue, Mar 24, 5:32 PM

Sorted then

sbassett renamed this task from User content can redirect the logout button to different URL to User content can redirect the logout button to different URL (CVE-2020-10959).Thu, Mar 26, 3:40 AM
Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Thu, Mar 26, 5:41 PM
Restricted Application changed the visibility from "Public (No Login Required)" to "Custom Policy". · View Herald TranscriptThu, Mar 26, 5:41 PM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript
Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".
Reedy changed the edit policy from "Custom Policy" to "All Users".

this issue is being held for the next security release, so please keep this task private for now (adding PermanentlyPrivate to be safe, until release)

Am I the only one confused about the use of PermanentlyPrivate here? It sounds like this was not permanently private, just private until the next security release. The visibility policy on the task already accounted for that.

this issue is being held for the next security release, so please keep this task private for now (adding PermanentlyPrivate to be safe, until release)

Am I the only one confused about the use of PermanentlyPrivate here? It sounds like this was not permanently private, just private until the next security release. The visibility policy on the task already accounted for that.

There's a Herald rule that doesn't let users mistakenly "open" a ticket to public when it has PermanentlyPrivate tag on it. I think here it was used as more of a tool. Fixing it should not be that hard, create a tag like "StayPrivate" and use that instead (with the proper herald rules added)

There's a Herald rule that doesn't let users mistakenly "open" a ticket to public when it has PermanentlyPrivate tag on it. I think here it was used as more of a tool. Fixing it should not be that hard, create a tag like "StayPrivate" and use that instead (with the proper herald rules added)

This is correct. Sorry for any confusion regarding my explanation above. If someone would like to file a bug to create a new "StayPrivate" tag, please feel free to do so.