ASF Bugzilla – Attachment 36501 Details for
Bug 63288
mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Fix parsing of quoted Cache-Control token arguments
2.4.x-mod_cache-PR63288.patch (text/plain), 18.76 KB, created by
Yann Ylavic
on 2019-03-28 21:18:05 UTC
(
hide
)
Description:
Fix parsing of quoted Cache-Control token arguments
Filename:
MIME Type:
Creator:
Yann Ylavic
Created:
2019-03-28 21:18:05 UTC
Size:
18.76 KB
patch
obsolete
>Merge r1856490, r1856491, r1856493, r1856500 from trunk: > >Provide TEST_CHAR marco in test_char.h > >For (internal) usage outside server/util.c, mod_log_forensic for now >and mod_cache (T_HTTP_TOKEN_STOP) in a few... > > >Follow up to r1856490: missing one mod_log_forensic test_char_table case. > > >mod_cache: Fix parsing of quoted Cache-Control token arguments. PR 63288. > >Make cache_strqtok() return both the token and its unquoted argument (if any), >or an error if the parsing fails. > >Cache-Control integer values (max-age, max-stale, ...) can then be parsed w/o >taking care of the (optional) quoting. > >Suggested by: fielding > > >mod_cache: follow up to r1856493: always terminate cache_strqtok() returns. > >Index: modules/cache/cache_storage.c >=================================================================== >--- modules/cache/cache_storage.c (revision 1856500) >+++ modules/cache/cache_storage.c (working copy) >@@ -275,13 +275,15 @@ int cache_select(cache_request_rec *cache, request > * > * RFC2616 13.6 and 14.44 describe the Vary mechanism. > */ >- vary = cache_strqtok( >- apr_pstrdup(r->pool, >- cache_table_getm(r->pool, h->resp_hdrs, "Vary")), >- CACHE_SEPARATOR, &last); >- while (vary) { >+ for (rv = cache_strqtok(apr_pstrdup(r->pool, >+ cache_table_getm(r->pool, >+ h->resp_hdrs, >+ "Vary")), >+ &vary, NULL, &last); >+ rv == APR_SUCCESS; >+ rv = cache_strqtok(NULL, &vary, NULL, &last)) >+ { > const char *h1, *h2; >- > /* > * is this header in the request and the header in the cached > * request identical? If not, we give up and do a straight get >@@ -301,7 +303,6 @@ int cache_select(cache_request_rec *cache, request > mismatch = 1; > break; > } >- vary = cache_strqtok(NULL, CACHE_SEPARATOR, &last); > } > > /* no vary match, try next provider */ >Index: modules/cache/cache_util.c >=================================================================== >--- modules/cache/cache_util.c (revision 1856500) >+++ modules/cache/cache_util.c (working copy) >@@ -19,6 +19,8 @@ > #include "cache_util.h" > #include <ap_provider.h> > >+#include "test_char.h" >+ > APLOG_USE_MODULE(cache); > > /* -------------------------------------------------------------- */ >@@ -923,75 +925,86 @@ CACHE_DECLARE(char *)ap_cache_generate_name(apr_po > } > > /** >- * String tokenizer that ignores separator characters within quoted strings >- * and escaped characters, as per RFC2616 section 2.2. >+ * String tokenizer per RFC 7234 section 5.2 (1#token[=["]arg["]]). >+ * If any (and arg not NULL), the argument is also returned (unquoted). > */ >-char *cache_strqtok(char *str, const char *sep, char **last) >+apr_status_t cache_strqtok(char *str, char **token, char **arg, char **last) > { >- char *token; >+#define CACHE_TOKEN_SEPS "\t ," >+ apr_status_t rv = APR_SUCCESS; > int quoted = 0; >+ char *wpos; > > if (!str) { /* subsequent call */ > str = *last; /* start where we left off */ > } >- > if (!str) { /* no more tokens */ >- return NULL; >+ return APR_EOF; > } > >- /* skip characters in sep (will terminate at '\0') */ >- while (*str && ap_strchr_c(sep, *str)) { >+ /* skip separators (will terminate at '\0') */ >+ while (*str && TEST_CHAR(*str, T_HTTP_TOKEN_STOP)) { >+ if (!ap_strchr_c(CACHE_TOKEN_SEPS, *str)) { >+ return APR_EINVAL; >+ } > ++str; > } >- > if (!*str) { /* no more tokens */ >- return NULL; >+ return APR_EOF; > } > >- token = str; >+ *token = str; >+ if (arg) { >+ *arg = NULL; >+ } > > /* skip valid token characters to terminate token and > * prepare for the next call (will terminate at '\0) >- * on the way, ignore all quoted strings, and within >+ * on the way, handle quoted strings, and within > * quoted strings, escaped characters. > */ >- *last = token; >- while (**last) { >+ for (wpos = str; *str; ++str) { > if (!quoted) { >- if (**last == '\"' && !ap_strchr_c(sep, '\"')) { >+ if (*str == '"') { > quoted = 1; >- ++*last; >+ continue; > } >- else if (!ap_strchr_c(sep, **last)) { >- ++*last; >+ if (arg && !*arg && *str == '=') { >+ *wpos++ = '\0'; >+ *arg = wpos; >+ continue; > } >- else { >+ if (TEST_CHAR(*str, T_HTTP_TOKEN_STOP)) { > break; > } > } > else { >- if (**last == '\"') { >- quoted = 0; >- ++*last; >+ if (*str == '"') { >+ ++str; >+ break; > } >- else if (**last == '\\') { >- ++*last; >- if (**last) { >- ++*last; >- } >+ if (*str == '\\' && *(str + 1)) { >+ ++str; > } >- else { >- ++*last; >- } > } >+ *wpos++ = *str; > } > >- if (**last) { >- **last = '\0'; >- ++*last; >+ /* anything after should be trailing OWS or comma */ >+ for (; *str; ++str) { >+ if (*str == ',') { >+ ++str; >+ break; >+ } >+ if (*str != '\t' && *str != ' ') { >+ rv = APR_EINVAL; >+ break; >+ } > } > >- return token; >+ *wpos = '\0'; >+ *last = str; >+ return rv; > } > > /** >@@ -1003,6 +1016,7 @@ int ap_cache_control(request_rec *r, cache_control > const char *cc_header, const char *pragma_header, apr_table_t *headers) > { > char *last; >+ apr_status_t rv; > > if (cc->parsed) { > return cc->cache_control || cc->pragma; >@@ -1015,33 +1029,35 @@ int ap_cache_control(request_rec *r, cache_control > cc->s_maxage_value = -1; > > if (pragma_header) { >- char *header = apr_pstrdup(r->pool, pragma_header); >- const char *token = cache_strqtok(header, CACHE_SEPARATOR, &last); >- while (token) { >+ char *header = apr_pstrdup(r->pool, pragma_header), *token; >+ for (rv = cache_strqtok(header, &token, NULL, &last); >+ rv == APR_SUCCESS; >+ rv = cache_strqtok(NULL, &token, NULL, &last)) { > if (!ap_cstr_casecmp(token, "no-cache")) { > cc->no_cache = 1; > } >- token = cache_strqtok(NULL, CACHE_SEPARATOR, &last); > } > cc->pragma = 1; > } > > if (cc_header) { >- char *endp; >- apr_off_t offt; >- char *header = apr_pstrdup(r->pool, cc_header); >- const char *token = cache_strqtok(header, CACHE_SEPARATOR, &last); >- while (token) { >+ char *header = apr_pstrdup(r->pool, cc_header), *token, *arg; >+ for (rv = cache_strqtok(header, &token, &arg, &last); >+ rv == APR_SUCCESS; >+ rv = cache_strqtok(NULL, &token, &arg, &last)) { >+ char *endp; >+ apr_off_t offt; >+ > switch (token[0]) { > case 'n': >- case 'N': { >- if (!ap_cstr_casecmpn(token, "no-cache", 8)) { >- if (token[8] == '=') { >+ case 'N': >+ if (!ap_cstr_casecmp(token, "no-cache")) { >+ if (!arg) { >+ cc->no_cache = 1; >+ } >+ else { > cc->no_cache_header = 1; > } >- else if (!token[8]) { >- cc->no_cache = 1; >- } > } > else if (!ap_cstr_casecmp(token, "no-store")) { > cc->no_store = 1; >@@ -1050,13 +1066,12 @@ int ap_cache_control(request_rec *r, cache_control > cc->no_transform = 1; > } > break; >- } >+ > case 'm': >- case 'M': { >- if (!ap_cstr_casecmpn(token, "max-age", 7)) { >- if (token[7] == '=' >- && !apr_strtoff(&offt, token + 8, &endp, 10) >- && endp > token + 8 && !*endp) { >+ case 'M': >+ if (arg && !ap_cstr_casecmp(token, "max-age")) { >+ if (!apr_strtoff(&offt, arg, &endp, 10) >+ && endp > arg && !*endp) { > cc->max_age = 1; > cc->max_age_value = offt; > } >@@ -1064,59 +1079,56 @@ int ap_cache_control(request_rec *r, cache_control > else if (!ap_cstr_casecmp(token, "must-revalidate")) { > cc->must_revalidate = 1; > } >- else if (!ap_cstr_casecmpn(token, "max-stale", 9)) { >- if (token[9] == '=' >- && !apr_strtoff(&offt, token + 10, &endp, 10) >- && endp > token + 10 && !*endp) { >+ else if (!ap_cstr_casecmp(token, "max-stale")) { >+ if (!arg) { > cc->max_stale = 1; >- cc->max_stale_value = offt; >+ cc->max_stale_value = -1; > } >- else if (!token[9]) { >+ else if (!apr_strtoff(&offt, arg, &endp, 10) >+ && endp > arg && !*endp) { > cc->max_stale = 1; >- cc->max_stale_value = -1; >+ cc->max_stale_value = offt; > } > } >- else if (!ap_cstr_casecmpn(token, "min-fresh", 9)) { >- if (token[9] == '=' >- && !apr_strtoff(&offt, token + 10, &endp, 10) >- && endp > token + 10 && !*endp) { >+ else if (arg && !ap_cstr_casecmp(token, "min-fresh")) { >+ if (!apr_strtoff(&offt, arg, &endp, 10) >+ && endp > arg && !*endp) { > cc->min_fresh = 1; > cc->min_fresh_value = offt; > } > } > break; >- } >+ > case 'o': >- case 'O': { >+ case 'O': > if (!ap_cstr_casecmp(token, "only-if-cached")) { > cc->only_if_cached = 1; > } > break; >- } >+ > case 'p': >- case 'P': { >+ case 'P': > if (!ap_cstr_casecmp(token, "public")) { > cc->public = 1; > } >- else if (!ap_cstr_casecmpn(token, "private", 7)) { >- if (token[7] == '=') { >+ else if (!ap_cstr_casecmp(token, "private")) { >+ if (!arg) { >+ cc->private = 1; >+ } >+ else { > cc->private_header = 1; > } >- else if (!token[7]) { >- cc->private = 1; >- } > } > else if (!ap_cstr_casecmp(token, "proxy-revalidate")) { > cc->proxy_revalidate = 1; > } > break; >- } >+ > case 's': >- case 'S': { >- if (!ap_cstr_casecmpn(token, "s-maxage", 8)) { >- if (token[8] == '=' >- && !apr_strtoff(&offt, token + 9, &endp, 10) >- && endp > token + 9 && !*endp) { >+ case 'S': >+ if (arg && !ap_cstr_casecmp(token, "s-maxage")) { >+ if (!apr_strtoff(&offt, arg, &endp, 10) >+ && endp > arg && !*endp) { > cc->s_maxage = 1; > cc->s_maxage_value = offt; > } >@@ -1123,8 +1135,6 @@ int ap_cache_control(request_rec *r, cache_control > } > break; > } >- } >- token = cache_strqtok(NULL, CACHE_SEPARATOR, &last); > } > cc->cache_control = 1; > } >@@ -1139,48 +1149,44 @@ int ap_cache_control(request_rec *r, cache_control > static int cache_control_remove(request_rec *r, const char *cc_header, > apr_table_t *headers) > { >- char *last, *slast; >+ char *last, *slast, *sheader; > int found = 0; > > if (cc_header) { >- char *header = apr_pstrdup(r->pool, cc_header); >- char *token = cache_strqtok(header, CACHE_SEPARATOR, &last); >- while (token) { >+ apr_status_t rv; >+ char *header = apr_pstrdup(r->pool, cc_header), *token, *arg; >+ for (rv = cache_strqtok(header, &token, &arg, &last); >+ rv == APR_SUCCESS; >+ rv = cache_strqtok(NULL, &token, &arg, &last)) { >+ if (!arg) { >+ continue; >+ } >+ > switch (token[0]) { > case 'n': >- case 'N': { >- if (!ap_cstr_casecmpn(token, "no-cache", 8)) { >- if (token[8] == '=') { >- const char *header = cache_strqtok(token + 9, >- CACHE_SEPARATOR "\"", &slast); >- while (header) { >- apr_table_unset(headers, header); >- header = cache_strqtok(NULL, CACHE_SEPARATOR "\"", >- &slast); >- } >- found = 1; >+ case 'N': >+ if (!ap_cstr_casecmp(token, "no-cache")) { >+ for (rv = cache_strqtok(arg, &sheader, NULL, &slast); >+ rv == APR_SUCCESS; >+ rv = cache_strqtok(NULL, &sheader, NULL, &slast)) { >+ apr_table_unset(headers, sheader); > } >+ found = 1; > } > break; >- } >+ > case 'p': >- case 'P': { >- if (!ap_cstr_casecmpn(token, "private", 7)) { >- if (token[7] == '=') { >- const char *header = cache_strqtok(token + 8, >- CACHE_SEPARATOR "\"", &slast); >- while (header) { >- apr_table_unset(headers, header); >- header = cache_strqtok(NULL, CACHE_SEPARATOR "\"", >- &slast); >- } >- found = 1; >+ case 'P': >+ if (!ap_cstr_casecmp(token, "private")) { >+ for (rv = cache_strqtok(arg, &sheader, NULL, &slast); >+ rv == APR_SUCCESS; >+ rv = cache_strqtok(NULL, &sheader, NULL, &slast)) { >+ apr_table_unset(headers, sheader); > } >+ found = 1; > } > break; > } >- } >- token = cache_strqtok(NULL, CACHE_SEPARATOR, &last); > } > } > >Index: modules/cache/cache_util.h >=================================================================== >--- modules/cache/cache_util.h (revision 1856500) >+++ modules/cache/cache_util.h (working copy) >@@ -99,7 +99,6 @@ extern "C" { > #define CACHE_LOCKNAME_KEY "mod_cache-lockname" > #define CACHE_LOCKFILE_KEY "mod_cache-lockfile" > #define CACHE_CTX_KEY "mod_cache-ctx" >-#define CACHE_SEPARATOR ", \t" > > /** > * cache_util.c >@@ -316,10 +315,10 @@ const char *cache_table_getm(apr_pool_t *p, const > const char *key); > > /** >- * String tokenizer that ignores separator characters within quoted strings >- * and escaped characters, as per RFC2616 section 2.2. >+ * String tokenizer per RFC 7234 section 5.2 (1#token[=["]arg["]]). >+ * If any (and arg not NULL), the argument is also returned (unquoted). > */ >-char *cache_strqtok(char *str, const char *sep, char **last); >+apr_status_t cache_strqtok(char *str, char **token, char **arg, char **last); > > /** > * Merge err_headers_out into headers_out and add request's Content-Type and >Index: modules/loggers/mod_log_forensic.c >=================================================================== >--- modules/loggers/mod_log_forensic.c (revision 1856500) >+++ modules/loggers/mod_log_forensic.c (working copy) >@@ -123,7 +123,7 @@ static char *log_escape(char *q, const char *e, co > { > for ( ; *p ; ++p) { > ap_assert(q < e); >- if (test_char_table[*(unsigned char *)p]&T_ESCAPE_FORENSIC) { >+ if (TEST_CHAR(*p, T_ESCAPE_FORENSIC)) { > ap_assert(q+2 < e); > *q++ = '%'; > ap_bin2hex(p, 1, q); >@@ -151,7 +151,7 @@ static int count_string(const char *p) > int n; > > for (n = 0 ; *p ; ++p, ++n) >- if (test_char_table[*(unsigned char *)p]&T_ESCAPE_FORENSIC) >+ if (TEST_CHAR(*p, T_ESCAPE_FORENSIC)) > n += 2; > return n; > } >Index: server/gen_test_char.c >=================================================================== >--- server/gen_test_char.c (revision 1856500) >+++ server/gen_test_char.c (working copy) >@@ -167,7 +167,16 @@ int main(int argc, char *argv[]) > printf("0x%03x%c", flags, (c < 255) ? ',' : ' '); > } > >- printf("\n};\n"); >+ printf("\n};\n\n"); > >+ printf( >+ "/* we assume the folks using this ensure 0 <= c < 256... which means\n" >+ " * you need a cast to (unsigned char) first, you can't just plug a\n" >+ " * char in here and get it to work, because if char is signed then it\n" >+ " * will first be sign extended.\n" >+ " */\n" >+ "#define TEST_CHAR(c, f) (test_char_table[(unsigned char)(c)] & (f))\n" >+ ); >+ > return 0; > } >Index: server/util.c >=================================================================== >--- server/util.c (revision 1856500) >+++ server/util.c (working copy) >@@ -74,13 +74,6 @@ > */ > #include "test_char.h" > >-/* we assume the folks using this ensure 0 <= c < 256... which means >- * you need a cast to (unsigned char) first, you can't just plug a >- * char in here and get it to work, because if char is signed then it >- * will first be sign extended. >- */ >-#define TEST_CHAR(c, f) (test_char_table[(unsigned char)(c)] & (f)) >- > /* Win32/NetWare/OS2 need to check for both forward and back slashes > * in ap_getparents() and ap_escape_url. > */ >Index: . >=================================================================== >--- . (revision 1856500) >+++ . (working copy) > >Property changes on: . >___________________________________________________________________ >Modified: svn:mergeinfo >## -0,0 +0,1 ## > Merged /httpd/httpd/trunk:r1856490-1856491,1856493,1856500
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
Attachments on
bug 63288
:
36501
|
36502