You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Manni Wood <mw...@digitas.com> on 2003/02/21 21:59:11 UTC
mod_usertrack bugfix patch
Hi.
I am submitting a patch to mod_usertrack for both Apache 2.0 and 1.3 for your consideration.
The patch fixes a bug where the use of strstr() to find the name of the cookie in the cookieheader can accidentally "find" the name of the cookie in what is actually the contents of a cookie if the contents happen to contain the name of the user tracking cookie.
The patch relies on a robust regexp to find the cookiename in the header instead of strstr().
More details are at http://www.manniwood.net/mod_usertrack_patch.html. Patches for 2.0 and 1.3 below.
-Manni
--------------------------------------------
2.0:
--------------------------------------------
--- 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;
}
--------------------------------------------
1.3:
--------------------------------------------
--- mod_usertrack.c 2002-03-13 16:05:34.000000000 -0500
+++ mod_usertrack_1.3_fixed.c 2003-01-31 12:10:46.000000000 -0500
@@ -119,6 +119,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;
/* Define this to allow post-2000 cookies. Cookies use two-digit dates,
@@ -250,31 +252,44 @@
{
cookie_dir_rec *dcfg = ap_get_module_config(r->per_dir_config,
&usertrack_module);
- const char *cookie;
- 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 = ap_table_get(r->headers_in,
- (dcfg->style == CT_COOKIE2
- ? "Cookie2"
- : "Cookie"))))
- if ((value = strstr(cookie, dcfg->cookie_name))) {
- char *cookiebuf, *cookieend;
-
- value += strlen(dcfg->cookie_name) + 1; /* Skip over the '=' */
- cookiebuf = ap_pstrdup(r->pool, value);
- cookieend = strchr(cookiebuf, ';');
- if (cookieend)
- *cookieend = '\0'; /* Ignore anything after a ; */
-
- /* Set the cookie in a note, for logging */
- ap_table_setn(r->notes, "cookie", cookiebuf);
+ if ((cookie_header = ap_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 */
+ ap_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 */
}
@@ -382,7 +397,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 = ap_pstrcat(cmd->pool, "^", name, "=([^;]+)|;[ \t]+", name, "=([^;]+)", NULL);
+
dcfg->cookie_name = ap_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;
}
--------------------------------------------
Manni Wood, Programmer, Digitas
800 Boylston Street, Boston, MA, 02199
617 867 1881 mwood@digitas.com
"Most men would rather die than think. Many do." --Bertrand Russell
Re: mod_usertrack bugfix patch
Posted by Sander Holthaus - Orange XL <in...@orangexl.com>.
> performance concern a.k.a. dumb question... is a regexp required for
> fixing this problem?
Same thought here. I do the same in PERL through substr instead of RegExp
'cause of the performance.
Kind Regards,
Sander Holthaus.
Re: mod_usertrack bugfix patch
Posted by Jeff Trawick <tr...@attglobal.net>.
Manni Wood wrote:
> I am submitting a patch to mod_usertrack for both Apache 2.0 and 1.3
> for your consideration.
> The patch fixes a bug where the use of strstr() to find the name of
> the cookie in the cookieheader can accidentally "find" the name of the
> cookie in what is actually the contents of a cookie if the contents
> happen to contain the name of the user tracking cookie.
>
> The patch relies on a robust regexp to find the cookiename in the
> header instead of strstr().
performance concern a.k.a. dumb question... is a regexp required for
fixing this problem?
> More details are at http://www.manniwood.net/mod_usertrack_patch.html.
excellent resource... I applaud you for taking the initiative to deal
with this so carefully