Closed Bug 643051 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Networking: Cookies, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla5
Tracking Status
firefox5 --- fixed
blocking2.0 --- -
status2.0 --- .x-fixed
blocking1.9.2 --- -
status1.9.2 --- .18-fixed

People

(Reporter: Biesinger, Assigned: Biesinger)

Details

(Keywords: verified1.9.2, Whiteboard: [sg:nse])

Attachments

(2 files, 4 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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+
Attached patch mochitest (obsolete) — Splinter Review
Attached patch final patch for checkin (obsolete) — Splinter Review
Attachment #520373 - Attachment is obsolete: true
Attachment #524724 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 12 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?
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?
Attached patch Patch v2Splinter Review
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
Closed: 12 years ago12 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: ? → -
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
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).
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.