Bug 64509

Summary: Rfc6265CookieProcessor mishandles commas in $Version=1 cookie header
Product: Tomcat 9 Reporter: WJCarpenter <bill-apache>
Component: UtilAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: normal    
Priority: P2    
Version: 9.0.36   
Target Milestone: -----   
Hardware: PC   
OS: Linux   
Attachments: test code

Description WJCarpenter 2020-06-09 20:26:23 UTC
Created attachment 37299 [details]
test code

(Tested in both 9.0.33 and 9.0.36, but 9.0.36 wasn't available in the drop-down when I opened this bug report.)

Rfc6265CookieProcessor tries to accept a comma as a cookie pair separator for $Version=1 cookie headers, but it gets it wrong. It also behave differently from the legacy parser. For this Cookie: header value

 $Version=1;first=1,second=2;third=3;case=justCOMMA

the new parser loses cookies "first" and "second" (logs an invalid cookie warning). The legacy parser does not.

Here is a small test program (also attached) that parses headers with both processors. The test values include both "$Version=1" cookie headers and the same with no version attribute. I believe the RFC-6265 parsing is not trying to honor the comma, which is fair enough.

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

package aside;

import org.apache.tomcat.util.http.LegacyCookieProcessor;
import org.apache.tomcat.util.http.MimeHeaders;
import org.apache.tomcat.util.http.Rfc6265CookieProcessor;
import org.apache.tomcat.util.http.ServerCookie;
import org.apache.tomcat.util.http.ServerCookies;

public class TC9CookieParsingTest {
  private static final String[] cookieHeaderValues = {
      "$Version=1;first=1;second=2;third=3;case=justSEMI",
      "first=1;second=2;third=3;case=justSEMI",
      "$Version=1;first=1,second=2;third=3;case=justCOMMA",
      "first=1,second=2;third=3;case=justCOMMA",
      "$Version=1;first=1,;second=2;third=3;case=COMMAthenSEMI",
      "first=1,;second=2;third=3;case=COMMAthenSEMI",
      "$Version=1;first=1;,second=2;third=3;case=SEMIthenCOMMA",
      "first=1;,second=2;third=3;case=SEMIthenCOMMA",
  };
  public static void main(String[] args) {
    for (final String cookieHeaderValue: TC9CookieParsingTest.cookieHeaderValues) {
      TC9CookieParsingTest.parseWithRfc6265Parser(cookieHeaderValue);
      TC9CookieParsingTest.parseWithLegacyParser(cookieHeaderValue);
    }
  }

  private static void parseWithRfc6265Parser(final String cookieHeaderValue) {
    final MimeHeaders headers = new MimeHeaders();
    headers.addValue("Cookie").setString(cookieHeaderValue);
    final ServerCookies serverCookies = new ServerCookies(10);
    final Rfc6265CookieProcessor processor = new Rfc6265CookieProcessor();
    processor.parseCookieHeader(headers, serverCookies);
    final int howMany = serverCookies.getCookieCount();
    System.out.println("\n====================\nRfc6265CookieProcessor 'Cookie: " + cookieHeaderValue + "'");
    for (int ii=0; ii<howMany; ++ii) {
      final ServerCookie sc = serverCookies.getCookie(ii);
      System.out.println(ii + " cookie: '" + sc.getName() + "' = '" + sc.getValue() + "'");
    }
  }

  private static void parseWithLegacyParser(final String cookieHeaderValue) {
    final MimeHeaders headers = new MimeHeaders();
    headers.addValue("Cookie").setString(cookieHeaderValue);
    final ServerCookies serverCookies = new ServerCookies(10);
    final LegacyCookieProcessor processor = new LegacyCookieProcessor();
    processor.parseCookieHeader(headers, serverCookies);
    final int howMany = serverCookies.getCookieCount();
    System.out.println("--------------------\nLegacyCookieProcessor 'Cookie: " + cookieHeaderValue + "'");
    for (int ii=0; ii<howMany; ++ii) {
      final ServerCookie sc = serverCookies.getCookie(ii);
      System.out.println(ii + " cookie: '" + sc.getName() + "' = '" + sc.getValue() + "'");
    }
  }
}

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

Here is the output from a run of the test program:

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

====================
Rfc6265CookieProcessor 'Cookie: $Version=1;first=1;second=2;third=3;case=justSEMI'
0 cookie: 'first' = '1'
1 cookie: 'second' = '2'
2 cookie: 'third' = '3'
3 cookie: 'case' = 'justSEMI'
--------------------
LegacyCookieProcessor 'Cookie: $Version=1;first=1;second=2;third=3;case=justSEMI'
0 cookie: 'first' = '1'
1 cookie: 'second' = '2'
2 cookie: 'third' = '3'
3 cookie: 'case' = 'justSEMI'

====================
Rfc6265CookieProcessor 'Cookie: first=1;second=2;third=3;case=justSEMI'
0 cookie: 'first' = '1'
1 cookie: 'second' = '2'
2 cookie: 'third' = '3'
3 cookie: 'case' = 'justSEMI'
--------------------
LegacyCookieProcessor 'Cookie: first=1;second=2;third=3;case=justSEMI'
0 cookie: 'first' = '1'
1 cookie: 'second' = '2'
2 cookie: 'third' = '3'
3 cookie: 'case' = 'justSEMI'

====================
Rfc6265CookieProcessor 'Cookie: $Version=1;first=1,second=2;third=3;case=justCOMMA'
0 cookie: 'third' = '3'
1 cookie: 'case' = 'justCOMMA'
--------------------
LegacyCookieProcessor 'Cookie: $Version=1;first=1,second=2;third=3;case=justCOMMA'
0 cookie: 'first' = '1'
1 cookie: 'second' = '2'
2 cookie: 'third' = '3'
3 cookie: 'case' = 'justCOMMA'

====================
Rfc6265CookieProcessor 'Cookie: first=1,second=2;third=3;case=justCOMMA'
0 cookie: 'third' = '3'
1 cookie: 'case' = 'justCOMMA'
--------------------
LegacyCookieProcessor 'Cookie: first=1,second=2;third=3;case=justCOMMA'
0 cookie: 'first' = '1'
1 cookie: 'second' = '2'
2 cookie: 'third' = '3'
3 cookie: 'case' = 'justCOMMA'

====================
Rfc6265CookieProcessor 'Cookie: $Version=1;first=1,;second=2;third=3;case=COMMAthenSEMI'
0 cookie: 'first' = '1'
1 cookie: 'second' = '2'
2 cookie: 'third' = '3'
3 cookie: 'case' = 'COMMAthenSEMI'
--------------------
LegacyCookieProcessor 'Cookie: $Version=1;first=1,;second=2;third=3;case=COMMAthenSEMI'
0 cookie: 'first' = '1'
1 cookie: 'second' = '2'
2 cookie: 'third' = '3'
3 cookie: 'case' = 'COMMAthenSEMI'

====================
Rfc6265CookieProcessor 'Cookie: first=1,;second=2;third=3;case=COMMAthenSEMI'
0 cookie: 'second' = '2'
1 cookie: 'third' = '3'
2 cookie: 'case' = 'COMMAthenSEMI'
--------------------
LegacyCookieProcessor 'Cookie: first=1,;second=2;third=3;case=COMMAthenSEMI'
0 cookie: 'first' = '1'
1 cookie: 'second' = '2'
2 cookie: 'third' = '3'
3 cookie: 'case' = 'COMMAthenSEMI'

====================
Rfc6265CookieProcessor 'Cookie: $Version=1;first=1;,second=2;third=3;case=SEMIthenCOMMA'
0 cookie: 'first' = '1'
1 cookie: 'third' = '3'
2 cookie: 'case' = 'SEMIthenCOMMA'
--------------------
LegacyCookieProcessor 'Cookie: $Version=1;first=1;,second=2;third=3;case=SEMIthenCOMMA'
0 cookie: 'first' = '1'
1 cookie: 'second' = '2'
2 cookie: 'third' = '3'
3 cookie: 'case' = 'SEMIthenCOMMA'

====================
Rfc6265CookieProcessor 'Cookie: first=1;,second=2;third=3;case=SEMIthenCOMMA'
0 cookie: 'first' = '1'
1 cookie: 'third' = '3'
2 cookie: 'case' = 'SEMIthenCOMMA'
--------------------
LegacyCookieProcessor 'Cookie: first=1;,second=2;third=3;case=SEMIthenCOMMA'
0 cookie: 'first' = '1'
1 cookie: 'second' = '2'
2 cookie: 'third' = '3'
3 cookie: 'case' = 'SEMIthenCOMMA'

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

I've had a quick look at the parser code. I suspect the problem lines in org.apache.tomcat.util.http.parser.Cookie, near line 274:

            skipResult = skipByte(bb, COMMA_BYTE);
            if (skipResult == SkipResult.FOUND) {
                parseAttributes = false;
            }
            skipResult = skipByte(bb, SEMICOLON_BYTE);
            if (skipResult == SkipResult.EOF) {
                parseAttributes = false;
                moreToProcess = false;
            } else if (skipResult == SkipResult.NOT_FOUND) {
                skipInvalidCookie(bb);
                continue;
            }

Even though we've just found the COMMA_BYTE, it's still a failure if we don't then fint the SEMICOLON_BYTE. I suspect that wanted to be an "else if", but since the parser is kind of complicated I hesitate to claim that's a definitive fix. It might break some other edge case.
Comment 1 Mark Thomas 2020-06-10 16:22:40 UTC
Thanks for the reminder. I've added 9.0.36 to the list of versions and updated the version for this issue. I'll look at the detail of the report next.
Comment 2 Mark Thomas 2020-06-10 19:59:19 UTC
Fixed in:
- master for 10.0.0-M7 onwards
- 9.0.x for 9.0.37 onwards
- 8.5.x for 8.5.57 onwards

7.0.x is not affected.

Thanks for the report. You were right about the location of the bug. There were a couple of other places the same bug was present. I've fixed them an added a parameterised test case that should test all combinations.
Comment 3 WJCarpenter 2020-06-10 20:46:19 UTC
Thanks for the quick action on this.