cookie parser: handle 'secure='

There are two keywords in cookie headers that don't follow the regular
name=value style: secure and httponly. Still we must support that they
are written like 'secure=' and then treat them as if they were written
'secure'. Test case 31 was much extended by Rob Ward to test this.

Bug: http://curl.haxx.se/bug/view.cgi?id=3349227
Reported by: "gnombat"

2 files changed, 205 insertions, 161 deletions

diff --git a/lib/cookie.c b/lib/cookie.c
index 86d264c45..0553efb98 100644
--- a/lib/cookie.c
+++ b/lib/cookie.c
@@ -206,7 +206,6 @@ Curl_cookie_add(struct SessionHandle *data,
   if(httpheader) {
     /* This line was read off a HTTP-header */
     const char *ptr;
-    const char *sep;
     const char *semiptr;
     char *what;
 
@@ -223,185 +222,186 @@ Curl_cookie_add(struct SessionHandle *data,
 
     ptr = lineptr;
     do {
-      /* we have a <what>=<this> pair or a 'secure' word here */
-      sep = strchr(ptr, '=');
-      if(sep && (!semiptr || (semiptr>sep)) ) {
-        /*
-         * There is a = sign and if there was a semicolon too, which make sure
-         * that the semicolon comes _after_ the equal sign.
-         */
-
-        name[0]=what[0]=0; /* init the buffers */
-        if(1 <= sscanf(ptr, "%" MAX_NAME_TXT "[^;=]=%"
-                       MAX_COOKIE_LINE_TXT "[^;\r\n]",
-                       name, what)) {
-          /* this is a <name>=<what> pair. We use strstore() below to properly
-             deal with received cookie headers that have the same string
-             property set more than once, and then we use the last one. */
-
-          const char *whatptr;
-
-          /* Strip off trailing whitespace from the 'what' */
-          size_t len=strlen(what);
-          while(len && ISBLANK(what[len-1])) {
-            what[len-1]=0;
-            len--;
-          }
+      /* we have a <what>=<this> pair or a stand-alone word here */
+      name[0]=what[0]=0; /* init the buffers */
+      if(1 <= sscanf(ptr, "%" MAX_NAME_TXT "[^;\r\n =]=%"
+                     MAX_COOKIE_LINE_TXT "[^;\r\n]",
+                     name, what)) {
+        /* Use strstore() below to properly deal with received cookie
+           headers that have the same string property set more than once,
+           and then we use the last one. */
+        const char *whatptr;
+        bool done = FALSE;
+        bool sep;
+        size_t len=strlen(what);
+        const char *endofn = &ptr[ strlen(name) ];
+
+        /* skip trailing spaces in name */
+        while(*endofn && ISBLANK(*endofn))
+          endofn++;
+
+        /* name ends with a '=' ? */
+        sep = *endofn == '='?TRUE:FALSE;
+
+        /* Strip off trailing whitespace from the 'what' */
+        while(len && ISBLANK(what[len-1])) {
+          what[len-1]=0;
+          len--;
+        }
 
-          /* Skip leading whitespace from the 'what' */
-          whatptr=what;
-          while(*whatptr && ISBLANK(*whatptr)) {
-            whatptr++;
-          }
+        /* Skip leading whitespace from the 'what' */
+        whatptr=what;
+        while(*whatptr && ISBLANK(*whatptr))
+          whatptr++;
 
-          if(Curl_raw_equal("path", name)) {
-            strstore(&co->path, whatptr);
-            if(!co->path) {
-              badcookie = TRUE; /* out of memory bad */
-              break;
+        if(!len) {
+          /* this was a "<name>=" with no content, and we must allow
+             'secure' and 'httponly' specified this weirdly */
+          done = TRUE;
+          if(Curl_raw_equal("secure", name))
+            co->secure = TRUE;
+          else if(Curl_raw_equal("httponly", name))
+            co->httponly = TRUE;
+          else if(sep)
+            /* there was a '=' so we're not done parsing this field */
+            done = FALSE;
+        }
+        if(done)
+          ;
+        else if(Curl_raw_equal("path", name)) {
+          strstore(&co->path, whatptr);
+          if(!co->path) {
+            badcookie = TRUE; /* out of memory bad */
+            break;
+          }
+        }
+        else if(Curl_raw_equal("domain", name)) {
+          /* note that this name may or may not have a preceding dot, but
+             we don't care about that, we treat the names the same anyway */
+
+          const char *domptr=whatptr;
+          const char *nextptr;
+          int dotcount=1;
+
+          /* Count the dots, we need to make sure that there are enough
+             of them. */
+
+          if('.' == whatptr[0])
+            /* don't count the initial dot, assume it */
+            domptr++;
+
+          do {
+            nextptr = strchr(domptr, '.');
+            if(nextptr) {
+              if(domptr != nextptr)
+                dotcount++;
+              domptr = nextptr+1;
             }
+          } while(nextptr);
+
+          /* The original Netscape cookie spec defined that this domain name
+             MUST have three dots (or two if one of the seven holy TLDs),
+             but it seems that these kinds of cookies are in use "out there"
+             so we cannot be that strict. I've therefore lowered the check
+             to not allow less than two dots. */
+
+          if(dotcount < 2) {
+            /* Received and skipped a cookie with a domain using too few
+               dots. */
+            badcookie=TRUE; /* mark this as a bad cookie */
+            infof(data, "skipped cookie with illegal dotcount domain: %s\n",
+                  whatptr);
           }
-          else if(Curl_raw_equal("domain", name)) {
-            /* note that this name may or may not have a preceding dot, but
-               we don't care about that, we treat the names the same anyway */
-
-            const char *domptr=whatptr;
-            const char *nextptr;
-            int dotcount=1;
-
-            /* Count the dots, we need to make sure that there are enough
-               of them. */
+          else {
+            /* Now, we make sure that our host is within the given domain,
+               or the given domain is not valid and thus cannot be set. */
 
             if('.' == whatptr[0])
-              /* don't count the initial dot, assume it */
-              domptr++;
-
-            do {
-              nextptr = strchr(domptr, '.');
-              if(nextptr) {
-                if(domptr != nextptr)
-                  dotcount++;
-                domptr = nextptr+1;
+              whatptr++; /* ignore preceding dot */
+
+            if(!domain || tailmatch(whatptr, domain)) {
+              const char *tailptr=whatptr;
+              if(tailptr[0] == '.')
+                tailptr++;
+              strstore(&co->domain, tailptr); /* don't prefix w/dots
+                                                 internally */
+              if(!co->domain) {
+                badcookie = TRUE;
+                break;
               }
-            } while(nextptr);
-
-            /* The original Netscape cookie spec defined that this domain name
-               MUST have three dots (or two if one of the seven holy TLDs),
-               but it seems that these kinds of cookies are in use "out there"
-               so we cannot be that strict. I've therefore lowered the check
-               to not allow less than two dots. */
-
-            if(dotcount < 2) {
-              /* Received and skipped a cookie with a domain using too few
-                 dots. */
-              badcookie=TRUE; /* mark this as a bad cookie */
-              infof(data, "skipped cookie with illegal dotcount domain: %s\n",
-                    whatptr);
+              co->tailmatch=TRUE; /* we always do that if the domain name was
+                                     given */
             }
             else {
-              /* Now, we make sure that our host is within the given domain,
-                 or the given domain is not valid and thus cannot be set. */
-
-              if('.' == whatptr[0])
-                whatptr++; /* ignore preceding dot */
-
-              if(!domain || tailmatch(whatptr, domain)) {
-                const char *tailptr=whatptr;
-                if(tailptr[0] == '.')
-                  tailptr++;
-                strstore(&co->domain, tailptr); /* don't prefix w/dots
-                                                   internally */
-                if(!co->domain) {
-                  badcookie = TRUE;
-                  break;
-                }
-                co->tailmatch=TRUE; /* we always do that if the domain name was
-                                       given */
-              }
-              else {
-                /* we did not get a tailmatch and then the attempted set domain
-                   is not a domain to which the current host belongs. Mark as
-                   bad. */
-                badcookie=TRUE;
-                infof(data, "skipped cookie with bad tailmatch domain: %s\n",
-                      whatptr);
-              }
-            }
-          }
-          else if(Curl_raw_equal("version", name)) {
-            strstore(&co->version, whatptr);
-            if(!co->version) {
-              badcookie = TRUE;
-              break;
+              /* we did not get a tailmatch and then the attempted set domain
+                 is not a domain to which the current host belongs. Mark as
+                 bad. */
+              badcookie=TRUE;
+              infof(data, "skipped cookie with bad tailmatch domain: %s\n",
+                    whatptr);
             }
           }
-          else if(Curl_raw_equal("max-age", name)) {
-            /* Defined in RFC2109:
-
-               Optional.  The Max-Age attribute defines the lifetime of the
-               cookie, in seconds.  The delta-seconds value is a decimal non-
-               negative integer.  After delta-seconds seconds elapse, the
-               client should discard the cookie.  A value of zero means the
-               cookie should be discarded immediately.
-
-             */
-            strstore(&co->maxage, whatptr);
-            if(!co->maxage) {
-              badcookie = TRUE;
-              break;
-            }
-            co->expires =
-              strtol((*co->maxage=='\"')?&co->maxage[1]:&co->maxage[0],NULL,10)
-                + (long)now;
+        }
+        else if(Curl_raw_equal("version", name)) {
+          strstore(&co->version, whatptr);
+          if(!co->version) {
+            badcookie = TRUE;
+            break;
           }
-          else if(Curl_raw_equal("expires", name)) {
-            strstore(&co->expirestr, whatptr);
-            if(!co->expirestr) {
-              badcookie = TRUE;
-              break;
-            }
-            /* Note that if the date couldn't get parsed for whatever reason,
-               the cookie will be treated as a session cookie */
-            co->expires = curl_getdate(what, &now);
-
-            /* Session cookies have expires set to 0 so if we get that back
-               from the date parser let's add a second to make it a
-               non-session cookie */
-            if(co->expires == 0)
-              co->expires = 1;
-            else if(co->expires < 0)
-              co->expires = 0;
+        }
+        else if(Curl_raw_equal("max-age", name)) {
+          /* Defined in RFC2109:
+
+             Optional.  The Max-Age attribute defines the lifetime of the
+             cookie, in seconds.  The delta-seconds value is a decimal non-
+             negative integer.  After delta-seconds seconds elapse, the
+             client should discard the cookie.  A value of zero means the
+             cookie should be discarded immediately.
+
+          */
+          strstore(&co->maxage, whatptr);
+          if(!co->maxage) {
+            badcookie = TRUE;
+            break;
           }
-          else if(!co->name) {
-            co->name = strdup(name);
-            co->value = strdup(whatptr);
-            if(!co->name || !co->value) {
-              badcookie = TRUE;
-              break;
-            }
+          co->expires =
+            strtol((*co->maxage=='\"')?&co->maxage[1]:&co->maxage[0],NULL,10)
+            + (long)now;
+        }
+        else if(Curl_raw_equal("expires", name)) {
+          strstore(&co->expirestr, whatptr);
+          if(!co->expirestr) {
+            badcookie = TRUE;
+            break;
           }
-          /*
-            else this is the second (or more) name we don't know
-            about! */
+          /* Note that if the date couldn't get parsed for whatever reason,
+             the cookie will be treated as a session cookie */
+          co->expires = curl_getdate(what, &now);
+
+          /* Session cookies have expires set to 0 so if we get that back
+             from the date parser let's add a second to make it a
+             non-session cookie */
+          if(co->expires == 0)
+            co->expires = 1;
+          else if(co->expires < 0)
+            co->expires = 0;
         }
-        else {
-          /* this is an "illegal" <what>=<this> pair */
+        else if(!co->name) {
+          co->name = strdup(name);
+          co->value = strdup(whatptr);
+          if(!co->name || !co->value) {
+            badcookie = TRUE;
+            break;
+          }
         }
+        /*
+          else this is the second (or more) name we don't know
+          about! */
       }
       else {
-        if(sscanf(ptr, "%" MAX_COOKIE_LINE_TXT "[^;\r\n]",
-                  what)) {
-          if(Curl_raw_equal("secure", what)) {
-            co->secure = TRUE;
-          }
-          else if(Curl_raw_equal("httponly", what)) {
-            co->httponly = TRUE;
-          }
-          /* else,
-             unsupported keyword without assign! */
-
-        }
+        /* this is an "illegal" <what>=<this> pair */
       }
+
       if(!semiptr || !*semiptr) {
         /* we already know there are no more cookies */
         semiptr = NULL;
diff --git a/tests/data/test31 b/tests/data/test31
index d06bc1180..5afe81df6 100644
--- a/tests/data/test31
+++ b/tests/data/test31
@@ -18,6 +18,28 @@ Content-Type: text/html
 Funny-head: yesyes

 Set-Cookie: foobar=name; domain=anything.com; path=/ ; secure

 Set-Cookie:ismatch=this  ; domain=127.0.0.1; path=/silly/

+Set-Cookie: sec1value=secure1  ; domain=127.0.0.1; path=/secure1/ ; secure

+Set-Cookie: sec2value=secure2  ; domain=127.0.0.1; path=/secure2/ ; secure=

+Set-Cookie: sec3value=secure3  ; domain=127.0.0.1; path=/secure3/ ; secure=

+Set-Cookie: sec4value=secure4  ; secure=; domain=127.0.0.1; path=/secure4/ ; 
+Set-Cookie: sec5value=secure5  ; secure; domain=127.0.0.1; path=/secure5/ ; 
+Set-Cookie: sec6value=secure6  ; secure ; domain=127.0.0.1; path=/secure6/ ; 
+Set-Cookie: sec7value=secure7  ; secure   ; domain=127.0.0.1; path=/secure7/ ; 
+Set-Cookie: sec8value=secure8  ; secure= ; domain=127.0.0.1; path=/secure8/ ; 
+Set-Cookie: secure=very1  ; secure=; domain=127.0.0.1; path=/secure9/; 
+Set-Cookie: httpo1=value1  ; domain=127.0.0.1; path=/p1/; httponly
+Set-Cookie: httpo2=value2  ; domain=127.0.0.1; path=/p2/; httponly=
+Set-Cookie: httpo3=value3  ; httponly; domain=127.0.0.1; path=/p3/;
+Set-Cookie: httpo4=value4  ; httponly=; domain=127.0.0.1; path=/p4/; 
+Set-Cookie: httponly=myvalue1  ; domain=127.0.0.1; path=/p4/; httponly
+Set-Cookie: httpandsec=myvalue2  ; domain=127.0.0.1; path=/p4/; httponly; secure
+Set-Cookie: httpandsec2=myvalue3; domain=127.0.0.1; path=/p4/; httponly=; secure
+Set-Cookie: httpandsec3=myvalue4  ; domain=127.0.0.1; path=/p4/; httponly; secure=
+Set-Cookie: httpandsec4=myvalue5  ; domain=127.0.0.1; path=/p4/; httponly=; secure=
+Set-Cookie: httpandsec5=myvalue6  ; domain=127.0.0.1; path=/p4/; secure; httponly=
+Set-Cookie: httpandsec6=myvalue7  ; domain=127.0.0.1; path=/p4/; secure=; httponly=
+Set-Cookie: httpandsec7=myvalue8  ; domain=127.0.0.1; path=/p4/; secure; httponly
+Set-Cookie: httpandsec8=myvalue9; domain=127.0.0.1; path=/p4/; secure=; httponly

 Set-Cookie: partmatch=present; domain=127.0.0.1 ; path=/;

 Set-Cookie:eat=this; domain=moo.foo.moo;

 Set-Cookie: eat=this-too; domain=.foo.moo;

@@ -69,6 +91,28 @@ Accept: */*
 # This file was generated by libcurl! Edit at your own risk.
 
 .127.0.0.1	TRUE	/silly/	FALSE	0	ismatch	this
+.127.0.0.1	TRUE	/secure1/	TRUE	0	sec1value	secure1
+.127.0.0.1	TRUE	/secure2/	TRUE	0	sec2value	secure2
+.127.0.0.1	TRUE	/secure3/	TRUE	0	sec3value	secure3
+.127.0.0.1	TRUE	/secure4/	TRUE	0	sec4value	secure4
+.127.0.0.1	TRUE	/secure5/	TRUE	0	sec5value	secure5
+.127.0.0.1	TRUE	/secure6/	TRUE	0	sec6value	secure6
+.127.0.0.1	TRUE	/secure7/	TRUE	0	sec7value	secure7
+.127.0.0.1	TRUE	/secure8/	TRUE	0	sec8value	secure8
+.127.0.0.1	TRUE	/secure9/	TRUE	0	secure	very1
+#HttpOnly_.127.0.0.1	TRUE	/p1/	FALSE	0	httpo1	value1
+#HttpOnly_.127.0.0.1	TRUE	/p2/	FALSE	0	httpo2	value2
+#HttpOnly_.127.0.0.1	TRUE	/p3/	FALSE	0	httpo3	value3
+#HttpOnly_.127.0.0.1	TRUE	/p4/	FALSE	0	httpo4	value4
+#HttpOnly_.127.0.0.1	TRUE	/p4/	FALSE	0	httponly	myvalue1
+#HttpOnly_.127.0.0.1	TRUE	/p4/	TRUE	0	httpandsec	myvalue2
+#HttpOnly_.127.0.0.1	TRUE	/p4/	TRUE	0	httpandsec2	myvalue3
+#HttpOnly_.127.0.0.1	TRUE	/p4/	TRUE	0	httpandsec3	myvalue4
+#HttpOnly_.127.0.0.1	TRUE	/p4/	TRUE	0	httpandsec4	myvalue5
+#HttpOnly_.127.0.0.1	TRUE	/p4/	TRUE	0	httpandsec5	myvalue6
+#HttpOnly_.127.0.0.1	TRUE	/p4/	TRUE	0	httpandsec6	myvalue7
+#HttpOnly_.127.0.0.1	TRUE	/p4/	TRUE	0	httpandsec7	myvalue8
+#HttpOnly_.127.0.0.1	TRUE	/p4/	TRUE	0	httpandsec8	myvalue9
 .127.0.0.1	TRUE	/	FALSE	0	partmatch	present
 127.0.0.1	FALSE	/we/want/	FALSE	2054030187	nodomain	value
 #HttpOnly_127.0.0.1	FALSE	/silly/	FALSE	0	magic	yessir