GHSA-rfgq-wgg8-662p S3-Proxy has Security Issues in its Resource Path Matching Implementation

• Go github.com/oxyno-zeta/s3-proxy: < 0.0.0-20260424211602-1320e4abd46a → fixed in 0.0.0-20260424211602-1320e4abd46a

Background The original concern is functional: a resource pattern should treat a percent-encoded segment like some%2Fvalue as a single opaque token rather than splitting it into two path segments at the decoded /. Investigation into why %2F was being decoded and how routes matched against the result surfaced three related security issues, documented below. Rather than landing a fix directly, the problem space warrants discussion first. Different fixes carry different compliance and compatibility tradeoffs, and every viable option is a breaking change in some form. Aligning on a direction before committing to an implementation is the safer path. Root cause: two different path representations Go's `net/http` decodes percent-encoded characters when it parses an incoming URL: `%2F` becomes `/` in `r.URL.Path`, while the original encoded form is preserved in `r.URL.RawPath`. Two different parts of s3-proxy use different fields: • The auth middleware calls `r.URL.RequestURI()`, which returns the encoded form (from `RawPath` when available). It sees `%2F` as literal characters, not as path separators. • The bucket handler reads `r.URL.Path` to build the S3 key. It sees the decoded form, where `%2F` has already become `/`. All three issues stem from this mismatch, combined with how glob patterns are compiled. The examples below use PUT for concreteness, but the auth bypass applies to any HTTP method — a config that restricts GET or DELETE on a namespace is equally affected, meaning an attacker could read from or delete objects in a protected namespace without credentials. A note on RFC 3986 RFC 3986 §2.2 states that `/` and `%2F` are not equivalent in a URI path: > URIs that differ in the replacement of a reserved character with its > corresponding percent-encoded octet are not equivalent. `/` is a reserved gen-delim used as a path segment separator. `%2F` is its percent-encoded form and, by the RFC, should be treated as data *within* a segment — not as a separator. So: • `/foo/bar/baz` → three segments: `foo`, `bar`, `baz` • `/foo%2Fbar/baz` → two segments: `foo/bar` (opaque data), `baz` The original functional concern (wanting `foo%2Fbar` to match as a single token against a single-segment wildcard) is therefore RFC-correct behaviour. Go's `r.URL.Path` violates this by decoding `%2F` to `/`, collapsing the two representations into one. This is the underlying tension that makes fixing these issues non-trivial: the simplest security fix makes s3-proxy *more* RFC non-compliant, while the RFC-correct fix requires a more significant refactor. A note on breaking changes Any of the proposed fixes for these issues should be treated as a breaking change. Each option alters how path patterns in existing configs are interpreted — whether by changing how `*` matches segments, by shifting which path representation auth matches against, or by normalising paths before they reach the router. Operators upgrading to a fixed version will need to review their resource path definitions, and a clear migration note in the changelog is essential regardless of which approach is chosen. One way to avoid a hard breaking change would be to introduce a new field — for example `route:` — that carries the fixed semantics, while keeping the existing `path:` field with its current behaviour (and marking it deprecated). Operators could migrate resource definitions incrementally, and the security fix would be available immediately without requiring a coordinated config update across all deployments. The obvious cost of this approach is maintaining two parallel implementations, duplicated test coverage, and the ongoing burden of supporting a deprecated code path until it can eventually be removed. --- Issue 1 — `*` in resource paths matches across `/` Background Resource paths are matched using `github.com/gobwas/glob`. The call site is: ```go // pkg/s3-proxy/authx/authentication/main.go g, err := glob.Compile(res.Path) ``` `glob.Compile` is called without a separator argument. Without a separator, `*` matches any character — including `/`. This means a pattern intended to protect a single path segment actually matches across directory boundaries. Example Consider a config with an open route and a protected route: ```yaml resources: # open — no auth required • path: /upload/*/drafts/ methods: [PUT] whiteList: true # protected — basic auth required • path: /upload/*/restricted/ methods: [PUT] basic: ... ``` The intent is clear: `drafts` is open, `restricted` is protected. The `*` is meant to match a single path segment (the object identifier). However, because `*` crosses `/`, the pattern `/upload/*/drafts/` also matches: ``` PUT /upload/foo/drafts/../restricted/ ``` The path segment matched by `*` is `foo`, and then `drafts/../restricted/` is consumed by the rest of the pattern — because without a separator, `*` is equivalent to `.*` and matches `/`, `.`, and everything else. The result: an unauthenticated request is accepted by the open route. Fix discussion The straightforward fix is to pass `'/'` as the separator to `glob.Compile`: ```go // before g, err := glob.Compile(res.Path) // after g, err := glob.Compile(res.Path, '/') ``` With a separator set: • `*` matches any sequence of non-`/` characters (a single path segment). • `**` matches any sequence including `/` (crossing path boundaries). This fix closes the Issue 1 attack above: with a separator, `drafts/../restricted/` is more than one segment and no longer matches the pattern `/upload/*/drafts/`. Breaking change Any existing config that relies on `*` crossing `/` must be updated to `**`. For example: ```yaml before — worked accidentally because * crossed / • path: /upload/*/drafts/ after — single-segment match (behaviour unchanged for single-segment IDs) • path: /upload/*/drafts/ after — multi-segment match (e.g. nested object IDs containing /) • path: /upload/**/drafts/ ``` A migration note in the changelog would be needed. --- Issue 2 — Percent-encoded slashes bypass auth via segment collapsing Background With Fix 1 applied, `*` only matches a single path segment. However, the auth middleware matches against `r.URL.RequestURI()` — the encoded path — while the bucket handler uses `r.URL.Path` — the decoded path. A client can use `%2F` to make what looks like a single segment in the encoded URI decode into multiple segments including a protected path component. Example Using the same config as Issue 1: ``` PUT /upload/foo%2Frestricted/drafts/ ``` Step by step: 1. `r.URL.RawPath` = `/upload/foo%2Frestricted/drafts/` 2. `r.URL.Path` (decoded) = `/upload/foo/restricted/drafts/` 3. Auth middleware calls `r.URL.RequestURI()` → returns the encoded form. 4. With Fix 1's separator `/`, glob splits on the literal `/`. The segment between the first and second slash is `foo%2Frestricted` — one token with no literal `/` — so `*` matches it. Pattern `/upload/*/drafts/` fires. 5. Open route → request proceeds without auth. 6. Bucket handler uses `r.URL.Path` → S3 key is `upload/foo/restricted/drafts/…` — written into the restricted namespace without credentials. Proof via integration test I added `TestPercentEncodedSlashBypass` to `pkg/s3-proxy/server/server_integration_test.go`. The test sends a complete multipart PUT without credentials and asserts a 401 response. It currently fails with 204 — the file is written in full to the restricted namespace without any authentication. Fix discussion This issue has two fundamentally different classes of fix, each with a different stance on RFC 3986 compliance. Option A — Match auth against the decoded path (`r.URL.Path`) Change the auth middleware to use `r.URL.Path` instead of `r.URL.RequestURI()`: ```go // before requestURI := r.URL.RequestURI() // after requestURI := r.URL.Path ``` Both auth and the bucket handler now operate on the same decoded string, cl…