fix(posix): skip non-regular files during assimilation

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

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

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

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

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