Sanitize paths in static file server; some cleanup

Also remove AutomaticHTTPSError for now
This commit is contained in:
Matthew Holt 2019-05-20 17:15:38 -06:00
parent d22f64e6d4
commit aaacab1bc3
3 changed files with 159 additions and 50 deletions

View file

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

View file

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

View file

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