Closed Bug 387543 Opened 17 years ago Closed 17 years ago

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


(Core :: Networking: Cookies, defect)

Not set





(Reporter: dveditz, Assigned: dveditz)



(Keywords: fixed1.8.0.15, fixed1.8.1.5)


(1 file, 1 obsolete file)

Attached patch patch and test (obsolete) — Splinter Review
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?
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.
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 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+
Attachment #271669 - Attachment is obsolete: true
Attachment #271669 - Flags: review?(dwitte)
Attachment #271669 - Flags: approval1.8.1.5?
Comment on attachment 271676 [details] [diff] [review]
alternate approach

approved for, 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+
checked in on trunk and 1.8 branch.
Closed: 17 years ago
Keywords: fixed1.8.1.5
Resolution: --- → FIXED
Can someone give a test case to repro this issue?
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+
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.