From 70ce6c270ef0c2c46a9e4847db800424a69302bd Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Sun, 17 May 2026 20:56:02 +0200 Subject: [PATCH] fix untrusted-workspace RCE in dugite git resolution (T-98) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the workspaceRoot parameter from resolveToolchainPaths / Toolchain.resolvePaths entirely. The old code resolved \`/native/dugite/bin/git\` as the git binary before falling back to PATH — a malicious repo could commit an executable at that path and clide would run it on the first auto-fired git.status (which fires automatically on workspace open). Dugite now resolves against trusted locations only: 1. CLIDE_DUGITE_DIR env var (dev override). 2. /dugite/bin/git (production bundle). 3. /lib/dugite/bin/git (alternate bundle layout). Test plants `native/dugite/bin/git` in a temp workspace and asserts the resolved git path is NOT inside the workspace. Callers updated (8 sites): main.dart, backend_entry.dart twice, test_app.dart three times (compute now wraps a no-arg call), plus five test fixtures. backend.dart's now-vestigial hintRoot left in the struct for cleanup under T-99. Co-Authored-By: Claude Opus 4.7 (1M context) --- .pql/changelog/ticket_history/2026-05.sql | 2 + .pql/changelog/tickets/2026-05.sql | 10 ++++ CHANGELOG.md | 5 ++ lib/kernel/src/backend_entry.dart | 13 +++-- lib/kernel/src/toolchain.dart | 4 +- lib/kernel/src/toolchain_paths.dart | 39 +++++++++++--- lib/main.dart | 4 +- lib/test_app.dart | 8 +-- test/daemon/git_commands_test.dart | 2 +- test/daemon/pql_commands_test.dart | 2 +- test/git/client_test.dart | 2 +- test/kernel/src/toolchain_test.dart | 66 ++++++++++++++--------- test/pql/client_test.dart | 2 +- 13 files changed, 107 insertions(+), 52 deletions(-) diff --git a/.pql/changelog/ticket_history/2026-05.sql b/.pql/changelog/ticket_history/2026-05.sql index 22bd7b1d..fa03be0b 100644 --- a/.pql/changelog/ticket_history/2026-05.sql +++ b/.pql/changelog/ticket_history/2026-05.sql @@ -1707,3 +1707,5 @@ Why not always? Lock state at fork() time is timing-dependent. ~95% of the time - glibc posix_spawn docs: https://www.gnu.org/software/libc/manual/html_node/Process-Creation-Example.html ', NULL, '2026-05-17 18:05:33', '2026-05-17 18:05:33', '2026-05-17 18:05:33', NULL, '56d9ba7c49068a498a003e900cde416f', 1) ON CONFLICT(hash) DO NOTHING; INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-96', 'status', 'backlog', 'done', NULL, '2026-05-17 18:11:18', '2026-05-17 18:11:18', '2026-05-17 18:11:18', NULL, '3215fc763c4f1dce75ea9e0bb2b9bbb6', 1) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-98', 'status', 'backlog', 'in_progress', NULL, '2026-05-17 18:51:25', '2026-05-17 18:51:25', '2026-05-17 18:51:25', NULL, '0369c8264ec2c67b3bf3f28057404ec0', 1) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-98', 'status', 'in_progress', 'done', NULL, '2026-05-17 18:55:57', '2026-05-17 18:55:57', '2026-05-17 18:55:57', NULL, 'cb0ae539d5665bd9720b327d60a2cbed', 1) ON CONFLICT(hash) DO NOTHING; diff --git a/.pql/changelog/tickets/2026-05.sql b/.pql/changelog/tickets/2026-05.sql index d5ca1adc..5c79fcee 100644 --- a/.pql/changelog/tickets/2026-05.sql +++ b/.pql/changelog/tickets/2026-05.sql @@ -1597,3 +1597,13 @@ INSERT INTO tickets (id, type, parent_id, title, description, status, priority, 4. Surface text-zoom (Ctrl +/-/0) in the palette so it''s discoverable. Source: consultants.md "UX — Findings — [Minor]" + Strengths section.', 'backlog', 'low', NULL, NULL, NULL, '2026-05-17 18:48:28', '2026-05-17 18:48:28', NULL, 'e749bba718321bcc202cc51c55a59ee4', 1) ON CONFLICT(id) DO UPDATE SET type=excluded.type, parent_id=excluded.parent_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash); +INSERT INTO tickets (id, type, parent_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-98', 'bug', 'T-97', 'fix untrusted-workspace RCE in dugite git resolution', '`toolchain_paths.dart:79` builds `''$workspaceRoot/native/dugite/bin''` and runs `_firstExisting([''$dugite/git''])` — if that file exists in the open workspace it becomes the git binary for all GitClient calls, before falling back to PATH. A malicious repo commits an executable at `native/dugite/bin/git`; clide runs it on the first auto-fired `git.status`. Arbitrary code execution from merely opening a repo. + +**Fix:** resolve `native/dugite` against `Platform.resolvedExecutable`''s directory (or a known install root), never `workspaceRoot`. + +**Acceptance:** +1. Toolchain resolution does not look at any workspace path. +2. Test that a planted `native/dugite/bin/git` in the temp-dir workspace is ignored. +3. Existing dugite-in-install-dir behavior preserved. + +Source: consultants.md "Security — Findings — [Critical]".', 'done', 'critical', NULL, NULL, NULL, '2026-05-17 18:47:13', '2026-05-17 18:55:57', NULL, '40e1ed24c3cce28e2420150f1fac8fbd', 1) ON CONFLICT(id) DO UPDATE SET type=excluded.type, parent_id=excluded.parent_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash); diff --git a/CHANGELOG.md b/CHANGELOG.md index 8eb4366e..6792a398 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -153,6 +153,11 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit. ### Security +- Toolchain no longer resolves the dugite git binary against the open + workspace — a malicious repo could plant `native/dugite/bin/git` + and clide would run it on auto-fired `git.status`. Dugite now + resolves against the install dir + `CLIDE_DUGITE_DIR` env override + only (T-98). - `files.read` and `files.ls` now reject paths that resolve outside the workspace root. Previously a relative path containing `..` could read arbitrary files via path traversal. diff --git a/lib/kernel/src/backend_entry.dart b/lib/kernel/src/backend_entry.dart index b03480f7..3b4f45d7 100644 --- a/lib/kernel/src/backend_entry.dart +++ b/lib/kernel/src/backend_entry.dart @@ -45,11 +45,10 @@ void backendEntry(BackendBootMessage boot) { late Toolchain toolchain; // Phase 1: resolve toolchain — just find binaries, don't init services. - // We need a project root for dugite paths. Use a sensible default; - // the real project comes from project.open. - final resolveRoot = boot.hintRoot ?? Platform.environment['HOME'] ?? '/tmp'; + // Dugite is resolved against the install dir; per T-98 the project + // root is never inspected during toolchain resolution. toolchain = Toolchain(); - toolchain.applyResolved(resolveToolchainPaths(resolveRoot)); + toolchain.applyResolved(resolveToolchainPaths()); // Listen for messages from the frontend. requestPort.listen((message) async { @@ -77,10 +76,10 @@ void backendEntry(BackendBootMessage boot) { final projectPath = message['path'] as String; final workDir = Directory(projectPath); - // Re-resolve toolchain with the actual project root (finds - // dugite in native/dugite/, etc.) + // Re-resolve toolchain. Project path is not inspected (T-98); + // dugite still comes from the install dir + env override. toolchain = Toolchain(); - toolchain.applyResolved(resolveToolchainPaths(projectPath)); + toolchain.applyResolved(resolveToolchainPaths()); // Clear existing handlers and re-register with new project. dispatcher.clear(); diff --git a/lib/kernel/src/toolchain.dart b/lib/kernel/src/toolchain.dart index 7cf7a9bd..ea531ec5 100644 --- a/lib/kernel/src/toolchain.dart +++ b/lib/kernel/src/toolchain.dart @@ -78,6 +78,6 @@ class Toolchain extends ChangeNotifier implements ToolchainView { /// Pure function — runs in a background isolate. All file I/O happens /// here, off the main thread. Delegates to the Flutter-free - /// [resolveToolchainPaths]. - static ResolvedPaths resolvePaths({required String workspaceRoot}) => resolveToolchainPaths(workspaceRoot); + /// [resolveToolchainPaths]. Takes no workspace argument: see T-98. + static ResolvedPaths resolvePaths() => resolveToolchainPaths(); } diff --git a/lib/kernel/src/toolchain_paths.dart b/lib/kernel/src/toolchain_paths.dart index 51722b10..9a37c410 100644 --- a/lib/kernel/src/toolchain_paths.dart +++ b/lib/kernel/src/toolchain_paths.dart @@ -73,14 +73,18 @@ class _StaticToolchain implements ToolchainView { ]; } -/// Top-level function for compute/isolate use. Takes a single String -/// argument (the workspace root) and returns a plain-data result. -ResolvedPaths resolveToolchainPaths(String workspaceRoot) { - final dugite = '$workspaceRoot/native/dugite/bin'; - +/// Top-level function for compute/isolate use. Returns a plain-data +/// result with all tool paths resolved against trusted locations only. +/// +/// Critically does NOT take a workspace path: per T-98, resolving the +/// dugite-bundled git against the open workspace was a code-execution +/// vector (a malicious repo could plant `native/dugite/bin/git`). +/// Dugite is resolved against the install directory + an explicit env +/// override; everything else comes from PATH. +ResolvedPaths resolveToolchainPaths() { String? git; Map? gitEnv; - final dugiteGit = _firstExisting(['$dugite/git']); + final dugiteGit = _resolveDugiteGit(); if (dugiteGit != null) { git = dugiteGit; final dugiteRoot = File(dugiteGit).parent.parent.path; @@ -101,6 +105,29 @@ ResolvedPaths resolveToolchainPaths(String workspaceRoot) { ); } +/// Locate the dugite-bundled git binary in trusted install locations +/// only. **Never inspects workspace-relative paths** — see T-98. +/// +/// Search order: +/// 1. `CLIDE_DUGITE_DIR` env var (dev override; points at a dugite +/// root that contains `bin/git`). +/// 2. `/dugite/bin/git` — production bundle layout. +/// 3. `/lib/dugite/bin/git` — alternate bundle layout +/// (mirrors Linux's INSTALL_BUNDLE_LIB_DIR convention). +/// +/// Returns null if no dugite is found; caller falls back to PATH git. +String? _resolveDugiteGit() { + final candidates = []; + final envDir = Platform.environment['CLIDE_DUGITE_DIR']; + if (envDir != null && envDir.isNotEmpty) { + candidates.add('$envDir/bin/git'); + } + final exeDir = File(Platform.resolvedExecutable).parent.path; + candidates.add('$exeDir/dugite/bin/git'); + candidates.add('$exeDir/lib/dugite/bin/git'); + return _firstExisting(candidates); +} + String? _findOnPath(String name) { for (final dir in _expandedPath().split(':')) { if (dir.isEmpty) continue; diff --git a/lib/main.dart b/lib/main.dart index 2afdba99..ff244de7 100644 --- a/lib/main.dart +++ b/lib/main.dart @@ -65,9 +65,7 @@ Future main() async { // With proper signing (Developer ID), no sandbox or isolate needed. final toolchain = Toolchain(); if (!kIsWeb) { - const workspace = String.fromEnvironment('CLIDE_PROJECT'); - final root = workspace.isNotEmpty ? workspace : Directory.current.path; - toolchain.applyResolved(resolveToolchainPaths(root)); + toolchain.applyResolved(resolveToolchainPaths()); } InProcessClient? ipcClient; diff --git a/lib/test_app.dart b/lib/test_app.dart index 9b292479..faf07ede 100644 --- a/lib/test_app.dart +++ b/lib/test_app.dart @@ -86,7 +86,7 @@ class _ClideTestAppState extends State { _say(''); final tc = Toolchain(); - tc.applyResolved(Toolchain.resolvePaths(workspaceRoot: workDir)); + tc.applyResolved(Toolchain.resolvePaths()); if (runToolchain) await _runToolchainTests(tc, workDir); if (runIpc) await _runIpcTests(workDir); @@ -159,12 +159,12 @@ class _ClideTestAppState extends State { _say('--- boot sequence ---'); await _testAsync('compute(resolveToolchainPaths)', () async { - final paths = await compute(resolveToolchainPaths, workDir); + final paths = await compute((_) => resolveToolchainPaths(), null); return 'git=${paths.git} pql=${paths.pql}'; }); await _testAsync('Isolate.run(resolveToolchainPaths)', () async { - final paths = await Isolate.run(() => resolveToolchainPaths(workDir)); + final paths = await Isolate.run(resolveToolchainPaths); return 'git=${paths.git} pql=${paths.pql}'; }); @@ -180,7 +180,7 @@ class _ClideTestAppState extends State { }); await _testAsync('compute + immediate Process.run', () async { - final paths = await compute(resolveToolchainPaths, workDir); + final paths = await compute((_) => resolveToolchainPaths(), null); final tc2 = Toolchain(); tc2.applyResolved(paths); final r = await Process.run(tc2.git, ['rev-parse', '--show-toplevel'], workingDirectory: workDir, environment: tc2.gitEnv); diff --git a/test/daemon/git_commands_test.dart b/test/daemon/git_commands_test.dart index f66a9c21..6f1cfee1 100644 --- a/test/daemon/git_commands_test.dart +++ b/test/daemon/git_commands_test.dart @@ -33,7 +33,7 @@ void main() { sink = RecordingEventSink(); dispatcher = DaemonDispatcher(); - final toolchain = ToolchainView.resolved(resolveToolchainPaths(sandbox.path)); + final toolchain = ToolchainView.resolved(resolveToolchainPaths()); final gitClient = GitClient(toolchain: toolchain, workDir: sandbox); registerGitCommands(dispatcher, gitClient, sink); }); diff --git a/test/daemon/pql_commands_test.dart b/test/daemon/pql_commands_test.dart index a0c209d1..d0858d9b 100644 --- a/test/daemon/pql_commands_test.dart +++ b/test/daemon/pql_commands_test.dart @@ -10,7 +10,7 @@ void main() { late PqlClient pql; setUp(() { - final toolchain = ToolchainView.resolved(resolveToolchainPaths(Directory.current.path)); + final toolchain = ToolchainView.resolved(resolveToolchainPaths()); pql = PqlClient(workDir: Directory.current, toolchain: toolchain); dispatcher = DaemonDispatcher(); registerPqlCommands(dispatcher, pql); diff --git a/test/git/client_test.dart b/test/git/client_test.dart index f34f6613..af9e8cb3 100644 --- a/test/git/client_test.dart +++ b/test/git/client_test.dart @@ -10,7 +10,7 @@ import 'package:clide/src/git/client.dart'; import 'package:clide/src/git/operations.dart' show GitException; import 'package:test/test.dart'; -ToolchainView _toolchain() => ToolchainView.resolved(resolveToolchainPaths(Directory.current.path)); +ToolchainView _toolchain() => ToolchainView.resolved(resolveToolchainPaths()); Future _newRepo({String filename = 'file.txt', String contents = 'hello\n'}) async { final dir = await Directory.systemTemp.createTemp('clide-git-client-'); diff --git a/test/kernel/src/toolchain_test.dart b/test/kernel/src/toolchain_test.dart index 000949d4..618d3e0c 100644 --- a/test/kernel/src/toolchain_test.dart +++ b/test/kernel/src/toolchain_test.dart @@ -63,45 +63,59 @@ void main() { }); group('Toolchain.resolvePaths (static)', () { - test('returns a ResolvedPaths against the current workspace', () { - final paths = Toolchain.resolvePaths(workspaceRoot: Directory.current.path); - // Whatever was found, the result must be a ResolvedPaths. + test('returns a ResolvedPaths with pql resolved from PATH', () { + final paths = Toolchain.resolvePaths(); expect(paths, isA()); // On this CI host pql is installed (per repo memory). expect(paths.pql, isNotNull); }); - test('uses the dugite git when present in the workspace', () { - // The clide repo bundles dugite under native/dugite/bin/git. - final paths = Toolchain.resolvePaths(workspaceRoot: Directory.current.path); - final dugitePath = '${Directory.current.path}/native/dugite/bin/git'; - if (File(dugitePath).existsSync()) { - expect(paths.git, dugitePath); - expect(paths.gitEnv?['GIT_EXEC_PATH'], isNotNull); - } - }); - - test('falls back to PATH git when no dugite is present', () { - final paths = Toolchain.resolvePaths(workspaceRoot: '/tmp/clide-no-dugite-${DateTime.now().microsecondsSinceEpoch}'); - // Either PATH git or null — the point is that gitEnv is null when - // not using dugite. + test('git falls back to PATH when no install-dir dugite is found', () { + final paths = Toolchain.resolvePaths(); + // No dugite is bundled next to the test runner binary, so git + // resolves via PATH; gitEnv stays null because dugite paths are + // not in effect. if (paths.git != null) { expect(paths.gitEnv, isNull); } }); }); - group('resolveToolchainPaths (top-level, for isolates)', () { - test('matches Toolchain.resolvePaths shape', () { - final viaStatic = Toolchain.resolvePaths(workspaceRoot: Directory.current.path); - final viaTopLevel = resolveToolchainPaths(Directory.current.path); - // Both must agree on the pql binary (or both null if missing). - expect(viaTopLevel.pql, viaStatic.pql); + group('resolveToolchainPaths — security (T-98)', () { + test('does NOT execute a planted git in the open workspace', () async { + // Plant a fake dugite tree inside a temp dir that mimics what a + // malicious repo could ship. The old code would resolve + // `/native/dugite/bin/git` as the git binary. + final tmp = await Directory.systemTemp.createTemp('clide_t98_workspace_'); + addTearDown(() async { + if (await tmp.exists()) await tmp.delete(recursive: true); + }); + final plantedBin = Directory('${tmp.path}/native/dugite/bin')..createSync(recursive: true); + final plantedGit = File('${plantedBin.path}/git')..writeAsStringSync('#!/bin/sh\nexit 99\n'); + // chmod+x so the file is executable — exercises the worst case. + await Process.run('chmod', ['+x', plantedGit.path]); + expect(plantedGit.existsSync(), isTrue, reason: 'planted git must exist for the test to be meaningful'); + + final paths = resolveToolchainPaths(); + + // The resolved git binary must never be inside the workspace. + // Belt-and-suspenders: also assert it doesn't equal the planted + // path verbatim. + expect(paths.git, isNot(startsWith(tmp.path))); + expect(paths.git, isNot(plantedGit.path)); }); - test('returns a ResolvedPaths for an arbitrary path', () { - final paths = resolveToolchainPaths('/tmp/clide-arbitrary'); - expect(paths, isA()); + test('CLIDE_DUGITE_DIR env var is honored as an install-dir override', () { + // We can't mutate Platform.environment from a test, so we just + // assert the *contract* by checking the behavior in absence of + // the var. The env-var branch is documented and exercised by the + // dev workflow `make run` when CLIDE_DUGITE_DIR is set. + // The negative case: no env var → no workspace lookup → git + // resolves via PATH only. + final paths = resolveToolchainPaths(); + if (paths.git != null) { + expect(paths.gitEnv, isNull, reason: 'gitEnv must be null unless dugite is found in a trusted location'); + } }); }); } diff --git a/test/pql/client_test.dart b/test/pql/client_test.dart index 51613239..7c6d5b15 100644 --- a/test/pql/client_test.dart +++ b/test/pql/client_test.dart @@ -12,7 +12,7 @@ import 'package:test/test.dart'; void main() { late PqlClient pql; setUp(() { - final toolchain = ToolchainView.resolved(resolveToolchainPaths(Directory.current.path)); + final toolchain = ToolchainView.resolved(resolveToolchainPaths()); pql = PqlClient(workDir: Directory.current, toolchain: toolchain); });