The default bug view has changed. See this FAQ.

web content can set httponly cookie by overwriting a non-httponly one

RESOLVED FIXED

Status

()

Core
Networking: Cookies
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: dveditz, Assigned: dveditz)

Tracking

({fixed1.8.0.15, fixed1.8.1.5})

Trunk
fixed1.8.0.15, fixed1.8.1.5
Points:
---
Bug Flags:
blocking1.8.1.5 +
wanted1.8.1.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
Created attachment 271669 [details] [diff] [review]
patch and test

The fix in bug 383181 missed a case: a script from web content is able to set an httponly cookie by replacing an existing non-httponly cookie. So if a server has not already set an httponly cookie, a script can first create a normal cookie and then replace it with the httponly attribute in order to set one.
Flags: wanted1.8.1.x+
Flags: blocking1.9?
Flags: blocking1.8.1.5+
Attachment #271669 - Flags: review?(dwitte)
Attachment #271669 - Flags: approval1.8.1.5?
(Assignee)

Comment 1

10 years ago
This could be used to DoS any web-app that relies on using cookies from script by hiding them. The server could reset them as non-httponly, but many web-apps don't send cookies on every request if they're getting a Cookie header from the client.
(Assignee)

Comment 2

10 years ago
Created attachment 271676 [details] [diff] [review]
alternate approach

Or we could just move the "if (!aFromHttp && aCookie->IsHttpOnly())" check into a common spot and fix it that way.

Have a preference? I think this one is clearer
Attachment #271676 - Flags: review?(dwitte)

Comment 3

10 years ago
Comment on attachment 271676 [details] [diff] [review]
alternate approach

good catch, i like this version. if you approve this one i'll land this on branch along with the rest.
Attachment #271676 - Flags: review?(dwitte) → review+
(Assignee)

Updated

10 years ago
Attachment #271669 - Attachment is obsolete: true
Attachment #271669 - Flags: review?(dwitte)
Attachment #271669 - Flags: approval1.8.1.5?
(Assignee)

Comment 4

10 years ago
Comment on attachment 271676 [details] [diff] [review]
alternate approach

approved for 1.8.1.5, a=dveditz for release-drivers
Attachment #271676 - Flags: approval1.8.1.5+
Comment on attachment 271676 [details] [diff] [review]
alternate approach

sr=mconnor, let's get this landed ASAP
Attachment #271676 - Flags: superreview+

Comment 6

10 years ago
checked in on trunk and 1.8 branch.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.8.1.5
Resolution: --- → FIXED
Can someone give a test case to repro this issue?

Updated

10 years ago
Flags: blocking1.9?
I dislike having a compiled-code test for this because 1) it's harder to edit and read, 2) the test environment is less like the real web than it needs to be, and 3) the cookies tests in particular are extremely noisy even when they pass, but this *is* in an automated test, I guess.
Flags: in-testsuite+

Comment 9

10 years ago
in most cases, i'm the one writing and maintaining the tests, and it's easy for me. if you want to write tests, please feel free to file a bug and convert them to a different language. if you want the noise reduced, file a bug, even better with a patch?
The conversion process is tedious and involved enough that I probably won't do it, especially for tests which already run automatically and already report success/failure in a useful manner such that failures can happen and actually get noticed when they do.

I've done this once before, in bug 398952, and the experience leaves me somewhat sour on rewriting large existing tests (particularly since in this case I'd port them to Mochitests to better emulate the real world, rather than just modifying the C++ tests to not spew so much), particularly ones which fit the bill above but which have some niggling problems.
fixed1.8.0.15 with combined checkin to bug 178993
Keywords: fixed1.8.0.15
You need to log in before you can comment on or make changes to this bug.