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) + }) + } +}