From b3209cbc6921e703b0e9984ce70c10b378665834 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Tue, 6 Feb 2024 08:55:45 -0500 Subject: [PATCH] migration: check symlink sources during archive unpack 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: https://github.com/hashicorp/nomad/issues/19887 --- .changelog/19887.txt | 3 + client/allocwatcher/alloc_watcher.go | 32 +++-- .../allocwatcher/alloc_watcher_unix_test.go | 136 +++++++++--------- helper/escapingfs/escapes.go | 21 +-- helper/escapingfs/escapes_test.go | 53 +++++++ 5 files changed, 161 insertions(+), 84 deletions(-) create mode 100644 .changelog/19887.txt diff --git a/.changelog/19887.txt b/.changelog/19887.txt new file mode 100644 index 000000000000..1370cfdb53ae --- /dev/null +++ b/.changelog/19887.txt @@ -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 +``` diff --git a/client/allocwatcher/alloc_watcher.go b/client/allocwatcher/alloc_watcher.go index 5e5be2451ea7..76f1d1c8d949 100644 --- a/client/allocwatcher/alloc_watcher.go +++ b/client/allocwatcher/alloc_watcher.go @@ -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" ) @@ -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 @@ -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 { @@ -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) } @@ -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", @@ -606,7 +607,7 @@ 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 @@ -614,28 +615,37 @@ func (p *remotePrevAlloc) streamAllocDir(ctx context.Context, resp io.ReadCloser // 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) } } @@ -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 } diff --git a/client/allocwatcher/alloc_watcher_unix_test.go b/client/allocwatcher/alloc_watcher_unix_test.go index 2e95d33654d4..6416175bfc9e 100644 --- a/client/allocwatcher/alloc_watcher_unix_test.go +++ b/client/allocwatcher/alloc_watcher_unix_test.go @@ -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 @@ -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) @@ -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 } diff --git a/helper/escapingfs/escapes.go b/helper/escapingfs/escapes.go index a94e85ad4c11..6bec33364de9 100644 --- a/helper/escapingfs/escapes.go +++ b/helper/escapingfs/escapes.go @@ -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. diff --git a/helper/escapingfs/escapes_test.go b/helper/escapingfs/escapes_test.go index 0f9c6a3ba281..a413fd87035d 100644 --- a/helper/escapingfs/escapes_test.go +++ b/helper/escapingfs/escapes_test.go @@ -9,6 +9,7 @@ import ( "path/filepath" "testing" + "github.com/shoenig/test/must" "github.com/stretchr/testify/require" ) @@ -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) + }) + } +}