Last Comment Bug 643051 - document.cookie should only allow setting one cookie at a time
: document.cookie should only allow setting one cookie at a time
Status: RESOLVED FIXED
[sg:nse]
: verified1.9.2
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: Trunk
: All All
: -- major with 1 vote (vote)
: mozilla5
Assigned To: Christian :Biesinger (don't email me, ping me on IRC)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-18 17:21 PDT by Christian :Biesinger (don't email me, ping me on IRC)
Modified: 2011-06-16 15:15 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
-
.x-fixed
-
.18-fixed


Attachments
patch (1.62 KB, patch)
2011-03-18 18:12 PDT, Christian :Biesinger (don't email me, ping me on IRC)
bzbarsky: review+
Details | Diff | Review
mochitest (1.83 KB, patch)
2011-04-08 13:47 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details | Diff | Review
final patch for checkin (3.45 KB, patch)
2011-04-08 14:05 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details | Diff | Review
test fix (1.57 KB, patch)
2011-04-08 15:43 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details | Diff | Review
Patch v2 (5.12 KB, patch)
2011-04-11 15:54 PDT, Christian :Biesinger (don't email me, ping me on IRC)
cbiesinger: review+
dveditz: approval2.0+
dveditz: approval1.9.2.18+
Details | Diff | Review
merged to 1.9.2 branch (5.12 KB, patch)
2011-05-10 16:49 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details | Diff | Review

Description Christian :Biesinger (don't email me, ping me on IRC) 2011-03-18 17:21:05 PDT
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.
Comment 1 Christian :Biesinger (don't email me, ping me on IRC) 2011-03-18 18:12:49 PDT
Created attachment 520373 [details] [diff] [review]
patch
Comment 2 Daniel Veditz [:dveditz] 2011-03-24 13:35:43 PDT
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 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-28 18:56:32 PDT
Comment on attachment 520373 [details] [diff] [review]
patch

r=me

Might be worth creating a mochitest with document.cookie too.
Comment 4 Christian :Biesinger (don't email me, ping me on IRC) 2011-04-08 13:47:22 PDT
Created attachment 524724 [details] [diff] [review]
mochitest
Comment 5 Christian :Biesinger (don't email me, ping me on IRC) 2011-04-08 14:05:42 PDT
Created attachment 524733 [details] [diff] [review]
final patch for checkin
Comment 6 Christian :Biesinger (don't email me, ping me on IRC) 2011-04-08 14:26:33 PDT
Comment on attachment 524733 [details] [diff] [review]
final patch for checkin

transfering r=bz

I would like to land this on the ff4 and 3.6 branches too, if possible.
Comment 7 Christian :Biesinger (don't email me, ping me on IRC) 2011-04-08 14:26:46 PDT
http://hg.mozilla.org/projects/cedar/rev/b980d0cf9847
Comment 8 Christian :Biesinger (don't email me, ping me on IRC) 2011-04-08 15:43:59 PDT
Created attachment 524774 [details] [diff] [review]
test fix

http://hg.mozilla.org/projects/cedar/rev/bf3bdd1d9671
Comment 9 Jonathan Kew (:jfkthame) 2011-04-08 20:36:42 PDT
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).
Comment 10 Christian :Biesinger (don't email me, ping me on IRC) 2011-04-11 15:54:38 PDT
Created attachment 525202 [details] [diff] [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.
Comment 11 Christian :Biesinger (don't email me, ping me on IRC) 2011-04-11 16:55:11 PDT
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 12 Christian :Biesinger (don't email me, ping me on IRC) 2011-04-11 19:34:22 PDT
Comment on attachment 525202 [details] [diff] [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 13 Daniel Veditz [:dveditz] 2011-05-10 16:37:28 PDT
Comment on attachment 525202 [details] [diff] [review]
Patch v2

Approved for 1.9.2.18, a=dveditz

clearing request for dead mozilla2.0 branch
Comment 14 Christian :Biesinger (don't email me, ping me on IRC) 2011-05-10 16:49:58 PDT
Created attachment 531501 [details] [diff] [review]
merged to 1.9.2 branch
Comment 15 Christian :Biesinger (don't email me, ping me on IRC) 2011-05-10 17:21:10 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d5f033c63fce
Comment 16 Cameron Kaiser [:spectre] 2011-05-21 16:31:24 PDT
Per security group discussion, requesting landing on mozilla-2.0.
Comment 17 Daniel Veditz [:dveditz] 2011-05-21 21:37:22 PDT
Comment on attachment 525202 [details] [diff] [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 ;-)
Comment 18 Daniel Veditz [:dveditz] 2011-05-21 21:42:42 PDT
Ah, flag was semi-disabled so you couldn't. Fixed now, request away.
Comment 19 Cameron Kaiser [:spectre] 2011-05-22 09:00:39 PDT
Yes -- thanks for reenabling it. I will properly remark the other bugs (it looks like some were done already).
Comment 20 Christian :Biesinger (don't email me, ping me on IRC) 2011-05-23 11:10:26 PDT
http://hg.mozilla.org/releases/mozilla-2.0/rev/5c5551f66bd1
Comment 21 Al Billings [:abillings] 2011-06-16 15:15:42 PDT
Verified fixed based on the updated tests passing for 1.9.2.18.

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