From 1d2a89223ee75e86d396d3e05b3f3478450a768b Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Sat, 31 Oct 2026 16:00:34 +0100 Subject: [PATCH] mod_cache: Check err_headers_out too for a Vary: * response. The cacheability check only looked at r->headers_out for a Vary: * response, so a Vary: * set via "Header always" (which writes to r->err_headers_out) was missed. Storage itself does merge err_headers_out into headers_out, so the response got stored and could later be served to a different client under the same key, handing one client's response to another. Also fix cache_select(): it tokenized a stored response's Vary header and, for a "*" token, looked up a request header literally named "*". Since no such header exists on either side, both sides were NULL and the code treated that as a match, serving a stored Vary: * entry to a mismatched request instead of always treating it as a mismatch per RFC 9111 4.1. This closes the read side for any Vary: * entry already on disk (e.g. from before this fix, or written during a rolling upgrade), the same way the save-time check above closes the write side. PR 70289 --- changes-entries/70289.txt | 10 ++ modules/cache/cache_storage.c | 13 ++ modules/cache/mod_cache.c | 7 +- test/modules/proxy/test_08_cache_vary_star.py | 133 ++++++++++++++++++ 4 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 changes-entries/70289.txt create mode 100644 test/modules/proxy/test_08_cache_vary_star.py diff --git a/changes-entries/70289.txt b/changes-entries/70289.txt new file mode 100644 index 00000000000..74253e43841 --- /dev/null +++ b/changes-entries/70289.txt @@ -0,0 +1,10 @@ + *) mod_cache: Do not store a response whose "Vary: *" was set via + "Header always", which mod_cache's cacheability check missed because + it only looked at r->headers_out and not r->err_headers_out. This + could let mod_cache serve one client's response to another client + under the Vary: * header, since storage itself does merge the two + tables. Also fix cache_select() to treat a stored "Vary: *" as an + automatic mismatch per RFC 9111 4.1, instead of as an ordinary + (absent) header name, so an existing "Vary: *" entry already on + disk cannot be served to a mismatched request either. + PR 70289 [Arturo Bernal] diff --git a/modules/cache/cache_storage.c b/modules/cache/cache_storage.c index dfda34b1c1d..66387b026c5 100644 --- a/modules/cache/cache_storage.c +++ b/modules/cache/cache_storage.c @@ -281,6 +281,19 @@ int cache_select(cache_request_rec *cache, request_rec *r) while (vary) { const char *h1, *h2; + /* + * RFC 9111 4.1: a stored response with a Vary header field + * value containing a member "*" always fails to match, as + * "*" is not the name of a real request header to compare. + */ + if (vary[0] == '*' && vary[1] == '\0') { + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, + r, APLOGNO(10620) "cache_select(): Vary header " + "contains '*', cannot match stored response."); + mismatch = 1; + break; + } + /* * is this header in the request and the header in the cached * request identical? If not, we give up and do a straight get diff --git a/modules/cache/mod_cache.c b/modules/cache/mod_cache.c index c69fc3932b0..2fc386e27ae 100644 --- a/modules/cache/mod_cache.c +++ b/modules/cache/mod_cache.c @@ -1115,7 +1115,12 @@ static apr_status_t cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in) */ reason = "Authorization required"; } - else if (ap_find_token(NULL, apr_table_get(r->headers_out, "Vary"), "*")) { + else if (ap_find_token(NULL, apr_table_get(r->headers_out, "Vary"), "*") + || ap_find_token(NULL, apr_table_get(r->err_headers_out, "Vary"), "*")) { + /* err_headers_out carries a Vary set via "Header always", which + * is merged into headers_out only when the response is actually + * stored (see cache_merge_headers_out()); check it here too so + * that decision matches what gets stored. */ reason = "Vary header contains '*'"; } else if (apr_table_get(r->subprocess_env, "no-cache") != NULL) { diff --git a/test/modules/proxy/test_08_cache_vary_star.py b/test/modules/proxy/test_08_cache_vary_star.py new file mode 100644 index 00000000000..d7729ef98a5 --- /dev/null +++ b/test/modules/proxy/test_08_cache_vary_star.py @@ -0,0 +1,133 @@ +import os +import time +import uuid + +import pytest + +from pyhttpd.conf import HttpdConf +from .env import TCPFaker + + +# Backend answering // with a body that depends on the request's +# Cookie, and a Cache-Control that makes the response otherwise cacheable. +class _IdentityFaker(TCPFaker): + + def __init__(self, host, port): + super().__init__(host, port) + self.hits = {} + + def _make_response(self, data): + head = data.split(b"\r\n\r\n")[0].decode("latin-1").split("\r\n") + path = head[0].split(" ")[1] + self.hits[path] = self.hits.get(path, 0) + 1 + cookie = "anonymous" + for line in head[1:]: + if line.lower().startswith("cookie:"): + cookie = line.split(":", 1)[1].strip() + body = f"body-for-{cookie}\n" + lines = ["HTTP/1.1 200 OK", + "Last-Modified: Thu, 01 Jan 2026 00:00:00 GMT", + 'ETag: "t"', + "Cache-Control: public, max-age=60", + "Content-Type: text/plain", + f"Content-Length: {len(body)}", + "Connection: close"] + return ("\r\n".join(lines) + "\r\n\r\n" + body).encode() + + +class TestCacheVaryStar: + """BZ 70289: mod_cache's cacheability check for "Vary: *" only looked + at r->headers_out, so a Vary: * set via "Header always" (which writes + to r->err_headers_out) was missed by the check, while storage itself + does merge err_headers_out in. This let a response meant for one + client be stored and served back to a different client.""" + + @pytest.fixture(autouse=True, scope='class') + def _class_scope(self, env): + faker = _IdentityFaker("127.0.0.1", env.http_port2) + faker.start() + cache_dir = os.path.join(env.gen_dir, "cache-vary-star") + os.makedirs(cache_dir, exist_ok=True) + + conf = HttpdConf(env) + conf.start_vhost(domains=[f"test1.{env.http_tld}"], port=env.http_port, + doc_root="htdocs", with_ssl=False) + conf.add([ + f'CacheRoot "{cache_dir}"', + 'CacheEnable disk "/cv/"', + 'CacheEnable disk "/cv2/"', + 'CacheEnable disk "/cv3/"', + 'CacheHeader on', + '', + ' Header always set Vary "*"', + '', + '', + ' Header set Vary "*"', + '', + f"ProxyPass /cv/ http://127.0.0.1:{env.http_port2}/", + f"ProxyPass /cv2/ http://127.0.0.1:{env.http_port2}/", + f"ProxyPass /cv3/ http://127.0.0.1:{env.http_port2}/", + ]) + conf.end_vhost() + conf.install() + assert env.apache_restart() == 0 + TestCacheVaryStar.faker = faker + TestCacheVaryStar.run = uuid.uuid4().hex[:12] + yield + faker.stop() + + def get(self, env, path_prefix="/cv/", index=0, cookie=None): + url = env.mkurl("http", "test1", f"{path_prefix}{self.run}/{index}") + options = ["-H", f"Cookie: {cookie}"] if cookie else [] + r = env.curl_get(url, options=options) + assert r.response["status"] == 200, f"{r}" + return r.response + + def backend_hits(self, index=0): + return self.faker.hits.get(f"/{self.run}/{index}", 0) + + # a victim's response must never be handed to another client under a + # Vary: * header that was set via "Header always" + def test_proxy_08_001(self, env): + r1 = self.get(env, cookie="session=victim") + assert r1["body"] == b"body-for-session=victim\n" + # give the CACHE_SAVE filter's commit a moment to land, the same + # way test_07_cache_control.py does for its own positive cases; + # here it only matters because we are proving a negative. + time.sleep(0.1) + r2 = self.get(env) + assert not r2["header"].get("x-cache", "").startswith("HIT"), \ + "Vary: * response (via Header always) was served from cache" + assert r2["body"] == b"body-for-anonymous\n", \ + "victim's response body leaked to an anonymous request" + assert self.backend_hits() == 2, "Vary: * response was stored" + + # positive control: the same disk cache setup, minus "Header always + # set Vary *", does cache and serve a repeat request from disk. This + # rules out test_proxy_08_001 passing merely because disk storage is + # broken in this setup (e.g. a missing CacheRoot directory) rather + # than because mod_cache correctly rejected the Vary: * response. + def test_proxy_08_002(self, env): + r1 = self.get(env, path_prefix="/cv2/", index=1) + assert r1["body"] == b"body-for-anonymous\n" + time.sleep(0.1) + r2 = self.get(env, path_prefix="/cv2/", index=1) + assert r2["header"]["x-cache"].startswith("HIT"), \ + "response without Vary: * was not served from the disk cache" + assert r2["body"] == r1["body"] + assert self.backend_hits(1) == 1, \ + "response without Vary: * was not cached" + + # a victim's response must never be handed to another client under a + # Vary: * header set via plain "Header set" (which writes to + # r->headers_out directly, the case the pre-fix code already checked) + def test_proxy_08_003(self, env): + r1 = self.get(env, path_prefix="/cv3/", index=2, cookie="session=victim") + assert r1["body"] == b"body-for-session=victim\n" + time.sleep(0.1) + r2 = self.get(env, path_prefix="/cv3/", index=2) + assert not r2["header"].get("x-cache", "").startswith("HIT"), \ + "Vary: * response (via headers_out) was served from cache" + assert r2["body"] == b"body-for-anonymous\n", \ + "victim's response body leaked to an anonymous request" + assert self.backend_hits(2) == 2, "Vary: * response was stored"