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?

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

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

In T232932#5524854, @Fomafix wrote:

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.

T232932.patch970 BDownload

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.

T232932-rev2.patch2 KBDownload

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.

In T232932#5884045, @Fomafix wrote:

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.

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.

Patch applies cleanly to master and REL1_34.

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

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?

In T232932#5995685, @Reedy wrote:

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

In T232932#5908474, @sbassett wrote:

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.

In T232932#6002996, @Krenair wrote:

In T232932#5908474, @sbassett wrote:

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)

In T232932#6003030, @Ladsgroup wrote:

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.

Is there any reason https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-10959 shows RESERVED?

This is public.

In T232932#6179379, @RhinosF1 wrote:

Is there any reason https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-10959 shows RESERVED?

https://cve.mitre.org/about/faqs.html#reserved_signify_in_cve_entry

In T232932#6179409, @Reedy wrote:

In T232932#6179379, @RhinosF1 wrote:

Is there any reason https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-10959 shows RESERVED?

https://cve.mitre.org/about/faqs.html#reserved_signify_in_cve_entry

Sorry, let me reword my question. Is there any reason the WMF have not yet asked for it to be populated or mitre are holding it?

I know why anything can be reserved, I meant this task specifically.

@RhinosF1 - I can follow up with Mitre on this one, but note that the population of CVE data on their end is somewhat out of our hands. From the documentation @Reedy mentioned above:

"A CVE Entry can change from the RESERVED state to being populated at any time based on a number of factors both internal and external to the CVE List"

(emphasis mine)

Update: I just reached out to Mitre about updating the CVE description, etc. on their end. I'll post any relevant information here once I have it.

Thanks @sbassett, I just wondered as the last few times I’ve dealt with CVEs they’ve been deal with within around a day.

@RhinosF1 - It depends. For issues that we fix and make public quickly, we can flip the acknowledge bit on Mitre's CVE request form and the CVE populates quickly on Mitre's end. For other issues (like this one) where we need to hold the patch for a quarterly security release, we can't really do that, and so sometimes the CVEs stay "RESERVED" for a long time.

Thanks for the clarification!

@RhinosF1 -

The CVE appears to be populated on Mitre's end now: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-10959

If, in the future, you notice any Wikimedia-related CVEs which still appear reserved and likely shouldn't be, feel free to comment on the relevant task or contact the Security-Team via security-help@wikimedia.org. Thanks.

Thanks for the help, glad it’s sorted!