code review 81680043: net/http: Support commas in cookie values

net/http: Support commas in cookie values

Fixes issue 7243.

[shanemhansen, 2014-03-28 02:50:50]

Hello golang-codereviews@googlegroups.com (cc: nigel.thorne@gmail.com,
rsc@golang.org),

I'd like you to review this change to
https://code.google.com/p/go

[rsc, 2014-03-28 02:54:17]

R=nigeltao

If there's no test, nothing is fixed.

[shanemhansen, 2014-03-28 03:21:39]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com, nigel.thorne@gmail.com, rsc@golang.org),

Please take another look.

[volker.dobler, 2014-03-28 09:21:03]

First: This CL addresses only commas in cookie values
and ignores the other bytes disallowed by RFC 6265
but properly handled by browser: space, backslash
and double quotes. 

Second: This CL looks innocent but allows a Set-Cookie
header of the following form
  Set-Cookie: name=value,
with a trailing comma which is _not_ handled properly
by Safari. Safari 5 gets confused and will combine
a following Set-Cookie into the one with the trailing
comma. This might have security implications e.g. if
the second cookie has different http-only or secure
attributes.

A Set-Cookie Header with a trailing comma should
be sent like
  Set-Cookie: name="value,"
which works on all (tested) browsers (ink. Safari).

We should first discuss if issue 7243 is really
only about (non-leading, non-trailing) commas in
cookie values or if we want to handle the other 
disallowed characters as well and how to do this
on all browsers.

I did some experiments how browsers handle
non-RFC 6265 compliant Set-Cookie headers:
FF 22, IE 10, Chrome 33, Safari 5.1.7 on Win7.

Disallowed characters:
 - Backslash: Works properly on all (tested) browsers.
 - Spaces: Works on all browsers.
     Restriction: A value with leading and/or trailing
     spaces has to be quoted on all browser.
 - Double quotes: Works on all browsers.
     Restrictions: A value with leading and trailing double
     quotes has to be quoted on all browsers.
     Additional restriction for Safari: Quoting is also
     needed if only a leading or only a trailing double
     quote is present.
 - Comma: Works on all browsers.
   Restrictions for Safari: Values with leading or trailing
   comma need to be quoted.
 - Semicolon: Illegal on all browsers.

Handling of control characters < 0x20:
 - Chrome: Cookie is ignored
 - IE: Control byte replaced by underscore 0x5f
 - Safari: Gets confused easily by control characters
 - FF: Allows almost any control bytes

Handling of bytes > 0x7f:
 - IE and Chrome: Allowed.
 - FF: Allowed but only if valid UTF-8.
 - Safari: Gets confused easily.

From this I would suggest:
Allow all bytes except control characters <0x20 and
the semicolon 0x38.
Send all cookie-values in a Set-Cookie header in the
quoted form and document this behavior.
Maybe: Force bytes > 0x7f to be valid UTF-8 encoded
like FF does.

https://codereview.appspot.com/81680043/diff/60001/src/pkg/net/http/cookie.go
File src/pkg/net/http/cookie.go (right):

https://codereview.appspot.com/81680043/diff/60001/src/pkg/net/http/cookie.go...
src/pkg/net/http/cookie.go:358: return isCookieByte(c) || c == ',' || c == ' '
Now this c == ',' is redundant.

[shanemhansen, 2014-03-28 14:48:43]

Thanks for the detailed feedback.

Cookie handling in general is super tricky, I was attempting to stay in a
somewhat safe zone by
using one of the cookie handling checks from chromium where the checks
overlapped with net/http/cookie.go

https://codereview.appspot.com/81680043/diff/60001/src/pkg/net/http/cookie.go...
http://src.chromium.org/viewvc/chrome/trunk/src/net/cookies/parsed_cookie.cc#...

However I'm not positive this is correct, especially since go's cookie
handling is used for server and client. I'm not incredibly nervous about
allowing a server writer to send a bad cookie since that was the impetus
for this change. Also I'm a fan of clients that deal with the occasional
buggy server. It's a maintenance mess, but I think people expect a client's
parsing to work somewhat like firefox and chrome. I certainly draw the line
at causing security regressions.



On Fri, Mar 28, 2014 at 3:21 AM, <dr.volker.dobler@gmail.com> wrote:

> First: This CL addresses only commas in cookie values
> and ignores the other bytes disallowed by RFC 6265
> but properly handled by browser: space, backslash
> and double quotes.
>
> Second: This CL looks innocent but allows a Set-Cookie
> header of the following form
>   Set-Cookie: name=value,
> with a trailing comma which is _not_ handled properly
> by Safari. Safari 5 gets confused and will combine
> a following Set-Cookie into the one with the trailing
> comma. This might have security implications e.g. if
> the second cookie has different http-only or secure
> attributes.
>
> A Set-Cookie Header with a trailing comma should
> be sent like
>   Set-Cookie: name="value,"
> which works on all (tested) browsers (ink. Safari).
>
> We should first discuss if issue 7243 is really
> only about (non-leading, non-trailing) commas in
> cookie values or if we want to handle the other
> disallowed characters as well and how to do this
> on all browsers.
>
> I did some experiments how browsers handle
> non-RFC 6265 compliant Set-Cookie headers:
> FF 22, IE 10, Chrome 33, Safari 5.1.7 on Win7.
>
> Disallowed characters:
>  - Backslash: Works properly on all (tested) browsers.
>  - Spaces: Works on all browsers.
>      Restriction: A value with leading and/or trailing
>      spaces has to be quoted on all browser.
>  - Double quotes: Works on all browsers.
>      Restrictions: A value with leading and trailing double
>      quotes has to be quoted on all browsers.
>      Additional restriction for Safari: Quoting is also
>      needed if only a leading or only a trailing double
>      quote is present.
>  - Comma: Works on all browsers.
>    Restrictions for Safari: Values with leading or trailing
>    comma need to be quoted.
>  - Semicolon: Illegal on all browsers.
>
> Handling of control characters < 0x20:
>  - Chrome: Cookie is ignored
>  - IE: Control byte replaced by underscore 0x5f
>  - Safari: Gets confused easily by control characters
>  - FF: Allows almost any control bytes
>
> Handling of bytes > 0x7f:
>  - IE and Chrome: Allowed.
>  - FF: Allowed but only if valid UTF-8.
>  - Safari: Gets confused easily.
>
> From this I would suggest:
> Allow all bytes except control characters <0x20 and
> the semicolon 0x38.
> Send all cookie-values in a Set-Cookie header in the
> quoted form and document this behavior.
> Maybe: Force bytes > 0x7f to be valid UTF-8 encoded
> like FF does.
>
>
>
>
> https://codereview.appspot.com/81680043/diff/60001/src/
> pkg/net/http/cookie.go
> File src/pkg/net/http/cookie.go (right):
>
> https://codereview.appspot.com/81680043/diff/60001/src/
> pkg/net/http/cookie.go#newcode358
> src/pkg/net/http/cookie.go:358: return isCookieByte(c) || c == ',' || c
> == ' '
> Now this c == ',' is redundant.
>
> https://codereview.appspot.com/81680043/
>

[rsc, 2014-03-28 14:51:47]

+nigeltao

[rsc, 2014-04-22 22:47:28]

R=close

I think CL 86050045 took care of this.