From 90adf5c0e78b0f34822070b7e41afd21237ef7cc Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Tue, 24 Sep 2024 19:49:33 +0200 Subject: [PATCH 1/2] mkdir: do not return errors for incorrect directory modes or owners We've had several examples of unexpected semantics with how modes are calculated, and there will likely be many more in the future. In addition, mounting filesystems like vfat with mount options that mess with ownership (like "uid=1234,gid=5678,umask=0") will result in unexpected behaviour that would be very difficult to emulate. To avoid further regressions, just remove the checks entirely. In theory we could switch to adding warnings, but there's no real benefit IMHO. The semantics of MkdirAll are quite loose already so arguably there is no practical difference between re-using a directory that already existed and being tricked into opening an intermediate directory you didn't create. Signed-off-by: Aleksa Sarai --- CHANGELOG.md | 6 ++++ mkdir_linux.go | 81 ++++++++++++++------------------------------- mkdir_linux_test.go | 25 ++++++-------- procfs_linux.go | 16 --------- 4 files changed, 42 insertions(+), 86 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 98172ce..03ed75c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ 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. + ## [0.3.2] - 2024-09-13 ## ### Changed ### diff --git a/mkdir_linux.go b/mkdir_linux.go index b13033e..55aff79 100644 --- a/mkdir_linux.go +++ b/mkdir_linux.go @@ -108,35 +108,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 +146,33 @@ 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) + // Try our best to check that the directory is empty and so is unlikely + // to have been swapped by an attacker. + // + // Ideally we would also 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. + // + // To be honest, since MkdirAll allows you to use existing directories, + // the practical scope of this protection seems very limited (if it + // even exists) so it really isn't that important. + + // We only need to check for a single entry to see if it's empty, 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()) } - // Reset the offset. - _, _ = currentDir.Seek(0, unix.SEEK_SET) + 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) } return currentDir, nil } diff --git a/mkdir_linux_test.go b/mkdir_linux_test.go index afc4e1b..aa22533 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,8 +343,8 @@ 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-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", []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}}, @@ -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 From 92b699d8f159c0736781603667414fc9595bba7a Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Fri, 27 Sep 2024 17:06:21 +0200 Subject: [PATCH 2/2] mkdir: don't check that the directory is empty Some pseudofilesystems (like cgroupfs) create non-empty directories, so this check is kind of questionable if someone tries to use MkdirAll on those filesystems. The semantics of MkdirAll already allow us to re-use a non-empty directory, so this check arguably didn't buy us anything anyway. Signed-off-by: Aleksa Sarai --- CHANGELOG.md | 4 ++++ mkdir_linux.go | 29 ++++++++++------------------- mkdir_linux_test.go | 8 ++++---- 3 files changed, 18 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03ed75c..c8c36fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/). 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 ## diff --git a/mkdir_linux.go b/mkdir_linux.go index 55aff79..e9801bb 100644 --- a/mkdir_linux.go +++ b/mkdir_linux.go @@ -9,7 +9,6 @@ package securejoin import ( "errors" "fmt" - "io" "os" "path/filepath" "slices" @@ -146,10 +145,13 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err _ = currentDir.Close() currentDir = nextDir - // Try our best to check that the directory is empty and so is unlikely - // to have been swapped by an attacker. + // 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 would also check that the owner and mode match what we + // 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 @@ -158,21 +160,10 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err // filesystems like vfat, etc etc). We used to try to verify this but // it just lead to a series of spurious errors. // - // To be honest, since MkdirAll allows you to use existing directories, - // the practical scope of this protection seems very limited (if it - // even exists) so it really isn't that important. - - // We only need to check for a single entry to see if it's empty, 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) + // 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 aa22533..9499057 100644 --- a/mkdir_linux_test.go +++ b/mkdir_linux_test.go @@ -345,10 +345,10 @@ func TestMkdirAllHandle_RacingRename(t *testing.T) { {"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", 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", []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-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}}, }