jboy...@apache.org wrote: >Author: jboynes >Date: Tue Dec 24 15:36:25 2013 >New Revision: 1553290 > >URL: http://svn.apache.org/r1553290 >Log: >revert 1553187
Thanks. I'm not saying that there isn't something that needs to be fixed here (I'm sure there are changes required) but given the history of problems whenever we have changed the cookie code we need to be very sure of what we are doing and why. Maybe the place to start is just with the test cases that demonstate the issue (we can comment out ones that fail). My concern as soon as we step outside the RFCs is if different elements (user agents, proxies, origin servers) deviate from the spec on different ways there is a risk of security issues. We have seen issues along these lines in the past so I'd like to be very sure there is no scope for similar mischief with cookies if we relax our parsing rules. I also think any changes should be focussed on solving observed issues rather than trying to address all the issues we can think of reading the specs. Refactoring the current cookie code is overdue but is probably best handled as a separate issue. I'm not sure of it would be better to refactor before or after fixing the recently raised issues with the current cookie processing. 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=1553290&r1=1553289&r2=1553290&view=diff >============================================================================== >--- tomcat/trunk/java/org/apache/tomcat/util/http/Cookies.java >(original) >+++ tomcat/trunk/java/org/apache/tomcat/util/http/Cookies.java Tue Dec >24 15:36:25 2013 >@@ -508,7 +508,14 @@ 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 && allowInToken(bytes[pos], version, isName)) >{ >+ 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)) { > pos++; > } > >@@ -518,34 +525,6 @@ 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=1553290&r1=1553289&r2=1553290&view=diff >============================================================================== >--- tomcat/trunk/test/org/apache/tomcat/util/http/TestCookies.java >(original) >+++ tomcat/trunk/test/org/apache/tomcat/util/http/TestCookies.java Tue >Dec 24 15:36:25 2013 >@@ -17,113 +17,9 @@ > > 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=1553290&r1=1553289&r2=1553290&view=diff >============================================================================== >--- tomcat/trunk/webapps/docs/changelog.xml (original) >+++ tomcat/trunk/webapps/docs/changelog.xml Tue Dec 24 15:36:25 2013 >@@ -225,10 +225,6 @@ > 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-unsubscr...@tomcat.apache.org >For additional commands, e-mail: dev-h...@tomcat.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
