From 9d4ed3a3236df06e54c80c4f6633b66d68ad3673 Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Thu, 17 Jun 2021 09:59:08 -0600 Subject: caddyhttp: Refactor and export SanitizedPathJoin for use in fastcgi (#4207) --- modules/caddyhttp/fileserver/browse.go | 2 +- modules/caddyhttp/fileserver/matcher.go | 2 +- modules/caddyhttp/fileserver/matcher_test.go | 4 +- modules/caddyhttp/fileserver/staticfiles.go | 40 +---------- modules/caddyhttp/fileserver/staticfiles_test.go | 84 ------------------------ 5 files changed, 6 insertions(+), 126 deletions(-) (limited to 'modules/caddyhttp/fileserver') diff --git a/modules/caddyhttp/fileserver/browse.go b/modules/caddyhttp/fileserver/browse.go index 3122f12..cd9bcbc 100644 --- a/modules/caddyhttp/fileserver/browse.go +++ b/modules/caddyhttp/fileserver/browse.go @@ -207,7 +207,7 @@ func isSymlinkTargetDir(f os.FileInfo, root, urlPath string) bool { if !isSymlink(f) { return false } - target := sanitizedPathJoin(root, path.Join(urlPath, f.Name())) + target := caddyhttp.SanitizedPathJoin(root, path.Join(urlPath, f.Name())) targetInfo, err := os.Stat(target) if err != nil { return false diff --git a/modules/caddyhttp/fileserver/matcher.go b/modules/caddyhttp/fileserver/matcher.go index 064398c..739b9e0 100644 --- a/modules/caddyhttp/fileserver/matcher.go +++ b/modules/caddyhttp/fileserver/matcher.go @@ -185,7 +185,7 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) { if strings.HasSuffix(file, "/") { suffix += "/" } - fullpath = sanitizedPathJoin(root, suffix) + fullpath = caddyhttp.SanitizedPathJoin(root, suffix) return } diff --git a/modules/caddyhttp/fileserver/matcher_test.go b/modules/caddyhttp/fileserver/matcher_test.go index e319907..746a5d7 100644 --- a/modules/caddyhttp/fileserver/matcher_test.go +++ b/modules/caddyhttp/fileserver/matcher_test.go @@ -94,7 +94,7 @@ func TestFileMatcher(t *testing.T) { t.Fatalf("Test %d: actual path: %v, expected: %v", i, rel, tc.expectedPath) } - fileType, ok := repl.Get("http.matchers.file.type") + fileType, _ := repl.Get("http.matchers.file.type") if fileType != tc.expectedType { t.Fatalf("Test %d: actual file type: %v, expected: %v", i, fileType, tc.expectedType) } @@ -197,7 +197,7 @@ func TestPHPFileMatcher(t *testing.T) { t.Fatalf("Test %d: actual path: %v, expected: %v", i, rel, tc.expectedPath) } - fileType, ok := repl.Get("http.matchers.file.type") + fileType, _ := repl.Get("http.matchers.file.type") if fileType != tc.expectedType { t.Fatalf("Test %d: actual file type: %v, expected: %v", i, fileType, tc.expectedType) } diff --git a/modules/caddyhttp/fileserver/staticfiles.go b/modules/caddyhttp/fileserver/staticfiles.go index 9266332..45f8610 100644 --- a/modules/caddyhttp/fileserver/staticfiles.go +++ b/modules/caddyhttp/fileserver/staticfiles.go @@ -161,7 +161,7 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c filesToHide := fsrv.transformHidePaths(repl) root := repl.ReplaceAll(fsrv.Root, ".") - filename := sanitizedPathJoin(root, r.URL.Path) + filename := caddyhttp.SanitizedPathJoin(root, r.URL.Path) fsrv.logger.Debug("sanitized path join", zap.String("site_root", root), @@ -185,7 +185,7 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c var implicitIndexFile bool if info.IsDir() && len(fsrv.IndexNames) > 0 { for _, indexPage := range fsrv.IndexNames { - indexPath := sanitizedPathJoin(filename, indexPage) + indexPath := caddyhttp.SanitizedPathJoin(filename, indexPage) if fileHidden(indexPath, filesToHide) { // pretend this file doesn't exist fsrv.logger.Debug("hiding index file", @@ -435,42 +435,6 @@ func (fsrv *FileServer) transformHidePaths(repl *caddy.Replacer) []string { return hide } -// sanitizedPathJoin performs filepath.Join(root, reqPath) that -// is safe against directory traversal attacks. It uses logic -// similar to that in the Go standard library, specifically -// in the implementation of http.Dir. The root is assumed to -// be a trusted path, but reqPath is not. -func sanitizedPathJoin(root, reqPath string) string { - // TODO: Caddy 1 uses this: - // prevent absolute path access on Windows, e.g. http://localhost:5000/C:\Windows\notepad.exe - // if runtime.GOOS == "windows" && len(reqPath) > 0 && filepath.IsAbs(reqPath[1:]) { - // TODO. - // } - - // TODO: whereas std lib's http.Dir.Open() uses this: - // if filepath.Separator != '/' && strings.ContainsRune(name, filepath.Separator) { - // return nil, errors.New("http: invalid character in file path") - // } - - // TODO: see https://play.golang.org/p/oh77BiVQFti for another thing to consider - - if root == "" { - root = "." - } - - path := filepath.Join(root, filepath.Clean("/"+reqPath)) - - // filepath.Join also cleans the path, and cleaning strips - // the trailing slash, so we need to re-add it afterwards. - // if the length is 1, then it's a path to the root, - // and that should return ".", so we don't append the separator. - if strings.HasSuffix(reqPath, "/") && len(reqPath) > 1 { - path += separator - } - - return path -} - // fileHidden returns true if filename is hidden according to the hide list. // filename must be a relative or absolute file system path, not a request // URI path. It is expected that all the paths in the hide list are absolute diff --git a/modules/caddyhttp/fileserver/staticfiles_test.go b/modules/caddyhttp/fileserver/staticfiles_test.go index 690f46a..5d6133c 100644 --- a/modules/caddyhttp/fileserver/staticfiles_test.go +++ b/modules/caddyhttp/fileserver/staticfiles_test.go @@ -15,96 +15,12 @@ package fileserver import ( - "net/url" "path/filepath" "runtime" "strings" "testing" ) -func TestSanitizedPathJoin(t *testing.T) { - // For easy reference: - // %2e = . - // %2f = / - // %5c = \ - for i, tc := range []struct { - inputRoot string - inputPath string - expect string - }{ - { - inputPath: "", - expect: ".", - }, - { - inputPath: "/", - expect: ".", - }, - { - inputPath: "/foo", - expect: "foo", - }, - { - inputPath: "/foo/", - expect: "foo" + separator, - }, - { - inputPath: "/foo/bar", - expect: filepath.Join("foo", "bar"), - }, - { - inputRoot: "/a", - inputPath: "/foo/bar", - expect: filepath.Join("/", "a", "foo", "bar"), - }, - { - inputPath: "/foo/../bar", - expect: "bar", - }, - { - inputRoot: "/a/b", - inputPath: "/foo/../bar", - expect: filepath.Join("/", "a", "b", "bar"), - }, - { - inputRoot: "/a/b", - inputPath: "/..%2fbar", - expect: filepath.Join("/", "a", "b", "bar"), - }, - { - inputRoot: "/a/b", - inputPath: "/%2e%2e%2fbar", - expect: filepath.Join("/", "a", "b", "bar"), - }, - { - inputRoot: "/a/b", - inputPath: "/%2e%2e%2f%2e%2e%2f", - expect: filepath.Join("/", "a", "b") + separator, - }, - { - inputRoot: "C:\\www", - inputPath: "/foo/bar", - expect: filepath.Join("C:\\www", "foo", "bar"), - }, - // TODO: test more windows paths... on windows... sigh. - } { - // we don't *need* to use an actual parsed URL, but it - // adds some authenticity to the tests since real-world - // values will be coming in from URLs; thus, the test - // corpus can contain paths as encoded by clients, which - // more closely emulates the actual attack vector - u, err := url.Parse("http://test:9999" + tc.inputPath) - if err != nil { - t.Fatalf("Test %d: invalid URL: %v", i, err) - } - actual := sanitizedPathJoin(tc.inputRoot, u.Path) - if actual != tc.expect { - t.Errorf("Test %d: [%s %s] => %s (expected %s)", - i, tc.inputRoot, tc.inputPath, actual, tc.expect) - } - } -} - func TestFileHidden(t *testing.T) { for i, tc := range []struct { inputHide []string -- cgit v1.2.3