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

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

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)]

Attachment: patch

Comment 2

[Daniel Veditz [:dveditz]]

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 [:bzbarsky]]

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)]

Attachment: mochitest

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: final patch for checkin

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

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)]

http://hg.mozilla.org/projects/cedar/rev/b980d0cf9847

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: test fix

http://hg.mozilla.org/projects/cedar/rev/bf3bdd1d9671

Comment 9

[Jonathan Kew [:jfkthame]]

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)]

Attachment: 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)]

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)]

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]]

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)]

Attachment: merged to 1.9.2 branch

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d5f033c63fce

Comment 16

[Cameron Kaiser [:spectre]]

Per security group discussion, requesting landing on mozilla-2.0.

Comment 17

[Daniel Veditz [:dveditz]]

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]]

Ah, flag was semi-disabled so you couldn't. Fixed now, request away.

Comment 19

[Cameron Kaiser [:spectre]]

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)]

http://hg.mozilla.org/releases/mozilla-2.0/rev/5c5551f66bd1

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Verified fixed based on the updated tests passing for 1.9.2.18.