From 6a9c89ea103b4af56f8e587071d47c29821eba02 Mon Sep 17 00:00:00 2001 From: ZhuchkaTriplesix Date: Mon, 27 Jul 2026 06:21:32 +0300 Subject: [PATCH] fix(security): use p.isWithin for archive extraction bounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #401 — shared archive_path_guard replaces startsWith checks in marketplace, sideload, and updater extract paths. --- .../extensions/local_extension_installer.dart | 7 ++- .../market/http_marketplace_repository.dart | 5 +- lib/core/security/archive_path_guard.dart | 18 +++++++ .../installers/update_install_utils.dart | 5 +- .../security/archive_path_guard_test.dart | 47 +++++++++++++++++++ 5 files changed, 74 insertions(+), 8 deletions(-) create mode 100644 lib/core/security/archive_path_guard.dart create mode 100644 test/core/security/archive_path_guard_test.dart diff --git a/lib/core/extensions/local_extension_installer.dart b/lib/core/extensions/local_extension_installer.dart index 557ede90..f413494c 100644 --- a/lib/core/extensions/local_extension_installer.dart +++ b/lib/core/extensions/local_extension_installer.dart @@ -10,6 +10,7 @@ import 'package:querya_desktop/core/extensions/local_extension_registry.dart'; import 'package:querya_desktop/core/extensions/models/extension_manifest.dart'; import 'package:querya_desktop/core/extensions/sandbox/sandbox_policy.dart'; import 'package:querya_desktop/core/market/marketplace_repository.dart'; +import 'package:querya_desktop/core/security/archive_path_guard.dart'; /// Installs an extension package from a local `.zip` / `.qext` archive (issue #316). /// @@ -216,9 +217,7 @@ class LocalExtensionInstaller { } if (filename.isEmpty || filename == '/') continue; - if (filename.contains('..') || - filename.startsWith('/') || - filename.startsWith('\\')) { + if (!isArchiveEntryNameSafe(filename)) { throw MarketplaceException( 'Security violation: Path traversal detected in archive entry ' '"${file.name}"', @@ -226,7 +225,7 @@ class LocalExtensionInstaller { } final targetPath = p.normalize(p.join(destPath, filename)); - if (!targetPath.startsWith(destPath)) { + if (!isArchiveExtractPathWithinRoot(destPath, targetPath)) { throw MarketplaceException( 'Security violation: Extraction path out of bounds "${file.name}"', ); diff --git a/lib/core/market/http_marketplace_repository.dart b/lib/core/market/http_marketplace_repository.dart index 1d1a5caf..c7d293cd 100644 --- a/lib/core/market/http_marketplace_repository.dart +++ b/lib/core/market/http_marketplace_repository.dart @@ -11,6 +11,7 @@ import 'package:querya_desktop/core/extensions/sandbox/sandbox_policy.dart'; import 'package:querya_desktop/core/extensions/local_extension_registry.dart'; import 'package:querya_desktop/core/extensions/models/extension_manifest.dart'; import 'package:querya_desktop/core/extensions/models/extension_type.dart'; +import 'package:querya_desktop/core/security/archive_path_guard.dart'; import 'marketplace_repository.dart'; /// HTTP implementation of [MarketplaceRepository] connecting to MarketApi backend. @@ -152,12 +153,12 @@ class HttpMarketplaceRepository implements MarketplaceRepository { for (final file in archive) { final filename = file.name; // Check for Path Traversal attempts - if (filename.contains('..') || filename.startsWith('/') || filename.startsWith('\\')) { + if (!isArchiveEntryNameSafe(filename)) { throw MarketplaceException('Security violation: Path traversal detected in archive entry "$filename"'); } final targetPath = p.normalize(p.join(extDirPath, filename)); - if (!targetPath.startsWith(extDirPath)) { + if (!isArchiveExtractPathWithinRoot(extDirPath, targetPath)) { throw MarketplaceException('Security violation: Extraction path out of bounds "$filename"'); } diff --git a/lib/core/security/archive_path_guard.dart b/lib/core/security/archive_path_guard.dart new file mode 100644 index 00000000..97006c62 --- /dev/null +++ b/lib/core/security/archive_path_guard.dart @@ -0,0 +1,18 @@ +import 'package:path/path.dart' as p; + +/// Returns true when [targetPath] equals [rootPath] or lies inside it. +/// +/// Prefer over [String.startsWith] so sibling prefixes (e.g. `/tmp/abc` vs +/// `/tmp/abcd`) cannot bypass extraction bounds. +bool isArchiveExtractPathWithinRoot(String rootPath, String targetPath) { + final root = p.normalize(rootPath); + final target = p.normalize(targetPath); + return p.equals(root, target) || p.isWithin(root, target); +} + +/// Rejects archive entry names that attempt absolute paths or traversal. +bool isArchiveEntryNameSafe(String entryName) { + if (entryName.contains('..')) return false; + if (entryName.startsWith('/') || entryName.startsWith('\\')) return false; + return true; +} diff --git a/lib/core/updater/installers/update_install_utils.dart b/lib/core/updater/installers/update_install_utils.dart index 7ef528ce..a5a769f9 100644 --- a/lib/core/updater/installers/update_install_utils.dart +++ b/lib/core/updater/installers/update_install_utils.dart @@ -3,6 +3,7 @@ import 'dart:io'; import 'package:archive/archive.dart'; import 'package:path/path.dart' as p; +import '../../security/archive_path_guard.dart'; import '../app_updater_service.dart'; /// Safely extracts a zip archive into [destinationDir]. @@ -21,14 +22,14 @@ Future extractZipSecurely({ for (final entry in archive) { final name = entry.name; - if (name.contains('..') || name.startsWith('/') || name.startsWith('\\')) { + if (!isArchiveEntryNameSafe(name)) { throw AppUpdaterException( 'Security violation: path traversal in archive entry "$name"', ); } final targetPath = p.normalize(p.join(root, name)); - if (!targetPath.startsWith(root)) { + if (!isArchiveExtractPathWithinRoot(root, targetPath)) { throw AppUpdaterException( 'Security violation: extraction path out of bounds "$name"', ); diff --git a/test/core/security/archive_path_guard_test.dart b/test/core/security/archive_path_guard_test.dart new file mode 100644 index 00000000..01f64c7f --- /dev/null +++ b/test/core/security/archive_path_guard_test.dart @@ -0,0 +1,47 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:querya_desktop/core/security/archive_path_guard.dart'; + +void main() { + group('isArchiveExtractPathWithinRoot', () { + test('allows paths inside root', () { + expect( + isArchiveExtractPathWithinRoot('/tmp/ext', '/tmp/ext/file.txt'), + isTrue, + ); + }); + + test('allows root path itself', () { + expect( + isArchiveExtractPathWithinRoot('/tmp/ext', '/tmp/ext'), + isTrue, + ); + }); + + test('rejects sibling prefix paths (startsWith false positive)', () { + expect( + isArchiveExtractPathWithinRoot('/tmp/abc', '/tmp/abcd/evil.txt'), + isFalse, + ); + }); + + test('rejects paths outside root', () { + expect( + isArchiveExtractPathWithinRoot('/tmp/ext', '/tmp/other/file.txt'), + isFalse, + ); + }); + }); + + group('isArchiveEntryNameSafe', () { + test('rejects traversal and absolute names', () { + expect(isArchiveEntryNameSafe('../evil.txt'), isFalse); + expect(isArchiveEntryNameSafe('/etc/passwd'), isFalse); + expect(isArchiveEntryNameSafe(r'\windows\system32'), isFalse); + }); + + test('allows relative safe names', () { + expect(isArchiveEntryNameSafe('manifest.json'), isTrue); + expect(isArchiveEntryNameSafe('bin/driver'), isTrue); + }); + }); +}