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 {