Closed
Bug 1421324
(CVE-2018-5114)
Opened 7 years ago
Closed 7 years ago
Overwrite of existing cookie with HttpOnly flagged cookie results in HttpOnly being ignored by Firefox
Categories
(Core :: Networking: Cookies, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | wontfix |
firefox58 | --- | verified |
firefox59 | --- | fixed |
People
(Reporter: inko, Assigned: amchung)
Details
(Keywords: regression, reporter-external, sec-moderate, Whiteboard: [necko-triaged][adv-main58+][post-critsmash-triage])
Attachments
(2 files, 3 obsolete files)
4.19 KB,
patch
|
jdm
:
feedback+
|
Details | Diff | Splinter Review |
4.88 KB,
patch
|
amchung
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36
Steps to reproduce:
A HTTP Request is sent to the server (192.168.1.1), which responds with with a Set-Cookie (without HttpOnly flag) statement to set a cookie:
Example Response:
HTTP/1.1 302 Found
Content-Type: text/html
Content-Length: 287
Location: http://192.168.1.1/webpage.php?id=1
Connection: close
Set-Cookie: PHPSESSID=qevb2lkbjviniu2i3ie278tp45; path=/
<...>
The cookie PHPSESSID is set in Firefox, and the Browser follows the Location redirect to the webpage (same server 192.168.1.1) issuing the following request:
GET /webpage.php?id=1 HTTP/1.1
Host: 192.168.1.1
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Referer: http://192.168.1.1/login.php
Cookie: PHPSESSID=qevb2lkbjviniu2i3ie278tp45
DNT: 1
Connection: close
Upgrade-Insecure-Requests: 1
This request results in the following response from the server setting a HttpOnly protected cookie (overwriting the existing one with additional HttpOnly flag):
HTTP/1.1 200 OK
Content-Type: text/html
Content-Length: 303
Connection: close
Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0
Pragma: no-cache
Set-Cookie: PHPSESSID=qevb2lkbjviniu2i3ie278tp45; path=/; httponly
Vary: Accept-Encoding
<...>
Summary:
=========
"Upgrading/Overwriting" a normal cookie to a httponly flagged cookie.
Actual results:
The cookie is, although now flagged with httponly, still accessible via JavaScript:
PoC 1:
Web Console:
document.cookie
"PHPSESSID=qevb2lkbjviniu2i3ie278tp45"
PoC 2:
also works via Cross-Site Scripting vector, aka:
<script>alert(document.cookie)</script>
-> resulting in an alert box disclosing the cookie value
Expected results:
After setting the httpOnly flag, the cookie should be protected from accessing it via JavaScript. This is a Cross-Site Scripting prevention meassure which seems to not work as intended anymore (worked as intended in earlier versions of Firefox, and works as intended in other Browsers (Chrome)).
Easy way to reproduce the issue:
Step 1:
-------
PHP script on a webserver:
<?php
setcookie("PHPSESSID", "123456", 0, "/", "", false);
//setcookie("PHPSESSID", "123456", 0, "/", "", false, true);
echo "<html><head><title></title></head>";
echo "<body><script>alert(document.cookie)</script></body></html>";
?>
Visit the PHP Script with the Firefox Browser.
Actual result:
Popup with cookie value.
Expected Result:
Popup with cookie value.
Step 2:
-------
Now comment the first setcookie line, and uncomment the second line and visit the webpage again. This line (last parameter setting HttpOnly to true) now overwrites the existing cookie with a HttpOnly protected cookie with the same name and value.
The HttpOnly flag is in fact set (can be checked by a HTTP proxy like Burp, or via a Firefox Addon like Cookie Manager).
Actual result:
Popup with cookie value.
Expected Result:
Empty Popup cause the cookie is protected by HttpOnly.
Comment 2•7 years ago
|
||
We seem to be creating _two_ cookies. The first, unprotected, cookie stays available to website script. The second httponly one is what we send in HTTP requests. Are we using httponly (and maybe other attributes) as a key to the cookie store?
The original non-httponly cookie is not visible in the devtools storage panel. Dev Tools seems to get the same (correct) view as network requests. Where is the bogus original cookie hiding?
This can be tested by changing the cookie value in the commented-out line of your testcase.
Group: firefox-core-security → network-core-security
Component: Untriaged → Networking: Cookies
Product: Firefox → Core
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
status-firefox57:
--- → affected
status-firefox58:
--- → affected
status-firefox59:
--- → affected
status-firefox-esr52:
--- → unaffected
Ever confirmed: true
Comment 4•7 years ago
|
||
This also works fine if you disable multi-process.
Amy: could this be fall-out from bug 1331680? Just guessing that network code in the parent process updates the "real" cookies store, but because the new value is httpOnly we bail out before we sync the child process ("web content doesn't need to know about httponly cookies") and never tell it to drop the old value.
Flags: needinfo?(amchung)
Keywords: sec-moderate
Updated•7 years ago
|
Flags: sec-bounty?
Assignee | ||
Comment 5•7 years ago
|
||
Hi Daniel,
Yes, this problem is the regression bug of bug 1331680.
I will fix this bug.
Thanks!
Flags: needinfo?(amchung)
Priority: -- → P1
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → amchung
Assignee | ||
Comment 6•7 years ago
|
||
Hi Josh,
I found we didn't consider the situation as below:
1. Server side sets a non-httponly cookie whose name is test1, and test1 will store in hashtable on CookieServiceChild.
2. Server side modifies test1 to httponly, but CookieServiceChild will reject httponly cookie on CookieServiceChild::SetCookieInternal().
3. Still can access test1 through document.cookie.
I have modified above situation as below:
1. Removed if (aCookieAttributes.isHttpOnly) {return;} from CookieServiceChild::SetCookieInternal().
2. Moved the rejection the httponly cookie setting on CookieServiceChild::RecordDocumentCookie().
Would you help me to review my patch?
Thanks!
Attachment #8933187 -
Flags: review?(josh)
Updated•7 years ago
|
Whiteboard: [necko-triaged]
Updated•7 years ago
|
Attachment #8933187 -
Flags: review?(josh) → review+
Comment 7•7 years ago
|
||
Comment on attachment 8933187 [details] [diff] [review]
implementation -- Modified rejection timing when processing httponly cookie on CookieServiceChild.
Can we add an automated test for this, too?
Assignee | ||
Comment 8•7 years ago
|
||
Hi Josh,
I think we could create new bug to add an automated test for this bug.
I will land this patch first.
Thanks!
Flags: needinfo?(josh)
Comment 9•7 years ago
|
||
I would feel more comfortable about merging this change with a test demonstrating that it works as expected and does not expose HttpOnly cookies to document.cookie.
Flags: needinfo?(josh)
Assignee | ||
Comment 10•7 years ago
|
||
Hi Josh,
I have finished the test case and found I have to do more modification as below:
1. CookieServiceParent has to send all of cookie-change to CookieServiceChild.
2. CookieStatus has to add isHttpOnly.
Would you help me to review this patch?
Thanks!
Attachment #8933187 -
Attachment is obsolete: true
Attachment #8935319 -
Flags: review?(josh)
Assignee | ||
Comment 11•7 years ago
|
||
Hi Josh,
I have finished the test case, and security team suggested that us try not to create test case on security bug.
Base on this reason, I used feedback? on this test case, after f+, I will create new bug and r? this patch.
The steps of this test case:
1. Used XHR to set a cookie whose name is xhr1 and set it to non-httponly.
2. Modified xhr1 to httponly cookie.
3. This mochitest receive the notification from observer is earlier than CookieServiceParent sends the ipc msg of notifying CookieServiceChild to add/modify cookie.
base on above reason, the mochitest will confirm document.cookie can't access xhr1 after using document.cookie to set xhr2.
4. Confirmed document.cookie only accesses xhr2.
Would you help me to review this patch?
Thanks!
Attachment #8935325 -
Flags: feedback?(josh)
Comment 12•7 years ago
|
||
Comment on attachment 8935325 [details] [diff] [review]
Test case -- Can't use document.cookie to access cookie after the non-httponly cookie modifies to httponly cookie.
Review of attachment 8935325 [details] [diff] [review]:
-----------------------------------------------------------------
::: netwerk/test/mochitests/mochitest.ini
@@ +32,5 @@
> [test_1331680.html]
> [test_1331680_iframe.html]
> [test_1331680_xhr.html]
> [test_1396395.html]
> +[test_1421324.html]
These files are missing from the patch.
Attachment #8935325 -
Flags: feedback?(josh) → feedback-
Updated•7 years ago
|
Attachment #8935319 -
Flags: review?(josh) → review+
Assignee | ||
Comment 13•7 years ago
|
||
Hi Josh,
Sorry for forgetting to add test_1421324.html & reset_cookie_xhr.sjs.
I have added these test files on this patch.
Would you help me to review my patch?
Thanks!
Attachment #8935325 -
Attachment is obsolete: true
Attachment #8935627 -
Flags: feedback?(josh)
Comment 14•7 years ago
|
||
Comment on attachment 8935627 [details] [diff] [review]
Test case -- Can't use document.cookie to access cookie after the non-httponly cookie modifies to httponly cookie.
Review of attachment 8935627 [details] [diff] [review]:
-----------------------------------------------------------------
::: netwerk/test/mochitests/test_1421324.html
@@ +9,5 @@
> +
> +<script type="text/javascript">
> +SimpleTest.waitForExplicitFinish();
> +
> +let {utils: Cu, classes: Cc, interfaces: Ci } = Components;
This isn't used.
Attachment #8935627 -
Flags: feedback?(josh) → feedback+
Assignee | ||
Comment 15•7 years ago
|
||
Attachment #8935319 -
Attachment is obsolete: true
Attachment #8936902 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 16•7 years ago
|
||
Comment 17•7 years ago
|
||
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 18•7 years ago
|
||
Please request Beta approval on this when you get a chance.
Flags: needinfo?(amchung)
Assignee | ||
Comment 19•7 years ago
|
||
Comment on attachment 8936902 [details] [diff] [review]
implementation -- Modified rejection timing when processing httponly cookie on CookieServiceChild. r=jdm
Approval Request Comment
[Feature/Bug causing the regression]:Regression of bug 1331680.
[User impact if declined]:document.cookie can get the cookie's value when this cookie's httponly flag modifies from true to false.
The above situation occurs a security weaknesses on xss attack.
[Is this code covered by automated tests?]:no, the bug 1425292 is the test case of this bug.
[Has the fix been verified in Nightly?]:Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]:None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This modification can protect the httponly cookie and doesn't impact other cookie's setting on content process.
[String changes made/needed]:None.
Flags: needinfo?(amchung)
Attachment #8936902 -
Flags: approval-mozilla-beta?
Comment 20•7 years ago
|
||
Comment on attachment 8936902 [details] [diff] [review]
implementation -- Modified rejection timing when processing httponly cookie on CookieServiceChild. r=jdm
sec fix, let's take this on beta58.
Attachment #8936902 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 21•7 years ago
|
||
uplift |
Updated•7 years ago
|
Group: network-core-security → core-security-release
Updated•7 years ago
|
Flags: sec-bounty? → sec-bounty+
Updated•7 years ago
|
Whiteboard: [necko-triaged] → [necko-triaged][adv-main58+]
Updated•7 years ago
|
Alias: CVE-2018-5114
Updated•7 years ago
|
Whiteboard: [necko-triaged][adv-main58+] → [necko-triaged][adv-main58+][post-critsmash-triage]
Comment 22•7 years ago
|
||
I was not able to reproduce the initial issue on old build Fx 57.0 using an Apache server and running the script following steps from comment 1.
On both builds Fx 57.0 (that does not have this fix) and Fx 58.0 (that does have this fix) I'm getting the same result after running the script for the first time (Popup with cookie value) and for the second time after changing the comment (Popup without cookie value, empty due to HttpOnly).
Reporter, would you be comfortable verifying that the fix works for you on Firefox 58.0?
https://archive.mozilla.org/pub/firefox/candidates/58.0-candidates/build6/win64/en-US/firefox-58.0.zip
Flags: needinfo?(inko)
Reporter | ||
Comment 23•7 years ago
|
||
I reproduced the issue with the old (unfixed) Fx 57 (https://archive.mozilla.org/pub/firefox/candidates/57.0-candidates/build4/win64/en-US/firefox-57.0.zip) in multiple VMs. Maybe same issue as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1421324#c4 ?
Fix in 58.0 worked on my test system (Win10, x64, Fx 58.0), Popup empty.
Flags: needinfo?(inko)
Comment 24•7 years ago
|
||
(In reply to inkognito from comment #23)
> I reproduced the issue with the old (unfixed) Fx 57
> (https://archive.mozilla.org/pub/firefox/candidates/57.0-candidates/build4/
> win64/en-US/firefox-57.0.zip) in multiple VMs. Maybe same issue as described
> in https://bugzilla.mozilla.org/show_bug.cgi?id=1421324#c4 ?
>
> Fix in 58.0 worked on my test system (Win10, x64, Fx 58.0), Popup empty.
Thanks for this! marking as verified on 58.
Updated•7 years ago
|
Group: core-security-release
Updated•8 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•