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/24 16:36:26 UTC
svn commit: r1553290 - 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: Tue Dec 24 15:36:25 2013
New Revision: 1553290
URL: http://svn.apache.org/r1553290
Log:
revert 1553187
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1553290 - 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>.
jboynes@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-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