diff --git a/CHANGELOG.md b/CHANGELOG.md index 98172ce..c8c36fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,16 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] ## +### Fixed ### +- The mode and owner verification logic in `MkdirAll` has been removed. This + was originally intended to protect against some theoretical attacks but upon + further consideration these protections don't actually buy us anything and + they were causing spurious errors with more complicated filesystem setups. +- The "is the created directory empty" logic in `MkdirAll` has also been + removed. This was not causing us issues yet, but some pseudofilesystems (such + as `cgroup`) create non-empty directories and so this logic would've been + wrong for such cases. + ## [0.3.2] - 2024-09-13 ## ### Changed ### diff --git a/mkdir_linux.go b/mkdir_linux.go index b13033e..e9801bb 100644 --- a/mkdir_linux.go +++ b/mkdir_linux.go @@ -9,7 +9,6 @@ package securejoin import ( "errors" "fmt" - "io" "os" "path/filepath" "slices" @@ -108,35 +107,6 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err // Make sure the mode doesn't have any type bits. mode &^= unix.S_IFMT - // What properties do we expect any newly created directories to have? - var ( - // While umask(2) is a per-thread property, and thus this value could - // vary between threads, a functioning Go program would LockOSThread - // threads with different umasks and so we don't need to LockOSThread - // for this entire mkdirat loop (if we are in the locked thread with a - // different umask, we are already locked and there's nothing for us to - // do -- and if not then it doesn't matter which thread we run on and - // there's nothing for us to do). - expectedMode = uint32(unix.S_IFDIR | (mode &^ getUmask())) - - // We would want to get the fs[ug]id here, but we can't access those - // from userspace. In practice, nobody uses setfs[ug]id() anymore, so - // just use the effective [ug]id (which is equivalent to the fs[ug]id - // for programs that don't use setfs[ug]id). - expectedUid = uint32(unix.Geteuid()) - expectedGid = uint32(unix.Getegid()) - ) - - // The setgid bit (S_ISGID = 0o2000) is inherited to child directories and - // affects the group of any inodes created in said directory, so if the - // starting directory has it set we need to adjust our expected mode and - // owner to match. - if st, err := fstatFile(currentDir); err != nil { - return nil, fmt.Errorf("failed to stat starting path for mkdir %q: %w", currentDir.Name(), err) - } else if st.Mode&unix.S_ISGID == unix.S_ISGID { - expectedMode |= unix.S_ISGID - expectedGid = st.Gid - } // Create the remaining components. for _, part := range remainingParts { @@ -175,35 +145,25 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err _ = currentDir.Close() currentDir = nextDir - // Make sure that the directory matches what we expect. An attacker - // could have swapped the directory between us making it and opening - // it. There's no way for us to be sure that the directory is - // _precisely_ the same as the directory we created, but if we are in - // an empty directory with the same owner and mode as the one we - // created then there is nothing the attacker could do with this new - // directory that they couldn't do with the old one. - if stat, err := fstat(currentDir); err != nil { - return nil, fmt.Errorf("check newly created directory: %w", err) - } else { - if stat.Mode != expectedMode { - return nil, fmt.Errorf("%w: newly created directory %q has incorrect mode 0o%.3o (expected 0o%.3o)", errPossibleAttack, currentDir.Name(), stat.Mode, expectedMode) - } - if stat.Uid != expectedUid || stat.Gid != expectedGid { - return nil, fmt.Errorf("%w: newly created directory %q has incorrect owner %d:%d (expected %d:%d)", errPossibleAttack, currentDir.Name(), stat.Uid, stat.Gid, expectedUid, expectedGid) - } - // Check that the directory is empty. We only need to check for - // a single entry, and we should get EOF if the directory is - // empty. - _, err := currentDir.Readdirnames(1) - if !errors.Is(err, io.EOF) { - if err == nil { - err = fmt.Errorf("%w: newly created directory %q is non-empty", errPossibleAttack, currentDir.Name()) - } - return nil, fmt.Errorf("check if newly created directory %q is empty: %w", currentDir.Name(), err) - } - // Reset the offset. - _, _ = currentDir.Seek(0, unix.SEEK_SET) - } + // It's possible that the directory we just opened was swapped by an + // attacker. Unfortunately there isn't much we can do to protect + // against this, and MkdirAll's behaviour is that we will reuse + // existing directories anyway so the need to protect against this is + // incredibly limited (and arguably doesn't even deserve mention here). + // + // Ideally we might want to check that the owner and mode match what we + // would've created -- unfortunately, it is non-trivial to verify that + // the owner and mode of the created directory match. While plain Unix + // DAC rules seem simple enough to emulate, there are a bunch of other + // factors that can change the mode or owner of created directories + // (default POSIX ACLs, mount options like uid=1,gid=2,umask=0 on + // filesystems like vfat, etc etc). We used to try to verify this but + // it just lead to a series of spurious errors. + // + // We could also check that the directory is non-empty, but + // unfortunately some pseduofilesystems (like cgroupfs) create + // non-empty directories, which would result in different spurious + // errors. } return currentDir, nil } diff --git a/mkdir_linux_test.go b/mkdir_linux_test.go index afc4e1b..9499057 100644 --- a/mkdir_linux_test.go +++ b/mkdir_linux_test.go @@ -314,12 +314,9 @@ func (m *racingMkdirMeta) checkMkdirAllHandle_Racing(t *testing.T, root, unsafeP } defer handle.Close() - // Make sure the handle has the right owner/mode. - unixStat, err := fstat(handle) - require.NoError(t, err, "stat mkdirall handle") - assert.Equal(t, uint32(unix.S_IFDIR|mode), unixStat.Mode, "mkdirall handle mode") - assert.Equal(t, uint32(unix.Geteuid()), unixStat.Uid, "mkdirall handle uid") - assert.Equal(t, uint32(unix.Getegid()), unixStat.Gid, "mkdirall handle gid") + // It's possible for an attacker to have swapped the final directory, but + // this is okay because MkdirAll will use pre-existing directories anyway. + // So there's no need to check the returned handle. // TODO: Does it make sense to even try to check the handle path? m.passOkCount++ } @@ -346,12 +343,12 @@ func TestMkdirAllHandle_RacingRename(t *testing.T) { tests := []test{ {"good", "target/a/b/c/d/e", "swapdir-empty-ok", "target/a/b/c/d/e/f/g/h/i/j/k", nil}, - {"trailing", "target/a/b/c/d/e", "swapdir-empty-badmode", "target/a/b/c/d/e", []error{errPossibleAttack}}, - {"partial", "target/a/b/c/d/e", "swapdir-empty-badmode", "target/a/b/c/d/e/f/g/h/i/j/k", []error{errPossibleAttack}}, - {"trailing", "target/a/b/c/d/e", "swapdir-nonempty1", "target/a/b/c/d/e", []error{errPossibleAttack}}, - {"partial", "target/a/b/c/d/e", "swapdir-nonempty1", "target/a/b/c/d/e/f/g/h/i/j/k", []error{errPossibleAttack}}, - {"trailing", "target/a/b/c/d/e", "swapdir-nonempty2", "target/a/b/c/d/e", []error{errPossibleAttack}}, - {"partial", "target/a/b/c/d/e", "swapdir-nonempty2", "target/a/b/c/d/e/f/g/h/i/j/k", []error{errPossibleAttack}}, + {"trailing", "target/a/b/c/d/e", "swapdir-empty-badmode", "target/a/b/c/d/e", nil}, + {"partial", "target/a/b/c/d/e", "swapdir-empty-badmode", "target/a/b/c/d/e/f/g/h/i/j/k", nil}, + {"trailing", "target/a/b/c/d/e", "swapdir-nonempty1", "target/a/b/c/d/e", nil}, + {"partial", "target/a/b/c/d/e", "swapdir-nonempty1", "target/a/b/c/d/e/f/g/h/i/j/k", nil}, + {"trailing", "target/a/b/c/d/e", "swapdir-nonempty2", "target/a/b/c/d/e", nil}, + {"partial", "target/a/b/c/d/e", "swapdir-nonempty2", "target/a/b/c/d/e/f/g/h/i/j/k", []error{unix.EEXIST}}, {"trailing", "target/a/b/c/d/e", "swapfile", "target/a/b/c/d/e", []error{unix.ENOTDIR}}, {"partial", "target/a/b/c/d/e", "swapfile", "target/a/b/c/d/e/f/g/h/i/j/k", []error{unix.ENOTDIR}}, } @@ -364,12 +361,12 @@ func TestMkdirAllHandle_RacingRename(t *testing.T) { "dir swapdir-empty-badowner3 111:222:0711", ) tests = append(tests, []test{ - {"trailing", "target/a/b/c/d/e", "swapdir-empty-badowner1", "target/a/b/c/d/e", []error{errPossibleAttack}}, - {"partial", "target/a/b/c/d/e", "swapdir-empty-badowner1", "target/a/b/c/d/e/f/g/h/i/j/k", []error{errPossibleAttack}}, - {"trailing", "target/a/b/c/d/e", "swapdir-empty-badowner2", "target/a/b/c/d/e", []error{errPossibleAttack}}, - {"partial", "target/a/b/c/d/e", "swapdir-empty-badowner2", "target/a/b/c/d/e/f/g/h/i/j/k", []error{errPossibleAttack}}, - {"trailing", "target/a/b/c/d/e", "swapdir-empty-badowner3", "target/a/b/c/d/e", []error{errPossibleAttack}}, - {"partial", "target/a/b/c/d/e", "swapdir-empty-badowner3", "target/a/b/c/d/e/f/g/h/i/j/k", []error{errPossibleAttack}}, + {"trailing", "target/a/b/c/d/e", "swapdir-empty-badowner1", "target/a/b/c/d/e", nil}, + {"partial", "target/a/b/c/d/e", "swapdir-empty-badowner1", "target/a/b/c/d/e/f/g/h/i/j/k", nil}, + {"trailing", "target/a/b/c/d/e", "swapdir-empty-badowner2", "target/a/b/c/d/e", nil}, + {"partial", "target/a/b/c/d/e", "swapdir-empty-badowner2", "target/a/b/c/d/e/f/g/h/i/j/k", nil}, + {"trailing", "target/a/b/c/d/e", "swapdir-empty-badowner3", "target/a/b/c/d/e", nil}, + {"partial", "target/a/b/c/d/e", "swapdir-empty-badowner3", "target/a/b/c/d/e/f/g/h/i/j/k", nil}, }...) } diff --git a/procfs_linux.go b/procfs_linux.go index 6d8c584..fa7929a 100644 --- a/procfs_linux.go +++ b/procfs_linux.go @@ -411,22 +411,6 @@ func isDeadInode(file *os.File) error { return nil } -func getUmask() int { - // umask is a per-thread property, but it is inherited by children, so we - // need to lock our OS thread to make sure that no other goroutine runs in - // this thread and no goroutines are spawned from this thread until we - // revert to the old umask. - // - // We could parse /proc/self/status to avoid this get-set problem, but - // /proc/thread-self requires LockOSThread anyway, so there's no real - // benefit over just using umask(2). - runtime.LockOSThread() - umask := unix.Umask(0) - unix.Umask(umask) - runtime.UnlockOSThread() - return umask -} - func checkProcSelfFdPath(path string, file *os.File) error { if err := isDeadInode(file); err != nil { return err