The problem that this patch solves is one where cookie names are mis-identified

by mod_usertrack. This is because of the use of strstr() in spot_cookie() the
original mod_usertrack.c to find the name of the cookie. strstr(), by virtue of
looking for a substring instead of an exact match, can mis-identify the cookie
"MyID" as the cookie "ID" or "My". So, if you were looking for the value of the
cookie "ID", but only the cookie "MyID" was returned by the browser,
mod_usertrack.c would return the value of the "MyID" cookie in place of the
"ID" you were looking for.

Even more seriously, because strstr is invoked before the cookie name is
separated from its cookie value, a cookie and value like
"myCookie=thisisnotIDeal" will be a false positive if you told mod_usertrack
the cookie name was ID. Furthermore, using this example, "eal" will get logged
as the value of the cookie; now that strstr has incorrectly identified the
substring "ID" as the cookie name, the following "e" (assumed to be the "="
sign) gets discarded, and the remaining content used as the value of
the cookie.

Replacing the strstr() with a more robust regex match fixes this problem.

PR:    16661
Submitted by:   Manni Wood <manniwood@planet-save.com>

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@101306 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 12df1c940404a0cf047f7e97bba8629b0b8a1692..ae77aa6d0666ce5bc18bd647d666349396eb353c 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -2,6 +2,10 @@ Changes with Apache 2.1.0-dev
 
   [Remove entries to the current 2.0 section below, when backported]
 
+  *) Fixed mod_usertrack to not get false positive matches on the
+     user-tracking cookie's name.  PR 16661.
+     [Manni Wood <manniwood@planet-save.com>]
+
   *) Fix mod_info to use the real config file name, not the default
      config file name.  [Aryeh Katz <aryeh@secured-services.com>]
 

diff --git a/modules/metadata/mod_usertrack.c b/modules/metadata/mod_usertrack.c
index ad144ea9e6661da295bfc598b60209776ed700ca..03516d9afb1c9e4d5a8e1ed569146d7e9ea70753 100644 (file)
--- a/modules/metadata/mod_usertrack.c
+++ b/modules/metadata/mod_usertrack.c
@@ -125,6 +125,8 @@ typedef struct {
     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
@@ -193,36 +195,48 @@ static void make_cookie(request_rec *r)
     return;
 }
 
+/* dcfg->regexp is "^cookie_name=([^;]+)|;[ \t]+cookie_name=([^;]+)",
+ * which has three subexpressions, $0..$2 */
+#define NUM_SUBS 3
+
 static int spot_cookie(request_rec *r)
 {
     cookie_dir_rec *dcfg = ap_get_module_config(r->per_dir_config,
                                                &usertrack_module);
-    const char *cookie;
-    const char *value;
+    const char *cookie_header;
+    regmatch_t regm[NUM_SUBS];
 
     /* Do not run in subrequests */
     if (!dcfg->enabled || r->main) {
         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 ; */
-
+    if ((cookie_header = apr_table_get(r->headers_in,
+                                       (dcfg->style == CT_COOKIE2
+                                        ? "Cookie2"
+                                        : "Cookie")))) {
+        if (!ap_regexec(dcfg->regexp, cookie_header, NUM_SUBS, 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,
+                                       NUM_SUBS, regm);
+            }
+            if (regm[2].rm_so != -1) {
+                cookieval = ap_pregsub(r->pool, "$2", cookie_header,
+                                       NUM_SUBS, regm);
+            }
             /* Set the cookie in a note, for logging */
-            apr_table_setn(r->notes, "cookie", cookiebuf);
+            apr_table_setn(r->notes, "cookie", cookieval);
 
             return DECLINED;    /* There's already a cookie, no new one */
         }
+    }
     make_cookie(r);
     return OK;                  /* We set our cookie */
 }
@@ -331,7 +345,26 @@ static const char *set_cookie_name(cmd_parms *cmd, void *mconfig,
 {
     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.";
+    }
+    if (dcfg->regexp->re_nsub + 1 != NUM_SUBS) {
+        return apr_pstrcat(cmd->pool, "Invalid cookie name \"",
+                           name, "\"", NULL);
+    }
+
     return NULL;
 }