Skip to content

Commit

Permalink
migration: check symlink sources during archive unpack
Browse files Browse the repository at this point in the history
During allocation directory migration, the client was not checking that any
symlinks in the archive aren't pointing to somewhere outside the allocation
directory. While task driver sandboxing will protect against processes inside
the task from reading/writing thru the symlink, this doesn't protect against the
client itself from performing unintended operations outside the sandbox.

This changeset includes two changes:

* Update the archive unpacking to check the source of symlinks and require that
  they fall within the sandbox.
* Fix a bug in the symlink check where it was using `filepath.Rel` which doesn't
  work for paths in the sibling directories of the sandbox directory. This bug
  doesn't appear to be exploitable but caused errors in testing.

Fixes: #19887
  • Loading branch information
tgross committed Feb 7, 2024
1 parent 3b0bd14 commit d1721c7
Show file tree
Hide file tree
Showing 5 changed files with 161 additions and 84 deletions.
3 changes: 3 additions & 0 deletions .changelog/19887.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:security
migration: Fixed a bug where archives used for migration were not checked for symlinks that escaped the allocation directory
```
32 changes: 21 additions & 11 deletions client/allocwatcher/alloc_watcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/hashicorp/nomad/client/config"
cstructs "github.com/hashicorp/nomad/client/structs"
"github.com/hashicorp/nomad/helper"
"github.com/hashicorp/nomad/helper/escapingfs"
"github.com/hashicorp/nomad/nomad/structs"
)

Expand Down Expand Up @@ -515,7 +516,7 @@ func (p *remotePrevAlloc) migrateAllocDir(ctx context.Context, nodeAddr string)
// Create the previous alloc dir
prevAllocDir := allocdir.NewAllocDir(p.logger, p.config.AllocDir, p.prevAllocID)
if err := prevAllocDir.Build(); err != nil {
return nil, fmt.Errorf("error building alloc dir for previous alloc %q: %v", p.prevAllocID, err)
return nil, fmt.Errorf("error building alloc dir for previous alloc %q: %w", p.prevAllocID, err)
}

// Create an API client
Expand All @@ -537,7 +538,7 @@ func (p *remotePrevAlloc) migrateAllocDir(ctx context.Context, nodeAddr string)
resp, err := apiClient.Raw().Response(url, qo)
if err != nil {
prevAllocDir.Destroy()
return nil, fmt.Errorf("error getting snapshot from previous alloc %q: %v", p.prevAllocID, err)
return nil, fmt.Errorf("error getting snapshot from previous alloc %q: %w", p.prevAllocID, err)
}

if err := p.streamAllocDir(ctx, resp, prevAllocDir.AllocDir); err != nil {
Expand Down Expand Up @@ -582,7 +583,7 @@ func (p *remotePrevAlloc) streamAllocDir(ctx context.Context, resp io.ReadCloser
}

if err != nil {
return fmt.Errorf("error streaming previous alloc %q for new alloc %q: %v",
return fmt.Errorf("error streaming previous alloc %q for new alloc %q: %w",
p.prevAllocID, p.allocID, err)
}

Expand All @@ -591,7 +592,7 @@ func (p *remotePrevAlloc) streamAllocDir(ctx context.Context, resp io.ReadCloser
// the message out of the file and return it.
errBuf := make([]byte, int(hdr.Size))
if _, err := tr.Read(errBuf); err != nil && err != io.EOF {
return fmt.Errorf("error streaming previous alloc %q for new alloc %q; failed reading error message: %v",
return fmt.Errorf("error streaming previous alloc %q for new alloc %q; failed reading error message: %w",
p.prevAllocID, p.allocID, err)
}
return fmt.Errorf("error streaming previous alloc %q for new alloc %q: %s",
Expand All @@ -606,36 +607,45 @@ func (p *remotePrevAlloc) streamAllocDir(ctx context.Context, resp io.ReadCloser
// Can't change owner if not root or on Windows.
if euid == 0 {
if err := os.Chown(name, hdr.Uid, hdr.Gid); err != nil {
return fmt.Errorf("error chowning directory %v", err)
return fmt.Errorf("error chowning directory %w", err)
}
}
continue
}
// If the header is for a symlink we create the symlink
if hdr.Typeflag == tar.TypeSymlink {
if err = os.Symlink(hdr.Linkname, filepath.Join(dest, hdr.Name)); err != nil {
return fmt.Errorf("error creating symlink: %v", err)
return fmt.Errorf("error creating symlink: %w", err)
}

escapes, err := escapingfs.PathEscapesAllocDir(dest, "", hdr.Name)
if err != nil {
return fmt.Errorf("error evaluating symlink: %w", err)
}
if escapes {
return fmt.Errorf("archive contains symlink that escapes alloc dir")
}

continue
}
// If the header is a file, we write to a file
if hdr.Typeflag == tar.TypeReg {
f, err := os.Create(filepath.Join(dest, hdr.Name))
if err != nil {
return fmt.Errorf("error creating file: %v", err)
return fmt.Errorf("error creating file: %w", err)
}

// Setting the permissions of the file as the origin.
if err := f.Chmod(os.FileMode(hdr.Mode)); err != nil {
f.Close()
return fmt.Errorf("error chmoding file %v", err)
return fmt.Errorf("error chmoding file %w", err)
}

// Can't change owner if not root or on Windows.
if euid == 0 {
if err := f.Chown(hdr.Uid, hdr.Gid); err != nil {
f.Close()
return fmt.Errorf("error chowning file %v", err)
return fmt.Errorf("error chowning file %w", err)
}
}

Expand All @@ -646,14 +656,14 @@ func (p *remotePrevAlloc) streamAllocDir(ctx context.Context, resp io.ReadCloser
if n > 0 && (err == nil || err == io.EOF) {
if _, err := f.Write(buf[:n]); err != nil {
f.Close()
return fmt.Errorf("error writing to file %q: %v", f.Name(), err)
return fmt.Errorf("error writing to file %q: %w", f.Name(), err)
}
}

if err != nil {
f.Close()
if err != io.EOF {
return fmt.Errorf("error reading snapshot: %v", err)
return fmt.Errorf("error reading snapshot: %w", err)
}
break
}
Expand Down
136 changes: 72 additions & 64 deletions client/allocwatcher/alloc_watcher_unix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/hashicorp/nomad/ci"
ctestutil "github.com/hashicorp/nomad/client/testutil"
"github.com/hashicorp/nomad/helper/testlog"
"github.com/shoenig/test/must"
)

// TestPrevAlloc_StreamAllocDir_Ok asserts that streaming a tar to an alloc dir
Expand All @@ -32,47 +33,90 @@ func TestPrevAlloc_StreamAllocDir_Ok(t *testing.T) {

// Create foo/
fooDir := filepath.Join(dir, "foo")
if err := os.Mkdir(fooDir, 0777); err != nil {
t.Fatalf("err: %v", err)
}
must.NoError(t, os.Mkdir(fooDir, 0777))

// Change ownership of foo/ to test #3702 (any non-root user is fine)
const uid, gid = 1, 1
if err := os.Chown(fooDir, uid, gid); err != nil {
t.Fatalf("err : %v", err)
}
must.NoError(t, os.Chown(fooDir, uid, gid))

dirInfo, err := os.Stat(fooDir)
if err != nil {
t.Fatalf("err: %v", err)
}
must.NoError(t, err)

// Create foo/bar
f, err := os.Create(filepath.Join(fooDir, "bar"))
if err != nil {
t.Fatalf("err: %v", err)
}
if _, err := f.WriteString("123"); err != nil {
t.Fatalf("err: %v", err)
}
if err := f.Chmod(0644); err != nil {
t.Fatalf("err: %v", err)
}
must.NoError(t, err)

_, err = f.WriteString("123")
must.NoError(t, err)

err = f.Chmod(0644)
must.NoError(t, err)

fInfo, err := f.Stat()
if err != nil {
t.Fatalf("err: %v", err)
}
must.NoError(t, err)
f.Close()

// Create foo/baz -> bar symlink
if err := os.Symlink("bar", filepath.Join(dir, "foo", "baz")); err != nil {
t.Fatalf("err: %v", err)
}
err = os.Symlink("bar", filepath.Join(dir, "foo", "baz"))
must.NoError(t, err)

linkInfo, err := os.Lstat(filepath.Join(dir, "foo", "baz"))
if err != nil {
t.Fatalf("err: %v", err)
must.NoError(t, err)

buf, err := testTar(dir)

dir1 := t.TempDir()

rc := io.NopCloser(buf)
prevAlloc := &remotePrevAlloc{logger: testlog.HCLogger(t)}
err = prevAlloc.streamAllocDir(context.Background(), rc, dir1)
must.NoError(t, err)

// Ensure foo is present
fi, err := os.Stat(filepath.Join(dir1, "foo"))
must.NoError(t, err)
must.Eq(t, dirInfo.Mode(), fi.Mode(), must.Sprintf("unexpected file mode"))

stat := fi.Sys().(*syscall.Stat_t)
if stat.Uid != uid || stat.Gid != gid {
t.Fatalf("foo/ has incorrect ownership: expected %d:%d found %d:%d",
uid, gid, stat.Uid, stat.Gid)
}

fi1, err := os.Stat(filepath.Join(dir1, "bar"))
must.NoError(t, err)
must.Eq(t, fInfo.Mode(), fi1.Mode(), must.Sprintf("unexpected file mode"))

fi2, err := os.Lstat(filepath.Join(dir1, "baz"))
must.NoError(t, err)
must.Eq(t, linkInfo.Mode(), fi2.Mode(), must.Sprintf("unexpected file mode"))
}

func TestPrevAlloc_StreamAllocDir_BadSymlink(t *testing.T) {
ci.Parallel(t)

dir := t.TempDir()
sensitiveDir := t.TempDir()

fooDir := filepath.Join(dir, "foo")
err := os.Mkdir(fooDir, 0777)
must.NoError(t, err)

// Create sensitive -> foo/bar symlink
err = os.Symlink(sensitiveDir, filepath.Join(dir, "foo", "baz"))
must.NoError(t, err)

buf, err := testTar(dir)
rc := io.NopCloser(buf)

dir1 := t.TempDir()
prevAlloc := &remotePrevAlloc{logger: testlog.HCLogger(t)}
err = prevAlloc.streamAllocDir(context.Background(), rc, dir1)
must.EqError(t, err, "archive contains symlink that escapes alloc dir")
}

func testTar(dir string) (*bytes.Buffer, error) {

buf := new(bytes.Buffer)
tw := tar.NewWriter(buf)

Expand Down Expand Up @@ -118,45 +162,9 @@ func TestPrevAlloc_StreamAllocDir_Ok(t *testing.T) {
}

if err := filepath.Walk(dir, walkFn); err != nil {
t.Fatalf("err: %v", err)
return nil, err
}
tw.Close()

dir1 := t.TempDir()

rc := io.NopCloser(buf)
prevAlloc := &remotePrevAlloc{logger: testlog.HCLogger(t)}
if err := prevAlloc.streamAllocDir(context.Background(), rc, dir1); err != nil {
t.Fatalf("err: %v", err)
}

// Ensure foo is present
fi, err := os.Stat(filepath.Join(dir1, "foo"))
if err != nil {
t.Fatalf("err: %v", err)
}
if fi.Mode() != dirInfo.Mode() {
t.Fatalf("mode: %v", fi.Mode())
}
stat := fi.Sys().(*syscall.Stat_t)
if stat.Uid != uid || stat.Gid != gid {
t.Fatalf("foo/ has incorrect ownership: expected %d:%d found %d:%d",
uid, gid, stat.Uid, stat.Gid)
}

fi1, err := os.Stat(filepath.Join(dir1, "bar"))
if err != nil {
t.Fatalf("err: %v", err)
}
if fi1.Mode() != fInfo.Mode() {
t.Fatalf("mode: %v", fi1.Mode())
}

fi2, err := os.Lstat(filepath.Join(dir1, "baz"))
if err != nil {
t.Fatalf("err: %v", err)
}
if fi2.Mode() != linkInfo.Mode() {
t.Fatalf("mode: %v", fi2.Mode())
}
return buf, nil
}
21 changes: 12 additions & 9 deletions helper/escapingfs/escapes.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,19 @@ func pathEscapesBaseViaSymlink(base, full string) (bool, error) {
return false, err
}

rel, err := filepath.Rel(resolveSym, base)
if err != nil {
return true, nil
}
// Nomad owns most of the prefix path, which includes the alloc UUID, so
// it's safe to assume that we can do a case insensitive check regardless of
// filesystem, as even if the cluster admin remounted the datadir with a
// slightly different capitalization, you'd only be able to escape into that
// same directory.
return !hasPrefixCaseInsensitive(resolveSym, base), nil
}

// note: this is not the same as !filesystem.IsAbs; we are asking if the relative
// path is descendent of the base path, indicating it does not escape.
isRelative := strings.HasPrefix(rel, "..") || rel == "."
escapes := !isRelative
return escapes, nil
func hasPrefixCaseInsensitive(path, prefix string) bool {
if len(prefix) > len(path) {
return false
}
return strings.EqualFold(path[:len(prefix)], prefix)
}

// PathEscapesAllocDir returns true if base/prefix/path escapes the given base directory.
Expand Down
53 changes: 53 additions & 0 deletions helper/escapingfs/escapes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"path/filepath"
"testing"

"github.com/shoenig/test/must"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -229,3 +230,55 @@ func TestPathEscapesSandbox(t *testing.T) {
})
}
}

func TestHasPrefixCaseInsensitive(t *testing.T) {
cases := []struct {
name string
path string
prefix string
expected bool
}{
{
name: "has prefix",
path: "/foo/bar",
prefix: "/foo",
expected: true,
},
{
name: "has prefix different case",
path: "/FOO/bar",
prefix: "/foo",
expected: true,
},
{
name: "short path",
path: "/foo",
prefix: "/foo/bar",
expected: false,
},
{
name: "exact match",
path: "/foo",
prefix: "/foo",
expected: true,
},
{
name: "no prefix",
path: "/baz/bar",
prefix: "/foo",
expected: false,
},
{
name: "no prefix different case",
path: "/BAZ/bar",
prefix: "/foo",
expected: false,
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
got := hasPrefixCaseInsensitive(tc.path, tc.prefix)
must.Eq(t, tc.expected, got)
})
}
}

0 comments on commit d1721c7

Please sign in to comment.