nic-6443 commented on code in PR #13787:
URL: https://github.com/apache/apisix/pull/13787#discussion_r3733484884
##########
t/plugin/proxy-rewrite.t:
##########
@@ -1724,3 +1724,38 @@ GET /test/echo
x-src: from-src
--- response_body_like eval
qr/x-multi: cap-echo\nx-multi: from-src/
+
+
+
+=== TEST 66: CRLF in a reflected uri is encoded, not injected into the
upstream request line
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1', ngx.HTTP_PUT, {
+ uri = "/reflect*",
+ plugins = {
+ ["proxy-rewrite"] = {
+ regex_uri = {"^(/reflect.*)",
"/print_request_received?orig=$1"}
+ }
+ },
+ upstream = {
+ type = "roundrobin",
+ nodes = {["127.0.0.1:1980"] = 1}
+ }
+ })
+ if code >= 300 then ngx.status = code; ngx.say(body); return end
+
+ local http = require("resty.http")
+ local httpc = http.new()
+ local res = httpc:request_uri("http://127.0.0.1:"; ..
ngx.var.server_port
+ .. "/reflect%0d%0aX-Injected:pwn")
+ ngx.print(res.body)
+ }
+ }
+--- request
+GET /t
+--- response_body eval
+qr{request_uri: /print_request_received\?orig=/reflect%0[Dd]%0[Aa]X-Injected}
+--- no_error_log
+[error]
Review Comment:
Done — TEST 66 now also asserts (via `response_body_unlike`) that no
`x-injected` header line reached the upstream, on top of checking the
`request_uri` is percent-encoded. So it fails if a CR/LF is ever interpreted as
a header boundary, independent of how the upstream prints the request.
##########
apisix/init.lua:
##########
@@ -813,8 +813,9 @@ function _M.http_access_phase()
api_ctx.var.uri = new_uri
-- forward the original uri so the servlet upstream
- -- can consume the param after ';'
- api_ctx.var.upstream_uri = uri
+ -- can consume the param after ';'. Encode control characters so a
+ -- CR/LF in the decoded uri cannot inject into the upstream
request line.
+ api_ctx.var.upstream_uri = core.utils.escape_uri_control_chars(uri)
end
Review Comment:
The escaping here is `core.utils.escape_uri_control_chars`, which TEST 66
already exercises end-to-end: a reflected `%0d%0a` ends up percent-encoded in
the upstream request line and no header is injected. The
`normalize_uri_like_servlet` call site is a one-line application of that same
function.
I did try to add a dedicated test with `normalize_uri_like_servlet` enabled,
but couldn't get a request to both match a route and carry a control char
through to `upstream_uri` at that site in test-nginx: a raw CR/LF path doesn't
route through radixtree, and when the CR/LF rides in a `;` servlet parameter
the routing interaction didn't cooperate in the harness. Since the escaping
function itself is already covered, I left the servlet site without its own
integration test rather than ship a flaky one. Happy to add it if you have a
harness pattern that reliably drives that path.
--
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]
