[Bug 63288] New: mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug 63288] New: mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63288

            Bug ID: 63288
           Summary: mod_cache (util_cache.c) fails to read quoted
                    Cache-Control parameters like max-age
           Product: Apache httpd-2
           Version: 2.4.38
          Hardware: All
                OS: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: mod_cache
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: ---

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

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63288] mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63288

Julian Reschke <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63288] mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63288

--- Comment #1 from Mark Nottingham <[hidden email]> ---
Relevant test:
  https://cache-tests.fyi/?id=freshness-max-age-quoted

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63288] mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63288

--- Comment #2 from Yann Ylavic <[hidden email]> ---
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?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63288] mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63288

--- Comment #3 from Julian Reschke <[hidden email]> ---
> 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."

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63288] mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63288

--- Comment #4 from Yann Ylavic <[hidden email]> ---
Created attachment 36501
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=36501&action=edit
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?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63288] mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63288

Yann Ylavic <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #36501|0                           |1
        is obsolete|                            |

--- Comment #5 from Yann Ylavic <[hidden email]> ---
Created attachment 36502
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=36502&action=edit
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).

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63288] mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63288

--- Comment #6 from Roy T. Fielding <[hidden email]> ---
Comment on attachment 36502
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=36502
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63288] mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63288

--- Comment #7 from Yann Ylavic <[hidden email]> ---
> 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?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63288] mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63288

--- Comment #8 from Mark Nottingham <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63288] mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63288

--- Comment #9 from Julian Reschke <[hidden email]> ---
What needs to be done to get this committed and backported?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63288] mod_cache (util_cache.c) fails to read quoted Cache-Control parameters like max-age

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63288

Eric Covener <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |FixedInTrunk

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]