nic-6443 commented on code in PR #13831:
URL: https://github.com/apache/apisix/pull/13831#discussion_r3792151636
##########
t/plugin/proxy-cache/memory.t:
##########
@@ -1554,3 +1554,68 @@ GET /t
--- response_body
purge=200
leftover=0
+
+
+
+=== TEST 48: a crafted URI cannot collide with another request's Vary variant
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local http = require("resty.http")
+
+ local code, body =
t('/apisix/admin/routes/proxy-cache-vary-collision', ngx.HTTP_PUT, [[{
+ "uri": "/vary-encoding*",
+ "plugins": {
+ "proxy-cache": {
+ "cache_strategy": "memory",
+ "cache_key": ["$host", "$uri"],
+ "cache_zone": "memory_cache",
+ "cache_method": ["GET"],
+ "cache_http_status": [200],
+ "cache_ttl": 300
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1986": 1
+ },
+ "type": "roundrobin"
+ }
+ }]])
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
+
+ local base = "http://127.0.0.1:"; .. ngx.var.server_port
+ local function fetch(path, enc)
+ local res, err = http.new():request_uri(base .. path, {
+ headers = { ["Accept-Encoding"] = enc },
+ })
+ if not res then return nil, err end
+ return res.headers["Apisix-Cache-Status"],
res.body:gsub("%s+$", "")
Review Comment:
Fixed in 112b6c9b4, it now guards the body like the other helpers in this
file.
##########
apisix/plugins/proxy-cache/memory_handler.lua:
##########
@@ -43,6 +43,30 @@ local VARY_INDEX_SUFFIX = "::__vary"
local MAX_VARIANTS = 64
+-- 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
Review 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 112b6c9b4, so any
entry written under the old layout is bypassed and purged on first access.
##########
apisix/plugins/proxy-cache/memory_handler.lua:
##########
@@ -116,15 +140,14 @@ end
-- cache_control derives them per response), and an expired index must still
Review Comment:
Reworded in 112b6c9b4 — it now describes the entry stored directly under the
cache key instead of calling it the legacy base key.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
