From 6cc50a1c818fc638bcfe6eca981253fe78396577 Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Thu, 10 Sep 2026 13:10:48 +0200 Subject: [PATCH 1/2] fix(posix): skip non-regular files during assimilation --- pkg/storage/fs/posix/blobstore/blobstore.go | 7 +- .../posix/blobstore/posix_blobstore_test.go | 155 ++++++++++++++++++ pkg/storage/fs/posix/tree/assimilation.go | 15 ++ pkg/storage/fs/posix/tree/tree_test.go | 56 +++++++ 4 files changed, 230 insertions(+), 3 deletions(-) create mode 100644 pkg/storage/fs/posix/blobstore/posix_blobstore_test.go diff --git a/pkg/storage/fs/posix/blobstore/blobstore.go b/pkg/storage/fs/posix/blobstore/blobstore.go index 7a43eda6fd5..193519ecef9 100644 --- a/pkg/storage/fs/posix/blobstore/blobstore.go +++ b/pkg/storage/fs/posix/blobstore/blobstore.go @@ -22,6 +22,7 @@ import ( "bufio" "io" "os" + "syscall" "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/node" "github.com/pkg/errors" @@ -53,7 +54,7 @@ func (bs *Blobstore) Upload(node *node.Node, source string) error { } defer file.Close() - f, err := os.OpenFile(node.InternalPath(), os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0700) + f, err := os.OpenFile(node.InternalPath(), os.O_CREATE|os.O_WRONLY|os.O_TRUNC|syscall.O_NOFOLLOW, 0700) if err != nil { return errors.Wrapf(err, "could not open blob '%s' for writing", node.InternalPath()) } @@ -85,7 +86,7 @@ func (bs *Blobstore) UploadFromReader(node *node.Node, r io.Reader, size int64) fi, _ := os.Stat(path) - f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0700) + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC|syscall.O_NOFOLLOW, 0700) if err != nil { return errors.Wrapf(err, "could not open blob '%s' for writing", path) } @@ -110,7 +111,7 @@ func (bs *Blobstore) UploadFromReader(node *node.Node, r io.Reader, size int64) // Download retrieves a blob from the blobstore for reading func (bs *Blobstore) Download(node *node.Node) (io.ReadCloser, error) { - file, err := os.Open(node.InternalPath()) + file, err := os.OpenFile(node.InternalPath(), os.O_RDONLY|syscall.O_NOFOLLOW, 0) if err != nil { return nil, errors.Wrapf(err, "could not read blob '%s'", node.InternalPath()) } diff --git a/pkg/storage/fs/posix/blobstore/posix_blobstore_test.go b/pkg/storage/fs/posix/blobstore/posix_blobstore_test.go new file mode 100644 index 00000000000..f85427fd0cf --- /dev/null +++ b/pkg/storage/fs/posix/blobstore/posix_blobstore_test.go @@ -0,0 +1,155 @@ +package blobstore_test + +import ( + "bytes" + "context" + "errors" + "io" + "os" + "path/filepath" + "syscall" + + userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + posixblobstore "github.com/owncloud/reva/v2/pkg/storage/fs/posix/blobstore" + "github.com/owncloud/reva/v2/pkg/storage/fs/posix/lookup" + "github.com/owncloud/reva/v2/pkg/storage/fs/posix/options" + "github.com/owncloud/reva/v2/pkg/storage/fs/posix/timemanager" + "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/metadata" + "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/node" + "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/usermapper" + "github.com/owncloud/reva/v2/tests/helpers" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Posix blobstore", func() { + const ( + spaceID = "1284d238-aa92-42ce-bdc4-0b0000009157" + nodeID = "4c510ada-c86b-4815-8820-42cdf82c3d51" + ) + + var ( + ctx context.Context + + tmpRoot string + nodePath string + data []byte + + // a file outside of the storage tree that the service account can read but + // the user must never be able to reach + secretPath string + secretData []byte + + lu *lookup.Lookup + bs *posixblobstore.Blobstore + n *node.Node + ) + + // swapInSymlink atomically replaces the already assimilated node with a symlink + // pointing at secretPath, the way an attacker with out-of-band write access to the + // tree would. Renaming over the target is what makes this reach the blobstore: the + // tree only ever sees a MOVED_TO and the cached id -> path mapping stays valid. + swapInSymlink := func() { + staging := filepath.Join(filepath.Dir(nodePath), "staging") + Expect(os.Symlink(secretPath, staging)).To(Succeed()) + Expect(os.Rename(staging, nodePath)).To(Succeed()) + } + + BeforeEach(func() { + ctx = context.Background() + data = []byte("the blob the user uploaded") + secretData = []byte("the secret the user must not read") + + var err error + tmpRoot, err = helpers.TempDir("reva-unit-tests-*-root") + Expect(err).ToNot(HaveOccurred()) + + secretPath = filepath.Join(GinkgoT().TempDir(), "secret.txt") + Expect(os.WriteFile(secretPath, secretData, 0600)).To(Succeed()) + + o, err := options.New(map[string]interface{}{"root": tmpRoot}) + Expect(err).ToNot(HaveOccurred()) + lu = lookup.New(metadata.NewMessagePackBackend(o.Root, o.FileMetadataCache), &usermapper.NullMapper{}, o, &timemanager.Manager{}) + + bs, err = posixblobstore.New(tmpRoot) + Expect(err).ToNot(HaveOccurred()) + + // in the posix driver the node is the file itself, so lay it down in the tree + // and cache the id -> path mapping the way assimilation would + nodePath = filepath.Join(tmpRoot, "users", "username", "blob.txt") + Expect(os.MkdirAll(filepath.Dir(nodePath), 0700)).To(Succeed()) + Expect(os.WriteFile(nodePath, data, 0600)).To(Succeed()) + Expect(lu.CacheID(ctx, spaceID, nodeID, nodePath)).To(Succeed()) + + n = node.New(spaceID, nodeID, "", "blob.txt", int64(len(data)), "", provider.ResourceType_RESOURCE_TYPE_FILE, &userpb.UserId{OpaqueId: "someone"}, lu) + Expect(n.InternalPath()).To(Equal(nodePath)) + }) + + AfterEach(func() { + if tmpRoot != "" { + os.RemoveAll(tmpRoot) + } + }) + + Describe("Download", func() { + It("reads a regular file", func() { + reader, err := bs.Download(n) + Expect(err).ToNot(HaveOccurred()) + defer reader.Close() + + Expect(io.ReadAll(reader)).To(Equal(data)) + }) + + It("does not read through a symlink", func() { + swapInSymlink() + + reader, err := bs.Download(n) + Expect(err).To(HaveOccurred()) + Expect(errors.Is(err, syscall.ELOOP)).To(BeTrue()) + Expect(reader).To(BeNil()) + }) + }) + + Describe("Upload", func() { + var source string + + BeforeEach(func() { + source = filepath.Join(GinkgoT().TempDir(), "source") + Expect(os.WriteFile(source, []byte("new content"), 0600)).To(Succeed()) + }) + + It("writes a regular file", func() { + Expect(bs.Upload(n, source)).To(Succeed()) + Expect(os.ReadFile(nodePath)).To(Equal([]byte("new content"))) + }) + + It("does not write through a symlink", func() { + swapInSymlink() + + err := bs.Upload(n, source) + Expect(err).To(HaveOccurred()) + Expect(errors.Is(err, syscall.ELOOP)).To(BeTrue()) + Expect(os.ReadFile(secretPath)).To(Equal(secretData)) + }) + }) + + Describe("UploadFromReader", func() { + It("writes a regular file", func() { + content := []byte("new content") + Expect(bs.UploadFromReader(n, bytes.NewReader(content), int64(len(content)))).To(Succeed()) + Expect(os.ReadFile(nodePath)).To(Equal(content)) + }) + + It("does not write through a symlink", func() { + swapInSymlink() + + content := []byte("new content") + err := bs.UploadFromReader(n, bytes.NewReader(content), int64(len(content))) + Expect(err).To(HaveOccurred()) + Expect(errors.Is(err, syscall.ELOOP)).To(BeTrue()) + Expect(os.ReadFile(secretPath)).To(Equal(secretData)) + }) + }) +}) diff --git a/pkg/storage/fs/posix/tree/assimilation.go b/pkg/storage/fs/posix/tree/assimilation.go index c116fd9cde9..5352114fdfd 100644 --- a/pkg/storage/fs/posix/tree/assimilation.go +++ b/pkg/storage/fs/posix/tree/assimilation.go @@ -354,6 +354,16 @@ func (t *Tree) assimilate(item scanItem) error { var id []byte var err error + // only assimilate directories and regular files, Lstat so symlinks are not followed + fi, err := os.Lstat(item.Path) + if err != nil { + return err + } + if !fi.IsDir() && !fi.Mode().IsRegular() { + t.log.Debug().Str("path", item.Path).Msg("skipping non-regular file") + return nil + } + // First find the space id spaceID, spaceAttrs, err := t.findSpaceId(item.Path) if err != nil { @@ -635,6 +645,11 @@ func (t *Tree) WarmupIDCache(root string, assimilate, onlyDirty bool) error { return err } + // skip non-regular files, they are not assimilated and cannot hold metadata + if !info.IsDir() && !info.Mode().IsRegular() { + return nil + } + // calculate tree sizes if !info.IsDir() { dir := path diff --git a/pkg/storage/fs/posix/tree/tree_test.go b/pkg/storage/fs/posix/tree/tree_test.go index 60e94db39dd..b55686490f3 100644 --- a/pkg/storage/fs/posix/tree/tree_test.go +++ b/pkg/storage/fs/posix/tree/tree_test.go @@ -7,6 +7,7 @@ import ( "os/exec" "runtime" "strings" + "syscall" "time" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" @@ -373,6 +374,61 @@ var _ = Describe("Tree", func() { }).Should(Succeed()) }) }) + + Describe("of non-regular files", func() { + var outside string + + BeforeEach(func() { + outside = GinkgoT().TempDir() + "/secret.txt" + Expect(os.WriteFile(outside, []byte("secret"), 0600)).To(Succeed()) + }) + + // cachedID reports whether the item has been assimilated, i.e. whether it made + // it into the id cache + cachedID := func(name string) error { + _, _, err := env.Lookup.IDsForPath(env.Ctx, root+"/"+name) + return err + } + + // waitForScanner blocks until a plain file created after the item under test has + // been assimilated. By then the scanner has worked through the earlier event. + waitForScanner := func() { + _, err := os.Create(root + "/sentinel.txt") + Expect(err).ToNot(HaveOccurred()) + + Eventually(func() error { + return cachedID("sentinel.txt") + }).ProbeEvery(200 * time.Millisecond).Should(Succeed()) + } + + It("skips symlinks", func() { + Expect(os.Symlink(outside, root+"/link.txt")).To(Succeed()) + waitForScanner() + + Consistently(func() error { + return cachedID("link.txt") + }, 2*time.Second, 200*time.Millisecond).ShouldNot(Succeed()) + }) + + It("skips fifos without stalling the scanner", func() { + Expect(syscall.Mkfifo(root+"/fifo", 0600)).To(Succeed()) + waitForScanner() + + Consistently(func() error { + return cachedID("fifo") + }, 2*time.Second, 200*time.Millisecond).ShouldNot(Succeed()) + }) + + It("walks past them when warming up the id cache", func() { + Expect(os.Symlink(outside, root+"/link.txt")).To(Succeed()) + Expect(syscall.Mkfifo(root+"/fifo", 0600)).To(Succeed()) + + Expect(env.Tree.WarmupIDCache(env.Root, true, false)).To(Succeed()) + + Expect(cachedID("link.txt")).ToNot(Succeed()) + Expect(cachedID("fifo")).ToNot(Succeed()) + }) + }) }) Describe("propagation", func() { From 92b2caa0a814ad9ceddaa5363a4bdfb978db8ec1 Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Thu, 10 Sep 2026 13:13:51 +0200 Subject: [PATCH 2/2] fix: add changelog file --- .../bugfix-posix-skip-non-regular-files.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 changelog/unreleased/bugfix-posix-skip-non-regular-files.md diff --git a/changelog/unreleased/bugfix-posix-skip-non-regular-files.md b/changelog/unreleased/bugfix-posix-skip-non-regular-files.md new file mode 100644 index 00000000000..59f6c6c7ca9 --- /dev/null +++ b/changelog/unreleased/bugfix-posix-skip-non-regular-files.md @@ -0,0 +1,12 @@ +Bugfix: Skip non-regular files in the posix driver + +The posix storage driver assimilated anything it found in the watched tree, +including symlinks. A symlink placed in the tree out of band therefore became an +ordinary node whose content was whatever it pointed at, and uploading to that +node overwrote the symlink target. Symlinks, sockets and fifos are now skipped, +both when assimilating single items and when warming up the id cache, and blobs +are opened with `O_NOFOLLOW` so an already assimilated file cannot be swapped +for a symlink afterwards. Note that symlinks in the tree are no longer listed at +all. Only the posix driver is affected. + +https://github.com/owncloud/reva/pull/731