nic-6443 commented on code in PR #14035:
URL: https://github.com/apache/apisix/pull/14035#discussion_r4292601465
##########
apisix/init.lua:
##########
@@ -887,6 +898,20 @@ function _M.http_access_phase()
end
end
+ -- Nginx resolved dot segments in $uri, so forward the path the route was
+ -- matched on.
+ if not (local_conf.apisix and
local_conf.apisix.normalize_uri_like_servlet) then
+ local path = api_ctx.var.request_uri
+ local args_pos = core.string.find(path, "?")
+ if args_pos then
+ path = str_sub(path, 1, args_pos - 1)
+ end
+ if path ~= uri and has_dot_segment(path) then
+ api_ctx.var.upstream_uri = core.utils.uri_safe_encode(uri)
Review Comment:
Setting `$upstream_uri` here, before any plugin has run, freezes both the
path and the raw query for every dot-segment request. A non-empty
`$upstream_uri` wins in `proxy_pass`, so anything that later edits the request
through `ngx.req.set_uri` / `set_uri_args` instead of writing `$upstream_uri`
gets silently thrown away. The worst case is `hide_credentials`: key-auth /
jwt-auth strip a query credential with `set_uri_args`, so the credential now
reaches the upstream as soon as the client adds a `./`:
```
# consumer key SECRET; route /ka/* with key-auth {hide_credentials = true}
--- request
GET /ka/./x?apikey=SECRET&a=1
--- response_body
request-uri=/ka/x?a=1
```
That passes on master. With this PR the upstream gets
`/ka/x?apikey=SECRET&a=1`. Same thing with a serverless function:
`ngx.req.set_uri("/svl/changed")` on `GET /svl/./x` reaches the upstream as
`/svl/x`, and `ngx.req.set_uri_args("added=1")` on `GET /sva/./x?b=2` still
sends `?b=2`. Reading the code, degraphql's GET path (`set_uri_args`) and
grpc-web (`ngx.req.set_uri`) have the same problem. proxy-mirror mirrors the
frozen value, credential included.
ext-plugin is affected too, because it treats a non-empty `$upstream_uri` as
"rewritten" (ext-plugin/init.lua ~494). On dot-segment requests the runner gets
the encoded path instead of `var.uri`, and the frozen query is written back
into `$args` with `set_uri_args`, so ext-plugin-post-req puts back the
credential key-auth just removed. A runner that rewrites only args loses the
change (~708), because `upstream_uri` is only rebuilt when the runner also
returns a path. The comment in the websocket path ("upstream_uri is only ever
set by plugins like proxy-rewrite", ~1227), which ext-plugin,
ext-plugin-post-resp and proxy-mirror also depend on, is no longer true either.
The raw query also ends up in `$upstream_uri`, and the default
`access_log_format` logs that variable, so data-mask's query masking no longer
covers these requests. With data-mask replacing `password`, `GET
/mask/./x?password=abc` logs `"http://localhost/mask/x?password=abc"` next to
the masked `$request_line`. On master nothing unmasked is logged.
I'd leave `$upstream_uri` alone and invalidate the unparsed URI instead.
Nginx then rebuilds the request line from the live `r->uri` / `r->args` at
proxy time, which is already what happens on master whenever key-auth strips a
query credential:
```lua
if uri_normalized_by_nginx(api_ctx.var.request_uri, uri) then
-- binary: $uri may hold decoded control chars, proxy_pass escapes them
ngx.req.set_uri(uri, false, true)
end
```
I tried this locally (with the `$uri`-oracle helper from my other comment).
Every case above goes back to master's behaviour, the bypass cases stay fixed,
the PR's test file passes, and t/router, key-auth, jwt-auth, proxy-rewrite,
proxy-mirror, data-mask, serverless and core/ctx show no new failures compared
with this PR head. You need the `binary` flag: without it, `GET /api/./a%0Ab`
becomes a 500 (`unsafe byte "0x0a"`).
I also tried the other direction: keep the flag here, but set
`$upstream_uri` late (right before `handle_upstream`, only if it is still
empty), from `ngx.var.uri` plus `ngx.var.args`. That's the same split Kong uses
between access.before and access.after. It fixes hide_credentials, the
set_uri/set_uri_args cases and ext-plugin, because all of those run before the
assignment. But the raw query still ends up in the access log, so I prefer
`set_uri`.
##########
apisix/init.lua:
##########
@@ -887,6 +898,20 @@ function _M.http_access_phase()
end
end
+ -- Nginx resolved dot segments in $uri, so forward the path the route was
+ -- matched on.
+ if not (local_conf.apisix and
local_conf.apisix.normalize_uri_like_servlet) then
+ local path = api_ctx.var.request_uri
Review Comment:
nit: this query strip duplicates the one in the `match_uri_encoded_slash`
block a few lines below. If the helper takes `request_uri` directly (sketch in
the comment on `has_dot_segment`), the raw path is derived in one place.
##########
apisix/init.lua:
##########
@@ -591,6 +591,17 @@ local function build_match_uri_keep_encoded_slash(path,
current_uri)
end
+-- whether the decoded path has a "." or ".." segment
+local function has_dot_segment(path)
+ if not core.string.find(path, ".") and not core.string.find(path, "%") then
Review Comment:
`has_dot_segment` re-implements part of nginx's normalization and misses the
forms it doesn't model. One of them is a literal `#`: nginx cuts the path there
before it resolves dot segments, so `GET /admin/..#x` gets `$uri` = `/`. The
decoded raw path `/admin/..#x/` contains no `/../`, so the raw line is
forwarded unchanged even though the request was matched as `/`:
```
# route uri = "/" without auth, upstream echoes $request_uri
--- raw_request eval
"GET /admin/..#x HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n"
--- response_body
request-uri=/
```
(It currently returns `request-uri=/admin/..#x`.) This is narrower in
practice than `..%2F`, but it's the same class. Rather than modelling nginx,
I'd use `$uri` itself as the oracle, the way
`build_match_uri_keep_encoded_slash` already does: the raw path is safe to pass
through only if its percent-decoding, with slashes merged, equals `$uri`. That
covers dot segments, fragments and anything else nginx does, and it still
leaves `%2F`-only and `//`-only requests alone (TEST 6 and TEST 7 here still
pass with it).
It's also cheaper. Right now every request with a query pays for a `str_sub`
copy, and every percent-encoded path pays for a `re_gsub` decode with a Lua
callback. With a `has_prefix` check first, an unchanged request costs a single
comparison and no allocation:
```lua
local function uri_normalized_by_nginx(request_uri, uri)
if request_uri == uri then
return false
end
if core.string.has_prefix(request_uri, uri)
and str_byte(request_uri, #uri + 1) == str_byte("?") then
return false
end
local path = request_uri
local args_pos = str_find(path, "?", 1, true)
if args_pos then
path = str_sub(path, 1, args_pos - 1)
end
local decoded = percent_decode(path, false)
if decoded == uri then
return false
end
return re_gsub(decoded, [[//+]], "/", "jo") ~= uri
end
```
##########
docs/en/latest/router-radixtree.md:
##########
@@ -240,6 +240,20 @@ when either actually rewrites the URI (a stripped trailing
slash, a servlet-styl
matching without keeping `%2F`. The fallback is safe; the encoded-slash match
simply does not apply to such requests.
+#### 6. Path forwarded to the upstream
+
+Routes are matched on the normalized `$uri`: Nginx percent-decodes the path,
+resolves dot segments (including encoded ones such as `..%2F` and `%2e%2e`) and
+merges consecutive slashes. When the decoded request path has no `.` or `..`
+segment, APISIX forwards the original request line to the upstream, consecutive
+slashes included. When it has one, APISIX forwards the matched path re-encoded,
+followed by the original query string. For example, a request to
Review Comment:
Two things here don't match what actually happens.
With `normalize_uri_like_servlet` this block is skipped, and the servlet
branch sets `$upstream_uri` without args, so the query is dropped. That's
already the case on master, and for every request, not just dot-segment ones:
```
# yaml: apisix.normalize_uri_like_servlet: true, route /api/*
--- request
GET /api/x?q=1
--- response_body
request-uri=/api/x?q=1
```
The upstream gets `/api/x`, both on master and on this PR. That probably
needs its own fix, but this sentence shouldn't read as if it also applied to
servlet mode.
It's also worth documenting a trade-off: the forwarded path is `$uri`
re-encoded, so encoded delimiters near a dot segment arrive decoded. `GET
/api/./a%2Fb%3Bc` reaches the upstream as `/api/a/b;c`, while `GET /api/a%2Fb`
is forwarded raw. For the `..%2F` form this can't be avoided, since that `%2F`
is exactly the slash nginx treated as a separator when matching. For
`/./a%2Fb`-style requests it could be avoided by removing dot segments on the
raw path and using the result only when its decoding equals `$uri`. I don't
think that's worth the extra code, though, so a sentence here is enough.
##########
t/router/radixtree-uri-normalized-upstream.t:
##########
@@ -0,0 +1,185 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('info');
+worker_connections(256);
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (!$block->upstream_server_config) {
+ $block->set_value("upstream_server_config", <<'_EOC_');
+ location /admin/ {
+ content_by_lua_block {
+ ngx.say("request-uri=", ngx.var.request_uri)
+ }
+ }
+
+ location /api/ {
+ content_by_lua_block {
+ ngx.say("request-uri=", ngx.var.request_uri)
+ }
+ }
+
+ location = /fixed {
+ content_by_lua_block {
+ ngx.say("request-uri=", ngx.var.request_uri)
+ }
+ }
+_EOC_
+ }
+
+ if (!$block->request) {
+ $block->set_value("request", "GET /t");
+ }
+});
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: set routes with and without auth on the same upstream
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local routes = {
+ {
+ id = "admin",
+ uri = "/admin/*",
+ plugins = {
+ ["key-auth"] = {},
+ },
+ },
+ {
+ id = "api",
+ uri = "/api/*",
+ },
+ {
+ id = "rewrite",
+ uri = "/rewrite/*",
+ plugins = {
+ ["proxy-rewrite"] = {
+ uri = "/fixed",
+ },
+ },
+ },
+ }
+
+ for _, route in ipairs(routes) do
+ route.upstream = {
+ nodes = { ["127.0.0.1:1980"] = 1 },
+ type = "roundrobin",
+ }
+
+ local code, body = t("/apisix/admin/routes/" .. route.id,
+ ngx.HTTP_PUT, route)
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
+ end
+
+ ngx.say("passed")
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 2: route with key-auth requires an API key
+--- request
+GET /admin/x
+--- error_code: 401
+--- response_body
+{"message":"Missing API key in request"}
+
+
+
+=== TEST 3: encoded slash dot segment forwards the matched path
+--- request
+GET /admin/..%2Fapi/x
+--- response_body
+request-uri=/api/x
+
+
+
+=== TEST 4: encoded dot segment forwards the matched path with args
+--- request
+GET /admin/%2e%2e/api/x?a=1&b=%2F
+--- response_body
+request-uri=/api/x?a=1&b=%2F
+
+
+
+=== TEST 5: plain dot segment forwards the matched path
+--- request
+GET /admin/../api/x
+--- response_body
+request-uri=/api/x
+
+
+
+=== TEST 6: merged slashes alone are forwarded unchanged
+--- request
+GET /api/https://example.com//x
+--- response_body
+request-uri=/api/https://example.com//x
+
+
+
+=== TEST 7: encoded path without normalization is forwarded unchanged
+--- request
+GET /api/a%2Fb%20c?x=1
+--- response_body
+request-uri=/api/a%2Fb%20c?x=1
+
+
+
+=== TEST 8: proxy-rewrite uri still takes precedence
Review Comment:
The cases that would have caught the problems above are missing:
- a plugin that changes the args or the path after matching (key-auth with
`hide_credentials` and the key in the query, a serverless `ngx.req.set_uri` /
`set_uri_args`)
- a single `.` segment (`/api/./x`; every case here uses `..`)
- a `#` after `..`
- the interaction with `delete_uri_tail_slash` (`/api/./x/` should forward
`/api/x/` like `/api/x/` does, which works today because the pre-strip `uri` is
used) and with servlet mode
Snippets for most of these are in the init.lua comments.
##########
apisix/init.lua:
##########
@@ -887,6 +898,20 @@ function _M.http_access_phase()
end
end
+ -- Nginx resolved dot segments in $uri, so forward the path the route was
+ -- matched on.
+ if not (local_conf.apisix and
local_conf.apisix.normalize_uri_like_servlet) then
Review Comment:
Minor: this changes what upstreams receive by default for every dot-segment
request, and there's no switch to get the old behaviour back. I think that's
fine for a security fix. Kong did the same in 2.3.2 (Kong/kong#6821): it
changed the default in a patch release without a switch. It's still worth a
CHANGELOG / upgrade note, because an upstream that relied on seeing the raw
form will notice the difference.
--
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]
