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: #19887

.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
+```

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,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)
 				}
 			}
 
@@ -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
 				}

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
 }

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.

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