You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "nic-6443 (via GitHub)" <gi...@apache.org> on 2026/08/06 10:48:40 UTC
[PR] fix: encode control characters in $upstream_uri to prevent CRLF injection [apisix]
nic-6443 opened a new pull request, #13787:
URL: https://github.com/apache/apisix/pull/13787
### Description
`ngx.var.uri` is percent-decoded, so `%0d%0a` in a client URI becomes raw CR/LF in it. The nginx template proxies with `proxy_pass $upstream_scheme://apisix_backend$upstream_uri;`, and nginx writes a Lua-set `$upstream_uri` into the upstream request line verbatim (no re-escaping). Two paths reach `$upstream_uri` with unescaped bytes:
- `proxy-rewrite` `uri_safe_encode()`s only the path portion of the rewritten URI and concatenates the query portion (after `?`) raw. A CR/LF reflected from `$uri` / `$request_uri` / a regex capture therefore lands in the upstream request line — a header-injection / request-smuggling primitive against the upstream.
- The `normalize_uri_like_servlet` path assigns the raw decoded URI to `upstream_uri` with no encoding at all.
This adds `core.utils.escape_uri_control_chars`, which percent-encodes control characters (`0x00-0x1F`, `0x7F`, including CR and LF) while leaving `?`, `=`, `&` intact, and applies it to the query portion in `proxy-rewrite` and to the servlet `upstream_uri`.
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change (internal encoding change; no config/behavior change for well-formed URIs)
- [x] I have verified that this change is backward compatible (only control characters, which are invalid in a request line, are now encoded)
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: encode control characters in $upstream_uri to prevent CRLF injection [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 merged PR #13787:
URL: https://github.com/apache/apisix/pull/13787
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: encode control characters in $upstream_uri to prevent CRLF injection [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: encode control characters in $upstream_uri to prevent CRLF injection [apisix]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13787:
URL: https://github.com/apache/apisix/pull/13787#discussion_r3733270805
##########
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:
The test verifies that CR/LF is percent-encoded in the upstream `request_uri`, but it doesn’t assert that the CR/LF wasn’t interpreted as a header boundary by the upstream (i.e., that no injected `x-injected` header line was received). Adding a negative assertion makes the test robust against alternative upstream parsing/printing behaviors.
##########
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:
This change also alters the `normalize_uri_like_servlet` path by escaping control characters before assigning `api_ctx.var.upstream_uri`, but there’s no accompanying test to prevent regressions on that code path. Consider adding a test case (similar to TEST 66) with `normalize_uri_like_servlet: true` that reflects `%0d%0a` from `$uri` into the upstream request and asserts it is encoded and not injected.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org