From aaacab1bc3790e3a207ae5d70ca6559cac265bff Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Mon, 20 May 2019 17:15:38 -0600 Subject: Sanitize paths in static file server; some cleanup Also remove AutomaticHTTPSError for now --- modules/caddyhttp/caddyhttp.go | 16 +-- modules/caddyhttp/staticfiles/staticfiles.go | 115 +++++++++++++++------- modules/caddyhttp/staticfiles/staticfiles_test.go | 78 +++++++++++++++ 3 files changed, 159 insertions(+), 50 deletions(-) create mode 100644 modules/caddyhttp/staticfiles/staticfiles_test.go (limited to 'modules/caddyhttp') diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go index 41b0c66..e5b9f61 100644 --- a/modules/caddyhttp/caddyhttp.go +++ b/modules/caddyhttp/caddyhttp.go @@ -95,21 +95,11 @@ func (app *App) Validate() error { return nil } -// AutomaticHTTPSError represents an error received when attempting to enable automatic https. -type AutomaticHTTPSError struct { - internal error -} - -// Error returns the error string for automaticHTTPSError. -func (a AutomaticHTTPSError) Error() string { - return fmt.Sprintf("enabling automatic HTTPS: %v", a.internal.Error()) -} - // Start runs the app. It sets up automatic HTTPS if enabled. func (app *App) Start() error { err := app.automaticHTTPS() if err != nil { - return &AutomaticHTTPSError{internal: err} + return fmt.Errorf("enabling automatic HTTPS: %v", err) } for srvName, srv := range app.Servers { @@ -243,7 +233,7 @@ func (app *App) automaticHTTPS() error { if parts := strings.SplitN(port, "-", 2); len(parts) == 2 { port = parts[0] } - redirTo := "https://{request.host}" + redirTo := "https://{http.request.host}" httpsPort := app.HTTPSPort if httpsPort == 0 { @@ -252,7 +242,7 @@ func (app *App) automaticHTTPS() error { if port != strconv.Itoa(httpsPort) { redirTo += ":" + port } - redirTo += "{request.uri}" + redirTo += "{http.request.uri}" redirRoutes = append(redirRoutes, ServerRoute{ matchers: []RequestMatcher{ diff --git a/modules/caddyhttp/staticfiles/staticfiles.go b/modules/caddyhttp/staticfiles/staticfiles.go index e3af352..f9fd8d2 100644 --- a/modules/caddyhttp/staticfiles/staticfiles.go +++ b/modules/caddyhttp/staticfiles/staticfiles.go @@ -72,14 +72,21 @@ func (sf *StaticFiles) Provision(ctx caddy2.Context) error { return nil } +const ( + selectionPolicyFirstExisting = "first_existing" + selectionPolicyLargestSize = "largest_size" + selectionPolicySmallestSize = "smallest_size" + selectionPolicyRecentlyMod = "most_recently_modified" +) + // Validate ensures that sf has a valid configuration. func (sf *StaticFiles) Validate() error { switch sf.SelectionPolicy { case "", - "first_existing", - "largest_size", - "smallest_size", - "most_recently_modified": + selectionPolicyFirstExisting, + selectionPolicyLargestSize, + selectionPolicySmallestSize, + selectionPolicyRecentlyMod: default: return fmt.Errorf("unknown selection policy %s", sf.SelectionPolicy) } @@ -87,15 +94,6 @@ func (sf *StaticFiles) Validate() error { } func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error { - // TODO: Prevent directory traversal, see https://play.golang.org/p/oh77BiVQFti - - // TODO: Still needed? - // // Prevent absolute path access on Windows, e.g.: http://localhost:5000/C:\Windows\notepad.exe - // // TODO: does stdlib http.Dir handle this? see first check of http.Dir.Open()... - // if runtime.GOOS == "windows" && len(reqPath) > 0 && filepath.IsAbs(reqPath[1:]) { - // return caddyhttp.Error(http.StatusNotFound, fmt.Errorf("request path was absolute")) - // } - repl := r.Context().Value(caddy2.ReplacerCtxKey).(caddy2.Replacer) // map the request to a filename @@ -113,7 +111,6 @@ func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error { // if the ultimate destination has changed, submit // this request for a rehandling (internal redirect) // if configured to do so - // TODO: double check this against https://docs.nginx.com/nginx/admin-guide/web-server/serving-static-content/ if r.URL.Path != pathBefore && sf.Rehandle { return caddyhttp.ErrRehandle } @@ -121,6 +118,7 @@ func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error { // get information about the file info, err := os.Stat(filename) if err != nil { + err = mapDirOpenError(err, filename) if os.IsNotExist(err) { return caddyhttp.Error(http.StatusNotFound, err) } else if os.IsPermission(err) { @@ -136,7 +134,7 @@ func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error { filesToHide := sf.transformHidePaths(repl) for _, indexPage := range sf.IndexNames { - indexPath := path.Join(filename, indexPage) + indexPath := sanitizedPathJoin(filename, indexPage) if fileHidden(indexPath, filesToHide) { // pretend this file doesn't exist continue @@ -150,8 +148,6 @@ func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error { // we found an index file that might work, // so rewrite the request path and, if // configured, do an internal redirect - // TODO: I don't know if the logic for rewriting - // the URL here is the right logic r.URL.Path = path.Join(r.URL.Path, indexPage) if sf.Rehandle { return caddyhttp.ErrRehandle @@ -172,6 +168,8 @@ func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error { return caddyhttp.Error(http.StatusNotFound, nil) } + // TODO: content negotiation (brotli sidecar files, etc...) + // open the file file, err := sf.openFile(filename, w) if err != nil { @@ -179,9 +177,7 @@ func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error { } defer file.Close() - // TODO: Etag? - - // TODO: content negotiation? (brotli sidecar files, etc...) + // TODO: Etag // let the standard library do what it does best; note, however, // that errors generated by ServeContent are written immediately @@ -199,6 +195,7 @@ func (sf *StaticFiles) ServeHTTP(w http.ResponseWriter, r *http.Request) error { func (sf *StaticFiles) openFile(filename string, w http.ResponseWriter) (*os.File, error) { file, err := os.Open(filename) if err != nil { + err = mapDirOpenError(err, filename) if os.IsNotExist(err) { return nil, caddyhttp.Error(http.StatusNotFound, err) } else if os.IsPermission(err) { @@ -213,6 +210,34 @@ func (sf *StaticFiles) openFile(filename string, w http.ResponseWriter) (*os.Fil return file, nil } +// mapDirOpenError maps the provided non-nil error from opening name +// to a possibly better non-nil error. In particular, it turns OS-specific errors +// about opening files in non-directories into os.ErrNotExist. See golang/go#18984. +// Adapted from the Go standard library; originally written by Nathaniel Caza. +// https://go-review.googlesource.com/c/go/+/36635/ +// https://go-review.googlesource.com/c/go/+/36804/ +func mapDirOpenError(originalErr error, name string) error { + if os.IsNotExist(originalErr) || os.IsPermission(originalErr) { + return originalErr + } + + parts := strings.Split(name, string(filepath.Separator)) + for i := range parts { + if parts[i] == "" { + continue + } + fi, err := os.Stat(strings.Join(parts[:i+1], string(filepath.Separator))) + if err != nil { + return originalErr + } + if !fi.IsDir() { + return os.ErrNotExist + } + } + + return originalErr +} + // transformHidePaths performs replacements for all the elements of // sf.Hide and returns a new list of the transformed values. func (sf *StaticFiles) transformHidePaths(repl caddy2.Replacer) []string { @@ -223,42 +248,60 @@ func (sf *StaticFiles) transformHidePaths(repl caddy2.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. +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 = "." + } + return filepath.Join(root, filepath.FromSlash(path.Clean("/"+reqPath))) +} + // selectFile uses the specified selection policy (or first_existing // by default) to map the request r to a filename. The full path to // the file is returned if one is found; otherwise, an empty string // is returned. func (sf *StaticFiles) selectFile(r *http.Request, repl caddy2.Replacer) string { root := repl.ReplaceAll(sf.Root, "") - if root == "" { - root = "." - } if sf.Files == nil { - return filepath.Join(root, r.URL.Path) + return sanitizedPathJoin(root, r.URL.Path) } switch sf.SelectionPolicy { - // TODO: Make these policy names constants - case "", "first_existing": + case "", selectionPolicyFirstExisting: filesToHide := sf.transformHidePaths(repl) for _, f := range sf.Files { suffix := repl.ReplaceAll(f, "") - // TODO: sanitize path - fullpath := filepath.Join(root, suffix) + fullpath := sanitizedPathJoin(root, suffix) if !fileHidden(fullpath, filesToHide) && fileExists(fullpath) { r.URL.Path = suffix return fullpath } } - case "largest_size": + case selectionPolicyLargestSize: var largestSize int64 var largestFilename string var largestSuffix string for _, f := range sf.Files { suffix := repl.ReplaceAll(f, "") - // TODO: sanitize path - fullpath := filepath.Join(root, suffix) + fullpath := sanitizedPathJoin(root, suffix) info, err := os.Stat(fullpath) if err == nil && info.Size() > largestSize { largestSize = info.Size() @@ -269,14 +312,13 @@ func (sf *StaticFiles) selectFile(r *http.Request, repl caddy2.Replacer) string r.URL.Path = largestSuffix return largestFilename - case "smallest_size": + case selectionPolicySmallestSize: var smallestSize int64 var smallestFilename string var smallestSuffix string for _, f := range sf.Files { suffix := repl.ReplaceAll(f, "") - // TODO: sanitize path - fullpath := filepath.Join(root, suffix) + fullpath := sanitizedPathJoin(root, suffix) info, err := os.Stat(fullpath) if err == nil && (smallestSize == 0 || info.Size() < smallestSize) { smallestSize = info.Size() @@ -287,14 +329,13 @@ func (sf *StaticFiles) selectFile(r *http.Request, repl caddy2.Replacer) string r.URL.Path = smallestSuffix return smallestFilename - case "most_recently_modified": + case selectionPolicyRecentlyMod: var recentDate time.Time var recentFilename string var recentSuffix string for _, f := range sf.Files { suffix := repl.ReplaceAll(f, "") - // TODO: sanitize path - fullpath := filepath.Join(root, suffix) + fullpath := sanitizedPathJoin(root, suffix) info, err := os.Stat(fullpath) if err == nil && (recentDate.IsZero() || info.ModTime().After(recentDate)) { diff --git a/modules/caddyhttp/staticfiles/staticfiles_test.go b/modules/caddyhttp/staticfiles/staticfiles_test.go new file mode 100644 index 0000000..f2e1c89 --- /dev/null +++ b/modules/caddyhttp/staticfiles/staticfiles_test.go @@ -0,0 +1,78 @@ +package staticfiles + +import ( + "net/url" + "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/bar", + expect: "foo/bar", + }, + { + inputRoot: "/a", + inputPath: "/foo/bar", + expect: "/a/foo/bar", + }, + { + inputPath: "/foo/../bar", + expect: "bar", + }, + { + inputRoot: "/a/b", + inputPath: "/foo/../bar", + expect: "/a/b/bar", + }, + { + inputRoot: "/a/b", + inputPath: "/..%2fbar", + expect: "/a/b/bar", + }, + { + inputRoot: "/a/b", + inputPath: "/%2e%2e%2fbar", + expect: "/a/b/bar", + }, + { + inputRoot: "/a/b", + inputPath: "/%2e%2e%2f%2e%2e%2f", + expect: "/a/b", + }, + } { + // 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) + } + } +} -- cgit v1.2.3