Bugzilla@Mozilla – Bug 643051
document.cookie should only allow setting one cookie at a time
Last modified: 2011-06-16 15:15:42 PDT
Summon comment box
Currently, document.cookie allows setting multiple cookies in a single call. This is inconsistent with other browsers as well as our documentation: https://developer.mozilla.org/en/document.cookie "Note that you can only set/update a single cookie at a time using this method." This can lead to security problems in web applications: When an external script takes untrusted input (e.g. from a URL parameter), that input can contain line breaks, and thus set multiple cookies to the including web site. Therefore, we should disallow setting multiple cookies using document.cookie.
Created attachment 520373 [details] [review] patch
As a security problem this really arises because of insufficient filtering on the web app side more than a problem in Firefox. But it's clearly a bug and we should fix it.
Comment on attachment 520373 [details] [review] patch r=me Might be worth creating a mochitest with document.cookie too.
Created attachment 524724 [details] [review] mochitest
Created attachment 524733 [details] [review] final patch for checkin
Comment on attachment 524733 [details] [review] final patch for checkin transfering r=bz I would like to land this on the ff4 and 3.6 branches too, if possible.
http://hg.mozilla.org/projects/cedar/rev/b980d0cf9847
Created attachment 524774 [details] [review] test fix http://hg.mozilla.org/projects/cedar/rev/bf3bdd1d9671
This failed mochitest-1 persistently, and the followups didn't fix it, so I backed out the original patch and both followups from cedar: http://hg.mozilla.org/projects/cedar/rev/4e6e2a9c48c6 http://hg.mozilla.org/projects/cedar/rev/865870bbe148 http://hg.mozilla.org/projects/cedar/rev/a998aa8043f1 http://hg.mozilla.org/projects/cedar/rev/9409b0c8864f http://hg.mozilla.org/projects/cedar/rev/c04359d5920f Reopening the bug (which shouldn't have been marked Resolved:Fixed yet anyway as it hadn't landed on m-c).
Created attachment 525202 [details] [review] Patch v2 Let's try this again. Currently running this version through a local "make mochitest-1", will push to cedar if that worked. This version clears the "a" cookie in the test that sets it.
Landed again, this time on m-c. Let's hope this will stick. Once I have test results, will request approval again. http://hg.mozilla.org/mozilla-central/rev/61b822ac2d41
Comment on attachment 525202 [details] [review] Patch v2 This is now passing the tests on Tinderbox. Transferring r=bz. Since this affects webapp security I'd like to check this in to the branches too.
Comment on attachment 525202 [details] [review] Patch v2 Approved for 1.9.2.18, a=dveditz clearing request for dead mozilla2.0 branch
Created attachment 531501 [details] [review] merged to 1.9.2 branch
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d5f033c63fce
Per security group discussion, requesting landing on mozilla-2.0.
Comment on attachment 525202 [details] [review] Patch v2 Approved for the mozilla2.0 repository, a=dveditz Cameron: if you request approval on these using flags it'd be easier to find than watching random IRC conversations ;-)
Ah, flag was semi-disabled so you couldn't. Fixed now, request away.
Yes -- thanks for reenabling it. I will properly remark the other bugs (it looks like some were done already).
http://hg.mozilla.org/releases/mozilla-2.0/rev/5c5551f66bd1
Verified fixed based on the updated tests passing for 1.9.2.18.