From 8503ba61ff51d48a7313896483d130eb6a5abfe0 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Sat, 27 Jun 2026 08:39:14 +0200 Subject: [PATCH] fix(raw): neutralize backslashes in archive entry names (GHSA-83xp-526h-j3ww) The fix for CVE-2026-54093 rewrote backslashes to the path separator "/" in archive entry names. On POSIX hosts a backslash is a legal filename byte, so that rewrite manufactured a traversal sequence ("..\..\x" -> "../../x") out of a single in-scope file, turning a Windows-only zip-slip into a cross-platform one. Neutralize backslashes to an inert character instead, and reject any entry whose name is not already a normalized root-relative path. Adds a regression test that downloads a folder containing a backslash-named file as a zip. --- http/raw.go | 20 +++++++++++----- http/raw_test.go | 61 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/http/raw.go b/http/raw.go index 860d6aa7..8f8c5751 100644 --- a/http/raw.go +++ b/http/raw.go @@ -2,6 +2,7 @@ package fbhttp import ( "errors" + "fmt" "io/fs" "log" "net/http" @@ -125,12 +126,19 @@ func getFiles(d *data, path, commonPath string) ([]archives.FileInfo, error) { nameInArchive := strings.TrimPrefix(path, commonPath) nameInArchive = strings.TrimPrefix(nameInArchive, string(filepath.Separator)) nameInArchive = filepath.ToSlash(nameInArchive) - // filepath.ToSlash only rewrites the host separator, so on a Linux - // host a stored backslash survives and is emitted verbatim into the - // archive. Windows extractors then treat "\" as a path separator, - // allowing the entry to escape the extraction directory (zip-slip). - // Strip Windows separators regardless of host OS. - nameInArchive = strings.ReplaceAll(nameInArchive, "\\", "/") + // A backslash is a legal filename character on POSIX hosts, so it can + // reach here verbatim. Rewriting it to the path separator "/" would + // manufacture a traversal sequence (e.g. "..\..\x" -> "../../x") that + // escapes the extraction directory on the victim's machine, while + // leaving it as "\" lets Windows extractors treat it as a separator. + // Neutralize it to an inert character instead of turning it into one. + nameInArchive = strings.ReplaceAll(nameInArchive, "\\", "_") + + // Defense in depth: never emit an archive entry whose path escapes the + // archive root, regardless of how the name was produced. + if cleaned := gopath.Clean("/" + nameInArchive); cleaned != "/"+nameInArchive { + return nil, fmt.Errorf("refusing unsafe archive entry name: %q", nameInArchive) + } archiveFiles = append(archiveFiles, archives.FileInfo{ FileInfo: info, diff --git a/http/raw_test.go b/http/raw_test.go index c35334f6..7fe3e916 100644 --- a/http/raw_test.go +++ b/http/raw_test.go @@ -1,14 +1,75 @@ package fbhttp import ( + "archive/zip" + "bytes" "net/http" "net/http/httptest" "net/url" + "os" + "path" + "path/filepath" + "strings" "testing" "github.com/filebrowser/filebrowser/v2/files" + "github.com/filebrowser/filebrowser/v2/settings" + "github.com/filebrowser/filebrowser/v2/users" ) +// Regression for the archive backslash-to-slash zip-slip (GHSA-83xp-526h-j3ww): +// a single in-scope file whose name contains backslashes is a legal POSIX +// filename, not a traversal. The archive builder must never rewrite "\" into the +// path separator "/", which would manufacture an entry like "../../evil.sh" that +// escapes the extraction directory on the downloader's machine. +func TestRawArchiveDoesNotManufactureTraversal(t *testing.T) { + root := t.TempDir() + userScope := filepath.Join(root, "user") + if err := os.MkdirAll(filepath.Join(userScope, "ziptest"), 0o755); err != nil { + t.Fatal(err) + } + + // One legal Linux/macOS filename whose bytes include backslashes. It does not + // traverse on the server; it only becomes "../../evil.sh" if the builder + // turns "\" into "/". + planted := filepath.Join(userScope, "ziptest", "..\\..\\evil.sh") + if err := os.WriteFile(planted, []byte("#!/bin/sh\necho PWNED"), 0o644); err != nil { + t.Skipf("cannot create backslash-named file: %v", err) + } + + key := []byte("test-signing-key") + perm := users.Permissions{Download: true} + st := scopedUserStorage(t, userScope, perm, key) + signed := signToken(t, perm, key) + + req, _ := http.NewRequest(http.MethodGet, "/ziptest?algo=zip", http.NoBody) + req.Header.Set("X-Auth", signed) + rec := httptest.NewRecorder() + handle(rawHandler, "", st, &settings.Server{}).ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d body=%q", rec.Code, rec.Body.String()) + } + + zr, err := zip.NewReader(bytes.NewReader(rec.Body.Bytes()), int64(rec.Body.Len())) + if err != nil { + t.Fatalf("failed to read zip: %v", err) + } + if len(zr.File) == 0 { + t.Fatal("archive has no entries") + } + + for _, f := range zr.File { + // The entry must be a normalized, root-relative path: no ".." segments + // and no leading "/". Note a name may legitimately contain ".." as part of + // a single filename (e.g. ".._.._evil.sh"), which Clean leaves untouched — + // so compare against the normalized form rather than searching for "..". + if strings.HasPrefix(f.Name, "/") || path.Clean("/"+f.Name) != "/"+f.Name { + t.Errorf("VULNERABLE: archive entry escapes root: %q", f.Name) + } + } +} + func TestSetContentDisposition(t *testing.T) { t.Parallel()