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
2026-04-11 12:01:42 +08:00 · 2025-12-07 23:44:31 +01:00 · 2025-12-07 23:44:31 +01:00 · a78a07d3a9
commit a78a07d3a9
parent a093c93994
3 changed files with 87 additions and 105 deletions
--- 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;
 }

--- 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>
--- 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