diff --git a/README.md b/README.md index 3b07ab6..fefb78c 100644 --- a/README.md +++ b/README.md @@ -230,6 +230,14 @@ users: # noPassword: true ``` +### Rules + +Rules are matched against the request path after dot segments have been resolved, so `/public/../secret/file` is matched as `/secret/file`. The last rule that matches wins. + +A `path` rule is a prefix match. A rule written with a trailing slash also covers the collection it names, so `path: /secret/` applies to a request for `/secret` as well. Such a rule can only restrict that collection: acting on the collection itself also requires the permissions that apply outside the rule, since the operation takes place in the parent collection. + +A `regex` rule is matched literally against the path, and gets none of the above handling. In particular `regex: "^/secret/"` does **not** match a request for `/secret` itself. Write `regex: "^/secret(/|$)"` if you want to cover the collection too. + ### CORS The `allowed_*` properties are optional, the default value for each of them will be `*`. `exposed_headers` is optional as well, but is not set if not defined. Setting `credentials` to `true` will allow you to: diff --git a/lib/handler.go b/lib/handler.go index 047e694..e1b88e7 100644 --- a/lib/handler.go +++ b/lib/handler.go @@ -3,7 +3,6 @@ package lib import ( "net/http" "os" - "strings" "github.com/rs/cors" "go.uber.org/zap" @@ -167,8 +166,8 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // collection resources. // // GET (or HEAD), when applied to collection, will return the same as PROPFIND method. - if (r.Method == "GET" || r.Method == "HEAD") && strings.HasPrefix(r.URL.Path, user.Prefix) { - info, err := user.FileSystem.Stat(r.Context(), strings.TrimPrefix(r.URL.Path, user.Prefix)) + if r.Method == "GET" || r.Method == "HEAD" { + info, err := user.FileSystem.Stat(r.Context(), req.path) if err == nil && info.IsDir() { r.Method = "PROPFIND" diff --git a/lib/handler_test.go b/lib/handler_test.go index 166c814..ec503c2 100644 --- a/lib/handler_test.go +++ b/lib/handler_test.go @@ -1012,3 +1012,243 @@ users: require.ErrorContains(t, err, "403") }) } + +func TestServerRulesDotSegments(t *testing.T) { + t.Parallel() + + // Sends a request with an unmodified request-target, since a WebDAV client + // would normalize the dot segments away before they reach the server. + do := func(t *testing.T, method, url string, header map[string]string) int { + t.Helper() + + req, err := http.NewRequest(method, url, nil) + require.NoError(t, err) + for k, v := range header { + req.Header.Set(k, v) + } + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + return resp.StatusCode + } + + makeServer := func(t *testing.T) (*httptest.Server, string) { + dir := makeTestDirectory(t, map[string][]byte{ + "public/pub.txt": []byte("public"), + "secret/flag.txt": []byte("secret"), + "secret/keep.txt": []byte("keep"), + }) + + srv := makeTestServer(t, fmt.Sprintf(` +directory: %s +permissions: CRUD +rules: + - path: "/secret/" + permissions: none +`, dir)) + t.Cleanup(srv.Close) + return srv, dir + } + + t.Run("Source Path", func(t *testing.T) { + t.Parallel() + srv, _ := makeServer(t) + + for _, path := range []string{ + "/secret/flag.txt", + "/public/../secret/flag.txt", + "/public/%2e%2e/secret/flag.txt", + "/public/../secret/", + "/secret/.", + "/secret/%2e", + } { + require.Equal(t, http.StatusForbidden, do(t, "GET", srv.URL+path, nil), path) + } + + require.Equal(t, http.StatusForbidden, do(t, "PUT", srv.URL+"/public/%2e%2e/secret/new.txt", nil)) + require.Equal(t, http.StatusForbidden, do(t, "DELETE", srv.URL+"/public/%2e%2e/secret/keep.txt", nil)) + + // A request that resolves outside a rule must still succeed. + require.Equal(t, http.StatusOK, do(t, "GET", srv.URL+"/secret/../public/pub.txt", nil)) + }) + + t.Run("Destination Header", func(t *testing.T) { + t.Parallel() + srv, _ := makeServer(t) + + for _, destination := range []string{ + srv.URL + "/public/%2e%2e/secret/moved.txt", + srv.URL + "/public/../secret/moved.txt", + "/public/%2e%2e/secret/moved.txt", + } { + code := do(t, "MOVE", srv.URL+"/public/pub.txt", map[string]string{ + "Destination": destination, + "Overwrite": "T", + }) + require.Equal(t, http.StatusForbidden, code, destination) + } + }) + + t.Run("Regex Rule", func(t *testing.T) { + t.Parallel() + + dir := makeTestDirectory(t, map[string][]byte{ + "public/pub.txt": []byte("public"), + "secret/flag.txt": []byte("secret"), + }) + + srv := makeTestServer(t, fmt.Sprintf(` +directory: %s +permissions: CRUD +rules: + - regex: "^/secret/" + permissions: none +`, dir)) + defer srv.Close() + + require.Equal(t, http.StatusForbidden, do(t, "GET", srv.URL+"/secret/flag.txt", nil)) + require.Equal(t, http.StatusForbidden, do(t, "GET", srv.URL+"/public/%2e%2e/secret/flag.txt", nil)) + }) + + t.Run("Directory Mounts", func(t *testing.T) { + t.Parallel() + + alpha := makeTestDirectory(t, map[string][]byte{"a.txt": []byte("a")}) + beta := makeTestDirectory(t, map[string][]byte{"b.txt": []byte("b")}) + + srv := makeTestServer(t, fmt.Sprintf(` +permissions: CRUD +directories: + - name: alpha + path: %s + - name: beta + path: %s +rules: + - path: "/beta/" + permissions: none +`, alpha, beta)) + defer srv.Close() + + require.Equal(t, http.StatusForbidden, do(t, "GET", srv.URL+"/beta/b.txt", nil)) + require.Equal(t, http.StatusForbidden, do(t, "GET", srv.URL+"/alpha/%2e%2e/beta/b.txt", nil)) + require.Equal(t, http.StatusOK, do(t, "GET", srv.URL+"/alpha/a.txt", nil)) + }) + + t.Run("No Users", func(t *testing.T) { + t.Parallel() + srv, _ := makeServer(t) + + // Without a users block no authentication runs, so the rule is the only + // access control there is. + require.Equal(t, http.StatusForbidden, do(t, "GET", srv.URL+"/public/%2e%2e/secret/flag.txt", nil)) + }) +} + +func TestServerRulesBareCollection(t *testing.T) { + t.Parallel() + + do := func(t *testing.T, method, url string) int { + t.Helper() + + req, err := http.NewRequest(method, url, nil) + require.NoError(t, err) + req.Header.Set("Depth", "1") + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + return resp.StatusCode + } + + dir := makeTestDirectory(t, map[string][]byte{ + "c/secret.txt": []byte("secret"), + "cd/open.txt": []byte("open"), + }) + + srv := makeTestServer(t, fmt.Sprintf(` +directory: %s +permissions: CRUD +rules: + - path: "/c/" + permissions: none +`, dir)) + defer srv.Close() + + // A rule written "/c/" must also cover the collection named without the + // trailing slash, otherwise the denied directory can be listed or deleted. + for _, path := range []string{"/c/", "/c", "/c/secret.txt"} { + require.Equal(t, http.StatusForbidden, do(t, "PROPFIND", srv.URL+path), path) + require.Equal(t, http.StatusForbidden, do(t, "DELETE", srv.URL+path), path) + } + + // A sibling whose name merely starts with the same characters is unaffected. + require.Equal(t, http.StatusMultiStatus, do(t, "PROPFIND", srv.URL+"/cd")) + require.Equal(t, http.StatusOK, do(t, "GET", srv.URL+"/cd/open.txt")) + + // A rule governs the collection it names, but must not grant access to it + // that would not otherwise exist: removing "/pub" acts on the root, which + // the global permissions still deny. + grantDir := makeTestDirectory(t, map[string][]byte{"pub/x.txt": []byte("x")}) + grantSrv := makeTestServer(t, fmt.Sprintf(` +directory: %s +permissions: none +rules: + - path: "/pub/" + permissions: CRUD +`, grantDir)) + defer grantSrv.Close() + + require.Equal(t, http.StatusNoContent, do(t, "DELETE", grantSrv.URL+"/pub/x.txt")) + require.Equal(t, http.StatusForbidden, do(t, "DELETE", grantSrv.URL+"/pub")) + require.Equal(t, http.StatusForbidden, do(t, "PROPFIND", grantSrv.URL+"/pub")) +} + +func TestServerRulesEmptyPrefixDestination(t *testing.T) { + t.Parallel() + + dir := makeTestDirectory(t, map[string][]byte{ + "public/a.txt": []byte("a"), + "public/b.txt": []byte("b"), + "secret/x.txt": []byte("secret"), + }) + + srv := makeTestServer(t, fmt.Sprintf(` +directory: %s +prefix: "" +permissions: CRUD +rules: + - path: "/secret/" + permissions: none +`, dir)) + defer srv.Close() + + // RFC 4918 has Destination as an absolute URI, so the host must not end up + // in the value the rules are matched against. + for name, destination := range map[string]string{ + "absolute": srv.URL + "/secret/moved.txt", + "bare path": "/secret/moved.txt", + "dot": srv.URL + "/public/%2e%2e/secret/moved.txt", + } { + req, err := http.NewRequest("MOVE", srv.URL+"/public/a.txt", nil) + require.NoError(t, err) + req.Header.Set("Destination", destination) + req.Header.Set("Overwrite", "T") + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.Equal(t, http.StatusForbidden, resp.StatusCode, name) + } + + // A destination outside any rule must still work. + req, err := http.NewRequest("MOVE", srv.URL+"/public/b.txt", nil) + require.NoError(t, err) + req.Header.Set("Destination", srv.URL+"/public/moved.txt") + req.Header.Set("Overwrite", "T") + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.Equal(t, http.StatusCreated, resp.StatusCode) +} diff --git a/lib/permissions.go b/lib/permissions.go index c7d1d39..ca3456b 100644 --- a/lib/permissions.go +++ b/lib/permissions.go @@ -35,6 +35,17 @@ func (r *Rule) Matches(path string) bool { return strings.HasPrefix(path, r.Path) } +// matchesCollection checks if [Rule] names path as the collection it governs, +// such as a rule for "/c/" and a request for "/c". Regex rules are matched +// literally and are not considered here. +func (r *Rule) matchesCollection(path string) bool { + if r.Regex != nil || !strings.HasSuffix(r.Path, "/") { + return false + } + + return path == strings.TrimSuffix(r.Path, "/") +} + type RulesBehavior string const ( @@ -67,35 +78,39 @@ func (p UserPermissions) Allowed(r *request, fileExists func(string) bool) bool // path. As soon as a rule matches and does not allow the operation at the destination, // we fail immediately. If no rule matches, we check the global permissions. if r.method == "COPY" || r.method == "MOVE" { - dst := r.destination - ruleMatched := false - - for i := len(p.Rules) - 1; i >= 0; i-- { - if p.Rules[i].Matches(dst) { - ruleMatched = true - if !p.Rules[i].Permissions.AllowedDestination(r, fileExists) { - return false - } - - // Only check the first rule that matches, similarly to the source rules. - break - } - } - - if !ruleMatched && !p.Permissions.AllowedDestination(r, fileExists) { + if !p.allowedAt(r.destination, func(perms Permissions) bool { + return perms.AllowedDestination(r, fileExists) + }) { return false } } - // Go through rules beginning from the last one, and check the permissions at - // the source. The first matched rule returns. + return p.allowedAt(r.path, func(perms Permissions) bool { + return perms.Allowed(r, fileExists) + }) +} + +// allowedAt resolves the permissions that govern path and applies check to them. +func (p UserPermissions) allowedAt(path string, check func(Permissions) bool) bool { + // Go through rules beginning from the last one. The first matched rule returns. for i := len(p.Rules) - 1; i >= 0; i-- { - if p.Rules[i].Matches(r.path) { - return p.Rules[i].Permissions.Allowed(r, fileExists) + if p.Rules[i].Matches(path) { + return check(p.Rules[i].Permissions) } } - return p.Permissions.Allowed(r, fileExists) + // A rule written with a trailing slash also governs the collection it names, + // so that a rule for "/c/" cannot be evaded by asking for "/c". Such a request + // acts on an entry of the parent collection, so it needs the permissions that + // apply there too. Requiring both means the rule can restrict the collection + // without granting access that would otherwise not exist. + for i := len(p.Rules) - 1; i >= 0; i-- { + if p.Rules[i].matchesCollection(path) { + return check(p.Rules[i].Permissions) && check(p.Permissions) + } + } + + return check(p.Permissions) } func (p *UserPermissions) Validate() error { diff --git a/lib/request.go b/lib/request.go index 9468577..7422334 100644 --- a/lib/request.go +++ b/lib/request.go @@ -4,9 +4,39 @@ import ( "errors" "net/http" "net/url" + "path" "strings" ) +// cleanPath resolves dot segments so that the permission checks see the same +// path that the backing file system will ultimately open. The file systems in +// golang.org/x/net/webdav apply path.Clean before joining the backing +// directory, so without this the two layers disagree on which file a request +// names and a rule can be bypassed with e.g. "/public/../secret/file.txt". +func cleanPath(p string) string { + if !strings.HasPrefix(p, "/") { + p = "/" + p + } + + cleaned := path.Clean(p) + + // path.Clean drops the trailing slash, but rules are prefix matches and are + // commonly written with one, such as "/c/". Dropping it would stop a request + // for the collection itself from matching the rule that names it. + if cleaned != "/" && isCollectionPath(p) { + cleaned += "/" + } + + return cleaned +} + +// isCollectionPath reports whether p names a collection rather than a resource +// within it. Besides an explicit trailing slash, a trailing "." or ".." segment +// also resolves to the collection itself. +func isCollectionPath(p string) bool { + return strings.HasSuffix(p, "/") || strings.HasSuffix(p, "/.") || strings.HasSuffix(p, "/..") +} + type request struct { method string path string @@ -24,6 +54,11 @@ func newRequest(r *http.Request, prefix string) (*request, error) { return nil, errors.New("invalid destination header") } + // RFC 4918, section 10.3, has Destination as an absolute URI, which is + // what clients send in practice. Only the path is relevant here, and + // taking it unconditionally keeps the host out of the matched value. + destination = u.Path + if prefix != "" { destination = strings.TrimPrefix(u.Path, prefix) if len(destination) >= len(u.Path) { @@ -31,11 +66,7 @@ func newRequest(r *http.Request, prefix string) (*request, error) { } } - if !strings.HasPrefix(destination, "/") { - destination = "/" + destination - } - - ctx.destination = destination + ctx.destination = cleanPath(destination) } path := r.URL.Path @@ -47,11 +78,7 @@ func newRequest(r *http.Request, prefix string) (*request, error) { } } - if !strings.HasPrefix(path, "/") { - path = "/" + path - } - - ctx.path = path + ctx.path = cleanPath(path) return ctx, nil }