From afbbbca326e1724eb73f6ce8120457a3a7303518 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Tue, 9 Jul 2024 10:54:48 +1000 Subject: [PATCH] tests: add racing tests for partialLokupInRoot and MkdirAllHandle In order to get some confidence in saying that our code safe against these kinds of racing attacks, add some tests to cover this. The full suite (with 50k runs of each new subtest) takes about 10 minutes, so we should use "go test -short" for the full matrix and add a single separate test run which can run for longer. While working on this, I found what seems to be a kernel bug with readlink(/proc/self/fd/$n). There is a workaround in the tests for this, but it is possible that the test will become flaky. This brings the test coverage up to 91.1% on Linux (__% combined). Signed-off-by: Aleksa Sarai --- .github/workflows/ci.yml | 29 ++++- lookup_linux_test.go | 271 ++++++++++++++++++++++++++++++++++++++- mkdir_linux_test.go | 225 +++++++++++++++++++++++++++++++- procfs_linux_test.go | 10 +- util_linux_test.go | 89 +++++++++++-- 5 files changed, 603 insertions(+), 21 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 09f1d79..929e68a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,7 +49,7 @@ jobs: name: coverage-${{ runner.os }}-${{ github.job }}-${{ strategy.job-index }} path: ${{ env.GOCOVERDIR }} - test-unix: + test-unix-short: strategy: fail-fast: false matrix: @@ -76,8 +76,28 @@ jobs: echo "GOCOVERDIR=$GOCOVERDIR" >>"$GITHUB_ENV" - name: unit tests run: | - go test -v -cover -test.gocoverdir="$GOCOVERDIR" ./... - sudo go test -v -cover -test.gocoverdir="$GOCOVERDIR" ./... + go test -short -v -cover -test.gocoverdir="$GOCOVERDIR" ./... + sudo go test -short -v -cover -test.gocoverdir="$GOCOVERDIR" ./... + - name: upload coverage + uses: actions/upload-artifact@v4 + with: + name: coverage-${{ runner.os }}-${{ github.job }}-${{ strategy.job-index }} + path: ${{ env.GOCOVERDIR }} + + test-linux-racing: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - uses: actions/setup-go@v4 + with: + go-version: "^1" + - name: mkdir gocoverdir + run: | + GOCOVERDIR="$(mktemp --tmpdir -d gocoverdir.XXXXXXXX)" + echo "GOCOVERDIR=$GOCOVERDIR" >>"$GITHUB_ENV" + - name: unit tests + run: | + sudo go test -timeout 1h -v -cover -test.gocoverdir="$GOCOVERDIR" -run 'Test.*Racing' ./... - name: upload coverage uses: actions/upload-artifact@v4 with: @@ -88,7 +108,8 @@ jobs: runs-on: ubuntu-latest needs: - test-windows - - test-unix + - test-unix-short + - test-linux-racing steps: - uses: actions/checkout@v3 - uses: actions/setup-go@v4 diff --git a/lookup_linux_test.go b/lookup_linux_test.go index 79dfabe..07e46fb 100644 --- a/lookup_linux_test.go +++ b/lookup_linux_test.go @@ -7,9 +7,11 @@ package securejoin import ( + "errors" "fmt" "os" "path/filepath" + "slices" "testing" "github.com/stretchr/testify/assert" @@ -251,7 +253,7 @@ func testPartialLookup(t *testing.T, partialLookupFn partialLookupFunc) { } func TestPartialLookupInRoot(t *testing.T) { - withWithoutOpenat2(t, func(t *testing.T) { + withWithoutOpenat2(t, true, func(t *testing.T) { testPartialLookup(t, partialLookupInRoot) }) } @@ -263,7 +265,7 @@ func TestPartialOpenat2(t *testing.T) { func TestPartialLookupInRoot_BadInode(t *testing.T) { requireRoot(t) // mknod - withWithoutOpenat2(t, func(t *testing.T) { + withWithoutOpenat2(t, true, func(t *testing.T) { partialLookupFn := partialLookupInRoot tree := []string{ @@ -306,6 +308,271 @@ func TestPartialLookupInRoot_BadInode(t *testing.T) { }) } +type racingLookupMeta struct { + pauseCh chan struct{} + passOkCount, passErrCount, skipCount, failCount, badErrCount int // test state counts + badNameCount, fixRemainingPathCount, unstableProcSelfFdCount int // workaround counts + skipErrCounts map[error]int +} + +func newRacingLookupMeta(pauseCh chan struct{}) *racingLookupMeta { + return &racingLookupMeta{ + pauseCh: pauseCh, + skipErrCounts: map[error]int{}, + } +} + +func (m *racingLookupMeta) checkPartialLookup(t *testing.T, rootDir *os.File, unsafePath string, skipErrs []error, allowedResults []lookupResult) { + // Similar to checkPartialLookup, but with extra logic for + // handling the lookup stopping partly through the lookup. + handle, remainingPath, err := partialLookupInRoot(rootDir, unsafePath) + if err != nil { + for _, skipErr := range skipErrs { + if errors.Is(err, skipErr) { + m.skipErrCounts[skipErr]++ + m.skipCount++ + return + } + } + for _, allowed := range allowedResults { + if allowed.err != nil && errors.Is(err, allowed.err) { + m.passErrCount++ + return + } + } + // If we didn't hit any of the allowed errors, it's an + // unexpected error. + assert.NoError(t, err) + m.badErrCount++ + return + } + + var ( + handleName string + realPath string + unixStat unix.Stat_t + realPaths []string + ) + if handle != nil { + handleName = handle.Name() + + // Get the "proper" name from procSelfFdReadlink. + m.pauseCh <- struct{}{} + // For some reason, it seems that (at a rate of 0.01% or so) even + // though we pause the rename thread is a nonsensical path from + // procSelfFdReadlink that looks like the path is still swapped. The + // key thing to note is that adding sleeps doesn't seem to help much, + // but retrying several times usually ends up with us getting the + // "right" result eventually. + // + // This seems like a kernel bug (or a weird issue with Go channels), + // but for now let's just retry a few times and keep track of how many + // transitions we saw. If we only see one change, then we can use the + // last one and just track the statistics. + const readlinkRetryMax = 500 + realPath, err = procSelfFdReadlink(handle) + for i := 0; err == nil && i < readlinkRetryMax; i++ { + if last := len(realPaths) - 1; last < 0 || realPath != realPaths[last] { + realPaths = append(realPaths, realPath) + } + realPath, err = procSelfFdReadlink(handle) + } + <-m.pauseCh + require.NoError(t, err, "get real path of returned handle") + + unixStat, err = fstat(handle) + require.NoError(t, err, "stat handle") + + _ = handle.Close() + } + + assert.LessOrEqualf(t, len(realPaths), 2, "saw more than one transition in procSelfFdReadlink results: %v", realPaths) + if len(realPaths) > 1 { + m.unstableProcSelfFdCount++ + assert.Contains(t, realPaths, handleName, "at least one real path should match the handle name if we got more than one real path") + } + + if realPath != handleName { + // It's possible for handle.Name() to be wrong because while it was + // correct when it was set, it might not match if the path was swapped + // afterwards (for both openat2 and partialLookupInRoot). + m.badNameCount++ + } + + // It's possible for lookups with ".." components to decide to cut off the + // lookup partially through the resolution when dealing with a swapping + // attack, so for the purposes of validating our tests we clean up the + // remainingPath so that it has all of the ".." components removed (but + // include this in our statistics). + fullLogicalPath := filepath.Join(realPath, remainingPath) + newRemainingPath, err := filepath.Rel(realPath, fullLogicalPath) + require.NoErrorf(t, err, "clean remaining path %s", remainingPath) + if remainingPath != newRemainingPath { + m.fixRemainingPathCount++ + } + remainingPath = newRemainingPath + + gotResult := lookupResult{ + handlePath: realPath, + remainingPath: remainingPath, + fileType: unixStat.Mode & unix.S_IFMT, + } + counter := &m.passOkCount + if !assert.Contains(t, allowedResults, gotResult) { + counter = &m.failCount + } + (*counter)++ +} + +func TestPartialLookup_RacingRename(t *testing.T) { + if !hasRenameExchange() { + t.Skip("test requires RENAME_EXCHANGE support") + } + + withWithoutOpenat2(t, false, func(t *testing.T) { + tree := []string{ + "dir a/b/c/d", + "symlink b-link ../b/../b/../b/../b/../b/../b/../b/../b/../b/../b/../b/../b/../b", + "symlink c-link ../../b/c/../../b/c/../../b/c/../../b/c/../../b/c/../../b/c/../../b/c/../../b/c/../../b/c", + "file file", + "symlink bad-link /foobar", + } + + var ( + handlePath = "/a/b/c/d" + remainingPath = "e" + defaultExpected []lookupResult + ) + // The lookup could stop at any component other than /a, so allow all + // of them. + for handlePath != "/" { + defaultExpected = append(defaultExpected, lookupResult{ + handlePath: handlePath, + remainingPath: remainingPath, + fileType: unix.S_IFDIR, + }) + handlePath, remainingPath = filepath.Dir(handlePath), filepath.Join(filepath.Base(handlePath), remainingPath) + } + for name, test := range map[string]struct { + subPathA, subPathB string + unsafePath string + skipErrs []error + allowedResults []lookupResult + }{ + // Swap a symlink in and out. + "swap-dir-link1-basic": {"a/b", "b-link", "a/b/c/d/e", nil, slices.Clone(defaultExpected)}, + "swap-dir-link2-basic": {"a/b/c", "c-link", "a/b/c/d/e", nil, slices.Clone(defaultExpected)}, + "swap-dir-link1-dotdot1": {"a/b", "b-link", "a/b/../b/../b/../b/../b/../b/../b/c/d/../d/../d/../d/../d/../d/e", nil, slices.Clone(defaultExpected)}, + "swap-dir-link1-dotdot2": {"a/b", "b-link", "a/b/c/../c/../c/../c/../c/../c/../c/d/../d/../d/../d/../d/../d/e", nil, slices.Clone(defaultExpected)}, + "swap-dir-link2-dotdot": {"a/b/c", "c-link", "a/b/c/../c/../c/../c/../c/../c/../c/d/../d/../d/../d/../d/../d/e", nil, slices.Clone(defaultExpected)}, + // TODO: Swap a directory. + // Swap a non-directory. + "swap-dir-file-basic": {"a/b", "file", "a/b/c/d/e", []error{unix.ENOTDIR, unix.ENOENT}, append( + // We could hit one of the final paths. + slices.Clone(defaultExpected), + // We could hit the file and stop resolving. + lookupResult{handlePath: "/file", remainingPath: "c/d/e", fileType: unix.S_IFREG}, + )}, + "swap-dir-file-dotdot": {"a/b", "file", "a/b/c/../c/../c/../c/../c/../c/../c/d/../d/../d/../d/../d/../d/e", []error{unix.ENOTDIR, unix.ENOENT}, append( + // We could hit one of the final paths. + slices.Clone(defaultExpected), + // We could hit the file and stop resolving. + lookupResult{handlePath: "/file", remainingPath: "c/d/e", fileType: unix.S_IFREG}, + )}, + // Swap a dangling symlink. + "swap-dir-danglinglink-basic": {"a/b", "bad-link", "a/b/c/d/e", []error{unix.ENOENT}, slices.Clone(defaultExpected)}, + "swap-dir-danglinglink-dotdot": {"a/b", "bad-link", "a/b/c/../c/../c/../c/../c/../c/../c/d/../d/../d/../d/../d/../d/e", []error{unix.ENOENT}, slices.Clone(defaultExpected)}, + // Swap the root. + "swap-root-basic": {".", "../outsideroot", "a/b/c/d/e", nil, slices.Clone(defaultExpected)}, + "swap-root-dotdot": {".", "../outsideroot", "a/b/../../a/b/../../a/b/../../a/b/../../a/b/../../a/b/../../a/b/../../a/b/c/d/e", nil, slices.Clone(defaultExpected)}, + // Swap one of our walking paths outside the root. + "swap-dir-outsideroot-basic": {"a/b", "../outsideroot", "a/b/c/d/e", nil, append( + // We could hit the expected path. + slices.Clone(defaultExpected), + // We could also land in the "outsideroot" path. This is okay + // because there was a moment when this directory was inside + // the root, and the attacker moved it outside the root. If we + // were to go into "..", the lookup would've failed (and we + // would get an error here if that wasn't the case). + lookupResult{handlePath: "../outsideroot", remainingPath: "c/d/e", fileType: unix.S_IFDIR}, + lookupResult{handlePath: "../outsideroot/c", remainingPath: "d/e", fileType: unix.S_IFDIR}, + lookupResult{handlePath: "../outsideroot/c/d", remainingPath: "e", fileType: unix.S_IFDIR}, + )}, + "swap-dir-outsideroot-dotdot": {"a/b", "../outsideroot", "a/b/../../a/b/../../a/b/../../a/b/../../a/b/../../a/b/../../a/b/../../a/b/c/d/e", nil, append( + // We could hit the expected path. + slices.Clone(defaultExpected), + // We could also land in the "outsideroot" path. This is okay + // because there was a moment when this directory was inside + // the root, and the attacker moved it outside the root. + // + // Neither openat2 nor partialLookupInRoot will allow us to + // walk into ".." in this case (escaping the root), and we + // would catch that if it did happen. + lookupResult{handlePath: "../outsideroot", remainingPath: "c/d/e", fileType: unix.S_IFDIR}, + lookupResult{handlePath: "../outsideroot/c", remainingPath: "d/e", fileType: unix.S_IFDIR}, + lookupResult{handlePath: "../outsideroot/c/d", remainingPath: "e", fileType: unix.S_IFDIR}, + )}, + } { + test := test // copy iterator + test.skipErrs = append(test.skipErrs, errPossibleAttack, errPossibleBreakout) + t.Run(name, func(t *testing.T) { + root := createTree(t, tree...) + + // Update the handlePath to be inside our root. + for idx := range test.allowedResults { + test.allowedResults[idx].handlePath = filepath.Join(root, test.allowedResults[idx].handlePath) + } + + // Create an "outsideroot" path as a sibling to our root, for + // swapping. + err := os.MkdirAll(filepath.Join(root, "../outsideroot"), 0o755) + require.NoError(t, err) + + rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + require.NoError(t, err) + defer rootDir.Close() + + // Run a goroutine that spams a rename in the root. + pauseCh := make(chan struct{}) + exitCh := make(chan struct{}) + defer close(exitCh) + go doRenameExchangeLoop(pauseCh, exitCh, rootDir, test.subPathA, test.subPathB) + + // Do several runs to try to catch bugs. + var testRuns = 10000 + if testing.Short() { + testRuns = 300 + } + m := newRacingLookupMeta(pauseCh) + for i := 0; i < testRuns; i++ { + m.checkPartialLookup(t, rootDir, test.unsafePath, test.skipErrs, test.allowedResults) + } + + pct := func(count int) string { + return fmt.Sprintf("%d(%.3f%%)", count, 100.0*float64(count)/float64(testRuns)) + } + + // Output some stats. + t.Logf("after %d runs: passOk=%s passErr=%s skip=%s fail=%s (+badErr=%s)", + // runs and breakdown of path-related (pass, fail) as well as skipped runs + testRuns, pct(m.passOkCount), pct(m.passErrCount), pct(m.skipCount), pct(m.failCount), + // failures due to incorrect errors (rather than bad paths) + pct(m.badErrCount)) + t.Logf(" badHandleName=%s fixRemainingPath=%s unstableProcSelfFdCount=%s", + // stats for how many test runs had to have some "workarounds" + pct(m.badNameCount), pct(m.fixRemainingPathCount), pct(m.unstableProcSelfFdCount)) + if len(m.skipErrCounts) > 0 { + t.Logf(" skipErr breakdown:") + for err, count := range m.skipErrCounts { + t.Logf(" %3.d: %v", count, err) + } + } + }) + } + + }) +} + type ssOperation interface { String() string Do(*testing.T, *symlinkStack) error diff --git a/mkdir_linux_test.go b/mkdir_linux_test.go index c2afd09..879e62e 100644 --- a/mkdir_linux_test.go +++ b/mkdir_linux_test.go @@ -7,6 +7,8 @@ package securejoin import ( + "errors" + "fmt" "os" "path/filepath" "testing" @@ -46,7 +48,7 @@ func testMkdirAll_Basic(t *testing.T, mkdirAll func(t *testing.T, root, unsafePa "symlink loop/link ../loop/link", } - withWithoutOpenat2(t, func(t *testing.T) { + withWithoutOpenat2(t, true, func(t *testing.T) { for name, test := range map[string]struct { unsafePath string expectedErr error @@ -240,3 +242,224 @@ func TestMkdirAllHandle_InvalidMode(t *testing.T) { return nil }) } + +type racingMkdirMeta struct { + passOkCount, passErrCount, failCount int + passErrCounts map[error]int +} + +func newRacingMkdirMeta() *racingMkdirMeta { + return &racingMkdirMeta{ + passErrCounts: map[error]int{}, + } +} + +func (m *racingMkdirMeta) checkMkdirAllHandle_Racing(t *testing.T, root, unsafePath string, mode int, allowedErrs []error) { + rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + require.NoError(t, err, "open root") + defer rootDir.Close() + + handle, err := MkdirAllHandle(rootDir, unsafePath, mode) + if err != nil { + for _, allowedErr := range allowedErrs { + if errors.Is(err, allowedErr) { + m.passErrCounts[allowedErr]++ + m.passErrCount++ + return + } + } + assert.NoError(t, err) + m.failCount++ + return + } + 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") + // TODO: Does it make sense to even try to check the handle path? + m.passOkCount++ +} + +func TestMkdirAllHandle_RacingRename(t *testing.T) { + withWithoutOpenat2(t, false, func(t *testing.T) { + treeSpec := []string{ + "dir target/a/b/c", + "dir swapdir-empty-ok ::0711", + "dir swapdir-empty-badmode ::0777", + "dir swapdir-nonempty1 ::0711", + "file swapdir-nonempty1/aaa", + "dir swapdir-nonempty2 ::0711", + "dir swapdir-nonempty2/f ::0711", + "file swapfile foobar ::0711", + } + + type test struct { + name string + pathA, pathB string + unsafePath string + allowedErrs []error + } + + 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", "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}}, + } + + if unix.Geteuid() == 0 { + // Add some wrong-uid cases if we are root. + treeSpec = append(treeSpec, + "dir swapdir-empty-badowner1 123:0:0711", + "dir swapdir-empty-badowner2 0:456:0711", + "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}}, + }...) + } + + for _, test := range tests { + test := test // copy iterator + t.Run(fmt.Sprintf("%s-%s", test.pathB, test.name), func(t *testing.T) { + rootCh := make(chan string) + defer close(rootCh) + go func(rootCh <-chan string) { + var root string + for { + select { + case newRoot, ok := <-rootCh: + if !ok { + return + } + root = newRoot + default: + if root != "" { + pathA := filepath.Join(root, test.pathA) + pathB := filepath.Join(root, test.pathB) + _ = unix.Renameat2(unix.AT_FDCWD, pathA, unix.AT_FDCWD, pathB, unix.RENAME_EXCHANGE) + } + } + } + }(rootCh) + + // Do several runs to try to catch bugs. + var testRuns = 10000 + if testing.Short() { + testRuns = 300 + } + m := newRacingMkdirMeta() + for i := 0; i < testRuns; i++ { + root := createTree(t, treeSpec...) + rootCh <- root + + m.checkMkdirAllHandle_Racing(t, root, test.unsafePath, 0o711, test.allowedErrs) + + // Clean up the root after each run so we don't exhaust all + // space in the tmpfs. + _ = os.RemoveAll(root) + } + + pct := func(count int) string { + return fmt.Sprintf("%d(%.3f%%)", count, 100.0*float64(count)/float64(testRuns)) + } + + // Output some stats. + t.Logf("after %d runs: passOk=%s passErr=%s fail=%s", + testRuns, pct(m.passOkCount), pct(m.passErrCount), pct(m.failCount)) + if len(m.passErrCounts) > 0 { + t.Logf(" passErr breakdown:") + for err, count := range m.passErrCounts { + t.Logf(" %3.d: %v", count, err) + } + } + }) + } + }) +} + +func TestMkdirAllHandle_RacingDelete(t *testing.T) { + withWithoutOpenat2(t, false, func(t *testing.T) { + treeSpec := []string{ + "dir target/a/b/c", + } + + for _, test := range []struct { + name string + rmPath string + unsafePath string + allowedErrs []error + }{ + {"rm-top", "target", "target/a/b/c/d/e/f/g/h/i/j/k", []error{errInvalidDirectory, unix.ENOENT}}, + {"rm-existing", "target/a/b/c", "target/a/b/c/d/e/f/g/h/i/j/k", []error{errInvalidDirectory, unix.ENOENT}}, + {"rm-nonexisting", "target/a/b/c/d/e", "target/a/b/c/d/e/f/g/h/i/j/k", []error{errInvalidDirectory, unix.ENOENT}}, + } { + test := test // copy iterator + t.Run(test.rmPath, func(t *testing.T) { + rootCh := make(chan string) + defer close(rootCh) + go func(rootCh <-chan string) { + var root string + for { + select { + case newRoot, ok := <-rootCh: + if !ok { + return + } + root = newRoot + default: + if root != "" { + _ = os.RemoveAll(filepath.Join(root, test.rmPath)) + } + } + } + }(rootCh) + + // Do several runs to try to catch bugs. + var testRuns = 10000 + if testing.Short() { + testRuns = 300 + } + m := newRacingMkdirMeta() + for i := 0; i < testRuns; i++ { + root := createTree(t, treeSpec...) + rootCh <- root + + m.checkMkdirAllHandle_Racing(t, root, test.unsafePath, 0o711, test.allowedErrs) + + // Clean up the root after each run so we don't exhaust all + // space in the tmpfs. + _ = os.RemoveAll(root + "/..") + } + + pct := func(count int) string { + return fmt.Sprintf("%d(%.3f%%)", count, 100.0*float64(count)/float64(testRuns)) + } + + // Output some stats. + t.Logf("after %d runs: passOk=%s passErr=%s fail=%s", + testRuns, pct(m.passOkCount), pct(m.passErrCount), pct(m.failCount)) + if len(m.passErrCounts) > 0 { + t.Logf(" passErr breakdown:") + for err, count := range m.passErrCounts { + t.Logf(" %3.d: %v", count, err) + } + } + }) + } + }) +} diff --git a/procfs_linux_test.go b/procfs_linux_test.go index 57a37d0..947f387 100644 --- a/procfs_linux_test.go +++ b/procfs_linux_test.go @@ -159,7 +159,7 @@ func testProcOvermountSubdir(t *testing.T, procRootFn procRootFunc, expectOvermo } func TestProcOvermountSubdir_unsafeHostProcRoot(t *testing.T) { - withWithoutOpenat2(t, func(t *testing.T) { + withWithoutOpenat2(t, true, func(t *testing.T) { // If we use the host /proc directly, we should see overmounts. testProcOvermountSubdir(t, unsafeHostProcRoot, true) }) @@ -169,7 +169,7 @@ func TestProcOvermountSubdir_newPrivateProcMount(t *testing.T) { if !hasNewMountApi() { t.Skip("test requires fsopen/open_tree support") } - withWithoutOpenat2(t, func(t *testing.T) { + withWithoutOpenat2(t, true, func(t *testing.T) { // If we create our own procfs, the overmounts shouldn't appear. testProcOvermountSubdir(t, newPrivateProcMount, false) }) @@ -179,7 +179,7 @@ func TestProcOvermountSubdir_clonePrivateProcMount(t *testing.T) { if !hasNewMountApi() { t.Skip("test requires fsopen/open_tree support") } - withWithoutOpenat2(t, func(t *testing.T) { + withWithoutOpenat2(t, true, func(t *testing.T) { // If we use open_tree(2), we don't use AT_RECURSIVE when running in // this test (because the overmounts are not locked mounts) and so we // don't expect to see overmounts. @@ -188,7 +188,7 @@ func TestProcOvermountSubdir_clonePrivateProcMount(t *testing.T) { } func TestProcOvermountSubdir_doGetProcRoot(t *testing.T) { - withWithoutOpenat2(t, func(t *testing.T) { + withWithoutOpenat2(t, true, func(t *testing.T) { // We expect to not get overmounts if we have the new mount API. // FIXME: It's possible to hit overmounts if there are locked mounts // and we hit the AT_RECURSIVE case... @@ -200,7 +200,7 @@ func TestProcOvermountSubdir_doGetProcRoot_Mocked(t *testing.T) { if !hasNewMountApi() { t.Skip("test requires fsopen/open_tree support") } - withWithoutOpenat2(t, func(t *testing.T) { + withWithoutOpenat2(t, true, func(t *testing.T) { testForceGetProcRoot(t, func(t *testing.T, expectOvermounts bool) { testProcOvermountSubdir(t, doGetProcRoot, expectOvermounts) }) diff --git a/util_linux_test.go b/util_linux_test.go index 57a65ef..b3f94f5 100644 --- a/util_linux_test.go +++ b/util_linux_test.go @@ -7,6 +7,7 @@ package securejoin import ( + "errors" "fmt" "os" "path/filepath" @@ -24,9 +25,10 @@ func requireRoot(t *testing.T) { } } -func withWithoutOpenat2(t *testing.T, testFn func(t *testing.T)) { - t.Run("openat2=auto", testFn) - +func withWithoutOpenat2(t *testing.T, doAuto bool, testFn func(t *testing.T)) { + if doAuto { + t.Run("openat2=auto", testFn) + } for _, useOpenat2 := range []bool{true, false} { useOpenat2 := useOpenat2 // copy iterator t.Run(fmt.Sprintf("openat2=%v", useOpenat2), func(t *testing.T) { @@ -81,15 +83,48 @@ func testForceProcThreadSelf(t *testing.T, testFn func(t *testing.T)) { } } +func hasRenameExchange() bool { + err := unix.Renameat2(unix.AT_FDCWD, ".", unix.AT_FDCWD, ".", unix.RENAME_EXCHANGE) + return !errors.Is(err, unix.ENOSYS) +} + +func doRenameExchangeLoop(pauseCh chan struct{}, exitCh <-chan struct{}, dir *os.File, pathA, pathB string) { + var swapped bool + swap := func() { + _ = unix.Renameat2(int(dir.Fd()), pathA, int(dir.Fd()), pathB, unix.RENAME_EXCHANGE) + //unix.Sync() + swapped = !swapped + } + + for { + select { + case <-exitCh: + return + case <-pauseCh: + if swapped { + swap() + } + // Wait for caller to unpause us. + select { + case pauseCh <- struct{}{}: + case <-exitCh: + return + } + default: + swap() + } + } +} + // Format: // -// dir -// file +// dir +// file // symlink -// char -// block -// fifo -// sock +// char +// block +// fifo +// sock func createInTree(t *testing.T, root, spec string) { f := strings.Fields(spec) if len(f) < 2 { @@ -97,8 +132,13 @@ func createInTree(t *testing.T, root, spec string) { } inoType, subPath, f := f[0], f[1], f[2:] fullPath := filepath.Join(root, subPath) + + var setOwnerMode *string switch inoType { case "dir": + if len(f) >= 1 { + setOwnerMode = &f[0] + } err := os.MkdirAll(fullPath, 0o755) require.NoError(t, err) case "file": @@ -106,6 +146,9 @@ func createInTree(t *testing.T, root, spec string) { if len(f) >= 1 { contents = []byte(f[0]) } + if len(f) >= 2 { + setOwnerMode = &f[1] + } err := os.WriteFile(fullPath, contents, 0o644) require.NoError(t, err) case "symlink": @@ -119,6 +162,9 @@ func createInTree(t *testing.T, root, spec string) { if len(f) < 2 { t.Fatalf("invalid spec %q", spec) } + if len(f) >= 3 { + setOwnerMode = &f[2] + } major, err := strconv.Atoi(f[0]) require.NoErrorf(t, err, "mknod %s: parse major", subPath) @@ -136,6 +182,9 @@ func createInTree(t *testing.T, root, spec string) { err = unix.Mknod(fullPath, mode, int(dev)) require.NoErrorf(t, err, "mknod (%s %d:%d) %s", inoType, major, minor, fullPath) case "fifo", "sock": + if len(f) >= 1 { + setOwnerMode = &f[0] + } var mode uint32 = 0o644 switch inoType { case "fifo": @@ -146,6 +195,28 @@ func createInTree(t *testing.T, root, spec string) { err := unix.Mknod(fullPath, mode, 0) require.NoErrorf(t, err, "mk%s %s", inoType, fullPath) } + if setOwnerMode != nil { + // :: + fields := strings.Split(*setOwnerMode, ":") + require.Lenf(t, fields, 3, "set owner-mode format uid:gid:mode") + uidStr, gidStr, modeStr := fields[0], fields[1], fields[2] + + if uidStr != "" && gidStr != "" { + uid, err := strconv.Atoi(uidStr) + require.NoErrorf(t, err, "chown %s: parse uid", fullPath) + gid, err := strconv.Atoi(gidStr) + require.NoErrorf(t, err, "chown %s: parse gid", fullPath) + err = unix.Chown(fullPath, uid, gid) + require.NoErrorf(t, err, "chown %s", fullPath) + } + + if modeStr != "" { + mode, err := strconv.ParseUint(modeStr, 8, 32) + require.NoErrorf(t, err, "chmod %s: parse mode", fullPath) + err = unix.Chmod(fullPath, uint32(mode)) + require.NoErrorf(t, err, "chmod %s", fullPath) + } + } } func createTree(t *testing.T, specs ...string) string {