Add tests for layer entry path validation (#8332)

Also return a non-retryable error.
buildbuddy-io · Feb 6, 2025 · ff6f2e7 · ff6f2e7
1 parent 625bb68
commit ff6f2e7
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 2 deletions.
diff --git a/enterprise/server/remote_execution/containers/ociruntime/ociruntime.go b/enterprise/server/remote_execution/containers/ociruntime/ociruntime.go
@@ -1396,7 +1396,7 @@ func downloadLayer(ctx context.Context, layer ctr.Layer, destDir string) error {
 		}
 
 		if slices.Contains(strings.Split(header.Name, string(os.PathSeparator)), "..") {
-			return status.UnavailableErrorf("tar entry is not clean: %q", header.Name)
+			return status.InvalidArgumentErrorf("invalid tar header: name %q is invalid", header.Name)
 		}
 
 		// filepath.Join applies filepath.Clean to all arguments
@@ -1471,7 +1471,7 @@ func downloadLayer(ctx context.Context, layer ctr.Layer, destDir string) error {
 		case tar.TypeLink:
 			target := filepath.Join(tempUnpackDir, header.Linkname)
 			if !strings.HasPrefix(target, tempUnpackDir) {
-				return status.UnavailableErrorf("breakout attempt detected with link: %q -> %q", header.Name, header.Linkname)
+				return status.InvalidArgumentErrorf("invalid tar header: link name %q is invalid", header.Linkname)
 			}
 			// Note that this will call linkat(2) without AT_SYMLINK_FOLLOW,
 			// so if target is a symlink, the hardlink will point to the symlink itself and not the symlink target.

diff --git a/enterprise/server/remote_execution/containers/ociruntime/ociruntime_test.go b/enterprise/server/remote_execution/containers/ociruntime/ociruntime_test.go
@@ -1192,6 +1192,59 @@ func TestFileOwnership(t *testing.T) {
 	)
 }
 
+func TestPathSanitization(t *testing.T) {
+	setupNetworking(t)
+	for _, test := range []struct {
+		Name          string
+		Tar           []byte
+		ExpectedError string
+	}{
+		{
+			Name: "entry name",
+			Tar: testtar.EntryBytes(t, &tar.Header{
+				Name:     "../foo.txt",
+				Uid:      os.Getuid(),
+				Gid:      os.Getgid(),
+				Typeflag: tar.TypeReg,
+			}, nil),
+			ExpectedError: "invalid tar header: name",
+		},
+		{
+			Name: "hardlink target",
+			Tar: testtar.EntryBytes(t, &tar.Header{
+				Name:     "/foo.txt",
+				Linkname: "../test-link-target",
+				Typeflag: tar.TypeLink,
+			}, nil),
+			ExpectedError: "invalid tar header: link name",
+		},
+	} {
+		t.Run(test.Name, func(t *testing.T) {
+			cacheRoot := testfs.MakeTempDir(t)
+			testfs.WriteAllFileContents(t, cacheRoot, map[string]string{
+				"test-link-target": "Hello",
+			})
+			imageStore := ociruntime.NewImageStore(cacheRoot)
+			// Load busybox oci image
+			busyboxImg := testregistry.ImageFromRlocationpath(t, ociBusyboxRlocationpath)
+			// Append an invalid layer
+			layer := testregistry.NewBytesLayer(t, test.Tar)
+			img, err := mutate.AppendLayers(busyboxImg, layer)
+			require.NoError(t, err)
+			// Start a test registry and push the mutated busybox image to it
+			reg := testregistry.Run(t, testregistry.Opts{})
+			image := reg.Push(t, img, "test-file-ownership:latest")
+
+			// Make sure we get an error when pulling this image.
+			ctx := context.Background()
+			_, err = imageStore.Pull(ctx, image, oci.Credentials{})
+			require.Error(t, err)
+			assert.True(t, status.IsInvalidArgumentError(err), "expected InvalidArgument, got %T", err)
+			assert.Contains(t, err.Error(), test.ExpectedError)
+		})
+	}
+}
+
 func TestPersistentWorker(t *testing.T) {
 	setupNetworking(t)