diff --git a/docs/superpowers/plans/2026-08-10-backup-zip-slip.md b/docs/superpowers/plans/2026-08-10-backup-zip-slip.md new file mode 100644 index 0000000..ee175e2 --- /dev/null +++ b/docs/superpowers/plans/2026-08-10-backup-zip-slip.md @@ -0,0 +1,128 @@ +# Backup ZIP Path Traversal Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Ensure backup restoration cannot write ZIP entries outside its restore directory. + +**Architecture:** Add a small path-validation helper in `task.dart`. Validate the whole decoded archive before opening an output stream, then extract only the returned safe paths. Regression tests exercise the public `restoreBackupArchive` function using real ZIP bytes and temporary files. + +**Tech Stack:** Dart, Flutter test, `archive`, `path`. + +## Global Constraints + +- Keep valid relative backup entries compatible. +- Reject unsafe entries before any archive content is written. +- Preserve input and output stream cleanup on both success and failure. +- Run the focused Dart test before the full Flutter suite and complete CI before merging. + +--- + +### Task 1: Prove unsafe ZIP entries escape today + +**Files:** + +- Modify: `test/common/task_test.dart` +- Test: `test/common/task_test.dart` + +**Interfaces:** + +- Consumes: `Future restoreBackupArchive(String backupFilePath, String restoreDirPath)`. +- Produces: regression examples for unsafe and valid ZIP paths. + +- [ ] **Step 1: Write failing tests** + +Add a test that creates an archive containing `safe.txt` followed by +`../escaped.txt`, calls `restoreBackupArchive`, expects a `FileSystemException`, +and asserts neither `restore/safe.txt` nor the sibling `escaped.txt` exists. +Add a second test with `profiles/nested.yaml` and assert its exact contents are +written beneath the restore directory. + +- [ ] **Step 2: Run the focused test to verify the safety case fails** + +Run: + +```bash +/Volumes/SING_02/flclash/flutter-sdk-3.44.8/bin/flutter test test/common/task_test.dart --reporter expanded +``` + +Expected: the unsafe-path assertion fails because the current implementation +writes the safe entry before it reaches `../escaped.txt`. + +### Task 2: Validate all archive paths before extraction + +**Files:** + +- Modify: `lib/common/task.dart` +- Test: `test/common/task_test.dart` + +**Interfaces:** + +- Produces: `String resolveRestoreArchiveEntryPath(String restoreDirPath, String entryName)`. +- Consumes: decoded `ArchiveFile.name` values in `restoreBackupArchive`. + +- [ ] **Step 1: Implement the minimal validator** + +Add a helper that rejects empty names, absolute POSIX names, backslashes, +Windows drive prefixes, `.` and `..` traversal, and any resolved path that is +not strictly inside `absolute(restoreDirPath)`. It throws +`FileSystemException('Invalid backup archive entry', entryName)` for rejection. + +- [ ] **Step 2: Validate before writing** + +Map every decoded archive file to its validated output path before calling +`Directory(restoreDirPath).create` or constructing an `OutputFileStream`. +Extract only after this mapping succeeds for the whole archive. + +- [ ] **Step 3: Run focused tests** + +Run: + +```bash +/Volumes/SING_02/flclash/flutter-sdk-3.44.8/bin/flutter test test/common/task_test.dart --reporter expanded +``` + +Expected: all backup restore, stream-release, and existing task tests pass. + +- [ ] **Step 4: Commit** + +```bash +git add lib/common/task.dart test/common/task_test.dart +git commit -m "fix: reject path traversal in backup restore" +``` + +### Task 3: Verify the complete client suite + +**Files:** + +- Verify only: `lib/common/task.dart`, `test/common/task_test.dart` + +- [ ] **Step 1: Format and analyze changed files** + +Run: + +```bash +/Volumes/SING_02/flclash/flutter-sdk-3.44.8/bin/dart format lib/common/task.dart test/common/task_test.dart +/Volumes/SING_02/flclash/flutter-sdk-3.44.8/bin/flutter analyze lib/common/task.dart test/common/task_test.dart +``` + +Expected: no formatting changes and no analysis issues. + +- [ ] **Step 2: Run full tests** + +Run: + +```bash +/Volumes/SING_02/flclash/flutter-sdk-3.44.8/bin/flutter test --reporter compact +``` + +Expected: the full suite passes. + +- [ ] **Step 3: Review diff safety** + +Run: + +```bash +git diff --check origin/main...HEAD +``` + +Expected: no whitespace or conflict-marker errors. diff --git a/docs/superpowers/specs/2026-08-10-backup-zip-slip-design.md b/docs/superpowers/specs/2026-08-10-backup-zip-slip-design.md new file mode 100644 index 0000000..09efdcb --- /dev/null +++ b/docs/superpowers/specs/2026-08-10-backup-zip-slip-design.md @@ -0,0 +1,41 @@ +# Backup ZIP Path Traversal Design + +## Goal + +Prevent a backup archive from creating or overwriting files outside FlClash's +temporary restore directory, while preserving restoration of valid backups. + +## Decision + +Validate every archive entry before creating the restore directory or opening +an output stream. An entry is rejected when its ZIP name is empty, absolute, +uses a Windows drive prefix or backslash separator, resolves to the restore +directory itself, or resolves outside that directory after normalization. + +The validator returns the final absolute output path only for a safe relative +file. `restoreBackupArchive` first validates the complete archive, then writes +the already-validated entries. A single unsafe entry therefore fails the whole +restore before any archive entry is written. + +## Error Handling + +Unsafe archive names raise `FileSystemException` naming the invalid entry. The +input ZIP stream is still closed in `finally`; callers retain their existing +restore-directory cleanup. Ordinary write failures retain the existing error +behaviour and also close each output stream. + +## Compatibility + +Normal relative entries such as `database.sqlite`, `config.json`, +`profiles/123.yaml`, and `scripts/456.js` remain valid. No backup format, +database migration, UI, or platform-specific code changes. + +## Verification + +Regression tests create ZIP archives in a temporary directory and prove that: + +1. a `../escaped.txt` entry fails without creating the sibling file; +2. an absolute entry fails without writing any valid entry that appears before + it in the archive; +3. a nested valid entry restores with its original content; and +4. the existing stream-release test continues to pass. diff --git a/lib/common/task.dart b/lib/common/task.dart index bb737fd..5ae6cbf 100644 --- a/lib/common/task.dart +++ b/lib/common/task.dart @@ -641,6 +641,32 @@ Future restoreTask() async { ); } +String resolveRestoreArchiveEntryPath(String restoreDirPath, String entryName) { + final invalidEntry = FileSystemException( + 'Invalid backup archive entry', + entryName, + ); + if (entryName.isEmpty || + posix.isAbsolute(entryName) || + entryName.contains('\\') || + RegExp(r'^[a-zA-Z]:').hasMatch(entryName) || + posix + .split(entryName) + .any((component) => component == '.' || component == '..')) { + throw invalidEntry; + } + + final restoreDirAbsolutePath = absolute(restoreDirPath); + final outputPath = absolute(join(restoreDirAbsolutePath, entryName)); + final restoreDirPrefix = restoreDirAbsolutePath.endsWith(separator) + ? restoreDirAbsolutePath + : '$restoreDirAbsolutePath$separator'; + if (!outputPath.startsWith(restoreDirPrefix)) { + throw invalidEntry; + } + return outputPath; +} + Future restoreBackupArchive( String backupFilePath, String restoreDirPath, @@ -648,13 +674,19 @@ Future restoreBackupArchive( final input = InputFileStream(backupFilePath); try { final archive = ZipDecoder().decodeStream(input); + final outputPaths = [ + for (final file in archive.files) + ( + file: file, + path: resolveRestoreArchiveEntryPath(restoreDirPath, file.name), + ), + ]; final restoreDir = Directory(restoreDirPath); await restoreDir.create(recursive: true); - for (final file in archive.files) { - final outPath = join(restoreDirPath, posix.normalize(file.name)); - final outputStream = OutputFileStream(outPath); + for (final output in outputPaths) { + final outputStream = OutputFileStream(output.path); try { - file.writeContent(outputStream); + output.file.writeContent(outputStream); } finally { await outputStream.close(); } diff --git a/test/common/task_test.dart b/test/common/task_test.dart index 7101286..6e6f488 100644 --- a/test/common/task_test.dart +++ b/test/common/task_test.dart @@ -47,6 +47,63 @@ void main() { expect(result.stdout, isNot(contains(backupFile.path))); }); + test('rejects a backup archive before an unsafe entry can escape', () async { + final directory = await Directory.systemTemp.createTemp( + 'fl_clash_zip_slip_test_', + ); + addTearDown(() => directory.delete(recursive: true)); + final backupFile = File(p.join(directory.path, 'backup.zip')); + final restoreDir = Directory(p.join(directory.path, 'restore')); + final archive = Archive() + ..addFile(ArchiveFile.string('safe.txt', 'safe backup content')) + ..addFile(ArchiveFile.string('../escaped.txt', 'escaped backup content')); + final zipBytes = ZipEncoder().encodeBytes(archive); + await backupFile.writeAsBytes(zipBytes); + + Object? restoreError; + try { + await restoreBackupArchive(backupFile.path, restoreDir.path); + } catch (error) { + restoreError = error; + } + + expect( + [ + File(p.join(restoreDir.path, 'safe.txt')).existsSync(), + File(p.join(directory.path, 'escaped.txt')).existsSync(), + ], + [false, false], + ); + expect(restoreError, isA()); + }); + + test( + 'restores a nested backup entry beneath the restore directory', + () async { + final directory = await Directory.systemTemp.createTemp( + 'fl_clash_restore_nested_test_', + ); + addTearDown(() => directory.delete(recursive: true)); + final backupFile = File(p.join(directory.path, 'backup.zip')); + final restoreDir = Directory(p.join(directory.path, 'restore')); + final archive = Archive() + ..addFile( + ArchiveFile.string('profiles/nested.yaml', 'profile: nested\n'), + ); + final zipBytes = ZipEncoder().encodeBytes(archive); + await backupFile.writeAsBytes(zipBytes); + + await restoreBackupArchive(backupFile.path, restoreDir.path); + + expect( + await File( + p.join(restoreDir.path, 'profiles', 'nested.yaml'), + ).readAsString(), + 'profile: nested\n', + ); + }, + ); + test('profile ipv6 value wins over the client fallback', () { final result = applyCorePatchConfig( rawConfig: {'ipv6': true, 'ip-version': 'ipv6-prefer'},