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.
This commit is contained in:
Henrique Dias 2026-06-27 08:39:14 +02:00
parent 1fb05d65de
commit 8503ba61ff
No known key found for this signature in database
2 changed files with 75 additions and 6 deletions

View file

@ -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,

View file

@ -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()