fix(proxy-cache): keep Vary variant keys out of reach of a crafted request - #13831
Open
nic-6443 wants to merge 4 commits into
Open
fix(proxy-cache): keep Vary variant keys out of reach of a crafted request#13831nic-6443 wants to merge 4 commits into
nic-6443 wants to merge 4 commits into
Conversation
…quest The memory strategy stores a Vary variant at `<cache key>::<signature>` and the variant index at `<cache key>::__vary`. The cache key is client controlled -- it defaults to `$host` and `$request_uri` -- and the signature is the md5 of the request's values for the varied headers, so a request to `<victim uri>::<md5 of the varied values>` produces a cache key equal to the victim's variant key. Its response is stored where the victim's variant is looked up, and the victim's cached response is served to it: cache poisoning in one direction, a cross-request cache read in the other. Derive all three keys from the cache key prefixed with its length instead, which keeps the derivation injective, so no crafted cache key can be the storage key of another one.
dosubot
Bot
added
size:L
There was a problem hiding this comment.
Pull request overview
This PR hardens the proxy-cache plugin’s in-memory (shdict) storage key derivation to prevent crafted requests from colliding with (and thus reading/poisoning) another request’s Vary-variant cache entries.
Changes:
- Introduces length-prefixed base-key encoding (
entry_key) and uses it consistently for base entries, Vary index keys, and variant keys. - Updates purge/update/lookup paths to use the new derived key helpers (
entry_key,index_key,variant_key). - Adds a regression test that reproduces the crafted-URI collision scenario and asserts it no longer hits/overwrites the victim’s cached variant.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apisix/plugins/proxy-cache/memory_handler.lua | Switches shdict key derivation to a length-prefixed scheme for injective key construction across base/index/variant entries. |
| t/plugin/proxy-cache/memory.t | Adds a regression test ensuring a crafted URI cannot collide with another request’s Vary variant key. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1597
to
+1598
| if not res then return nil, err end | ||
| return res.headers["Apisix-Cache-Status"], res.body:gsub("%s+$", "") |
Member
Author
There was a problem hiding this comment.
Fixed in 112b6c9, it now guards the body like the other helpers in this file.
Comment on lines
+46
to
+50
| -- Every shdict key for one cache key is derived from it: the entry itself, | ||
| -- the Vary index, and one entry per variant. The cache key is client | ||
| -- controlled (`cache_key` defaults to `$host` and `$request_uri`), so | ||
| -- deriving them by appending a suffix lets a crafted request produce a cache | ||
| -- key byte-identical to another request's variant key -- the attacker's |
Member
Author
There was a problem hiding this comment.
Fair point — a cache key that happens to look like the new layout could otherwise survive the upgrade. Bumped CACHE_VERSION to 3 in 112b6c9, so any entry written under the old layout is bypassed and purged on first access.
Comment on lines
136
to
140
| -- Purge every variant entry referenced by the index, then the index itself, | ||
| -- and finally the legacy base-key entry (which may exist if the URL ever | ||
| -- cached a no-Vary response in the past). The index is read stale-tolerant: | ||
| -- it can outlive or be outlived by its variants (variant TTLs diverge when | ||
| -- cache_control derives them per response), and an expired index must still |
Member
Author
There was a problem hiding this comment.
Reworded in 112b6c9 — it now describes the entry stored directly under the cache key instead of calling it the legacy base key.
An entry written under the old layout must never be served under the new one, whatever the cache key looks like. Also guard the response body in the new test and describe the purged base entry by its current key.
Exact-match routes on the same URI left over from earlier tests took precedence over the prefix route, so the victim requests never reached it.
dosubot
Bot
added
size:M
dosubot
Bot
added
size:L
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The memory strategy of
proxy-cachelays its shdict keys out as<cache key>for the plain entry,<cache key>::__varyfor the variant index and<cache key>::<signature>for each variant, where the signature ismd5()over the request's values for the varied headers.The cache key is client controlled —
cache_keydefaults to$hostand$request_uri— and the signature is offline computable, so a request to/<victim uri>::<md5 of the varied values>gets a cache key byte-identical to the victim's variant key. Nothing on the entry records which key it belongs to, so:Same trick works against the
::__varyindex key.The fix keeps the derivation injective by prefixing the cache key with its length before appending anything: for two different cache keys either the length field or the key itself differs, so no crafted cache key can produce another one's storage key.
CACHE_VERSIONgoes 2 → 3 alongside it, so an entry written under the old layout is bypassed and purged rather than served if a cache key happens to look like the new one.Upgrade note: entries cached before the upgrade are unreachable under the new layout, so the first request per key is a MISS and a
PURGEfor a URL that only has pre-upgrade entries answers 404 — the old slots then sit in the shdict until their own TTL expires.Not addressed here:
base_keyis still the plain concatenation of thecache_keycomponents, so two different component tuples can produce the same base key (["a", "bc"]vs["ab", "c"]). The same length-prefix trick would fix it inutil.generate_complex_value; that is a separate change and a separate cache-key break.t/plugin/proxy-cache/memory.tTEST 48 caches aVary: Accept-Encodingresponse, then sends the crafted URI: on master the crafted request answersHITwith the victim's body, with this change it is a plainMISSwith its own body and the victim's entry stays intact.Which issue(s) this PR fixes:
N/A
Checklist