Page MenuHomePhabricator

Alert group Cookie(s) without HttpOnly flag set
Closed, ResolvedPublic

Description

Web Server
Alert group Cookie(s) without HttpOnly flag set
Severity Low
Description
This cookie does not have the HTTPOnly flag set. When a cookie is set with the HTTPOnly flag, it
instructs the browser that the cookie can only be accessed by the server and not by client-side scripts.
This is an important security protection for session cookies.
Recommendations If possible, you should set the HTTPOnly flag for this cookie.
Alert variants
Details mf_useformat=true; expires=Sat, 23-Nov-2019 09:47:45 GMT; path=/; domain=; Secure
GET /w/index.php?
go=%D8%A8%D8%B1%D9%88&mobileaction=toggle_view_mobile&search=the&title=%D9%88%DB%8C%DA%98%D9%87:
%D8%AC%D8%B3%D8%AA%D8%AC%D9%88 HTTP/1.1
Cookie: vector-nav-p-HTML_.D9.88_CSS=true;vector-navp-.D8.AC.D8.A7.D9.88.D8.A7_.D8.A7.D8.B3.DA.A9.D8.B1.DB.8C.D9.BE.D8.AA=false;vector-nav-p-
Server_Side=false;vector-nav-p-Programming=false;vector-nav-ptb=false;wikicod_coddb_wikicod__session=rqe73jqgdkt7g5f4svo838m7lv53h58t;stopMobileRedirect=true
Authorization: Basic YW5vbnltb3VzOmFub255bW91cw==
Accept: */*
Accept-Encoding: gzip,deflate
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.21 (KHTML, like Gecko)
Chrome/41.0.2228.0 Safari/537.21
Connection: Keep-alive

Event Timeline

sbassett triaged this task as Medium priority.Nov 12 2019, 4:38 PM
sbassett moved this task from Backlog / Other to Other WMF team on the acl*security board.
sbassett added a project: MobileFrontend.
sbassett added a subscriber: sbassett.

I'm guessing this is a false positive as it's being intentionally set to false here. @Jdlrobson - is this still necessary for certain MobileFrontend functionality?

Yep the mf_useformat cookie is used by 3rd parties or wikis which do not have a mobile domain setup.

Yep the mf_useformat cookie is used by 3rd parties or wikis which do not have a mobile domain setup.

Similar to T238076#5667248, is there a good reason not to be setting the HttpOnly flag to true? If there really isn't a good reason for it being set to false here, we should probably change it with a gerrit patch.

That I'm not sure.. this is very old code. @pmiazga may know.

Hmm, not sure we can change this one, since it looks like at least a few tests might break. Though I'm not certain how relevant, current or necessary any of that code is.

Provided this isn't impacting the Varnish logic, @sbassett those tests in MobileFrontend and Minerva look like they can be revised, so please go ahead and write the patch. My team will be happy to help out with fixing the tests. Note: not sure what mediawiki-moderation is.

@Jdlrobson - do we know if any of the tests would currently be used by jenkins-bot to -1 anything in CI? i.e. are they used in quibble or anywhere else? If not, then I'd be fine sending up another patch in gerrit sometime early next week, just before the train properly returns.

Jdlrobson edited projects, added MobileFrontend (Tracking); removed MobileFrontend.
Jdlrobson moved this task from Tracking to team:other on the MobileFrontend board.
Jdlrobson edited projects, added MobileFrontend; removed MobileFrontend (Tracking).

@sbassett I don't think this would cause any problems.

Proposed patch: P12038. Again, I'm only seeing this referenced within a few Selenium tests where it might break. Otherwise I'll plan to deploy as a security patch during this Monday's window.

@sbassett I don't think, but I'm not too familiar with the Selenium stack @zeljkofilipin do you know if this would create problems for the Selenium tests?

@sbassett I don't think, but I'm not too familiar with the Selenium stack @zeljkofilipin do you know if this would create problems for the Selenium tests?

@Jdlrobson as far as I can see at the codesearch, in the context of Selenium tests, mf_useformat is only used in MobileFrontend and MinervaNeue to set cookies related to using desktop or mobile mode. I'm not really familiar with tests in those repositories.

Proposed patch: P12038. Again, I'm only seeing this referenced within a few Selenium tests where it might break. Otherwise I'll plan to deploy as a security patch during this Monday's window.

I can try running the patch targeting a local mediawiki-docker instance, but I don't know how to get P12038.

@zeljkofilipin - I've subbed you on the paste, though let me add a proper file here as I've found that copy-pasting patches from text out of Phab can be problematic in getting them to properly apply:

I was still going to apply it to production this afternoon during the security window as a security patch. The (possible) issues with tests breaking would only come into play for backports to master, etc.

sbassett lowered the priority of this task from Medium to Low.Jul 27 2020, 9:15 PM
sbassett moved this task from Waiting to In Progress on the user-sbassett board.

Assigning to myself until I check if Selenium tests break.

Assigning to myself until I check if Selenium tests break.

Ok. Since the patch has been successfully deployed to production (for now), we can make this task public and start on the backports if you'd like. That way everything could be easily run and tested via gerrit/jenkins. I'm also going to track this issue here: T256342.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Jul 28 2020, 6:35 PM

Change 616889 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/MobileFrontend@master] mf_useformat cookie should be HttpOnly

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

Interesting... no tests seemed to fail on the gerrit change set.

Change 616889 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] mf_useformat cookie should be HttpOnly

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

zeljkofilipin removed a project: User-zeljkofilipin.

Interesting... no tests seemed to fail on the gerrit change set.

👍

Change 617137 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/MobileFrontend@REL1_35] mf_useformat cookie should be HttpOnly

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

Change 617138 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/MobileFrontend@REL1_34] mf_useformat cookie should be HttpOnly

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

Change 617139 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/MobileFrontend@REL1_31] mf_useformat cookie should be HttpOnly

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

The last thing to do here, once the backports are all merged, is to request a CVE. Though this is a very borderline case in my opinion since it was more an (intentionally) insecure configuration and not an actual security bug within the code.

Change 617138 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@REL1_34] SECURITY: mf_useformat cookie should be HttpOnly

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

Change 617137 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@REL1_35] SECURITY: mf_useformat cookie should be HttpOnly

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

Change 617139 merged by SBassett:
[mediawiki/extensions/MobileFrontend@REL1_31] SECURITY: mf_useformat cookie should be HttpOnly

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

sbassett claimed this task.
sbassett moved this task from In Progress to Done on the user-sbassett board.
sbassett removed a project: Patch-For-Review.

Ok, all of the backports appear to be merged. Thinking about T238075#6345083 some more, I don't think this merits a CVE, though I still plan to include it within the announcement for T256342.

I have dropped the patch from /srv/patches/1.36.0-wmf.4/extensions/MobileFrontend since it is now included in the branch.