IsoHex/edge-router

Watch

Fork 0

Path traversal in the static file server: request path reaches Path::join unnormalised #35

New issue

Closed

opened 2026-10-20 08:25:31 +00:00 by james.oates · 2 comments

james.oates commented

2026-10-20 08:25:31 +00:00

Owner

serve_from_dir builds a filesystem path by lexically joining the raw request target to the configured directory, with no normalisation and no containment check, so literal ../ segments escape the directory and read arbitrary files the ER process can open.

The chain is three hops, all in proxy.rs:

proxy.rs:245 — let uri_path = req.uri().path().to_string(); — the origin-form target as received. Nothing in the stack normalises dot-segments: there is no NormalizePath layer, no canonicalize, no components() filter anywhere in proxy.rs or main.rs.
serve_static (proxy.rs:956) passes it straight through to serve_from_dir.

serve_from_dir (proxy.rs:1037):

let rel = uri_path.trim_start_matches('/');
let rel = if rel.is_empty() { "index.html" } else { rel };
for candidate in &[dir.join(rel), dir.join(rel).join("index.html")] {
    if let Ok(bytes) = tokio::fs::read(candidate).await {

Path::join with a relative path containing .. concatenates lexically; the OS then resolves the .. on read.

Reachable on any deployment with a [[static_sites]] entry or [default_site].dir — including v1.1.0.

Repro (curl normalises .. client-side unless told not to):

curl --path-as-is -k https://<static-host>/../../../../etc/hostname

Scope limits, so this isn't overstated: percent-encoded variants (%2e%2e%2f) do not work — serve_from_dir never percent-decodes, so the literal bytes are used as a filename. Only literal dot-segments in the target traverse. Reads are limited to what the ER process can open, and only files it can read in full.

The fix is a containment check rather than string filtering: resolve the candidate and verify it is still under dir before reading, rejecting anything that isn't. Note canonicalize on a non-existent path errors, and the dir.join(rel).join("index.html") candidate needs the same treatment.

Found by code inspection while answering a question about using ER's static server on a root-CA host; not fixed inline per the standing rule.

`serve_from_dir` builds a filesystem path by lexically joining the raw request target to the configured directory, with no normalisation and no containment check, so literal `../` segments escape the directory and read arbitrary files the ER process can open. The chain is three hops, all in `proxy.rs`: 1. `proxy.rs:245` — `let uri_path = req.uri().path().to_string();` — the origin-form target as received. Nothing in the stack normalises dot-segments: there is no `NormalizePath` layer, no `canonicalize`, no `components()` filter anywhere in `proxy.rs` or `main.rs`. 2. `serve_static` (`proxy.rs:956`) passes it straight through to `serve_from_dir`. 3. `serve_from_dir` (`proxy.rs:1037`): ```rust let rel = uri_path.trim_start_matches('/'); let rel = if rel.is_empty() { "index.html" } else { rel }; for candidate in &[dir.join(rel), dir.join(rel).join("index.html")] { if let Ok(bytes) = tokio::fs::read(candidate).await { ``` `Path::join` with a relative path containing `..` concatenates lexically; the OS then resolves the `..` on `read`. **Reachable on** any deployment with a `[[static_sites]]` entry or `[default_site].dir` — including v1.1.0. Repro (curl normalises `..` client-side unless told not to): ``` curl --path-as-is -k https://<static-host>/../../../../etc/hostname ``` **Scope limits, so this isn't overstated:** percent-encoded variants (`%2e%2e%2f`) do **not** work — `serve_from_dir` never percent-decodes, so the literal bytes are used as a filename. Only literal dot-segments in the target traverse. Reads are limited to what the ER process can open, and only files it can read in full. The fix is a containment check rather than string filtering: resolve the candidate and verify it is still under `dir` before reading, rejecting anything that isn't. Note `canonicalize` on a non-existent path errors, and the `dir.join(rel).join("index.html")` candidate needs the same treatment. Found by code inspection while answering a question about using ER's static server on a root-CA host; not fixed inline per the standing rule.

james.oates commented

2026-10-26 13:36:43 +00:00

Author

Owner

Re-checked on d3d7d1f (v2.0.0 candidate) — still live, unchanged, and there is no test covering it. Recording that because it has been assumed fixed in passing more than once.

serve_from_dir in src/proxy.rs:1080 joins the request path straight onto the site directory:

let rel = uri_path.trim_start_matches('/');
let rel = if rel.is_empty() { "index.html" } else { rel };

for candidate in &[dir.join(rel), dir.join(rel).join("index.html")] {
    if let Ok(bytes) = tokio::fs::read(candidate).await {

No normalisation, no Component::ParentDir rejection, no containment check after the join, and grep finds no traversal guard anywhere upstream of it — nothing in the handler chain inspects the path before this point.

Reachability: hyper and axum do not collapse .. in the path, so a literal ../ sequence arrives intact. Most clients normalise before sending (curl does unless given --path-as-is), so this needs a raw request rather than a browser — which lowers the odds of stumbling into it, not the severity. Percent-encoded %2e%2e is not a vector, because Uri::path() leaves it encoded and the join looks for a literally-named directory.

dir is operator-configured, so the blast radius is whatever the ER process can read — which on the container image is root-owned, including the certificate database.

#36 proposed tower_http::services::ServeDir (feature fs, currently not enabled), which fixes this and the streaming gap together: it percent-decodes and then rejects ParentDir/RootDir, and it follows symlinks, which Kyoshi's Nix store paths need. That remains the right fix, and it is a dependency-feature change rather than hand-rolled path logic.

Whatever the fix, it needs a test asserting a literal ../ request is refused — there is none today, which is why this keeps being believed fixed.

Re-checked on `d3d7d1f` (v2.0.0 candidate) — **still live, unchanged, and there is no test covering it.** Recording that because it has been assumed fixed in passing more than once. `serve_from_dir` in `src/proxy.rs:1080` joins the request path straight onto the site directory: ```rust let rel = uri_path.trim_start_matches('/'); let rel = if rel.is_empty() { "index.html" } else { rel }; for candidate in &[dir.join(rel), dir.join(rel).join("index.html")] { if let Ok(bytes) = tokio::fs::read(candidate).await { ``` No normalisation, no `Component::ParentDir` rejection, no containment check after the join, and `grep` finds no traversal guard anywhere upstream of it — nothing in the handler chain inspects the path before this point. Reachability: hyper and axum do not collapse `..` in the path, so a literal `../` sequence arrives intact. Most clients normalise before sending (`curl` does unless given `--path-as-is`), so this needs a raw request rather than a browser — which lowers the odds of stumbling into it, not the severity. Percent-encoded `%2e%2e` is *not* a vector, because `Uri::path()` leaves it encoded and the join looks for a literally-named directory. `dir` is operator-configured, so the blast radius is whatever the ER process can read — which on the container image is root-owned, including the certificate database. #36 proposed `tower_http::services::ServeDir` (feature `fs`, currently not enabled), which fixes this and the streaming gap together: it percent-decodes and *then* rejects `ParentDir`/`RootDir`, and it follows symlinks, which Kyoshi's Nix store paths need. That remains the right fix, and it is a dependency-feature change rather than hand-rolled path logic. Whatever the fix, it needs a test asserting a literal `../` request is refused — there is none today, which is why this keeps being believed fixed.

james.oates referenced this issue from a commit

2026-10-26 14:24:42 +00:00

fix: refuse a static-site path that escapes its directory

james.oates commented

2026-10-26 14:24:52 +00:00

Author

Owner

Fixed in 911633a, before the v2.0.0 tag.

contained_path rebuilds the relative path from Component::Normal parts only, so .., an absolute path and a Windows prefix are all refused — rejected rather than sanitised, so a request trying to leave the directory fails instead of being quietly rewritten into one that stays. serve_from_dir resolves through it.

Not the ServeDir swap: that remains the right long-term fix and #36 stays open for it, since it also brings decoding, Range and conditional requests. Those are behaviour changes, and this needed to be a security fix on its own before the tag.

Three tests, each watched fail first:

serve_from_dir_refuses_a_path_that_escapes_the_site
serve_from_dir_refuses_an_escape_disguised_by_a_real_prefix
serve_from_dir_still_serves_a_legitimate_nested_path (so the guard rejects escapes, not nesting)

Worth recording about the second one: it initially passed against the unfixed code, because the assets prefix didn't exist and the kernel resolves .. against real directories — so the read failed with ENOENT rather than escaping. It now creates the directory and asserts the escape target is genuinely readable before asserting the refusal. That's the same trap this issue kept falling into: an assertion that holds for a reason unrelated to the thing being tested.

Two documented non-guarantees, at the function and in CLAUDE.md:

The check runs on the still-percent-encoded path, so %2e%2e is not a vector today — but any decoding added later must run before the check or %2e%2e%2f reopens the escape.
Symlinks under the directory are still followed, deliberately (Kyoshi serves a Nix store path through one), so containment is enforced on the requested path rather than the resolved target.

344 tests green, clippy clean, fmt clean in rust:1.98.0-bookworm.

Fixed in `911633a`, before the v2.0.0 tag. `contained_path` rebuilds the relative path from `Component::Normal` parts only, so `..`, an absolute path and a Windows prefix are all refused — rejected rather than sanitised, so a request trying to leave the directory fails instead of being quietly rewritten into one that stays. `serve_from_dir` resolves through it. Not the `ServeDir` swap: that remains the right long-term fix and **#36 stays open** for it, since it also brings decoding, Range and conditional requests. Those are behaviour changes, and this needed to be a security fix on its own before the tag. Three tests, each watched fail first: - `serve_from_dir_refuses_a_path_that_escapes_the_site` - `serve_from_dir_refuses_an_escape_disguised_by_a_real_prefix` - `serve_from_dir_still_serves_a_legitimate_nested_path` (so the guard rejects escapes, not nesting) Worth recording about the second one: it initially passed against the *unfixed* code, because the `assets` prefix didn't exist and the kernel resolves `..` against real directories — so the read failed with ENOENT rather than escaping. It now creates the directory and asserts the escape target is genuinely readable before asserting the refusal. That's the same trap this issue kept falling into: an assertion that holds for a reason unrelated to the thing being tested. Two documented non-guarantees, at the function and in CLAUDE.md: - The check runs on the still-percent-encoded path, so `%2e%2e` is not a vector today — but any decoding added later must run **before** the check or `%2e%2e%2f` reopens the escape. - Symlinks under the directory are still followed, deliberately (Kyoshi serves a Nix store path through one), so containment is enforced on the requested path rather than the resolved target. 344 tests green, clippy clean, fmt clean in `rust:1.98.0-bookworm`.

james.oates closed this issue

2026-10-26 14:25:02 +00:00

No labels

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

IsoHex/edge-router#35

No description provided.

Rows
Columns