Closed Bug 1421324 (CVE-2018-5114) Opened 7 years ago Closed 6 years ago

Overwrite of existing cookie with HttpOnly flagged cookie results in HttpOnly being ignored by Firefox

Categories

(Core :: Networking: Cookies, defect, P1)

Product:
Core
Component:
Networking: Cookies
Version:
57 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- verified
firefox59 --- fixed

People

(Reporter: inko, Assigned: amchung)

Details

(Keywords: regression, sec-moderate, Whiteboard: [necko-triaged][adv-main58+][post-critsmash-triage])

Attachments

(2 files, 3 obsolete files)

Test case -- Can't use document.cookie to access cookie after the non-httponly cookie modifies to httponly cookie.
4.19 KB, patch
jdm
: feedback+
Details | Diff | Splinter Review
implementation -- Modified rejection timing when processing httponly cookie on CookieServiceChild. r=jdm
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.
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
Status: UNCONFIRMED → NEW
status-firefox57: --- → affected
status-firefox58: --- → affected
status-firefox59: --- → affected
status-firefox-esr52: --- → unaffected
Ever confirmed: true
This works fine in Firefox 56 -- it regressed in 57.
Keywords: regression
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
Flags: sec-bounty?
Hi Daniel,
Yes, this problem is the regression bug of bug 1331680.
I will fix this bug.

Thanks!
Flags: needinfo?(amchung)
Priority: -- → P1
Assignee: nobody → amchung
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)
Whiteboard: [necko-triaged]
Attachment #8933187 - Flags: review?(josh) → review+
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?
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)
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)
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)
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 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-
Attachment #8935319 - Flags: review?(josh) → review+
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 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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4ccefbbbb1c3
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Please request Beta approval on this when you get a chance.
Flags: needinfo?(amchung)
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 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+
Group: network-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Whiteboard: [necko-triaged] → [necko-triaged][adv-main58+]
Alias: CVE-2018-5114
Whiteboard: [necko-triaged][adv-main58+] → [necko-triaged][adv-main58+][post-critsmash-triage]
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)
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)
(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.
status-firefox58: fixed → verified
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: