cookie: address secure domain overlay

Bug: https://hackerone.com/reports/1560324
Co-authored-by: Daniel Stenberg
Closes #8840

diff --git a/lib/cookie.c b/lib/cookie.c
index 0c2d49b478ca3ee1093b1d66764061e24518b69c..ac53d278481b2d662df7ce20341863bb22f3cc40 100644 (file)
--- a/lib/cookie.c
+++ b/lib/cookie.c
@@ -468,6 +468,8 @@ Curl_cookie_add(struct Curl_easy *data,
   struct Cookie *clist;
   struct Cookie *co;
   struct Cookie *lastc = NULL;
+  struct Cookie *replace_co = NULL;
+  struct Cookie *replace_clist;
   time_t now = time(NULL);
   bool replace_old = FALSE;
   bool badcookie = FALSE; /* cookies are good by default. mmmmm yummy */
@@ -1020,12 +1022,53 @@ Curl_cookie_add(struct Curl_easy *data,
   }
 #endif
 
+  /* A non-secure cookie may not overlay an existing secure cookie. */
   myhash = cookiehash(co->domain);
   clist = c->cookies[myhash];
-  replace_old = FALSE;
   while(clist) {
     if(strcasecompare(clist->name, co->name)) {
       /* the names are identical */
+      bool matching_domains = FALSE;
+
+      if(clist->domain && co->domain) {
+        if(strcasecompare(clist->domain, co->domain))
+          /* The domains are identical */
+          matching_domains = TRUE;
+      }
+      else if(!clist->domain && !co->domain)
+        matching_domains = TRUE;
+
+      if(matching_domains && /* the domains were identical */
+         clist->spath && co->spath && /* both have paths */
+         clist->secure && !co->secure && !secure) {
+        size_t cllen;
+        const char *sep;
+
+        /*
+         * A non-secure cookie may not overlay an existing secure cookie.
+         * For an existing cookie "a" with path "/login", refuse a new
+         * cookie "a" with for example path "/login/en", while the path
+         * "/loginhelper" is ok.
+         */
+
+        sep = strchr(clist->spath + 1, '/');
+
+        if(sep)
+          cllen = sep - clist->spath;
+        else
+          cllen = strlen(clist->spath);
+
+        if(strncasecompare(clist->spath, co->spath, cllen)) {
+          infof(data, "cookie '%s' for domain '%s' dropped, would "
+                "overlay an existing cookie", co->name, co->domain);
+          freecookie(co);
+          return NULL;
+        }
+      }
+    }
+
+    if(!replace_co && strcasecompare(clist->name, co->name)) {
+      /* the names are identical */
 
       if(clist->domain && co->domain) {
         if(strcasecompare(clist->domain, co->domain) &&
@@ -1040,30 +1083,7 @@ Curl_cookie_add(struct Curl_easy *data,
         /* the domains were identical */
 
         if(clist->spath && co->spath) {
-          if(clist->secure && !co->secure && !secure) {
-            size_t cllen;
-            const char *sep;
-
-            /*
-             * A non-secure cookie may not overlay an existing secure cookie.
-             * For an existing cookie "a" with path "/login", refuse a new
-             * cookie "a" with for example path "/login/en", while the path
-             * "/loginhelper" is ok.
-             */
-
-            sep = strchr(clist->spath + 1, '/');
-
-            if(sep)
-              cllen = sep - clist->spath;
-            else
-              cllen = strlen(clist->spath);
-
-            if(strncasecompare(clist->spath, co->spath, cllen)) {
-              freecookie(co);
-              return NULL;
-            }
-          }
-          else if(strcasecompare(clist->spath, co->spath))
+          if(strcasecompare(clist->spath, co->spath))
             replace_old = TRUE;
           else
             replace_old = FALSE;
@@ -1085,42 +1105,37 @@ Curl_cookie_add(struct Curl_easy *data,
         freecookie(co);
         return NULL;
       }
-
       if(replace_old) {
-        co->next = clist->next; /* get the next-pointer first */
-
-        /* when replacing, creationtime is kept from old */
-        co->creationtime = clist->creationtime;
-
-        /* then free all the old pointers */
-        free(clist->name);
-        free(clist->value);
-        free(clist->domain);
-        free(clist->path);
-        free(clist->spath);
-        free(clist->expirestr);
-        free(clist->version);
-        free(clist->maxage);
-
-        *clist = *co;  /* then store all the new data */
-
-        free(co);   /* free the newly allocated memory */
-        co = clist; /* point to the previous struct instead */
-
-        /*
-         * We have replaced a cookie, now skip the rest of the list but make
-         * sure the 'lastc' pointer is properly set
-         */
-        do {
-          lastc = clist;
-          clist = clist->next;
-        } while(clist);
-        break;
+        replace_co = co;
+        replace_clist = clist;
       }
     }
     lastc = clist;
     clist = clist->next;
   }
+  if(replace_co) {
+    co = replace_co;
+    clist = replace_clist;
+    co->next = clist->next; /* get the next-pointer first */
+
+    /* when replacing, creationtime is kept from old */
+    co->creationtime = clist->creationtime;
+
+    /* then free all the old pointers */
+    free(clist->name);
+    free(clist->value);
+    free(clist->domain);
+    free(clist->path);
+    free(clist->spath);
+    free(clist->expirestr);
+    free(clist->version);
+    free(clist->maxage);
+
+    *clist = *co;  /* then store all the new data */
+
+    free(co);   /* free the newly allocated memory */
+    co = clist;
+  }
 
   if(c->running)
     /* Only show this when NOT reading the cookies from a file */