Bug 16662 - use of strstr() in spot_cookie() mis-identifies cookies in other cookie names or cookie values
Summary: use of strstr() in spot_cookie() mis-identifies cookies in other cookie names...
Status: RESOLVED DUPLICATE of bug 16661
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_usertrack (show other bugs)
Version: 2.0.44
Hardware: Other other
: P3 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL: http://www.manniwood.net/mod_usertrac...
Keywords:
Depends on:
Blocks:
 
Reported: 2003-02-01 00:26 UTC by Manni Wood
Modified: 2004-11-16 19:05 UTC (History)
0 users



Attachments
patch for spot_cookie() bug in mod_usertrack.c (3.78 KB, patch)
2003-02-01 00:27 UTC, Manni Wood
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manni Wood 2003-02-01 00:26:16 UTC
Example: If you have CookieName set to "ID", then use of strstr() in
spot_cookie() mod_usertrack.c will get false positives on the following sorts of
cookies: "MyID=binky", "MyCookie=IDExpired". This bug got "ported" from Apache
1.3: see bugs 16661, 11998, 8906, 8048, 5811. Here is a patch that has been
thoroughly tested (more details at
http://www.manniwood.net/mod_usertrack_patch.html):

--- mod_usertrack.c	2002-07-17 12:08:53.000000000 -0400
+++ mod_usertrack_2.0_fixed.c	2003-01-31 13:12:59.000000000 -0500
@@ -125,6 +125,8 @@
     cookie_type_e style;
     char *cookie_name;
     char *cookie_domain;
+    char *regexp_string;  /* used to compile regexp; save for debugging */
+    regex_t *regexp;  /* used to find usertrack cookie in cookie header */
 } cookie_dir_rec;
 
 /* Make Cookie: Now we have to generate something that is going to be
@@ -197,31 +199,44 @@
 {
     cookie_dir_rec *dcfg = ap_get_module_config(r->per_dir_config,
 						&usertrack_module);
-    const char *cookie;
-    const char *value;
+    const char *cookie_header;
+
+    /* There are only three possibilities from the regexp
+     * ^cookie_name=([^;]+)|;[ \t]+cookie_name=([^;]+)
+     * because $0 is always filled with the whole match, and $1 and $2 will
+     * be filled with either of the parenthesis matches. So, I
+     * allocate regm[3] to cover all these cases. */
+    regmatch_t regm[3];
+    int i;
 
     if (!dcfg->enabled) {
         return DECLINED;
     }
 
-    if ((cookie = apr_table_get(r->headers_in,
-                                (dcfg->style == CT_COOKIE2
-                                 ? "Cookie2"
-                                 : "Cookie"))))
-        if ((value = ap_strstr_c(cookie, dcfg->cookie_name))) {
-            char *cookiebuf, *cookieend;
-
-            value += strlen(dcfg->cookie_name) + 1;  /* Skip over the '=' */
-            cookiebuf = apr_pstrdup(r->pool, value);
-            cookieend = strchr(cookiebuf, ';');
-            if (cookieend)
-                *cookieend = '\0';      /* Ignore anything after a ; */
-
-            /* Set the cookie in a note, for logging */
-            apr_table_setn(r->notes, "cookie", cookiebuf);
+    if ((cookie_header = apr_table_get(r->headers_in,
+                                       (dcfg->style == CT_COOKIE2
+                                        ? "Cookie2"
+                                        : "Cookie")))) {
+       if (!ap_regexec(dcfg->regexp, cookie_header, dcfg->regexp->re_nsub + 1,
regm, 0)) {
+           char *cookieval = NULL;
+           /* Our regexp,
+            * ^cookie_name=([^;]+)|;[ \t]+cookie_name=([^;]+)
+            * only allows for $1 or $2 to be available. ($0 is always
+            * filled with the entire matched expression, not just
+            * the part in parentheses.) So just check for either one
+            * and assign to cookieval if present. */
+           if (regm[1].rm_so != -1) {
+               cookieval = ap_pregsub(r->pool, "$1", cookie_header,
dcfg->regexp->re_nsub + 1, regm);
+           }
+           if (regm[2].rm_so != -1) {
+               cookieval = ap_pregsub(r->pool, "$2", cookie_header,
dcfg->regexp->re_nsub + 1, regm);
+           }
+           /* Set the cookie in a note, for logging */
+           apr_table_setn(r->notes, "cookie", cookieval);
 
-            return DECLINED;    /* There's already a cookie, no new one */
-        }
+           return DECLINED;    /* There's already a cookie, no new one */
+       }
+    }
     make_cookie(r);
     return OK;                  /* We set our cookie */
 }
@@ -330,7 +345,20 @@
 {
     cookie_dir_rec *dcfg = (cookie_dir_rec *) mconfig;
 
+    /* The goal is to end up with this regexp,
+     * ^cookie_name=([^;]+)|;[ \t]+cookie_name=([^;]+)
+     * with cookie_name
+     * obviously substituted with the real cookie name set by the
+     * user in httpd.conf. */
+    dcfg->regexp_string = apr_pstrcat(cmd->pool, "^", name, "=([^;]+)|;[ \t]+",
name, "=([^;]+)", NULL);
+
     dcfg->cookie_name = apr_pstrdup(cmd->pool, name);
+
+    dcfg->regexp = ap_pregcomp(cmd->pool, dcfg->regexp_string, REG_EXTENDED);
+    if (dcfg->regexp == NULL) {
+       return "Regular expression could not be compiled.";
+    }
+
     return NULL;
 }
Comment 1 Manni Wood 2003-02-01 00:27:27 UTC
Created attachment 4675 [details]
patch for spot_cookie() bug in mod_usertrack.c
Comment 2 Cliff Woolley 2003-09-23 22:38:13 UTC

*** This bug has been marked as a duplicate of 16661 ***