From cbb18acc4d6fefabfb18c9b3ce655245d6cd956e Mon Sep 17 00:00:00 2001 From: mptyl Date: Sun, 16 Aug 2026 01:38:48 +0200 Subject: [PATCH] fix(cli): harden restore preflight validation --- tools/tht/internal/backup/preflight.go | 258 +++++++++++++++++++- tools/tht/internal/backup/preflight_test.go | 124 +++++++++- 2 files changed, 367 insertions(+), 15 deletions(-) diff --git a/tools/tht/internal/backup/preflight.go b/tools/tht/internal/backup/preflight.go index 4e46232d..bb8fa20d 100644 --- a/tools/tht/internal/backup/preflight.go +++ b/tools/tht/internal/backup/preflight.go @@ -6,6 +6,7 @@ import ( "bytes" "context" "crypto/sha256" + "encoding/binary" "encoding/hex" "encoding/json" "errors" @@ -29,8 +30,25 @@ type PreflightRequest struct { Archive string Confirm bool AllowExternalSecrets bool + Limits PreflightLimits } +// PreflightLimits bounds hostile archive processing. Zero values select the conservative +// defaults; callers that need larger installation backups must opt in explicitly. +type PreflightLimits struct { + MaxMembers int + MaxArchiveBytes uint64 + MaxUncompressedBytes uint64 + MaxCompressionRatio uint64 +} + +const ( + defaultPreflightMaxMembers = 10_000 + defaultPreflightMaxArchiveBytes = 32 << 30 + defaultPreflightMaxUncompressedBytes = 128 << 30 + defaultPreflightMaxCompressionRatio = 100 +) + // PreflightDependencies supplies checks that require knowledge of the current Docker targets. // Part B can bind these callbacks to read-only Docker Compose and filesystem inspections before // it starts its restore transaction. @@ -61,6 +79,14 @@ type PreflightResult struct { RequiredBytes uint64 Manifest Manifest Entries []ArchiveEntryMetadata + archive *verifiedArchive +} + +type verifiedArchive struct { + file *os.File + info os.FileInfo + digest string + limits PreflightLimits } type inspectedArchiveEntry struct { @@ -78,21 +104,51 @@ func Preflight(ctx context.Context, installation config.Installation, request Pr if err := contextError(ctx); err != nil { return PreflightResult{}, err } + limits, err := normalizePreflightLimits(request.Limits) + if err != nil { + return PreflightResult{}, err + } archivePath, archiveInfo, err := openableArchivePath(request.Archive) if err != nil { return PreflightResult{}, err } + if uint64(archiveInfo.Size()) > limits.MaxArchiveBytes { + return PreflightResult{}, errors.New("backup archive exceeds the configured archive-size limit") + } archiveFile, err := os.Open(archivePath) if err != nil { return PreflightResult{}, fmt.Errorf("open backup archive: %w", err) } - defer archiveFile.Close() - reader, err := zip.NewReader(archiveFile, archiveInfo.Size()) + keepArchiveOpen := false + defer func() { + if !keepArchiveOpen { + _ = archiveFile.Close() + } + }() + openedInfo, err := archiveFile.Stat() + if err != nil { + return PreflightResult{}, fmt.Errorf("inspect opened backup archive: %w", err) + } + if !openedInfo.Mode().IsRegular() || openedInfo.Size() != archiveInfo.Size() { + return PreflightResult{}, errors.New("backup archive changed while opening") + } + memberCount, err := zipMemberCount(archiveFile, openedInfo.Size()) + if err != nil { + return PreflightResult{}, err + } + if memberCount > uint64(limits.MaxMembers) { + return PreflightResult{}, errors.New("backup archive exceeds the configured member limit") + } + initialDigest, err := digestArchive(ctx, archiveFile, limits.MaxArchiveBytes) + if err != nil { + return PreflightResult{}, err + } + reader, err := zip.NewReader(archiveFile, openedInfo.Size()) if err != nil { return PreflightResult{}, fmt.Errorf("read backup archive: %w", err) } - manifestBytes, entries, err := inspectArchiveMembers(ctx, reader) + manifestBytes, entries, err := inspectArchiveMembers(ctx, reader, limits) if err != nil { return PreflightResult{}, err } @@ -137,12 +193,74 @@ func Preflight(ctx context.Context, installation config.Installation, request Pr if err := dependencies.CheckImageConfigCompatibility(ctx, installation, manifest); err != nil { return PreflightResult{}, fmt.Errorf("validate image/config compatibility: %w", err) } + finalDigest, err := digestArchive(ctx, archiveFile, limits.MaxArchiveBytes) + if err != nil { + return PreflightResult{}, err + } + if initialDigest != finalDigest { + return PreflightResult{}, errors.New("backup archive changed during preflight") + } + keepArchiveOpen = true return PreflightResult{ - ArchivePath: archivePath, ArchiveSize: archiveInfo.Size(), RequiredBytes: requiredBytes, + ArchivePath: archivePath, ArchiveSize: openedInfo.Size(), RequiredBytes: requiredBytes, Manifest: manifest, Entries: metadata, + archive: &verifiedArchive{file: archiveFile, info: openedInfo, digest: finalDigest, limits: limits}, }, nil } +// RevalidateArchive binds a restore to the bytes inspected by Preflight. A caller must invoke +// it immediately before a restore transaction and use the returned retained handle, never reopen +// ArchivePath. It refuses a path replacement or in-place content change. +func (result PreflightResult) RevalidateArchive() (*os.File, error) { + if result.archive == nil || result.archive.file == nil { + return nil, errors.New("backup archive has not been retained by preflight") + } + pathInfo, err := os.Lstat(result.ArchivePath) + if err != nil { + return nil, errors.New("backup archive changed after preflight") + } + if !pathInfo.Mode().IsRegular() || !os.SameFile(result.archive.info, pathInfo) { + return nil, errors.New("backup archive changed after preflight") + } + heldInfo, err := result.archive.file.Stat() + if err != nil || !os.SameFile(result.archive.info, heldInfo) || heldInfo.Size() != result.ArchiveSize { + return nil, errors.New("backup archive changed after preflight") + } + digest, err := digestArchive(context.Background(), result.archive.file, result.archive.limits.MaxArchiveBytes) + if err != nil || digest != result.archive.digest { + return nil, errors.New("backup archive changed after preflight") + } + return result.archive.file, nil +} + +// CloseArchive releases the retained read-only archive handle after the caller finishes the +// restore transaction or decides not to proceed. +func (result PreflightResult) CloseArchive() error { + if result.archive == nil || result.archive.file == nil { + return nil + } + return result.archive.file.Close() +} + +func normalizePreflightLimits(requested PreflightLimits) (PreflightLimits, error) { + if requested.MaxMembers < 0 { + return PreflightLimits{}, errors.New("backup preflight member limit must be positive") + } + if requested.MaxMembers == 0 { + requested.MaxMembers = defaultPreflightMaxMembers + } + if requested.MaxArchiveBytes == 0 { + requested.MaxArchiveBytes = defaultPreflightMaxArchiveBytes + } + if requested.MaxUncompressedBytes == 0 { + requested.MaxUncompressedBytes = defaultPreflightMaxUncompressedBytes + } + if requested.MaxCompressionRatio == 0 { + requested.MaxCompressionRatio = defaultPreflightMaxCompressionRatio + } + return requested, nil +} + func validateRawManifestPaths(value []byte) error { var raw struct { Entries []struct { @@ -180,13 +298,20 @@ func openableArchivePath(requested string) (string, os.FileInfo, error) { return archivePath, info, nil } -func inspectArchiveMembers(ctx context.Context, reader *zip.Reader) ([]byte, map[string]inspectedArchiveEntry, error) { +func inspectArchiveMembers(ctx context.Context, reader *zip.Reader, limits PreflightLimits) ([]byte, map[string]inspectedArchiveEntry, error) { + if len(reader.File) > limits.MaxMembers { + return nil, nil, errors.New("backup archive exceeds the configured member limit") + } var manifestBytes []byte entries := make(map[string]inspectedArchiveEntry, len(reader.File)) + var totalUncompressed uint64 for _, member := range reader.File { if err := contextError(ctx); err != nil { return nil, nil, err } + if err := checkArchiveMemberLimits(member, limits, &totalUncompressed); err != nil { + return nil, nil, err + } name, err := validateArchiveMemberPath(member.Name) if err != nil { return nil, nil, err @@ -216,10 +341,13 @@ func inspectArchiveMembers(ctx context.Context, reader *zip.Reader) ([]byte, map if int64(len(manifestBytes)) > maxPreflightManifestBytes { return nil, nil, errors.New("backup manifest is too large") } + if uint64(len(manifestBytes)) != member.UncompressedSize64 { + return nil, nil, errors.New("backup manifest size does not match its archive metadata") + } continue } hashValue := sha256.New() - size, copyErr := io.Copy(hashValue, opened) + size, copyErr := io.Copy(hashValue, io.LimitReader(opened, int64(member.UncompressedSize64)+1)) closeErr := opened.Close() if copyErr != nil { return nil, nil, fmt.Errorf("read backup archive entry: %w", copyErr) @@ -230,6 +358,9 @@ func inspectArchiveMembers(ctx context.Context, reader *zip.Reader) ([]byte, map if size < 0 { return nil, nil, errors.New("backup archive entry size overflow") } + if uint64(size) != member.UncompressedSize64 { + return nil, nil, errors.New("backup archive entry size does not match its archive metadata") + } entries[name] = inspectedArchiveEntry{ metadata: ArchiveEntryMetadata{Path: name, Size: size, SHA256: digestForHash(hashValue), Mode: uint32(member.Mode().Perm())}, member: member, @@ -241,6 +372,118 @@ func inspectArchiveMembers(ctx context.Context, reader *zip.Reader) ([]byte, map return manifestBytes, entries, nil } +func checkArchiveMemberLimits(member *zip.File, limits PreflightLimits, total *uint64) error { + if member.UncompressedSize64 > limits.MaxUncompressedBytes || *total > limits.MaxUncompressedBytes-member.UncompressedSize64 { + return errors.New("backup archive exceeds the configured uncompressed-size limit") + } + *total += member.UncompressedSize64 + if member.CompressedSize64 == 0 { + if member.UncompressedSize64 != 0 { + return errors.New("backup archive exceeds the configured compression-ratio limit") + } + return nil + } + if limits.MaxCompressionRatio <= ^uint64(0)/member.CompressedSize64 && + member.UncompressedSize64 > limits.MaxCompressionRatio*member.CompressedSize64 { + return errors.New("backup archive exceeds the configured compression-ratio limit") + } + return nil +} + +func digestArchive(ctx context.Context, file *os.File, maximum uint64) (string, error) { + if _, err := file.Seek(0, io.SeekStart); err != nil { + return "", fmt.Errorf("seek backup archive: %w", err) + } + digest := sha256.New() + buffer := make([]byte, 128*1024) + var total uint64 + for { + if err := contextError(ctx); err != nil { + return "", err + } + count, err := file.Read(buffer) + if count > 0 { + if uint64(count) > maximum-total { + return "", errors.New("backup archive exceeds the configured archive-size limit") + } + total += uint64(count) + if _, writeErr := digest.Write(buffer[:count]); writeErr != nil { + return "", fmt.Errorf("hash backup archive: %w", writeErr) + } + } + if errors.Is(err, io.EOF) { + break + } + if err != nil { + return "", fmt.Errorf("read backup archive: %w", err) + } + } + if _, err := file.Seek(0, io.SeekStart); err != nil { + return "", fmt.Errorf("rewind backup archive: %w", err) + } + return digestForHash(digest), nil +} + +func zipMemberCount(file *os.File, size int64) (uint64, error) { + const ( + endOfCentralDirectorySignature = 0x06054b50 + zip64LocatorSignature = 0x07064b50 + zip64EndSignature = 0x06064b50 + endOfCentralDirectorySize = 22 + zipCommentMaximum = 1<<16 - 1 + zip64LocatorSize = 20 + zip64EndMinimumSize = 56 + ) + if size < endOfCentralDirectorySize { + return 0, errors.New("read backup archive directory: archive is too small") + } + tailSize := int64(endOfCentralDirectorySize + zipCommentMaximum) + if size < tailSize { + tailSize = size + } + tail := make([]byte, tailSize) + if _, err := file.ReadAt(tail, size-tailSize); err != nil { + return 0, fmt.Errorf("read backup archive directory: %w", err) + } + for index := len(tail) - endOfCentralDirectorySize; index >= 0; index-- { + if binary.LittleEndian.Uint32(tail[index:index+4]) != endOfCentralDirectorySignature { + continue + } + commentLength := int(binary.LittleEndian.Uint16(tail[index+20 : index+22])) + if index+endOfCentralDirectorySize+commentLength != len(tail) { + continue + } + count := uint64(binary.LittleEndian.Uint16(tail[index+10 : index+12])) + if count != 0xffff { + return count, nil + } + endOffset := size - tailSize + int64(index) + if endOffset < zip64LocatorSize { + return 0, errors.New("read backup archive directory: ZIP64 locator is missing") + } + locator := make([]byte, zip64LocatorSize) + if _, err := file.ReadAt(locator, endOffset-zip64LocatorSize); err != nil { + return 0, fmt.Errorf("read backup archive directory: %w", err) + } + if binary.LittleEndian.Uint32(locator[:4]) != zip64LocatorSignature { + return 0, errors.New("read backup archive directory: ZIP64 locator is invalid") + } + zip64Offset := binary.LittleEndian.Uint64(locator[8:16]) + if size < zip64EndMinimumSize || zip64Offset > uint64(size-zip64EndMinimumSize) { + return 0, errors.New("read backup archive directory: ZIP64 record is invalid") + } + zip64End := make([]byte, zip64EndMinimumSize) + if _, err := file.ReadAt(zip64End, int64(zip64Offset)); err != nil { + return 0, fmt.Errorf("read backup archive directory: %w", err) + } + if binary.LittleEndian.Uint32(zip64End[:4]) != zip64EndSignature || binary.LittleEndian.Uint64(zip64End[4:12]) < 44 { + return 0, errors.New("read backup archive directory: ZIP64 record is invalid") + } + return binary.LittleEndian.Uint64(zip64End[32:40]), nil + } + return 0, errors.New("read backup archive directory: end record is missing") +} + const maxPreflightManifestBytes = 16 << 20 func validateArchiveMemberPath(value string) (string, error) { @@ -277,6 +520,9 @@ func reconcileArchiveEntries(ctx context.Context, manifest Manifest, entries map if actual.metadata.Size != entry.Size || actual.metadata.SHA256 != entry.SHA256 { return nil, 0, fmt.Errorf("checksum or size mismatch for backup entry %q", entry.Path) } + if actual.metadata.Mode != entry.Mode { + return nil, 0, fmt.Errorf("mode mismatch for backup entry %q", entry.Path) + } if entry.Kind == EntryVolume { if err := validateVolumeTar(ctx, actual.member); err != nil { return nil, 0, fmt.Errorf("validate volume archive %q: %w", entry.Path, err) diff --git a/tools/tht/internal/backup/preflight_test.go b/tools/tht/internal/backup/preflight_test.go index 582ced74..b2672ea3 100644 --- a/tools/tht/internal/backup/preflight_test.go +++ b/tools/tht/internal/backup/preflight_test.go @@ -233,6 +233,91 @@ func TestPreflightRejectsTraversalAndSymlinkInsideVolumeTar(t *testing.T) { } } +func TestPreflightRejectsArchivesThatExceedConfiguredProcessingLimits(t *testing.T) { + installation := preflightTestInstallation(t) + tests := []struct { + name string + spec preflightArchiveSpec + limits PreflightLimits + wantErr string + }{ + { + name: "member count", + spec: preflightArchiveSpec{entries: []preflightArchiveEntry{{path: "one", body: []byte("one")}}}, + limits: PreflightLimits{MaxMembers: 1, MaxUncompressedBytes: 1 << 20, MaxCompressionRatio: 100}, + wantErr: "member limit", + }, + { + name: "uncompressed bytes", + spec: preflightArchiveSpec{entries: []preflightArchiveEntry{{path: "large", body: []byte("123456789")}}}, + limits: PreflightLimits{MaxMembers: 10, MaxUncompressedBytes: 1, MaxCompressionRatio: 100}, + wantErr: "uncompressed-size limit", + }, + { + name: "compression ratio", + spec: preflightArchiveSpec{entries: []preflightArchiveEntry{{ + path: "compressed", body: []byte(strings.Repeat("A", 4096)), method: zip.Deflate, + }}}, + limits: PreflightLimits{MaxMembers: 10, MaxUncompressedBytes: 1 << 20, MaxCompressionRatio: 2}, + wantErr: "compression-ratio limit", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + archive := filepath.Join(t.TempDir(), "limited.zip") + writePreflightArchive(t, archive, test.spec) + _, err := Preflight(context.Background(), installation, PreflightRequest{ + Archive: archive, Confirm: true, Limits: test.limits, + }, permissivePreflightDependencies()) + if err == nil || !strings.Contains(err.Error(), test.wantErr) { + t.Fatalf("Preflight() error = %v, want %q", err, test.wantErr) + } + }) + } +} + +func TestPreflightRevalidationRefusesAnArchivePathThatWasReplaced(t *testing.T) { + installation := preflightTestInstallation(t) + archive := filepath.Join(t.TempDir(), "checked.zip") + writePreflightArchive(t, archive, preflightArchiveSpec{ + entries: []preflightArchiveEntry{{path: "configuration/operator.env", body: []byte("validated")}}, + }) + + result, err := Preflight(context.Background(), installation, PreflightRequest{Archive: archive, Confirm: true}, permissivePreflightDependencies()) + if err != nil { + t.Fatal(err) + } + defer result.CloseArchive() + + replacement := filepath.Join(t.TempDir(), "replacement.zip") + writePreflightArchive(t, replacement, preflightArchiveSpec{ + entries: []preflightArchiveEntry{{path: "configuration/operator.env", body: []byte("unchecked")}}, + }) + if err := os.Rename(replacement, archive); err != nil { + t.Fatal(err) + } + if _, err := result.RevalidateArchive(); err == nil || !strings.Contains(err.Error(), "changed") { + t.Fatalf("RevalidateArchive() error = %v, want replaced path refusal", err) + } +} + +func TestPreflightRejectsArchiveEntryWithModeDifferentFromManifest(t *testing.T) { + installation := preflightTestInstallation(t) + archive := filepath.Join(t.TempDir(), "mode-mismatch.zip") + manifestMode := uint32(0o600) + writePreflightArchive(t, archive, preflightArchiveSpec{ + entries: []preflightArchiveEntry{{ + path: "configuration/operator.env", body: []byte("safe"), mode: 0o644, manifestMode: &manifestMode, + }}, + }) + + _, err := Preflight(context.Background(), installation, PreflightRequest{Archive: archive, Confirm: true}, permissivePreflightDependencies()) + if err == nil || !strings.Contains(err.Error(), "mode mismatch") { + t.Fatalf("Preflight() error = %v, want mode mismatch", err) + } +} + type preflightArchiveSpec struct { installationID string schemaVersion int @@ -243,12 +328,15 @@ type preflightArchiveSpec struct { } type preflightArchiveEntry struct { - path string - body []byte - checksum string - kind string - sensitive bool - symlink bool + path string + body []byte + checksum string + kind string + sensitive bool + symlink bool + mode os.FileMode + manifestMode *uint32 + method uint16 } type preflightRawArchiveEntry struct { @@ -302,7 +390,14 @@ func writePreflightArchive(t *testing.T, archivePath string, spec preflightArchi sourcePath = "/protected/secret" owner = "external-secret" } - manifest.Entries = append(manifest.Entries, Entry{Path: entry.path, Kind: kind, Owner: owner, SourcePath: sourcePath, SHA256: checksum, Size: int64(len(entry.body)), Archived: true, Sensitive: entry.sensitive}) + mode := uint32(entry.mode.Perm()) + if mode == 0 { + mode = 0o600 + } + if entry.manifestMode != nil { + mode = *entry.manifestMode + } + manifest.Entries = append(manifest.Entries, Entry{Path: entry.path, Kind: kind, Owner: owner, SourcePath: sourcePath, SHA256: checksum, Size: int64(len(entry.body)), Mode: mode, Archived: true, Sensitive: entry.sensitive}) } manifestBytes, err := manifest.JSON() if err != nil { @@ -322,7 +417,16 @@ func writePreflightArchive(t *testing.T, archivePath string, spec preflightArchi defer file.Close() writer := zip.NewWriter(file) for _, entry := range spec.entries { - header := &zip.FileHeader{Name: entry.path, Method: zip.Store} + method := entry.method + if method == 0 { + method = zip.Store + } + header := &zip.FileHeader{Name: entry.path, Method: method} + mode := entry.mode + if mode == 0 { + mode = 0o600 + } + header.SetMode(mode) if entry.symlink { header.SetMode(os.ModeSymlink | 0o777) } @@ -343,7 +447,9 @@ func writePreflightArchive(t *testing.T, archivePath string, spec preflightArchi t.Fatal(err) } } - created, err := writer.Create(ManifestPath) + manifestHeader := &zip.FileHeader{Name: ManifestPath, Method: zip.Store} + manifestHeader.SetMode(0o600) + created, err := writer.CreateHeader(manifestHeader) if err != nil { t.Fatal(err) }