Last Comment Bug 387543 - web content can set httponly cookie by overwriting a non-httponly one
: web content can set httponly cookie by overwriting a non-httponly one
Status: RESOLVED FIXED
: fixed1.8.0.15, fixed1.8.1.5
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
:
Mentors:
Depends on:
Blocks: 383181
  Show dependency treegraph
 
Reported: 2007-07-10 07:12 PDT by Daniel Veditz [:dveditz]
Modified: 2008-03-20 08:21 PDT (History)
5 users (show)
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
jwalden+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch and test (2.22 KB, patch)
2007-07-10 07:12 PDT, Daniel Veditz [:dveditz]
no flags Details | Diff | Splinter Review
alternate approach (3.81 KB, patch)
2007-07-10 07:40 PDT, Daniel Veditz [:dveditz]
dwitte: review+
mconnor: superreview+
dveditz: approval1.8.1.5+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2007-07-10 07:12:23 PDT
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.
Comment 1 Daniel Veditz [:dveditz] 2007-07-10 07:22:25 PDT
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.
Comment 2 Daniel Veditz [:dveditz] 2007-07-10 07:40:49 PDT
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
Comment 3 dwitte@gmail.com 2007-07-10 13:44:45 PDT
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.
Comment 4 Daniel Veditz [:dveditz] 2007-07-10 14:45:47 PDT
Comment on attachment 271676 [details] [diff] [review]
alternate approach

approved for 1.8.1.5, a=dveditz for release-drivers
Comment 5 Mike Connor [:mconnor] 2007-07-10 20:15:24 PDT
Comment on attachment 271676 [details] [diff] [review]
alternate approach

sr=mconnor, let's get this landed ASAP
Comment 6 dwitte@gmail.com 2007-07-10 20:30:09 PDT
checked in on trunk and 1.8 branch.
Comment 7 Al Billings [:abillings] 2007-07-13 17:32:09 PDT
Can someone give a test case to repro this issue?
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2007-10-07 21:50:48 PDT
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.
Comment 9 dwitte@gmail.com 2007-10-08 00:37:40 PDT
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?
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2007-10-08 02:14:30 PDT
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.
Comment 11 Christopher Aillon (sabbatical, not receiving bugmail) 2008-03-20 08:21:28 PDT
fixed1.8.0.15 with combined checkin to bug 178993

Note You need to log in before you can comment on or make changes to this bug.