Bug 63288 - mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age
Summary: mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like m...
Status: NEW
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_cache (show other bugs)
Version: 2.4.38
Hardware: All All
: P2 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords: FixedInTrunk
Depends on:
Blocks:
 
Reported: 2019-03-26 11:11 UTC by Roy T. Fielding
Modified: 2019-12-01 16:37 UTC (History)
1 user (show)



Attachments
Fix parsing of quoted Cache-Control token arguments (18.76 KB, patch)
2019-03-28 21:18 UTC, Yann Ylavic
Details | Diff
Fix parsing of quoted Cache-Control token arguments (v2) (19.45 KB, patch)
2019-03-28 22:49 UTC, Yann Ylavic
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roy T. Fielding 2019-03-26 11:11:49 UTC
I found a bug that causes Apache httpd's cache modules to fail on quoted max-age values. It correctly parses the field to extract either max-age=100 or max-age="100", but when it converts the value to a large integer it assumes the value is unquoted.

This is in modules/cache/cache_util.c: ap_cache_control()

                if (!ap_cstr_casecmpn(token, "max-age", 7)) {
                    if (token[7] == '='
                            && !apr_strtoff(&offt, token + 8, &endp, 10)
                            && endp > token + 8 && !*endp) {
                        cc->max_age = 1;
                        cc->max_age_value = offt;
                    }
                }

where the value of token is produced by cache_strqtok() as either

   max-age=100
   max-age="100"

but the above code assumes only the former is possible and tries to convert the DQUOTE into an off_t.

The same bug is present for the s-max-age, max-stale, and min-fresh parameters.

A reasonable fix would be to check if token[8] == '"' and then extract just the quoted value. A better fix would use a common parameter parser that returns both the parameter name and the optional value unquoted at the same time (rather than  returning the entire substring and requiring the caller to reparse it).

See also https://github.com/httpwg/http-core/issues/128
Comment 1 Mark Nottingham 2019-03-26 15:32:28 UTC
Relevant test:
  https://cache-tests.fyi/?id=freshness-max-age-quoted
Comment 2 Yann Ylavic 2019-03-28 09:44:02 UTC
Looking into this, the current cache_strqtok() seems to consider that token w/o values can be quoted as well (e.g. Cache-Control: "private"), which the BNF in rfc7234#section-5.2 does not mention.
If cache_strqtok() were to return both tokens (name, value), the value only should be (un)quoted right?
Comment 3 Julian Reschke 2019-03-28 10:09:02 UTC
> Looking into this, the current cache_strqtok() seems to consider that token w/o values can be quoted as well (e.g. Cache-Control: "private"), which the BNF in rfc7234#section-5.2 does not mention.

<https://httpwg.org/specs/rfc7234.html#header.cache-control>:

"Cache directives are identified by a token, to be compared case-insensitively, and have an optional argument, that can use both token and quoted-string syntax. For the directives defined below that define arguments, recipients ought to accept both forms, even if one is documented to be preferred. For any directive not defined by this specification, a recipient MUST accept both forms."
Comment 4 Yann Ylavic 2019-03-28 21:18:05 UTC
Created attachment 36501 [details]
Fix parsing of quoted Cache-Control token arguments

Fixed in trunk (r1856493), this patch is a backport to any recent 2.4.x.
Could you please try it with your tests suite?
Comment 5 Yann Ylavic 2019-03-28 22:49:52 UTC
Created attachment 36502 [details]
Fix parsing of quoted Cache-Control token arguments (v2)

Same but with a stricter version of cache_strqtok(), notably with regard to quoted tokens like Cache-Control: "private" (from Comment 3).
Comment 6 Roy T. Fielding 2019-03-29 10:28:02 UTC
Comment on attachment 36502 [details]
Fix parsing of quoted Cache-Control token arguments (v2)

Nice work!

Is it safe to change the signature of cache_strqtok()? It doesn't have the ap_prefix, but it also isn't static. Might it be used by third party cache modules? If so, we could add this as a new function in parallel to the existing one.

Meanwhile, I will try to get my laptop test setup working again. We could ask Mark Nottingham to run his test suite against a public-accessible server.
Comment 7 Yann Ylavic 2019-03-29 11:51:33 UTC
> Nice work!
Thanks!

> 
> Is it safe to change the signature of cache_strqtok()? It doesn't have the
> ap_prefix, but it also isn't static. Might it be used by third party cache
> modules? If so, we could add this as a new function in parallel to the
> existing one.
No need, cache_strqtok() is not in our API contract (cache_util.h is private, i.e. not installed and nothing AP_DECLARE()d in there).
One may forward declare and use it since it's not static, but at his/her own risk; like here :)

> 
> Meanwhile, I will try to get my laptop test setup working again. We could
> ask Mark Nottingham to run his test suite against a public-accessible server.
If needed we can possibly run a proxy somewhere (I suppose Mark needs to control both the client and the origin/server), but it's probably easier for him to build 2.4.x with this patch than having to coordonate on the origin's URI..
I made some minimal/manual tests already, but Mark's full tests suite is certainly more interesting.
Regarding the httpd test framework, mod_cache tests look very minimal as of now, and I'm not sure where to start to add such tests (my perl foo and knowledge of the framework's internals are quite limited :/ ). Possibly we could integrate Mark's easy enough?
Comment 8 Mark Nottingham 2019-04-01 10:12:36 UTC
Yes, the suite requires running both a client and a server.

FWIW it's pretty easy to set up yourself (ATS uses it to compare recent versions; see <https://ci.trafficserver.apache.org/http-cache-tests>) provided you have a way to run a recent version of NodeJS.
Comment 9 Julian Reschke 2019-11-18 12:31:41 UTC
What needs to be done to get this committed and backported?