cookie: cleanups and improvements

- Stricter cookie validation with earlier rejection of empty/invalid
  cookie names

- secure and httponly attributes no longer accept = with empty values
  (only bare keywords)

- Validation checks (length, TAB, prefixes) moved into the first
  name/value pair block for better code organization

- Deferred time(NULL) calls for better performance when expires/max-age
  aren't used

- Simplified loop control flow by removing done flag

- The cookie size restriction now only applies to name + value, not other
  parts of the header line.

- Fixed a gcc 4.8.1 quirk

Closes #19868

diff --git a/lib/cookie.c b/lib/cookie.c
index 7efce5611f2ad79da015446e558cc7200e2e22ee..716a487750e22e02a785717a05d5b2c036aec98d 100644 (file)
--- a/lib/cookie.c
+++ b/lib/cookie.c
@@ -441,85 +441,79 @@ parse_cookie_header(struct Curl_easy *data,
                                      origin */
 {
   /* This line was read off an HTTP-header */
-  time_t now;
+  time_t now = 0;
   size_t linelength = strlen(ptr);
-  CURLcode result;
-  struct Curl_str cookie[COOKIE_PIECES] = { 0 };
+  CURLcode result = CURLE_OK;
+  struct Curl_str cookie[COOKIE_PIECES];
   *okay = FALSE;
   if(linelength > MAX_COOKIE_LINE)
     /* discard overly long lines at once */
     return CURLE_OK;
 
-  now = time(NULL);
+  /* memset instead of initializer because gcc 4.8.1 is silly */
+  memset(cookie, 0, sizeof(cookie));
   do {
     struct Curl_str name;
     struct Curl_str val;
 
     /* we have a <name>=<value> pair or a stand-alone word here */
     if(!curlx_str_cspn(&ptr, &name, ";\t\r\n=")) {
-      bool done = FALSE;
       bool sep = FALSE;
       curlx_str_trimblanks(&name);
 
       if(!curlx_str_single(&ptr, '=')) {
         sep = TRUE; /* a '=' was used */
-        if(!curlx_str_cspn(&ptr, &val, ";\r\n")) {
+        if(!curlx_str_cspn(&ptr, &val, ";\r\n"))
           curlx_str_trimblanks(&val);
-
-          /* Reject cookies with a TAB inside the value */
-          if(memchr(curlx_str(&val), '\t', curlx_strlen(&val))) {
-            infof(data, "cookie contains TAB, dropping");
-            return CURLE_OK;
-          }
-        }
       }
-      else {
+      else
         curlx_str_init(&val);
-      }
-
-      /*
-       * Check for too long individual name or contents, or too long
-       * combination of name + contents. Chrome and Firefox support 4095 or
-       * 4096 bytes combo
-       */
-      if(curlx_strlen(&name) >= (MAX_NAME - 1) ||
-         curlx_strlen(&val) >= (MAX_NAME - 1) ||
-         ((curlx_strlen(&name) + curlx_strlen(&val)) > MAX_NAME)) {
-        infof(data, "oversized cookie dropped, name/val %zu + %zu bytes",
-              curlx_strlen(&name), curlx_strlen(&val));
-        return CURLE_OK;
-      }
-
-      /*
-       * Check if we have a reserved prefix set before anything else, as we
-       * otherwise have to test for the prefix in both the cookie name and
-       * "the rest". Prefixes must start with '__' and end with a '-', so
-       * only test for names where that can possibly be true.
-       */
-      if(!strncmp("__Secure-", curlx_str(&name), 9))
-        co->prefix_secure = TRUE;
-      else if(!strncmp("__Host-", curlx_str(&name), 7))
-        co->prefix_host = TRUE;
 
       if(!curlx_strlen(&cookie[COOKIE_NAME])) {
         /* The first name/value pair is the actual cookie name */
         if(!sep ||
            /* Bad name/value pair. */
            invalid_octets(curlx_str(&name), curlx_strlen(&name)) ||
-           invalid_octets(curlx_str(&val), curlx_strlen(&val))) {
+           invalid_octets(curlx_str(&val), curlx_strlen(&val)) ||
+           !curlx_strlen(&name)) {
           infof(data, "invalid octets in name/value, cookie dropped");
           return CURLE_OK;
         }
+
+        /*
+         * Check for too long individual name or contents, or too long
+         * combination of name + contents. Chrome and Firefox support 4095 or
+         * 4096 bytes combo
+         */
+        if(curlx_strlen(&name) >= (MAX_NAME - 1) ||
+           curlx_strlen(&val) >= (MAX_NAME - 1) ||
+           ((curlx_strlen(&name) + curlx_strlen(&val)) > MAX_NAME)) {
+          infof(data, "oversized cookie dropped, name/val %zu + %zu bytes",
+                curlx_strlen(&name), curlx_strlen(&val));
+          return CURLE_OK;
+        }
+
+        /* Reject cookies with a TAB inside the value */
+        if(curlx_strlen(&val) &&
+           memchr(curlx_str(&val), '\t', curlx_strlen(&val))) {
+          infof(data, "cookie contains TAB, dropping");
+          return CURLE_OK;
+        }
+
+        /* Check if we have a reserved prefix set. */
+        if(!strncmp("__Secure-", curlx_str(&name), 9))
+          co->prefix_secure = TRUE;
+        else if(!strncmp("__Host-", curlx_str(&name), 7))
+          co->prefix_host = TRUE;
+
         cookie[COOKIE_NAME] = name;
         cookie[COOKIE_VALUE] = val;
-        done = TRUE;
       }
-      else if(!curlx_strlen(&val)) {
+      else if(!sep) {
         /*
-         * this was a "<name>=" with no content, and we must allow
-         * 'secure' and 'httponly' specified this weirdly
+         * this is a "<name>" with no content
          */
-        done = TRUE;
+
         /*
          * secure cookies are only allowed to be set when the connection is
          * using a secure protocol, or when the cookie is being set by
@@ -529,18 +523,13 @@ parse_cookie_header(struct Curl_easy *data,
           if(secure || !ci->running)
             co->secure = TRUE;
           else {
-            infof(data, "skipped cookie %s because not 'secure'", co->name);
+            infof(data, "skipped cookie because not 'secure'");
             return CURLE_OK;
           }
         }
         else if(curlx_str_casecompare(&name, "httponly"))
           co->httponly = TRUE;
-        else if(sep)
-          /* there was a '=' so we are not done parsing this field */
-          done = FALSE;
       }
-      if(done)
-        ;
       else if(curlx_str_casecompare(&name, "path")) {
         cookie[COOKIE_PATH] = val;
       }
@@ -588,9 +577,6 @@ parse_cookie_header(struct Curl_easy *data,
           return CURLE_OK;
         }
       }
-      else if(curlx_str_casecompare(&name, "version")) {
-        /* just ignore */
-      }
       else if(curlx_str_casecompare(&name, "max-age") && curlx_strlen(&val)) {
         /*
          * Defined in RFC2109:
@@ -606,6 +592,8 @@ parse_cookie_header(struct Curl_easy *data,
         if(*maxage == '\"')
           maxage++;
         rc = curlx_str_number(&maxage, &co->expires, CURL_OFF_T_MAX);
+        if(!now)
+          now = time(NULL);
         switch(rc) {
         case STRE_OVERFLOW:
           /* overflow, used max value */
@@ -627,46 +615,38 @@ parse_cookie_header(struct Curl_easy *data,
         }
         cap_expires(now, co);
       }
-      else if(curlx_str_casecompare(&name, "expires") && curlx_strlen(&val)) {
-        if(!co->expires && (curlx_strlen(&val) < MAX_DATE_LENGTH)) {
-          /*
-           * Let max-age have priority.
-           *
-           * If the date cannot get parsed for whatever reason, the cookie
-           * will be treated as a session cookie
-           */
-          char dbuf[MAX_DATE_LENGTH + 1];
-          time_t date = 0;
-          memcpy(dbuf, curlx_str(&val), curlx_strlen(&val));
-          dbuf[curlx_strlen(&val)] = 0;
-          if(!Curl_getdate_capped(dbuf, &date)) {
-            if(!date)
-              date++;
-            co->expires = (curl_off_t)date;
-          }
-          else
-            co->expires = 0;
-          cap_expires(now, co);
+      else if(curlx_str_casecompare(&name, "expires") && curlx_strlen(&val) &&
+              !co->expires && (curlx_strlen(&val) < MAX_DATE_LENGTH)) {
+        /*
+         * Let max-age have priority.
+         *
+         * If the date cannot get parsed for whatever reason, the cookie
+         * will be treated as a session cookie
+         */
+        char dbuf[MAX_DATE_LENGTH + 1];
+        time_t date = 0;
+        memcpy(dbuf, curlx_str(&val), curlx_strlen(&val));
+        dbuf[curlx_strlen(&val)] = 0;
+        if(!Curl_getdate_capped(dbuf, &date)) {
+          if(!date)
+            date++;
+          co->expires = (curl_off_t)date;
         }
+        else
+          co->expires = 0;
+        if(!now)
+          now = time(NULL);
+        cap_expires(now, co);
       }
-
-      /*
-       * Else, this is the second (or more) name we do not know about!
-       */
     }
+  } while(!curlx_str_single(&ptr, ';'));
 
-    if(curlx_str_single(&ptr, ';'))
-      break;
-  } while(1);
-
-  if(!curlx_strlen(&cookie[COOKIE_NAME]))
-    /* If we did not get a cookie name, or a bad one, bail out. */
-    return CURLE_OK;
-
-  /* the header was fine, now store the data */
-  result = storecookie(co, &cookie[0], path, domain);
-  if(!result)
-    *okay = TRUE;
+  if(curlx_strlen(&cookie[COOKIE_NAME])) {
+    /* the header was fine, now store the data */
+    result = storecookie(co, &cookie[0], path, domain);
+    if(!result)
+      *okay = TRUE;
+  }
   return result;
 }
 

diff --git a/tests/data/test1160 b/tests/data/test1160
index 9eca524c658f51364f910fecc3f09f3a8cc79848..d26ad4133f216fa08e9f1d524ad4bb30db79ce61 100644 (file)
--- a/tests/data/test1160
+++ b/tests/data/test1160
@@ -14,7 +14,7 @@ cookies
 HTTP/1.1 200 OK
 Date: Tue, 09 Nov 2010 14:49:00 GMT
 Content-Length: 0
-Set-Cookie: ____________%hex[%ff]hex%=         ;%repeat[117 x %20]%%hex[%ff]hex%%repeat[2602 x %20]%%repeat[434 x z]%%hex[%86%85%85%80]hex%%repeat[49 x z]%%hex[%fa]hex%%repeat[540 x z]%%hex[%f3%a0%81%96]hex%zzzzzzzzzzzz~%repeat[82 x z]%%hex[%b6]hex%%repeat[364 x z]%
+Set-Cookie: %repeat[4000 x n]%=%repeat[4096 x v]%
 
 </data>
 </reply>

diff --git a/tests/data/test31 b/tests/data/test31
index 8353b709fc0937f7ccb8ba5d13d84b47dfb82c4f..e13fed55c625beb808ecf0cd4c643a4087664e42 100644 (file)
--- a/tests/data/test31
+++ b/tests/data/test31
@@ -18,6 +18,8 @@ Server: test-server/fake%CR
 Content-Length: 4%CR
 Content-Type: text/html%CR
 Funny-head: yesyes%CR
+Set-Cookie:
+Set-Cookie: ;
 Set-Cookie: blankdomain=sure; domain=; path=/
 Set-Cookie: foobar=name; domain=anything.com; path=/ ; secure%CR
 Set-Cookie:ismatch=this  ; domain=test31.curl; path=/silly/%CR
@@ -25,27 +27,27 @@ Set-Cookie:ISMATCH=this  ; domain=test31.curl; path=/silly/%CR
 Set-Cookie: overwrite=this  ; domain=test31.curl; path=/overwrite/%CR
 Set-Cookie: overwrite=this2  ; domain=test31.curl; path=/overwrite%CR
 Set-Cookie: sec1value=secure1  ; domain=test31.curl; path=/secure1/ ; secure%CR
-Set-Cookie: sec2value=secure2  ; domain=test31.curl; path=/secure2/ ; secure=%CR
-Set-Cookie: sec3value=secure3  ; domain=test31.curl; path=/secure3/ ; secure=%CR
-Set-Cookie: sec4value=secure4  ; secure=; domain=test31.curl; path=/secure4/ ; %CR
+Set-Cookie: sec2value=secure2  ; domain=test31.curl; path=/secure2/ ; secure%CR
+Set-Cookie: sec3value=secure3  ; domain=test31.curl; path=/secure3/ ; secure%CR
+Set-Cookie: sec4value=secure4  ; secure; domain=test31.curl; path=/secure4/ ; %CR
 Set-Cookie: sec5value=secure5  ; secure; domain=test31.curl; path=/secure5/ ; %CR
 Set-Cookie: sec6value=secure6  ; secure ; domain=test31.curl; path=/secure6/ ; %CR
 Set-Cookie: sec7value=secure7  ; secure   ; domain=test31.curl; path=/secure7/ ; %CR
-Set-Cookie: sec8value=secure8  ; secure= ; domain=test31.curl; path=/secure8/ ; %CR
-Set-Cookie: secure=very1  ; secure=; domain=test31.curl; path=/secure9/; %CR
+Set-Cookie: sec8value=secure8  ; secure ; domain=test31.curl; path=/secure8/ ; %CR
+Set-Cookie: secure=very1  ; secure; domain=test31.curl; path=/secure9/; %CR
 Set-Cookie: httpo1=value1  ; domain=test31.curl; path=/p1/; httponly%CR
-Set-Cookie: httpo2=value2  ; domain=test31.curl; path=/p2/; httponly=%CR
+Set-Cookie: httpo2=value2  ; domain=test31.curl; path=/p2/; httponly%CR
 Set-Cookie: httpo3=value3  ; httponly; domain=test31.curl; path=/p3/;%CR
-Set-Cookie: httpo4=value4  ; httponly=; domain=test31.curl; path=/p4/; %CR
+Set-Cookie: httpo4=value4  ; httponly; domain=test31.curl; path=/p4/; %CR
 Set-Cookie: httponly=myvalue1  ; domain=test31.curl; path=/p4/; httponly%CR
 Set-Cookie: httpandsec=myvalue2  ; domain=test31.curl; path=/p4/; httponly; secure%CR
-Set-Cookie: httpandsec2=myvalue3; domain=test31.curl; path=/p4/; httponly=; secure%CR
-Set-Cookie: httpandsec3=myvalue4  ; domain=test31.curl; path=/p4/; httponly; secure=%CR
-Set-Cookie: httpandsec4=myvalue5  ; domain=test31.curl; path=/p4/; httponly=; secure=%CR
-Set-Cookie: httpandsec5=myvalue6  ; domain=test31.curl; path=/p4/; secure; httponly=%CR
-Set-Cookie: httpandsec6=myvalue7  ; domain=test31.curl; path=/p4/; secure=; httponly=%CR
+Set-Cookie: httpandsec2=myvalue3; domain=test31.curl; path=/p4/; httponly; secure%CR
+Set-Cookie: httpandsec3=myvalue4  ; domain=test31.curl; path=/p4/; httponly; secure%CR
+Set-Cookie: httpandsec4=myvalue5  ; domain=test31.curl; path=/p4/; httponly; secure%CR
+Set-Cookie: httpandsec5=myvalue6  ; domain=test31.curl; path=/p4/; secure; httponly%CR
+Set-Cookie: httpandsec6=myvalue7  ; domain=test31.curl; path=/p4/; secure; httponly%CR
 Set-Cookie: httpandsec7=myvalue8  ; domain=test31.curl; path=/p4/; secure; httponly%CR
-Set-Cookie: httpandsec8=myvalue9; domain=test31.curl; path=/p4/; secure=; httponly%CR
+Set-Cookie: httpandsec8=myvalue9; domain=test31.curl; path=/p4/; secure; httponly%CR
 Set-Cookie: partmatch=present; domain=test31.curl ; path=/;%CR
 Set-Cookie:eat=this; domain=moo.foo.moo;%CR
 Set-Cookie: eat=this-too; domain=.foo.moo;%CR
@@ -67,7 +69,7 @@ Set-Cookie: partialip=nono; domain=.0.0.1;%CR
 Set-Cookie: withspaces=  yes  within and around    ;%CR
 Set-Cookie: withspaces2 =before equals;%CR
 Set-Cookie: prespace=  yes before;%CR
-Set-Cookie: securewithspace=after    ; secure =%CR
+Set-Cookie: securewithspace=after    ; secure %CR
 Set-Cookie: %hex[%c3%82%c2%b3%c3%83%5c%78%39%32%c3%83%5c%78%39%61%c3%83%5c%78%38%64%c3%83%5c%78%39%37]hex%=%96%A6g%9Ay%B0%A5g%A7tm%7C%95%9A
 %CR
 boo