fix(static): disable static path unescaping by default to prevent ACL bypass (GHSA-3pmx-cf9f-34xr)#3015
fix(static): disable static path unescaping by default to prevent ACL bypass (GHSA-3pmx-cf9f-34xr)#3015vishr wants to merge 1 commit into
Conversation
… bypass Fixes GHSA-3pmx-cf9f-34xr, a bypass of the GHSA-vfp3-v2gw-7wfq fix. The router matches the raw, still-encoded request path, so encoded separators and dot segments in a static wildcard are not seen as traversal during routing. Unescaping them in the static file resolver afterwards let an attacker reach a file across a route-level middleware guard the encoded path never matched: - /public/%2E%2E/admin/secret.txt resolved to admin/secret.txt (high severity, default router) - /public%2F..%2Fadmin%2Fsecret.txt under UseEscapedPathForMatching=true, where the router decodes the path itself before the handler sees it Rather than keep extending an encoding denylist, address the root cause: make static path unescaping opt-in. - echo: Config.EnablePathUnescapingStaticFiles (default false) controls unescaping for Echo.Static/StaticFS and Group.Static/StaticFS. - middleware: StaticConfig.EnablePathUnescaping replaces the now deprecated DisablePathUnescaping (default is the safe, no-unescape mode). With unescaping off, %2F/%5C/%2E%2E stay literal and never become separators or traversal. As defense in depth, and to also close the UseEscapedPathForMatching variant (where the router, not the handler, does the decoding), reject any ".." path segment in the resolved wildcard via pathutil.HasDotDotSegment, mirroring the fs.ValidPath "no .. element" invariant. The existing encoded-separator guard remains as a backstop on the opt-in unescaping path. BREAKING CHANGE: static files whose names contain URL-encoded characters (e.g. "hello world.txt" via /hello%20world.txt) are no longer served by default; set EnablePathUnescapingStaticFiles / EnablePathUnescaping to opt back in. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| // Only '/' is a separator here: encoded backslashes are rejected earlier by | ||
| // HasEncodedPathSeparator, and on a forward-slash fs.FS a literal backslash never | ||
| // acts as a separator, so a "..\\" segment cannot traverse. | ||
| func HasDotDotSegment(p string) bool { |
There was a problem hiding this comment.
@vishr , I do not think this HasDotDotSegment and any of pathutil is a maintainable solution. LLM can generate always another patch for leaking solution and complexity only grows.
There was a problem hiding this comment.
Root problem here is that router and Static file serving methods and Static middleware are using path inconsistently. Creating functions to check path is just a fix for the symptom not the cause.
|
@aldas Let me take a look. |
|
Superseding this in favor of #3016. My addition here was the #3016 takes the better route: disable unescaping by default (closing the high-severity encoded |
|
@vishr Hi, thanks for publishing the advisory and for the credit. We really appreciate it. I noticed the advisory currently shows "No known CVE". Could you request a CVE ID for GHSA-vfp3-v2gw-7wfq? We'd like to reference it properly in any future writeup. |
|
@a-tt-om Thanks again for the report and the thorough write-up. I've just requested a CVE for GHSA-vfp3-v2gw-7wfq — GitHub will assign and back-fill the ID onto the advisory shortly. Heads up on the follow-up: the related bypass you flagged (GHSA-3pmx-cf9f-34xr) is being fixed in #3016 and will go out in v5.2.1. We'll publish that advisory with its own CVE and credit once the release is tagged. |
Summary
Fixes GHSA-3pmx-cf9f-34xr, a bypass of the GHSA-vfp3-v2gw-7wfq fix (#3009).
The router matches the raw, still-encoded request path, so encoded separators and dot segments in a static wildcard are not treated as traversal during routing. Unescaping them in the static file resolver afterwards let an unauthenticated attacker read a file across a route-level middleware guard the encoded path never matched:
/public/%2E%2E/admin/secret.txt→admin/secret.txt— high severity, default router/public%2F..%2Fadmin%2Fsecret.txtunderUseEscapedPathForMatching=true, where the router decodes the path itself before the handler sees it — lower severity (non-default config)Both returned
200+ the protected file onmasterat5786024(the exact commit the advisory tested).Approach
Rather than keep extending an encoding denylist (the original fix blocked
%2F/%5Cbut missed%2E%2E), this addresses the root cause: make static path unescaping opt-in. This follows @aldas's proposal (e85ee8f), rebased onto currentmaster.Config.EnablePathUnescapingStaticFiles(defaultfalse) controls unescaping forEcho.Static/StaticFSandGroup.Static/StaticFS.StaticConfig.EnablePathUnescapingreplaces the now-deprecatedDisablePathUnescaping; the default is the safe, no-unescape mode.With unescaping off,
%2F/%5C/%2E%2Estay literal and never become separators or traversal.As defense in depth — and to also close the
UseEscapedPathForMatchingvariant, where the router (not the handler) does the decoding — any..path segment in the resolved wildcard is rejected viapathutil.HasDotDotSegment, mirroring thefs.ValidPath"no..element" invariant. The existing encoded-separator guard remains as a backstop on the opt-in unescaping path.Verification
master(5786024)/public/%2E%2E/admin/secret.txt(default router)TOP-SECRET/public%2F..%2Fadmin%2Fsecret.txt(UseEscapedPathForMatching)TOP-SECRETTOP-SECRETRegression tests cover both variants in both router modes, the opt-in mode (encoded
%20filenames serve, but%2F/..still rejected), andpathutil.HasDotDotSegmentunits.go test -race ./...andgo vet ./...pass.Static files whose names contain URL-encoded characters (e.g.
"hello world.txt"via/hello%20world.txt) are no longer served by default. SetEnablePathUnescapingStaticFiles(echo) /EnablePathUnescaping(middleware) to opt back in. Because this flips a default, suggest releasing as 5.3.0 with an upgrade note rather than a patch.Notes for reviewers
RawPath-preferring directory-redirect tweak frome85ee8fto keep this scoped to the vuln; happy to fold it in...rejection needed to close theUseEscapedPathForMatchingvariant your patch didn't reach.🤖 Generated with Claude Code