Merge pull request #10102 from chlins/fix/archive-item-path-containment

Verify extracted item paths stay inside the backup directory

changelogs/unreleased/10102-chlins

@@ -0,0 +1 @@
+Verify extracted item paths stay inside the backup directory

internal/delete/delete_item_action_handler.go

@@ -114,7 +114,10 @@ func InvokeDeleteActions(ctx *Context) error {
 
 			// Process individual items from the backup
 			for _, item := range items {
-				itemPath := archive.GetItemFilePath(dir, resource, namespace, item)
+				itemPath, err := archive.GetItemFilePath(dir, resource, namespace, item)
+				if err != nil {
+					return errors.Wrapf(err, "could not build item path: %v", item)
+				}
 
 				// obj is the Unstructured item from the backup
 				obj, err := archive.Unmarshal(ctx.Filesystem, itemPath)

pkg/archive/filesystem.go

@@ -19,7 +19,9 @@ package archive
 import (
 	"encoding/json"
 	"path/filepath"
+	"strings"
 
+	"github.com/cockroachdb/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
@@ -27,13 +29,37 @@ import (
 )
 
 // GetItemFilePath returns an item's file path once extracted from a Velero backup archive.
-func GetItemFilePath(rootDir, groupResource, namespace, name string) string {
+func GetItemFilePath(rootDir, groupResource, namespace, name string) (string, error) {
 	return GetVersionedItemFilePath(rootDir, groupResource, namespace, name, "")
 }
 
 // GetVersionedItemFilePath returns an item's file path once extracted from a Velero backup archive, with version included.
-func GetVersionedItemFilePath(rootDir, groupResource, namespace, name, versionPath string) string {
-	return filepath.Join(rootDir, velerov1api.ResourcesDir, groupResource, versionPath, GetScopeDir(namespace), namespace, name+".json")
+//
+// The namespace and name components can originate from backup contents - for example the
+// additional items a RestoreItemAction returns are built from annotations on a backed up
+// object - so the joined path is verified to stay within rootDir. Without that check a
+// component containing ".." escapes the extracted backup directory and addresses an
+// arbitrary file on the Velero pod's filesystem.
+func GetVersionedItemFilePath(rootDir, groupResource, namespace, name, versionPath string) (string, error) {
+	path := filepath.Join(rootDir, velerov1api.ResourcesDir, groupResource, versionPath, GetScopeDir(namespace), namespace, name+".json")
+
+	// rootDir is empty when building the path of an entry inside the backup tarball rather
+	// than of an extracted file on disk; "." is the containment base for that relative form.
+	base := rootDir
+	if base == "" {
+		base = "."
+	}
+
+	rel, err := filepath.Rel(base, path)
+	if err != nil {
+		return "", errors.Wrapf(err, "error resolving item path for %q/%q", namespace, name)
+	}
+
+	if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
+		return "", errors.Errorf("invalid item path for %q/%q: escapes the backup directory", namespace, name)
+	}
+
+	return path, nil
 }
 
 // GetScopeDir returns NamespaceScopedDir if namespace is present, or ClusterScopedDir if empty

pkg/archive/filesystem_test.go

@@ -27,31 +27,104 @@ import (
 )
 
 func TestGetItemFilePath(t *testing.T) {
-	res := GetItemFilePath("root", "resource", "", "item")
+	res, err := GetItemFilePath("root", "resource", "", "item")
+	require.NoError(t, err)
 	assert.Equal(t, "root/resources/resource/cluster/item.json", res)
 
-	res = GetItemFilePath("root", "resource", "namespace", "item")
+	res, err = GetItemFilePath("root", "resource", "namespace", "item")
+	require.NoError(t, err)
 	assert.Equal(t, "root/resources/resource/namespaces/namespace/item.json", res)
 
-	res = GetItemFilePath("", "resource", "", "item")
+	res, err = GetItemFilePath("", "resource", "", "item")
+	require.NoError(t, err)
 	assert.Equal(t, "resources/resource/cluster/item.json", res)
 
-	res = GetVersionedItemFilePath("root", "resource", "", "item", "")
+	res, err = GetVersionedItemFilePath("root", "resource", "", "item", "")
+	require.NoError(t, err)
 	assert.Equal(t, "root/resources/resource/cluster/item.json", res)
 
-	res = GetVersionedItemFilePath("root", "resource", "namespace", "item", "")
+	res, err = GetVersionedItemFilePath("root", "resource", "namespace", "item", "")
+	require.NoError(t, err)
 	assert.Equal(t, "root/resources/resource/namespaces/namespace/item.json", res)
 
-	res = GetVersionedItemFilePath("root", "resource", "namespace", "item", "v1")
+	res, err = GetVersionedItemFilePath("root", "resource", "namespace", "item", "v1")
+	require.NoError(t, err)
 	assert.Equal(t, "root/resources/resource/v1/namespaces/namespace/item.json", res)
 
-	res = GetVersionedItemFilePath("root", "resource", "", "item", "v1")
+	res, err = GetVersionedItemFilePath("root", "resource", "", "item", "v1")
+	require.NoError(t, err)
 	assert.Equal(t, "root/resources/resource/v1/cluster/item.json", res)
 
-	res = GetVersionedItemFilePath("", "resource", "", "item", "")
+	res, err = GetVersionedItemFilePath("", "resource", "", "item", "")
+	require.NoError(t, err)
 	assert.Equal(t, "resources/resource/cluster/item.json", res)
 }
 
+// TestGetItemFilePathRejectsPathTraversal verifies that a name or namespace containing
+// ".." cannot address a file outside the extracted backup directory. These components can
+// come from backup contents, for example the additional items a RestoreItemAction builds
+// from annotations on a backed up object.
+func TestGetItemFilePathRejectsPathTraversal(t *testing.T) {
+	tests := []struct {
+		name          string
+		rootDir       string
+		groupResource string
+		namespace     string
+		itemName      string
+	}{
+		{
+			name:          "traversal in name escapes root",
+			rootDir:       "/tmp/restore-dir",
+			groupResource: "secrets",
+			namespace:     "x",
+			itemName:      "../../../../../../root/.docker/config",
+		},
+		{
+			name:          "traversal in namespace escapes root",
+			rootDir:       "/tmp/restore-dir",
+			groupResource: "secrets",
+			namespace:     "../../../../../../etc",
+			itemName:      "passwd",
+		},
+		{
+			name:          "traversal in group resource escapes root",
+			rootDir:       "/tmp/restore-dir",
+			groupResource: "../../../../../../etc",
+			namespace:     "",
+			itemName:      "passwd",
+		},
+		{
+			name:          "traversal escapes archive-relative root",
+			rootDir:       "",
+			groupResource: "secrets",
+			namespace:     "x",
+			itemName:      "../../../../../../escape",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			res, err := GetItemFilePath(tc.rootDir, tc.groupResource, tc.namespace, tc.itemName)
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "escapes the backup directory")
+			assert.Empty(t, res)
+
+			res, err = GetVersionedItemFilePath(tc.rootDir, tc.groupResource, tc.namespace, tc.itemName, "v1")
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "escapes the backup directory")
+			assert.Empty(t, res)
+		})
+	}
+}
+
+// TestGetItemFilePathAllowsInnerDotDot verifies the containment check does not reject a
+// path whose ".." segments resolve back inside the root directory.
+func TestGetItemFilePathAllowsInnerDotDot(t *testing.T) {
+	res, err := GetItemFilePath("root", "resource", "namespaces/..", "item")
+	require.NoError(t, err)
+	assert.Equal(t, "root/resources/resource/namespaces/item.json", res)
+}
+
 func TestGetScopeDir(t *testing.T) {
 	res := GetScopeDir("")
 	assert.Equal(t, velerov1api.ClusterScopedDir, res)

pkg/backup/item_backupper.go

@@ -351,16 +351,28 @@ func (ib *itemBackupper) backupItemInternal(logger logrus.FieldLogger, obj runti
 	if versionPath == preferredGVR.Version {
 		// backing up preferred version backup without API Group version - for backward compatibility
 		log.Debugf("Resource %s/%s, version= %s, preferredVersion=%s", groupResource.String(), name, versionPath, preferredGVR.Version)
-		itemFiles = append(itemFiles, getFileForArchive(namespace, name, groupResource.String(), "", itemBytes))
+		fileForArchive, err := getFileForArchive(namespace, name, groupResource.String(), "", itemBytes)
+		if err != nil {
+			return false, itemFiles, err
+		}
+		itemFiles = append(itemFiles, fileForArchive)
 		versionPath = versionPath + velerov1api.PreferredVersionDir
 	}
 
-	itemFiles = append(itemFiles, getFileForArchive(namespace, name, groupResource.String(), versionPath, itemBytes))
+	fileForArchive, err := getFileForArchive(namespace, name, groupResource.String(), versionPath, itemBytes)
+	if err != nil {
+		return false, itemFiles, err
+	}
+	itemFiles = append(itemFiles, fileForArchive)
 	return true, itemFiles, nil
 }
 
-func getFileForArchive(namespace, name, groupResource, versionPath string, itemBytes []byte) FileForArchive {
-	filePath := archive.GetVersionedItemFilePath("", groupResource, namespace, name, versionPath)
+func getFileForArchive(namespace, name, groupResource, versionPath string, itemBytes []byte) (FileForArchive, error) {
+	filePath, err := archive.GetVersionedItemFilePath("", groupResource, namespace, name, versionPath)
+	if err != nil {
+		return FileForArchive{}, err
+	}
+
 	hdr := &tar.Header{
 		Name:     filePath,
 		Size:     int64(len(itemBytes)),
@@ -368,7 +380,7 @@ func getFileForArchive(namespace, name, groupResource, versionPath string, itemB
 		Mode:     0755,
 		ModTime:  time.Now(),
 	}
-	return FileForArchive{FilePath: filePath, Header: hdr, FileBytes: itemBytes}
+	return FileForArchive{FilePath: filePath, Header: hdr, FileBytes: itemBytes}, nil
 }
 
 // backupPodVolumes triggers pod volume backups of the specified pod volumes, and returns a list of PodVolumeBackups

pkg/restore/restore.go

@@ -1011,11 +1011,13 @@ func (ctx *restoreContext) processSelectedResource(
 			if namespace != "" && !existingNamespaces.Has(targetNS) {
 				logger := ctx.log.WithField("namespace", namespace)
 
-				ns := getNamespace(
-					logger,
-					archive.GetItemFilePath(ctx.restoreDir, "namespaces", "", namespace),
-					targetNS,
-				)
+				nsPath, err := archive.GetItemFilePath(ctx.restoreDir, "namespaces", "", namespace)
+				if err != nil {
+					errs.AddVeleroError(err)
+					continue
+				}
+
+				ns := getNamespace(logger, nsPath, targetNS)
 				_, nsCreated, err := kube.EnsureNamespaceExistsAndIsReady(
 					ns,
 					ctx.namespaceClient,
@@ -1440,7 +1442,13 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso
 		// If the namespace scoped resource should be restored, ensure that the
 		// namespace into which the resource is being restored into exists.
 		// This is the *remapped* namespace that we are ensuring exists.
-		nsToEnsure := getNamespace(restoreLogger, archive.GetItemFilePath(ctx.restoreDir, "namespaces", "", obj.GetNamespace()), namespace)
+		nsPath, err := archive.GetItemFilePath(ctx.restoreDir, "namespaces", "", obj.GetNamespace())
+		if err != nil {
+			errs.AddVeleroError(err)
+			return warnings, errs, itemExists
+		}
+
+		nsToEnsure := getNamespace(restoreLogger, nsPath, namespace)
 		_, nsCreated, err := kube.EnsureNamespaceExistsAndIsReady(nsToEnsure, ctx.namespaceClient, ctx.resourceTerminatingTimeout, ctx.resourceDeletionStatusTracker)
 		if err != nil {
 			errs.AddVeleroError(err)
@@ -1693,7 +1701,17 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso
 
 		var filteredAdditionalItems []velero.ResourceIdentifier
 		for _, additionalItem := range executeOutput.AdditionalItems {
-			itemPath := archive.GetItemFilePath(ctx.restoreDir, additionalItem.GroupResource.String(), additionalItem.Namespace, additionalItem.Name)
+			itemPath, err := archive.GetItemFilePath(ctx.restoreDir, additionalItem.GroupResource.String(), additionalItem.Namespace, additionalItem.Name)
+			if err != nil {
+				restoreLogger.WithError(err).WithFields(logrus.Fields{
+					"additionalResource":          additionalItem.GroupResource.String(),
+					"additionalResourceNamespace": additionalItem.Namespace,
+					"additionalResourceName":      additionalItem.Name,
+				}).Warn("unable to restore additional item")
+				warnings.Add(additionalItem.Namespace, err)
+
+				continue
+			}
 
 			if _, err := ctx.fileSystem.Stat(itemPath); err != nil {
 				restoreLogger.WithError(err).WithFields(logrus.Fields{
@@ -2671,9 +2689,9 @@ func (ctx *restoreContext) getSelectedRestoreableItems(resource string, original
 
 				// Peek-and-map logic for unresolvable kinds
 				if rf == nil && len(items) > 0 {
-					peekPath := archive.GetItemFilePath(ctx.restoreDir, resourceForPath, originalNamespace, items[0])
-					// Ignore unmarshal errors during peek; the main restore loop will catch and report them
-					if obj, err := archive.Unmarshal(ctx.fileSystem, peekPath); err == nil {
+					peekPath, pathErr := archive.GetItemFilePath(ctx.restoreDir, resourceForPath, originalNamespace, items[0])
+					// Ignore path and unmarshal errors during peek; the main restore loop will catch and report them
+					if obj, err := archive.Unmarshal(ctx.fileSystem, peekPath); pathErr == nil && err == nil {
 						actualKind := obj.GroupVersionKind().Kind
 						for _, filter := range nsFilter.resourceFilterMap {
 							for _, k := range filter.originalKinds {
@@ -2714,9 +2732,9 @@ func (ctx *restoreContext) getSelectedRestoreableItems(resource string, original
 				// Note: Unlike the namespaced path, this fallback is always reachable
 				// because the main restore loop does not have a fast-path skip for
 				// unlisted cluster-scoped resources.
-				peekPath := archive.GetItemFilePath(ctx.restoreDir, resourceForPath, originalNamespace, items[0])
-				// Ignore unmarshal errors during peek; the main restore loop will catch and report them
-				if obj, err := archive.Unmarshal(ctx.fileSystem, peekPath); err == nil {
+				peekPath, pathErr := archive.GetItemFilePath(ctx.restoreDir, resourceForPath, originalNamespace, items[0])
+				// Ignore path and unmarshal errors during peek; the main restore loop will catch and report them
+				if obj, err := archive.Unmarshal(ctx.fileSystem, peekPath); pathErr == nil && err == nil {
 					actualKind := obj.GroupVersionKind().Kind
 					for _, filter := range ctx.clusterScopedFilterMap {
 						for _, k := range filter.originalKinds {
@@ -2742,7 +2760,11 @@ func (ctx *restoreContext) getSelectedRestoreableItems(resource string, original
 	}
 
 	for _, item := range items {
-		itemPath := archive.GetItemFilePath(ctx.restoreDir, resourceForPath, originalNamespace, item)
+		itemPath, err := archive.GetItemFilePath(ctx.restoreDir, resourceForPath, originalNamespace, item)
+		if err != nil {
+			errs.Add(targetNamespace, err)
+			continue
+		}
 
 		obj, err := archive.Unmarshal(ctx.fileSystem, itemPath)
 		if err != nil {