152694 - cookies with spaces in value get mangled

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect, P2)

Description

[Jeff Rogers]

A cookie with multiple spaces in its value will get returned to the server with
the spaces collapsed.

The legality of spaces in the value isn't entirely clear to me; the value is
indicated to be opaque in some places and a word in other places (notably rfc2109)

In any case, other browsers (netscape 4, ie, lynx) do not collapse the spaces,
but return the cookie exactly as it was sent.

The problem manifests trying to log into PACER websites (e.g.,
http://pacer.cacd.uscourts.gov/) and a client code <32 chars is specified.

Comment 1

[Jeff Rogers]

Attachment: example of repro in javascript

The problem is reproducable using javascript to set cookies, even within a
single page.  The attachment sets and then examines document.cookie and finds
it different.

Comment 2

[Stephen P. Morse]

That's great!  You just demonstrated that an attacker can steal anyone's 
bugzilla cookies.  Your script was able to read my bugzilla login cookie and, if 
it wanted to (which fortunately it didn't) could have transmitted that back to 
your server and then you could impersonate me on a future login.

I guess that's a loophole in bugzilla.  But then again, there's not much damage 
someone can do with a buzilla login. ;-)

Off course this is all off the topic for this bug report.  I can confirm that 
this spacing problem is happening in bugzilla but not in N4, so I'll update it's 
status.

Comment 3

[Stephen P. Morse]

The problem is in the cookie_SetCookieString routine of nsCookies.cpp.  There is 
the following code to separate out the cookie name from the cookie value

  equal = PL_strchr(setCookieHeaderInternal, '=');
  if (equal)
    *equal = '\0';

    nsCAutoString cookieHeader(setCookieHeaderInternal);
    cookieHeader.CompressWhitespace();
    if(equal) {
      CKutil_StrAllocCopy(name_from_header, cookieHeader.get());
      nsCAutoString value(equal+1);
      value.CompressWhitespace();
      CKutil_StrAllocCopy(cookie_from_header, value.get());

I don't recall what problem the CompressWhitespace was solving, but it is this 
call (at least the second instance above) that is the cause of the current bug.

Comment 4

[Stephen P. Morse]

As best as I'm able to determine, there is no reason for that particular call to 
CompressWhitespace().  Therefore attaching a patch that removes it.

Comment 5

[Stephen P. Morse]

Attachment: don't collapse spaces in cookie value

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 90556 [details] [diff] [review]
don't collapse spaces in cookie value

r=bryner

Comment 7

[jag (Peter Annema)]

Comment on attachment 90556 [details] [diff] [review]
don't collapse spaces in cookie value

sr=jag. Was the intent perhaps to strip whitespace from the end?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It looks like the history of that CompressWhitespace is that it first appeared
in nsCookie.cpp, revision 1.78, when valeski converted XP_StripLine to its
nsString equivalent.  XP_StripLine only removed leading and trailing whitespace.
 Perhaps it should instead be replaced by a Trim?  (The XP_StripLine was present
in the original revsion 1.1 of nsCookie.cpp checked in by neeti with the comment
"The Cookie Module: Initial Version" (although it was moved between revision
1.11 and 1.12).)

Comment 9

[Stephen P. Morse]

Let me clarify what dbaron said above, because I was confused by this for a 
moment.

The file is today called nsCookies.cpp.  And if I look back to version 1.1, it 
has the CompressWhiteSpace, which made me think that it was there from day 1 of 
seamonkey.

What I forgot was that the file used to be called nsCookie.cpp (there is an 
nsCookie.cpp today, but that's a completely different file).  And what dbaron 
was referring to was in indeed the old nsCookie.cpp.

Comment 10

[Stephen P. Morse]

dbaron's assessement of what happened is absolutely correct.  Therefore I am 
retracting the previous patch and replacing it with one which indeed trims 
whitespace from both ends but not in the middle.  And this should apply to 
the other occurences in this file of CompressWhiteSpace as well.

Basically this is a regression that was introduced with version 1.78 of the old 
nsCookie.cpp on August 2, 2000.

Comment 11

[Stephen P. Morse]

Attachment: trim whitespace from the ends only

Comment 12

[Bill Law]

Comment on attachment 90898 [details] [diff] [review]
trim whitespace from the ends only

r=law

Comment 13

[jag (Peter Annema)]

Comment on attachment 90898 [details] [diff] [review]
trim whitespace from the ends only

sr=jag

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 90898 [details] [diff] [review]
trim whitespace from the ends only

a=asa (on behalf of drivers) for checkin to the 1.1 trunk.

Comment 15

[Stephen P. Morse]

Checked in on trunk.

Comment 16

[Jaime Rodriguez, Jr.]

marking as nsbeta1+ per verbal OK from navtriage, to evaluate the patch "on its
merits."

tever: tom can your verify this fix on the trunk tomorrow? thanks!

Comment 17

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 18

[Tom Everingham]

checked using supplied test case, cookie with spaces is now returned intact

verified 07/12/02 trunk builds - winNT4, linux rh6, mac osX

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

This has had mozilla1.0.1+ and nsbeta1+ for a month but has not been checked
into the branch.  Please resolve ASAP

Comment 20

[Stephen P. Morse]

It's not been checked into the branch because it never received adt approval.