document.cookie should only allow setting one cookie at a time

RESOLVED FIXED in Firefox 5

Status

()

Core
Networking: Cookies
--
major
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Biesinger, Assigned: Biesinger)

Tracking

({verified1.9.2})

Trunk
mozilla5
verified1.9.2
Points:
---

Firefox Tracking Flags

(firefox5 fixed, blocking2.0 -, status2.0 .x-fixed, blocking1.9.2 -, status1.9.2 .18-fixed)

Details

(Whiteboard: [sg:nse])

Attachments

(2 attachments, 4 obsolete attachments)

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] [diff] [review]
patch
Assignee: nobody → cbiesinger
Status: NEW → ASSIGNED
Attachment #520373 - Flags: review?
Attachment #520373 - Flags: review? → review?(jduell.mcbugs)
Whiteboard: [sg:low]
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.
Whiteboard: [sg:low] → [sg:nse]
Comment on attachment 520373 [details] [diff] [review]
patch

r=me

Might be worth creating a mochitest with document.cookie too.
Attachment #520373 - Flags: review?(jduell.mcbugs) → review+
Created attachment 524724 [details] [diff] [review]
mochitest
Created attachment 524733 [details] [diff] [review]
final patch for checkin
Attachment #520373 - Attachment is obsolete: true
Attachment #524724 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [sg:nse] → [sg:nse] fixed-in-cedar
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.
Attachment #524733 - Flags: approval2.0?
Attachment #524733 - Flags: approval1.9.2.18?
http://hg.mozilla.org/projects/cedar/rev/b980d0cf9847
Created attachment 524774 [details] [diff] [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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [sg:nse] fixed-in-cedar → [sg:nse]
Attachment #524733 - Flags: approval2.0?
Attachment #524733 - Flags: approval1.9.2.18?
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.
Attachment #524733 - Attachment is obsolete: true
Attachment #524774 - Attachment is obsolete: true
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
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → FIXED
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.
Attachment #525202 - Flags: review+
Attachment #525202 - Flags: approval2.0?
Attachment #525202 - Flags: approval1.9.1.20?
Attachment #525202 - Flags: approval1.9.1.20? → approval1.9.2.18?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Group: core-security
blocking1.9.2: ? → -
blocking2.0: ? → -
status1.9.2: --- → wanted
status2.0: --- → wanted
Comment on attachment 525202 [details] [diff] [review]
Patch v2

Approved for 1.9.2.18, a=dveditz

clearing request for dead mozilla2.0 branch
Attachment #525202 - Flags: approval2.0?
Attachment #525202 - Flags: approval1.9.2.18?
Attachment #525202 - Flags: approval1.9.2.18+
Target Milestone: --- → mozilla5
Created attachment 531501 [details] [diff] [review]
merged to 1.9.2 branch
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d5f033c63fce
status1.9.2: wanted → .18-fixed
Per security group discussion, requesting landing on mozilla-2.0.
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 ;-)
Attachment #525202 - Flags: approval2.0+
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
status2.0: wanted → .x-fixed
status-firefox5: --- → fixed
Verified fixed based on the updated tests passing for 1.9.2.18.
Keywords: verified1.9.2
You need to log in before you can comment on or make changes to this bug.