From 0f86521735cb98ee5f8cd71ffee6cd14a9ac7caa Mon Sep 17 00:00:00 2001 From: chlins Date: Wed, 29 Jul 2026 13:43:52 +0800 Subject: [PATCH] Verify extracted item paths stay inside the backup directory archive.GetItemFilePath/GetVersionedItemFilePath joined the group resource, namespace and name into a path without checking the result against rootDir. Those components can come from backup contents - the additional items a RestoreItemAction returns are built from annotations on a backed up object - so a component containing ".." resolved to an arbitrary file on the Velero pod, which was then Stat'd, unmarshalled and restored as a Kubernetes object. Both helpers now return an error when the joined path escapes rootDir, and all callers handle it. rootDir is empty when building an entry path inside the backup tarball, so "." is used as the containment base for that relative form. Signed-off-by: chlins --- changelogs/unreleased/10102-chlins | 1 + internal/delete/delete_item_action_handler.go | 5 +- pkg/archive/filesystem.go | 32 ++++++- pkg/archive/filesystem_test.go | 89 +++++++++++++++++-- pkg/backup/item_backupper.go | 22 +++-- pkg/restore/restore.go | 50 ++++++++--- 6 files changed, 168 insertions(+), 31 deletions(-) create mode 100644 changelogs/unreleased/10102-chlins diff --git a/changelogs/unreleased/10102-chlins b/changelogs/unreleased/10102-chlins new file mode 100644 index 000000000..70b4b5c44 --- /dev/null +++ b/changelogs/unreleased/10102-chlins @@ -0,0 +1 @@ +Verify extracted item paths stay inside the backup directory diff --git a/internal/delete/delete_item_action_handler.go b/internal/delete/delete_item_action_handler.go index 2a16044ee..89a638331 100644 --- a/internal/delete/delete_item_action_handler.go +++ b/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) diff --git a/pkg/archive/filesystem.go b/pkg/archive/filesystem.go index 73b0d1dcf..310ab64dc 100644 --- a/pkg/archive/filesystem.go +++ b/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 diff --git a/pkg/archive/filesystem_test.go b/pkg/archive/filesystem_test.go index bf7f16c76..c6225ff85 100644 --- a/pkg/archive/filesystem_test.go +++ b/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) diff --git a/pkg/backup/item_backupper.go b/pkg/backup/item_backupper.go index f43888252..c180092a5 100644 --- a/pkg/backup/item_backupper.go +++ b/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 diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index e7a284fb1..336add4de 100644 --- a/pkg/restore/restore.go +++ b/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 {