You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by jb...@apache.org on 2013/12/23 20:15:36 UTC
svn commit: r1553187 - in /tomcat/trunk:
java/org/apache/tomcat/util/http/Cookies.java
test/org/apache/tomcat/util/http/TestCookies.java webapps/docs/changelog.xml
Author: jboynes
Date: Mon Dec 23 19:15:35 2013
New Revision: 1553187
URL: http://svn.apache.org/r1553187
Log:
fix #55917 by allowing 8-bit ISO-8859-1 characters in V0 cookie values
Modified:
tomcat/trunk/java/org/apache/tomcat/util/http/Cookies.java
tomcat/trunk/test/org/apache/tomcat/util/http/TestCookies.java
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/org/apache/tomcat/util/http/Cookies.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/http/Cookies.java?rev=1553187&r1=1553186&r2=1553187&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/http/Cookies.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/http/Cookies.java Mon Dec 23 19:15:35 2013
@@ -508,14 +508,7 @@ public final class Cookies {
private static final int getTokenEndPosition(byte bytes[], int off, int end,
int version, boolean isName){
int pos = off;
- while (pos < end &&
- (!CookieSupport.isHttpSeparator((char)bytes[pos]) ||
- version == 0 &&
- CookieSupport.ALLOW_HTTP_SEPARATORS_IN_V0 &&
- bytes[pos] != '=' &&
- !CookieSupport.isV0Separator((char)bytes[pos]) ||
- !isName && bytes[pos] == '=' &&
- CookieSupport.ALLOW_EQUALS_IN_VALUE)) {
+ while (pos < end && allowInToken(bytes[pos], version, isName)) {
pos++;
}
@@ -525,6 +518,34 @@ public final class Cookies {
return pos;
}
+ private static boolean allowInToken(byte b, int version, boolean isName) {
+ // byte is signed so cast into a positive int for comparisons
+ int octet = ((int)b) & 0xff;
+
+ // disallow all controls
+ if (octet < 0x20 && octet != 0x09 || octet >= 0x7f && octet < 0xa0) {
+ throw new IllegalArgumentException(
+ "Control character in cookie value or attribute.");
+ }
+
+ // values 0xa0-0xff are allowed in V0 values, otherwise disallow
+ if (octet >= 0x80) {
+ if (isName || version != 0) {
+ throw new IllegalArgumentException(
+ "Control character in cookie value or attribute.");
+ }
+ return true;
+ }
+
+ return !CookieSupport.isHttpSeparator((char) b) ||
+ version == 0 &&
+ CookieSupport.ALLOW_HTTP_SEPARATORS_IN_V0 &&
+ b != '=' &&
+ !CookieSupport.isV0Separator((char) b) ||
+ !isName && b == '=' &&
+ CookieSupport.ALLOW_EQUALS_IN_VALUE;
+ }
+
/**
* Given a starting position after an initial quote character, this gets
* the position of the end quote. This escapes anything after a '\' char
Modified: tomcat/trunk/test/org/apache/tomcat/util/http/TestCookies.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/tomcat/util/http/TestCookies.java?rev=1553187&r1=1553186&r2=1553187&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/tomcat/util/http/TestCookies.java (original)
+++ tomcat/trunk/test/org/apache/tomcat/util/http/TestCookies.java Mon Dec 23 19:15:35 2013
@@ -17,9 +17,113 @@
package org.apache.tomcat.util.http;
+import java.nio.charset.StandardCharsets;
+
+import javax.servlet.http.Cookie;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Ignore;
import org.junit.Test;
public class TestCookies {
+ private Cookies cookies;
+
+ @Before
+ public void init() {
+ this.cookies = new Cookies(null);
+ }
+
+ @Test
+ public void skipJsonInV0Value() {
+ process("bad={\"v\":1,\"x\":2}; a=b");
+ expect(makeCookie("a", "b", 0));
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void disallow8bitInName() {
+ process("f\u00f6o=bar");
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void disallowControlInName() {
+ process("f\010o=bar");
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void disallow8BitControlInName() {
+ process("f\210o=bar");
+ }
+
+ @Test
+ public void allow8BitInV0Value() {
+ process("foo=b\u00e1r");
+ expect(makeCookie("foo", "b\u00e1r", 0));
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void disallow8bitInV1UnquotedValue() {
+ process("$Version=1; foo=b\u00e1r");
+ }
+
+ @Test
+ public void allow8bitInV1QuotedValue() {
+ process("$Version=1; foo=\"b\u00e1r\"");
+ expect(makeCookie("foo", "b\u00e1r", 1));
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void disallowControlInV0Value() {
+ process("foo=b\010r");
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void disallow8BitControlInV0Value() {
+ process("foo=b\210r");
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void disallowControlInV1UnquotedValue() {
+ process("$Version=1; foo=b\010r");
+ }
+
+ @Ignore
+ @Test(expected = IllegalArgumentException.class)
+ public void disallowControlInV1QuotedValue() {
+ process("$Version=1; foo=\"b\010r\"");
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void disallow8BitControlInV1UnquotedValue() {
+ process("$Version=1; foo=b\210r");
+ }
+
+ @Ignore
+ @Test
+ public void allow8BitControlInV1QuotedValue() {
+ process("$Version=1; foo=\"b\210r\"");
+ expect(makeCookie("foo", "b\210r", 1));
+ }
+
+ private void process(String header) {
+ byte[] bytes = header.getBytes(StandardCharsets.ISO_8859_1);
+ cookies.processCookieHeader(bytes, 0, bytes.length);
+ }
+
+ private void expect(Cookie... expected) {
+ Assert.assertEquals(expected.length, cookies.getCookieCount());
+ for (int i = 0; i < expected.length; i++) {
+ ServerCookie actual = cookies.getCookie(i);
+ Assert.assertEquals(expected[i].getName(), actual.getName().toString());
+ Assert.assertEquals(expected[i].getValue(), actual.getValue().toString());
+ }
+ }
+
+ private static Cookie makeCookie(String name, String value, int version) {
+ Cookie cookie = new Cookie(name, value);
+ cookie.setVersion(version);
+ return cookie;
+ }
@Test
public void testCookies() throws Exception {
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1553187&r1=1553186&r2=1553187&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Mon Dec 23 19:15:35 2013
@@ -225,6 +225,10 @@
Change the default URIEncoding for all connectors from ISO-8859-1 to
UTF-8. (markt)
</update>
+ <scode>
+ <bug>55917</bug>: Allow ISO-8859-1 characters 0xA0-0xFF in V0 cookie
+ values (jboynes).
+ </scode>
</changelog>
</subsection>
<subsection name="Jasper">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: 8-bit text in cookie values
Posted by Jeremy Boynes <jb...@apache.org>.
Adding more confusion to the pile, HTML5[1] now specifies that JavaScript can set Unicode characters through document.cookie and that they must be encoded as UTF-8 in the header. Quick testing with Chrome shows it does just that (i.e. U+00E1 is sent as 0xC3 0xA1). If client and server-side application code is going to interoperate then we would need to accept them in a Cookie header and allow them to be sent in a Set-Cookie header. However, this is ambiguous when compared to Netscape and its implicit assumption of ISO-8859-1.
[1] http://www.w3.org/html/wg/drafts/html/master/single-page.html#cookie
On Jan 1, 2014, at 10:18 AM, Jeremy Boynes <jb...@apache.org> wrote:
> On Jan 1, 2014, at 8:59 AM, Mark Thomas <ma...@apache.org> wrote:
>
>> Signed PGP part
>> On 26/12/2013 19:23, Jeremy Boynes wrote:
>>> On Dec 26, 2013, at 2:47 AM, Mark Thomas <ma...@apache.org> wrote:
>>>
>>> Focusing on the 8-bit issue address by the patch, leaving the other
>>> RFC6265 thread for broader discussion ...
>>>
>>>>> The change only allows these characters in values if version ==
>>>>> 0 where Netscape’s rather than RFC2109’s syntax applies (per
>>>>> the Servlet spec). The Netscape spec is vague in that it does
>>>>> not define “OPAQUE_STRING" at all and defines “VALUE” as
>>>>> containing equally undefined “characters” although
>>>>> historically[1] those have been taken to be OCTETs as permitted
>>>>> by RFC2616’s “*TEXT” variant of “field-content.” The change
>>>>> will continue to reject these characters in names and in
>>>>> unquoted values when version != 0 (RFC2109’s “word" rule)
>>>>>
>>>>> [1] based on comments by Fielding et al. on http-state and
>>>>> what I’ve seen in the wild
>>>>
>>>> Can you provide references for [1]?
>>>
>>> This is the mail in the run up to RFC6265 that triggered the
>>> discussion:
>>> http://www.ietf.org/mail-archive/web/http-state/current/msg01232.html
>>
>> Thanks
>>>
>> for that reference. What a complete mess. RFC6265 really
>> dropped the ball on this. The grammar for cookie-value is a disaster.
>> So far the issues include:
>> - no support for 0x80 to 0xFF
>> - no support for \" sequences
>> - no support for using whitespace, comma, semi-colon, backslash
>>
>> I was beginning to think that factoring out the cookie generation /
>> parsing and then providing different implementations (one for Netscape
>> + RFC2109 - roughly what we have now with a few fixes, one for RFC6265
>> and maybe one very relaxed) would be the way to go. Having looked at
>> the first issue that plan already looks like it needs a re-think.
>>
>> I'm still hoping that by documenting all the various issues in one
>> place we will be able to come up with a solution that both addresses
>> all the issues you have raised and is better than the handful of
>> system properties we have currently.
>
> I think they did a reasonable job given the mess cookies are in the wild today. They summarize this in the preamble:
>> The recommendations for cookie generation provided in Section 4 represent a preferred subset of current server behavior, and even the more liberal cookie processing algorithm provided in Section 5 does not recommend all of the syntactic and semantic variations in use today.
>
> Section 4 recommends guidelines for servers generating cookies. I interpret that as being “if you follow these guidelines, you have a good chance of actually getting back the value you tried to set.” The rules above (no 8-bit, no escaping, no Netscape delimiters) reflect that principle. A server application can step outside those guidelines but "thar ther be dragons."
>
> —
> Jeremy
Re: 8-bit text in cookie values
Posted by Jeremy Boynes <jb...@apache.org>.
On Jan 1, 2014, at 8:59 AM, Mark Thomas <ma...@apache.org> wrote:
> Signed PGP part
> On 26/12/2013 19:23, Jeremy Boynes wrote:
> > On Dec 26, 2013, at 2:47 AM, Mark Thomas <ma...@apache.org> wrote:
> >
> > Focusing on the 8-bit issue address by the patch, leaving the other
> > RFC6265 thread for broader discussion ...
> >
> >>> The change only allows these characters in values if version ==
> >>> 0 where Netscape’s rather than RFC2109’s syntax applies (per
> >>> the Servlet spec). The Netscape spec is vague in that it does
> >>> not define “OPAQUE_STRING" at all and defines “VALUE” as
> >>> containing equally undefined “characters” although
> >>> historically[1] those have been taken to be OCTETs as permitted
> >>> by RFC2616’s “*TEXT” variant of “field-content.” The change
> >>> will continue to reject these characters in names and in
> >>> unquoted values when version != 0 (RFC2109’s “word" rule)
> >>>
> >>> [1] based on comments by Fielding et al. on http-state and
> >>> what I’ve seen in the wild
> >>
> >> Can you provide references for [1]?
> >
> > This is the mail in the run up to RFC6265 that triggered the
> > discussion:
> > http://www.ietf.org/mail-archive/web/http-state/current/msg01232.html
>
> Thanks
> >
> for that reference. What a complete mess. RFC6265 really
> dropped the ball on this. The grammar for cookie-value is a disaster.
> So far the issues include:
> - no support for 0x80 to 0xFF
> - no support for \" sequences
> - no support for using whitespace, comma, semi-colon, backslash
>
> I was beginning to think that factoring out the cookie generation /
> parsing and then providing different implementations (one for Netscape
> + RFC2109 - roughly what we have now with a few fixes, one for RFC6265
> and maybe one very relaxed) would be the way to go. Having looked at
> the first issue that plan already looks like it needs a re-think.
>
> I'm still hoping that by documenting all the various issues in one
> place we will be able to come up with a solution that both addresses
> all the issues you have raised and is better than the handful of
> system properties we have currently.
I think they did a reasonable job given the mess cookies are in the wild today. They summarize this in the preamble:
> The recommendations for cookie generation provided in Section 4 represent a preferred subset of current server behavior, and even the more liberal cookie processing algorithm provided in Section 5 does not recommend all of the syntactic and semantic variations in use today.
Section 4 recommends guidelines for servers generating cookies. I interpret that as being “if you follow these guidelines, you have a good chance of actually getting back the value you tried to set.” The rules above (no 8-bit, no escaping, no Netscape delimiters) reflect that principle. A server application can step outside those guidelines but "thar ther be dragons."
—
Jeremy
Re: 8-bit text in cookie values
Posted by Mark Thomas <ma...@apache.org>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 26/12/2013 19:23, Jeremy Boynes wrote:
> On Dec 26, 2013, at 2:47 AM, Mark Thomas <ma...@apache.org> wrote:
>
> Focusing on the 8-bit issue address by the patch, leaving the other
> RFC6265 thread for broader discussion ...
>
>>> The change only allows these characters in values if version ==
>>> 0 where Netscape’s rather than RFC2109’s syntax applies (per
>>> the Servlet spec). The Netscape spec is vague in that it does
>>> not define “OPAQUE_STRING" at all and defines “VALUE” as
>>> containing equally undefined “characters” although
>>> historically[1] those have been taken to be OCTETs as permitted
>>> by RFC2616’s “*TEXT” variant of “field-content.” The change
>>> will continue to reject these characters in names and in
>>> unquoted values when version != 0 (RFC2109’s “word" rule)
>>>
>>> [1] based on comments by Fielding et al. on http-state and
>>> what I’ve seen in the wild
>>
>> Can you provide references for [1]?
>
> This is the mail in the run up to RFC6265 that triggered the
> discussion:
> http://www.ietf.org/mail-archive/web/http-state/current/msg01232.html
Thanks
>
for that reference. What a complete mess. RFC6265 really
dropped the ball on this. The grammar for cookie-value is a disaster.
So far the issues include:
- - no support for 0x80 to 0xFF
- - no support for \" sequences
- - no support for using whitespace, comma, semi-colon, backslash
I was beginning to think that factoring out the cookie generation /
parsing and then providing different implementations (one for Netscape
+ RFC2109 - roughly what we have now with a few fixes, one for RFC6265
and maybe one very relaxed) would be the way to go. Having looked at
the first issue that plan already looks like it needs a re-think.
I'm still hoping that by documenting all the various issues in one
place we will be able to come up with a solution that both addresses
all the issues you have raised and is better than the handful of
system properties we have currently.
Mark
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJSxEmLAAoJEBDAHFovYFnnyVcP+wfe+dxLyTEG856JW2NcyrBY
j3iszFdsriJHqGnFOI3YWzKflF5h72oZjBL5cKQ5MozlF2Ycx+UHsPu2p6f1wpy8
d2T2frCwaXIULpqMdsMVMIEMZbVjwWdB9zYKKZAxZm1uhHUhqNyzsIG3rs/dTJrP
Ytt9/hJCKEYEgFCNFCmDoCj4tWCkIFz/bdYb3D7kLe2AP/SF7rUrgkJgW9bF3/y+
BMZYUXIgBj1NZ0Ts9C7K/k8ngiWgpsCXiJos2b0lMU1ga9agadTTJU+2EJgrd9m9
NjVXlBMIraEbPp+Gj2WHPBuVMRhDKwTvyg7AnR0B1toEkqEK986YJU5wzOUHp/em
KW8M81oCY6t+JdvVZ48rAjuFBsj8DQVCyjIOBUNYZ1e/oS68Wjt84c2/NZfPUtVr
iCEWEgeUpb7fTwCQezn6+FdNu1urnuouaw/4szkRPruQKCBbh/ngLZ3PChuttozR
QpePdcXIyG0XRSIB682UGyuZoUWFQQ3Ug67sC6rb9yKu3oOlaMg6Ii32UulGUczA
SfoNIeQj2uz9pfqA79PqDY9Qkg7GcqvDQl7WKDb8tJ4Of+NAvh7affcm0Nvf+ldt
0hezWjhlhnSA9dowycSe7Z20OM+dWFXCwl3czMH0Ick4JX+QeqT8z9TDYKtDMYpq
EXHhPslORjxfHCf4zNQ0
=gHjq
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: 8-bit text in cookie values
Posted by Jeremy Boynes <jb...@apache.org>.
On Dec 26, 2013, at 2:47 AM, Mark Thomas <ma...@apache.org> wrote:
Focusing on the 8-bit issue address by the patch, leaving the other RFC6265 thread for broader discussion ...
>> The change only allows these characters in values if version == 0
>> where Netscape’s rather than RFC2109’s syntax applies (per the
>> Servlet spec). The Netscape spec is vague in that it does not
>> define “OPAQUE_STRING" at all and defines “VALUE” as containing
>> equally undefined “characters” although historically[1] those have
>> been taken to be OCTETs as permitted by RFC2616’s “*TEXT” variant
>> of “field-content.” The change will continue to reject these
>> characters in names and in unquoted values when version != 0
>> (RFC2109’s “word" rule)
>>
>> [1] based on comments by Fielding et al. on http-state and what
>> I’ve seen in the wild
>
> Can you provide references for [1]?
This is the mail in the run up to RFC6265 that triggered the discussion:
http://www.ietf.org/mail-archive/web/http-state/current/msg01232.html
The relevant bit was:
> Changing the ABNF
> to include base64 does not do that -- it is just another
> fantasy production that differs from all prior specs of
> the cookie algorithm. Changing it to
>
> cookie-value = %x21-2B / %x2D-3A / %x3C-7E / %x80-FF
>
> or just the minimum
>
> cookie-value = %x21-2B / %x2D-3A / %x3C-7E
>
> returns the definition to the original Netscape spec (at
> least in the first case), reflects how they are implemented
> on the Internet, and eliminates this artificial distinction
> between the server and user agent requirements.
with the observation that the rule including %x80-ff was the one matching the Netscape spec. The RFC6265 editor actually chose the latter production which led to the following exchange
http://www.ietf.org/mail-archive/web/http-state/current/msg01234.html
http://www.ietf.org/mail-archive/web/http-state/current/msg01236.html
asserting that the support for 8-bit characters implied by *TEXT was implicit in the original Netscape spec.
In this message:
http://www.ietf.org/mail-archive/web/http-state/current/msg01207.html
Roy asserts that the
cookie-value = %x21-2B / %x2D-3A / %x3C-7E / %x80-FF
production would be needed to support cookies currently in the wild, including the issue with the __utmz cookie that I’ve seen.
Further discussion resulted in the final production:
cookie-value = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
cookie-octet = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
; US-ASCII characters excluding CTLs,
; whitespace DQUOTE, comma, semicolon,
; and backslash
on the basis that setting headers with the top bit set was deemed a bad idea by httpbis (I don’t have a reference for that). It was noted though that conformance to this was qualified by "Servers SHOULD NOT send Set-Cookie headers that fail to conform to the following grammar” which discourages 8-bit values but still allows them to be sent and means that parsers receiving a cookie value need to be prepared to handle them.
Given cookies with these values may be set by other servers in the domain and are sent by user agents, failing hard as we do now prevents the application handling the request at all. The patch tolerates those characters and lets them through to the application. I don’t know of any security issue there given they are being decoded as ISO-8859-1 rather than UTF-8. I believe it’s backwards compatible in that the consequence to the application is that it will now see the request with a cookie that it either expects or that it would be ignoring anyway (on the basis that the cookie would be present if it didn’t have an 8-bit character).
The patch does not change the generation behaviour so any attempt to set a V0 cookie value containing one of these characters will still cause an IAE from HttpServletResponse#addCookie().
Cheers
Jeremy
Re: 8-bit text in cookie values, was: svn commit: r1553187
Posted by Mark Thomas <ma...@apache.org>.
On 24/12/2013 18:06, Jeremy Boynes wrote:
> On Dec 24, 2013, at 2:29 AM, Mark Thomas <ma...@apache.org> wrote:
>
>> On 23/12/2013 19:15, jboynes@apache.org wrote:
>>> Author: jboynes Date: Mon Dec 23 19:15:35 2013 New Revision:
>>> 1553187
>>>
>>> URL: http://svn.apache.org/r1553187 Log: fix #55917 by allowing
>>> 8-bit ISO-8859-1 characters in V0 cookie values
>>
>> -1 (veto)
>>
>> The Tomcat community has (to date) always taken the view that
>> cookie headers should be restricted (as per RFC2616 section 4.2)
>> to "combinations of token, separators, and quoted-string".
>>
>> That does not permit 0x80 to 0xFF in tokens.
>
> I thought the V2 patch had addressed your concern about limiting
> cookie names to tokens so went ahead and applied it.
I thought your proposal was to fix the issue with not allowing 0x80 to
0xFF in quoted strings in V1 cookies. That is a clear bug and I can't
see any issues in fixing it. (It probably needs fixing for setting and
parsing cookies.)
> RFC2616 4.2 defines “message-header” as message-header = field-name
> ":" [ field-value ] field-name = token field-value = *(
> field-content | LWS ) field-content = <the OCTETs making up the
> field-value and consisting of either *TEXT or combinations of
> token, separators, and quoted-string>
>
> where TEXT is defined in 2.2 as TEXT = <any OCTET except
> CTLs, but including LWS>
>
> The change only allows these characters in values if version == 0
> where Netscape’s rather than RFC2109’s syntax applies (per the
> Servlet spec). The Netscape spec is vague in that it does not
> define “OPAQUE_STRING" at all and defines “VALUE” as containing
> equally undefined “characters” although historically[1] those have
> been taken to be OCTETs as permitted by RFC2616’s “*TEXT” variant
> of “field-content.” The change will continue to reject these
> characters in names and in unquoted values when version != 0
> (RFC2109’s “word" rule)
This is the sort of ambiguity that makes me nervous. I'll expand on
that at the end of this e-mail.
> I don’t want to proliferate them but would it help to add another
> system property gating this behaviour? Perhaps with 3 values:
> “reject” (the default, causing an IAE as now), “skip” (logged but
> not returned to the application), and “allow” (returned to the
> application).
I'd prefer to avoid yet another system property if at all possible.
The only reasons not to do this are security or backwards
compatibility. If security is an issue it should never happen.
Backwards compatibility should be able to be handled with a Context
option rather than a system property.
> Thanks Jeremy
>
> [1] based on comments by Fielding et al. on http-state and what
> I’ve seen in the wild
Can you provide references for [1]?
Security concerns
The security issues around cookies have usually involved something
along the following lines:
- target application sets a cookie
- malicious application sets a cookie
- client returns both cookies to Tomcat
- Tomcat parses cookie header
- something, somewhere goes wrong and some/all of the cookie from the
target app is exposed to the malicious application
The various things that (could) have gone wrong:
a) Tomcat doesn't generate the Set-Cookie header correctly which
causes the client to merge the cookies
b) The client mis-handles the cookies causing them to merge
c) Tomcat doesn't parse the cookie header correctly and ends up
merging the values
To prevent a) we need to ensure that the Set-Cookie header we generate
is specification compliant. This is relatively simple.
To prevent b) we need to ensure that the Set-Cookie header is
specification compliant *and* takes account of known issues with
Set-Cookie handling by clients. This is not so simple since the known
issues often mean sending a header that is not specification compliant.
To prevent c) we need to ensure that our Cookie header parsing is
specification compliant and that and relaxation we allow does not
permit a malicious application from manipulating how the header is
parsed. This is primarily why, currently, an IAE is thrown. If the
cookie header is invalid then throwing an IAE ensures that no mischief
can take place.
A significant complicating factor is the ambiguity of the Netscape
specification. The security problems tend to occur when client and
server view one or both of the cookie headers differently and with V0
cookies that is very likely.
Random information
This has been at the back of my mind and here seems like as good a
place as any to document it.
With the various parts of the Servlet API that give direct(ish) access
to HTTP headers (setHeader etc.) the Tomcat developers have always
taken the view that:
a) Tomcat will not blindly use the value directly but validate it and
(where possible / necessary) escape it so it can be used
b) Applications should be able to use methods symmetrically. i.e. If
they call setHeader() with a given value and if that header is echoed
by the client then calling getHeader() should return exactly the same
value. The same applies to cookie values. This impacts how quoting,
escaping, unquoting and unescaping is handled for cookies.
Way forward
There are so many individual issues here that it is hard to keep track
of everything. Worse, potential solutions to some issues impact on
other issues. I'm struggling to figure out how we might best organise
this discussion so that we can reach some decisions on how to change
the cookie parsing and document those for future reference.
I'm wondering if a wiki page or set of pages might help. Use the wiki
to document current thinking, discuss things on list updating the wiki
as we go and, when we are happy, transfer the docs from the wiki to
the Tomcat docs ending up with a cookie equivalent of this page:
http://tomcat.apache.org/tomcat-8.0-doc/config/automatic-deployment.html
To be clear, I suspect that my veto for r1553187 will be withdrawn and
the patch (or something close to it) applied but given the past
experience of the problems that have accompanied any change to the
cookie parsing I think we need to think about this and discuss it a
lot more first.
Thoughts?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
8-bit text in cookie values, was: svn commit: r1553187
Posted by Jeremy Boynes <jb...@apache.org>.
On Dec 24, 2013, at 2:29 AM, Mark Thomas <ma...@apache.org> wrote:
> On 23/12/2013 19:15, jboynes@apache.org wrote:
>> Author: jboynes
>> Date: Mon Dec 23 19:15:35 2013
>> New Revision: 1553187
>>
>> URL: http://svn.apache.org/r1553187
>> Log:
>> fix #55917 by allowing 8-bit ISO-8859-1 characters in V0 cookie values
>
> -1 (veto)
>
> The Tomcat community has (to date) always taken the view that cookie
> headers should be restricted (as per RFC2616 section 4.2) to
> "combinations of token, separators, and quoted-string".
>
> That does not permit 0x80 to 0xFF in tokens.
I thought the V2 patch had addressed your concern about limiting cookie names to tokens so went ahead and applied it.
RFC2616 4.2 defines “message-header” as
message-header = field-name ":" [ field-value ]
field-name = token
field-value = *( field-content | LWS )
field-content = <the OCTETs making up the field-value
and consisting of either *TEXT or combinations
of token, separators, and quoted-string>
where TEXT is defined in 2.2 as
TEXT = <any OCTET except CTLs,
but including LWS>
The change only allows these characters in values if version == 0 where Netscape’s rather than RFC2109’s syntax applies (per the Servlet spec). The Netscape spec is vague in that it does not define “OPAQUE_STRING" at all and defines “VALUE” as containing equally undefined “characters” although historically[1] those have been taken to be OCTETs as permitted by RFC2616’s “*TEXT” variant of “field-content.” The change will continue to reject these characters in names and in unquoted values when version != 0 (RFC2109’s “word" rule)
I don’t want to proliferate them but would it help to add another system property gating this behaviour? Perhaps with 3 values: “reject” (the default, causing an IAE as now), “skip” (logged but not returned to the application), and “allow” (returned to the application).
Thanks
Jeremy
[1] based on comments by Fielding et al. on http-state and what I’ve seen in the wild
Re: svn commit: r1553187 - in /tomcat/trunk: java/org/apache/tomcat/util/http/Cookies.java
test/org/apache/tomcat/util/http/TestCookies.java webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 23/12/2013 19:15, jboynes@apache.org wrote:
> Author: jboynes
> Date: Mon Dec 23 19:15:35 2013
> New Revision: 1553187
>
> URL: http://svn.apache.org/r1553187
> Log:
> fix #55917 by allowing 8-bit ISO-8859-1 characters in V0 cookie values
-1 (veto)
The Tomcat community has (to date) always taken the view that cookie
headers should be restricted (as per RFC2616 section 4.2) to
"combinations of token, separators, and quoted-string".
That does not permit 0x80 to 0xFF in tokens.
Mark
> Modified:
> tomcat/trunk/java/org/apache/tomcat/util/http/Cookies.java
> tomcat/trunk/test/org/apache/tomcat/util/http/TestCookies.java
> tomcat/trunk/webapps/docs/changelog.xml
>
> Modified: tomcat/trunk/java/org/apache/tomcat/util/http/Cookies.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/http/Cookies.java?rev=1553187&r1=1553186&r2=1553187&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/http/Cookies.java (original)
> +++ tomcat/trunk/java/org/apache/tomcat/util/http/Cookies.java Mon Dec 23 19:15:35 2013
> @@ -508,14 +508,7 @@ public final class Cookies {
> private static final int getTokenEndPosition(byte bytes[], int off, int end,
> int version, boolean isName){
> int pos = off;
> - while (pos < end &&
> - (!CookieSupport.isHttpSeparator((char)bytes[pos]) ||
> - version == 0 &&
> - CookieSupport.ALLOW_HTTP_SEPARATORS_IN_V0 &&
> - bytes[pos] != '=' &&
> - !CookieSupport.isV0Separator((char)bytes[pos]) ||
> - !isName && bytes[pos] == '=' &&
> - CookieSupport.ALLOW_EQUALS_IN_VALUE)) {
> + while (pos < end && allowInToken(bytes[pos], version, isName)) {
> pos++;
> }
>
> @@ -525,6 +518,34 @@ public final class Cookies {
> return pos;
> }
>
> + private static boolean allowInToken(byte b, int version, boolean isName) {
> + // byte is signed so cast into a positive int for comparisons
> + int octet = ((int)b) & 0xff;
> +
> + // disallow all controls
> + if (octet < 0x20 && octet != 0x09 || octet >= 0x7f && octet < 0xa0) {
> + throw new IllegalArgumentException(
> + "Control character in cookie value or attribute.");
> + }
> +
> + // values 0xa0-0xff are allowed in V0 values, otherwise disallow
> + if (octet >= 0x80) {
> + if (isName || version != 0) {
> + throw new IllegalArgumentException(
> + "Control character in cookie value or attribute.");
> + }
> + return true;
> + }
> +
> + return !CookieSupport.isHttpSeparator((char) b) ||
> + version == 0 &&
> + CookieSupport.ALLOW_HTTP_SEPARATORS_IN_V0 &&
> + b != '=' &&
> + !CookieSupport.isV0Separator((char) b) ||
> + !isName && b == '=' &&
> + CookieSupport.ALLOW_EQUALS_IN_VALUE;
> + }
> +
> /**
> * Given a starting position after an initial quote character, this gets
> * the position of the end quote. This escapes anything after a '\' char
>
> Modified: tomcat/trunk/test/org/apache/tomcat/util/http/TestCookies.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/tomcat/util/http/TestCookies.java?rev=1553187&r1=1553186&r2=1553187&view=diff
> ==============================================================================
> --- tomcat/trunk/test/org/apache/tomcat/util/http/TestCookies.java (original)
> +++ tomcat/trunk/test/org/apache/tomcat/util/http/TestCookies.java Mon Dec 23 19:15:35 2013
> @@ -17,9 +17,113 @@
>
> package org.apache.tomcat.util.http;
>
> +import java.nio.charset.StandardCharsets;
> +
> +import javax.servlet.http.Cookie;
> +
> +import org.junit.Assert;
> +import org.junit.Before;
> +import org.junit.Ignore;
> import org.junit.Test;
>
> public class TestCookies {
> + private Cookies cookies;
> +
> + @Before
> + public void init() {
> + this.cookies = new Cookies(null);
> + }
> +
> + @Test
> + public void skipJsonInV0Value() {
> + process("bad={\"v\":1,\"x\":2}; a=b");
> + expect(makeCookie("a", "b", 0));
> + }
> +
> + @Test(expected = IllegalArgumentException.class)
> + public void disallow8bitInName() {
> + process("f\u00f6o=bar");
> + }
> +
> + @Test(expected = IllegalArgumentException.class)
> + public void disallowControlInName() {
> + process("f\010o=bar");
> + }
> +
> + @Test(expected = IllegalArgumentException.class)
> + public void disallow8BitControlInName() {
> + process("f\210o=bar");
> + }
> +
> + @Test
> + public void allow8BitInV0Value() {
> + process("foo=b\u00e1r");
> + expect(makeCookie("foo", "b\u00e1r", 0));
> + }
> +
> + @Test(expected = IllegalArgumentException.class)
> + public void disallow8bitInV1UnquotedValue() {
> + process("$Version=1; foo=b\u00e1r");
> + }
> +
> + @Test
> + public void allow8bitInV1QuotedValue() {
> + process("$Version=1; foo=\"b\u00e1r\"");
> + expect(makeCookie("foo", "b\u00e1r", 1));
> + }
> +
> + @Test(expected = IllegalArgumentException.class)
> + public void disallowControlInV0Value() {
> + process("foo=b\010r");
> + }
> +
> + @Test(expected = IllegalArgumentException.class)
> + public void disallow8BitControlInV0Value() {
> + process("foo=b\210r");
> + }
> +
> + @Test(expected = IllegalArgumentException.class)
> + public void disallowControlInV1UnquotedValue() {
> + process("$Version=1; foo=b\010r");
> + }
> +
> + @Ignore
> + @Test(expected = IllegalArgumentException.class)
> + public void disallowControlInV1QuotedValue() {
> + process("$Version=1; foo=\"b\010r\"");
> + }
> +
> + @Test(expected = IllegalArgumentException.class)
> + public void disallow8BitControlInV1UnquotedValue() {
> + process("$Version=1; foo=b\210r");
> + }
> +
> + @Ignore
> + @Test
> + public void allow8BitControlInV1QuotedValue() {
> + process("$Version=1; foo=\"b\210r\"");
> + expect(makeCookie("foo", "b\210r", 1));
> + }
> +
> + private void process(String header) {
> + byte[] bytes = header.getBytes(StandardCharsets.ISO_8859_1);
> + cookies.processCookieHeader(bytes, 0, bytes.length);
> + }
> +
> + private void expect(Cookie... expected) {
> + Assert.assertEquals(expected.length, cookies.getCookieCount());
> + for (int i = 0; i < expected.length; i++) {
> + ServerCookie actual = cookies.getCookie(i);
> + Assert.assertEquals(expected[i].getName(), actual.getName().toString());
> + Assert.assertEquals(expected[i].getValue(), actual.getValue().toString());
> + }
> + }
> +
> + private static Cookie makeCookie(String name, String value, int version) {
> + Cookie cookie = new Cookie(name, value);
> + cookie.setVersion(version);
> + return cookie;
> + }
>
> @Test
> public void testCookies() throws Exception {
>
> Modified: tomcat/trunk/webapps/docs/changelog.xml
> URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1553187&r1=1553186&r2=1553187&view=diff
> ==============================================================================
> --- tomcat/trunk/webapps/docs/changelog.xml (original)
> +++ tomcat/trunk/webapps/docs/changelog.xml Mon Dec 23 19:15:35 2013
> @@ -225,6 +225,10 @@
> Change the default URIEncoding for all connectors from ISO-8859-1 to
> UTF-8. (markt)
> </update>
> + <scode>
> + <bug>55917</bug>: Allow ISO-8859-1 characters 0xA0-0xFF in V0 cookie
> + values (jboynes).
> + </scode>
> </changelog>
> </subsection>
> <subsection name="Jasper">
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org