Skip to content

Commit

Permalink
tests: add racing tests for partialLokupInRoot and MkdirAllHandle
Browse files Browse the repository at this point in the history 
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 90.9% on Linux (91.1% combined).

Signed-off-by: Aleksa Sarai <[email protected]>
  • Loading branch information
@cyphar
cyphar committed Jul 9, 2024
1 parent eb480c7 commit 0282597
Show file tree
Hide file tree
Showing 5 changed files with 603 additions and 21 deletions.
29 changes: 25 additions & 4 deletions .github/workflows/ci.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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-full:
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 30m -v -cover -test.gocoverdir="$GOCOVERDIR" ./...
- name: upload coverage
uses: actions/upload-artifact@v4
with:
Expand All @@ -88,7 +108,8 @@ jobs:
runs-on: ubuntu-latest
needs:
- test-windows
- test-unix
- test-unix-short
- test-linux-full
steps:
- uses: actions/checkout@v3
- uses: actions/setup-go@v4
Expand Down
272 changes: 270 additions & 2 deletions lookup_linux_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@
package securejoin

import (
"errors"
"fmt"
"os"
"path/filepath"
"slices"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -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)
})
}
Expand All @@ -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{
Expand Down Expand Up @@ -306,6 +308,272 @@ 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.
var readlinkRetryMax = 100
if testing.Short() {
readlinkRetryMax = 30
}
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_RenameRace(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 _, test := range []struct {
subPathA, subPathB string
unsafePath string
skipErrs []error
allowedResults []lookupResult
}{
// Swap a symlink in and out.
{"a/b", "b-link", "a/b/c/d/e", nil, slices.Clone(defaultExpected)},
{"a/b/c", "c-link", "a/b/c/d/e", nil, slices.Clone(defaultExpected)},
{"a/b", "b-link", "a/b/../b/../b/../b/../b/../b/../b/c/d/../d/../d/../d/../d/../d/e", nil, slices.Clone(defaultExpected)},
{"a/b", "b-link", "a/b/c/../c/../c/../c/../c/../c/../c/d/../d/../d/../d/../d/../d/e", nil, slices.Clone(defaultExpected)},
{"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.
{"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.
{"a/b", "bad-link", "a/b/c/d/e", []error{unix.ENOENT}, slices.Clone(defaultExpected)},
{"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.
{".", "../outsideroot", "a/b/c/d/e", nil, slices.Clone(defaultExpected)},
{".", "../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.
{"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},
)},
{"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(fmt.Sprintf("swap(%q,%q):%q", test.subPathA, test.subPathB, test.unsafePath), func(t *testing.T) {
t.Parallel()

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 = 50000
if testing.Short() {
testRuns = 1000
}
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
Expand Down
Loading

0 comments on commit 0282597

Please sign in to comment.