diff --git a/README.es.md b/README.es.md index aae606c..9acb53e 100644 --- a/README.es.md +++ b/README.es.md @@ -72,6 +72,13 @@ volumen de datos, layout "multifilesystem" de Radicale Los metadatos de colección viven en los `.Radicale.props` propios de Radicale (mismas claves, incluido el string separado por comas del supported-calendar-component-set). +- **Los nombres que gestiona el servidor están reservados**: un nombre que + empieza por punto (el `.Radicale.props` propio de cada colección, los ficheros + `.tmp-*` de las escrituras atómicas) o que acaba en tilde no existe para los + clientes. Un `PUT` sobre ese nombre responde 400 y `DELETE`/`GET` responden + 404, así que ningún cliente puede romper una colección escribiendo encima de + sus metadatos o borrándolos. Radicale aplica la misma regla con + `pathutils.is_safe_filesystem_path_component`. - **La identidad es una cabecera**: se confía ciegamente en `X-Remote-User`, así que el puerto nunca debe ser alcanzable sin el proxy autenticador delante. En modo `-dav-root` el proxy elimina los prefijos diff --git a/README.md b/README.md index 12ce3e4..24e2690 100644 --- a/README.md +++ b/README.md @@ -67,6 +67,12 @@ data volume, Radicale "multifilesystem" layout indistinguishable from one written by Radicale - in both directions. Collection metadata lives in Radicale's own `.Radicale.props` files (same keys, including the comma separated supported-calendar-component-set). +- **Server-managed names are reserved**: a name with a leading dot (a + collection's own `.Radicale.props`, the `.tmp-*` files of atomic writes) or a + trailing tilde does not exist as far as clients are concerned. `PUT` on such a + name answers 400 and `DELETE`/`GET` answer 404, so no client can break a + collection by writing over, or deleting, its metadata. Radicale enforces the + same rule through `pathutils.is_safe_filesystem_path_component`. - **Identity is a header**: `X-Remote-User` is trusted blindly, so the port must never be reachable without the authenticating proxy in front. In `-dav-root` mode the proxy strips the `/caldav`/`/carddav` prefixes and diff --git a/cmd/server/dav_integration_test.go b/cmd/server/dav_integration_test.go index 610f893..7afd714 100644 --- a/cmd/server/dav_integration_test.go +++ b/cmd/server/dav_integration_test.go @@ -1076,3 +1076,66 @@ func TestSpaFallbackStillWorks(t *testing.T) { t.Errorf("spa fallback body = %q", body) } } + +// TestServerManagedNamesAreUnreachable covers issue #38: no DAV request may +// touch the ".Radicale.props" metadata file of a collection, because losing it +// makes the whole collection disappear from the tree (404) with no way back. +func TestServerManagedNamesAreUnreachable(t *testing.T) { + srv, dir := newTestServer(t) + + resp, body := do(t, "PROPFIND", srv.URL+"/caldav/alice/", propfindBody, map[string]string{"Depth": "1"}) + if resp.StatusCode != http.StatusMultiStatus { + t.Fatalf("PROPFIND home = %d, want 207: %s", resp.StatusCode, body) + } + props := filepath.Join(dir, "collection-root", "alice", "def-calendar", store.PropsFile) + before, err := os.ReadFile(props) + if err != nil { + t.Fatalf("metadata file missing before the requests: %v", err) + } + + // A valid stream whose UID matches the reserved name must still be refused. + evil := strings.Replace(testICS, "UID:e1", "UID:"+store.PropsFile, 1) + resp, body = do(t, "PUT", srv.URL+"/caldav/alice/def-calendar/"+store.PropsFile, evil, map[string]string{"Content-Type": "text/calendar"}) + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("PUT on the metadata file = %d, want 400: %s", resp.StatusCode, body) + } + + resp, body = do(t, "DELETE", srv.URL+"/caldav/alice/def-calendar/"+store.PropsFile, "", nil) + if resp.StatusCode != http.StatusNotFound { + t.Errorf("DELETE on the metadata file = %d, want 404: %s", resp.StatusCode, body) + } + + resp, body = do(t, "GET", srv.URL+"/caldav/alice/def-calendar/"+store.PropsFile, "", nil) + if resp.StatusCode != http.StatusNotFound { + t.Errorf("GET on the metadata file = %d, want 404: %s", resp.StatusCode, body) + } + + // Hidden names are reserved for every object, not only for the metadata file. + resp, body = do(t, "PUT", srv.URL+"/caldav/alice/def-calendar/.hidden.ics", testICS, map[string]string{"Content-Type": "text/calendar"}) + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("PUT on a hidden name = %d, want 400: %s", resp.StatusCode, body) + } + + // A collection cannot take a reserved name either. + resp, body = do(t, "MKCALENDAR", srv.URL+"/caldav/alice/.reserved/", "", nil) + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("MKCALENDAR with a reserved name = %d, want 400: %s", resp.StatusCode, body) + } + + // The metadata file survived, the collection still serves and the listing + // does not expose the reserved name. + after, err := os.ReadFile(props) + if err != nil { + t.Fatalf("metadata file is gone after the requests: %v", err) + } + if string(before) != string(after) { + t.Error("metadata file was modified") + } + resp, body = do(t, "PROPFIND", srv.URL+"/caldav/alice/def-calendar/", propfindBody, map[string]string{"Depth": "1"}) + if resp.StatusCode != http.StatusMultiStatus { + t.Errorf("PROPFIND on the collection after the requests = %d, want 207: %s", resp.StatusCode, body) + } + if strings.Contains(body, store.PropsFile) { + t.Error("the listing exposes the metadata file") + } +} diff --git a/internal/dav/dav.go b/internal/dav/dav.go index 63089de..e2ff2dd 100644 --- a/internal/dav/dav.go +++ b/internal/dav/dav.go @@ -114,6 +114,15 @@ func (h *Handler) resolve(r *http.Request) (*resource, error) { if segs[0] != user { return nil, errForbidden } + // Names the server reserves for itself (the ".Radicale.props" metadata file + // of every collection and the ".tmp-*" files of atomic writes) do not exist + // as far as clients are concerned, which is how Radicale behaves too + // (pathutils.is_safe_filesystem_path_component in its filesystem storage). + for _, s := range segs[1:] { + if !store.ValidName(s) { + return nil, errNotFound + } + } switch len(segs) { case 1: case 2: @@ -146,6 +155,9 @@ func (h *Handler) resolveForCreate(r *http.Request) (*resource, error) { if len(segs) != 2 || segs[0] != user { return nil, &davError{code: http.StatusForbidden, msg: "dav: collections can only be created under the user home set"} } + if !store.ValidName(segs[1]) { + return nil, errReservedName + } if _, err := h.Store.GetCollection(user, segs[1]); err == nil { return nil, &davError{code: http.StatusMethodNotAllowed, msg: "dav: collection already exists"} } @@ -269,6 +281,10 @@ func (e *davError) Error() string { return e.msg } var ( errForbidden = &davError{code: http.StatusForbidden, msg: "dav: access to another user's data is not allowed"} errNotFound = &davError{code: http.StatusNotFound, msg: "dav: resource not found"} + // errReservedName is the answer to a request that tries to create or write a + // name the server keeps for itself (leading dot, trailing tilde). Radicale + // answers a PUT with such an href with 400 as well. + errReservedName = &davError{code: http.StatusBadRequest, msg: "dav: names starting with a dot or ending with a tilde are reserved by the server"} ) func writeError(w http.ResponseWriter, err error) { diff --git a/internal/dav/objects.go b/internal/dav/objects.go index f74255b..372e7b8 100644 --- a/internal/dav/objects.go +++ b/internal/dav/objects.go @@ -41,6 +41,9 @@ func (h *Handler) resolveForWrite(r *http.Request) (*resource, error) { if len(segs) != 3 || segs[0] != user { return nil, &davError{code: http.StatusForbidden, msg: "dav: objects live at ///"} } + if !store.ValidName(segs[1]) || !store.ValidName(segs[2]) { + return nil, errReservedName + } c, err := h.Store.GetCollection(user, segs[1]) if err != nil || c.Tag != h.Kind.collectionTag() { return nil, &davError{code: http.StatusConflict, msg: "dav: the parent collection does not exist"} diff --git a/internal/store/store.go b/internal/store/store.go index e2877cb..07664b7 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -137,6 +137,30 @@ func cleanSeg(s string) (string, error) { return s, nil } +// cleanName is cleanSeg plus the rule that keeps server-managed files out of +// the reach of clients: names with a leading dot (the ".Radicale.props" +// metadata file of every collection and the ".tmp-*" files used by atomic +// writes) and names with a trailing tilde are reserved. Radicale applies the +// same rule through pathutils.is_safe_filesystem_path_component before it +// touches the filesystem, which is what stops a client from deleting or +// overwriting a collection's own metadata. +func cleanName(s string) (string, error) { + if _, err := cleanSeg(s); err != nil { + return "", err + } + if strings.HasPrefix(s, ".") || strings.HasSuffix(s, "~") { + return "", fmt.Errorf("store: reserved path segment %q", s) + } + return s, nil +} + +// ValidName reports whether name is usable as a collection or object name on +// the wire. +func ValidName(name string) bool { + _, err := cleanName(name) + return err == nil +} + func (s *Store) userDir(user string) (string, error) { u, err := cleanSeg(user) if err != nil { @@ -262,7 +286,7 @@ func (s *Store) ListCollections(user string) ([]Collection, error) { // GetCollection returns one collection by id. func (s *Store) GetCollection(user, id string) (Collection, error) { - if _, err := cleanSeg(id); err != nil { + if _, err := cleanName(id); err != nil { return Collection{}, err } dir, err := s.userDir(user) @@ -274,7 +298,7 @@ func (s *Store) GetCollection(user, id string) (Collection, error) { // CreateCollection creates a collection directory with props. func (s *Store) CreateCollection(user string, c Collection) error { - if _, err := cleanSeg(c.ID); err != nil { + if _, err := cleanName(c.ID); err != nil { return err } dir, err := s.userDir(user) @@ -296,7 +320,7 @@ func (s *Store) CreateCollection(user string, c Collection) error { // DeleteCollection removes a collection directory entirely. func (s *Store) DeleteCollection(user, id string) error { - if _, err := cleanSeg(id); err != nil { + if _, err := cleanName(id); err != nil { return err } dir, err := s.userDir(user) @@ -311,6 +335,9 @@ func (s *Store) DeleteCollection(user, id string) error { // Fixed keys are applied to their struct fields, everything else to Extra. // The write is atomic (same mechanism as items). func (s *Store) UpdateCollectionProps(user, id string, set map[string]string, remove []string) error { + if _, err := cleanName(id); err != nil { + return err + } dir, err := s.userDir(user) if err != nil { return err @@ -350,7 +377,7 @@ func (s *Store) UpdateCollectionProps(user, id string, set map[string]string, re } func (s *Store) collDir(user, coll string) (string, error) { - if _, err := cleanSeg(coll); err != nil { + if _, err := cleanName(coll); err != nil { return "", err } dir, err := s.userDir(user) @@ -372,7 +399,7 @@ func (s *Store) ListItems(user, coll string) ([]string, error) { } var out []string for _, e := range entries { - if e.IsDir() || e.Name() == PropsFile || strings.HasPrefix(e.Name(), ".tmp-") { + if e.IsDir() || !ValidName(e.Name()) { continue } out = append(out, e.Name()) @@ -382,7 +409,7 @@ func (s *Store) ListItems(user, coll string) ([]string, error) { // ReadItem returns the raw content of an item. func (s *Store) ReadItem(user, coll, name string) ([]byte, error) { - if _, err := cleanSeg(name); err != nil { + if _, err := cleanName(name); err != nil { return nil, err } dir, err := s.collDir(user, coll) @@ -407,7 +434,7 @@ func (s *Store) ItemETag(user, coll, name string) (string, error) { // WriteItem writes an item atomically and returns its new ETag (sha256). func (s *Store) WriteItem(user, coll, name string, data []byte) (string, error) { - if _, err := cleanSeg(name); err != nil { + if _, err := cleanName(name); err != nil { return "", err } dir, err := s.collDir(user, coll) @@ -425,7 +452,7 @@ func (s *Store) WriteItem(user, coll, name string, data []byte) (string, error) // DeleteItem removes an item. Returns os.ErrNotExist if missing. func (s *Store) DeleteItem(user, coll, name string) error { - if _, err := cleanSeg(name); err != nil { + if _, err := cleanName(name); err != nil { return err } dir, err := s.collDir(user, coll) @@ -437,7 +464,7 @@ func (s *Store) DeleteItem(user, coll, name string) error { // ItemModTime returns the modification time of an item. func (s *Store) ItemModTime(user, coll, name string) (time.Time, error) { - if _, err := cleanSeg(name); err != nil { + if _, err := cleanName(name); err != nil { return time.Time{}, err } dir, err := s.collDir(user, coll) diff --git a/internal/store/store_test.go b/internal/store/store_test.go index fbc205c..3474946 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -232,3 +232,69 @@ func TestCreateCollectionRejectsExisting(t *testing.T) { t.Error("collection still readable after delete") } } + +// TestReservedNamesAreRefused mirrors Radicale's +// pathutils.is_safe_filesystem_path_component: a collection's own metadata file +// and the files an atomic write uses must never be reachable through the store +// API, whichever call is used. +func TestReservedNamesAreRefused(t *testing.T) { + root := t.TempDir() + s := New(root) + if err := s.EnsureUser("alice"); err != nil { + t.Fatal(err) + } + + for _, name := range []string{PropsFile, ".hidden.ics", ".Radicale.cache", "trailing~"} { + if ValidName(name) { + t.Errorf("ValidName(%q) = true, want false", name) + } + if _, err := s.WriteItem("alice", "def-calendar", name, []byte("x")); err == nil { + t.Errorf("WriteItem(%q) succeeded, want an error", name) + } + if _, err := s.ReadItem("alice", "def-calendar", name); err == nil { + t.Errorf("ReadItem(%q) succeeded, want an error", name) + } + if err := s.DeleteItem("alice", "def-calendar", name); err == nil { + t.Errorf("DeleteItem(%q) succeeded, want an error", name) + } + if _, err := s.GetCollection("alice", name); err == nil { + t.Errorf("GetCollection(%q) succeeded, want an error", name) + } + if err := s.CreateCollection("alice", Collection{ID: name, Tag: "VCALENDAR"}); err == nil { + t.Errorf("CreateCollection(%q) succeeded, want an error", name) + } + } + + // The metadata file of a live collection survives a delete attempt. + props := filepath.Join(root, "collection-root", "alice", "def-calendar", PropsFile) + before, err := os.ReadFile(props) + if err != nil { + t.Fatal(err) + } + if err := s.DeleteItem("alice", "def-calendar", PropsFile); err == nil { + t.Fatal("DeleteItem on the metadata file reported success") + } + after, err := os.ReadFile(props) + if err != nil { + t.Fatalf("metadata file is gone after DeleteItem: %v", err) + } + if string(before) != string(after) { + t.Error("metadata file changed") + } + + // Hidden and reserved names are not listed, ordinary ones are. + if _, err := s.WriteItem("alice", "def-calendar", "e1.ics", []byte("x")); err != nil { + t.Fatal(err) + } + hidden := filepath.Join(root, "collection-root", "alice", "def-calendar", ".hidden.ics") + if err := os.WriteFile(hidden, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + items, err := s.ListItems("alice", "def-calendar") + if err != nil { + t.Fatal(err) + } + if len(items) != 1 || items[0] != "e1.ics" { + t.Errorf("ListItems = %v, want [e1.ics]", items) + } +}