From 720cfd7c8ae0f197ffa37873fe962cbb7945edf3 Mon Sep 17 00:00:00 2001 From: ruv Date: Mon, 12 Oct 2026 11:56:13 -0400 Subject: [PATCH] fix(remote): replace explicit Windows ACL grants with current-user access --- harness/ruview/.harness/manifest.json | 4 +-- harness/ruview/.harness/manifest.sha256 | 2 +- harness/ruview/src/remote.js | 29 ++++++++++++++++--- harness/ruview/test/remote.test.mjs | 38 +++++++++++++++++++++---- 4 files changed, 61 insertions(+), 12 deletions(-) diff --git a/harness/ruview/.harness/manifest.json b/harness/ruview/.harness/manifest.json index 84d7c157a..590a08714 100644 --- a/harness/ruview/.harness/manifest.json +++ b/harness/ruview/.harness/manifest.json @@ -66,7 +66,7 @@ "src/policy.js": "baad5eb8c3eb2e416e0a71b342ad850d7c8413d7d976571ebf596937f69da8e2", "src/process-runner.js": "49533b038044dfb8bc76ed01c030d06a9856ead0836157fb693e2a7d40f786d6", "src/redact.js": "ebf1afff46341078706b0401838c53db043603586e280d51ece5cf1feba35189", - "src/remote.js": "ca510804176205eca799b396c0f0bca72df4c08139547c66483885d38fed6815", + "src/remote.js": "68f901411063fd515a52dad41ffd5c599765a53de34a080715232825eaf207b3", "src/repo-trust.js": "06e2a94d7113ed936f208a12b7fcc785801c215a3e2c5e7418f6238d991a289c", "src/sdk.d.ts": "d4c80689248c197b2ddb1ebf4cd7d8f2e55716722074d9e1cdb8029b2dfa19f7", "src/sdk.js": "1ac29fb389ed86e0f62ecfddbbd31614c58e832a8181495caa83fe5cf8b30bc4", @@ -75,7 +75,7 @@ "src/training.js": "bd8a6c12eba4f9d2f8990fa9692cd04c743ea247f9b13e10f8717ceefcbd09ac", "src/ui/console-widget.js": "0d2f707d57b5ec54968dee1dc80f31c7db9152dcadfc48700c18379dabb55462" }, - "filesDigest": "52d75ef1509cb993b96291fd28836baeb284a05a99ae1e7fac6a1356f9d80b7a", + "filesDigest": "f609508982d1fdc67fa7b90e1286e25945f920ba45e8fde95f76234848cf3be9", "brainDigest": "c0fb7b079ded157059b91601361429944697dae3cc42abc00dfe1a680986b0f4", "gateFingerprint": "6e53c784eee38310188948fc75fb49e6b4ebc04e247d01b903fa8c8a92d67bdd", "developmentPins": { diff --git a/harness/ruview/.harness/manifest.sha256 b/harness/ruview/.harness/manifest.sha256 index 5cc32a1c3..6050050c5 100644 --- a/harness/ruview/.harness/manifest.sha256 +++ b/harness/ruview/.harness/manifest.sha256 @@ -1 +1 @@ -c76cd7d8c4e204d8b8ba2fa9fdc787fcaa6adcea7fb6f0c62328ce87d01ab390 manifest.json +4c69daba76af8460fe5101da07aeab58cd84376ca642280e038f52cf3c4676d6 manifest.json diff --git a/harness/ruview/src/remote.js b/harness/ruview/src/remote.js index 703396478..db0b7b659 100644 --- a/harness/ruview/src/remote.js +++ b/harness/ruview/src/remote.js @@ -9,7 +9,7 @@ import { spawnSync } from 'node:child_process'; import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; -import { homedir, userInfo } from 'node:os'; +import { homedir } from 'node:os'; import { dirname, join } from 'node:path'; const NAME_RE = /^[a-z0-9][a-z0-9-]{0,31}$/; @@ -36,16 +36,37 @@ export function loadHosts(path = hostsFile()) { return { path, hosts: (data.hosts || []).map(validateHost) }; } +// A fixed program receives the path as JSON on stdin, never PowerShell source. +// Construct a fresh protected DACL: icacls /inheritance:r /grant:r leaves other +// explicit grants intact (including grants on hosted Windows temporary files). +const OWNER_ONLY_ACL = [ + "$ErrorActionPreference = 'Stop'", + '[Console]::InputEncoding = [System.Text.UTF8Encoding]::new($false)', + '$request = [Console]::In.ReadToEnd() | ConvertFrom-Json', + '$sid = [System.Security.Principal.WindowsIdentity]::GetCurrent().User', + '$acl = [System.Security.AccessControl.FileSecurity]::new()', + '$acl.SetOwner($sid)', + '$acl.SetAccessRuleProtection($true, $false)', + '$rule = [System.Security.AccessControl.FileSystemAccessRule]::new($sid, [System.Security.AccessControl.FileSystemRights]::FullControl, [System.Security.AccessControl.AccessControlType]::Allow)', + '$acl.AddAccessRule($rule)', + '[System.IO.File]::SetAccessControl([string]$request.path, $acl)', + '$applied = Get-Acl -LiteralPath $request.path', + '$rules = @($applied.GetAccessRules($true, $true, [System.Security.Principal.SecurityIdentifier]))', + 'if (-not $applied.AreAccessRulesProtected -or $rules.Count -ne 1 -or $rules[0].IdentityReference.Value -ne $sid.Value -or $rules[0].AccessControlType -ne [System.Security.AccessControl.AccessControlType]::Allow -or $rules[0].FileSystemRights -ne [System.Security.AccessControl.FileSystemRights]::FullControl) { throw "owner-only ACL verification failed" }', +].join('\n'); + /** * Windows ignores POSIX modes, so 0o600 alone leaves the file readable through * inherited ACLs. Replace them with a single full-control grant for the owner. * Fails closed: an unrestricted hosts file is an error, not a warning. */ -export function restrictToOwner(path, { platform = process.platform, exec = spawnSync, user = () => userInfo().username } = {}) { +export function restrictToOwner(path, { platform = process.platform, exec = spawnSync } = {}) { if (platform !== 'win32') return 'posix-0600'; - const r = exec('icacls', [path, '/inheritance:r', '/grant:r', `${user()}:F`], { encoding: 'utf8', windowsHide: true }); + const r = exec('powershell.exe', ['-NoLogo', '-NoProfile', '-NonInteractive', '-Command', OWNER_ONLY_ACL], { + input: JSON.stringify({ path }), encoding: 'utf8', windowsHide: true, timeout: 10_000, maxBuffer: 16_384, + }); if (r.error || r.status !== 0) { - throw new Error(`could not restrict ${path} to the current user (icacls exit ${r.status ?? r.error?.code}); the hosts file was not made private`); + throw new Error(`could not restrict ${path} to the current user (ACL helper exit ${r.status ?? r.error?.code}); the hosts file was not made private`); } return 'windows-acl-owner-only'; } diff --git a/harness/ruview/test/remote.test.mjs b/harness/ruview/test/remote.test.mjs index 0eb08f466..bfe8f1fb8 100644 --- a/harness/ruview/test/remote.test.mjs +++ b/harness/ruview/test/remote.test.mjs @@ -2,7 +2,7 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { existsSync, mkdtempSync, readFileSync, rmSync, statSync } from 'node:fs'; import { spawnSync } from 'node:child_process'; -import { tmpdir } from 'node:os'; +import { tmpdir, userInfo } from 'node:os'; import { dirname, join, resolve } from 'node:path'; import { buildSshArgs, restrictToOwner, runRemote, saveHost, shellQuote, validateHost } from '../src/remote.js'; import { isRemoteReadOnlyTool, runTool, TOOLS } from '../src/tools.js'; @@ -64,6 +64,7 @@ test('hosts file is private and remote runs are read-only, validated, and versio const grants = acl.split('\n').map((l) => l.replace(path, '').trim()).filter((l) => /:\(/.test(l)); assert.equal(grants.length, 1, acl); assert.match(grants[0], /:\(F\)$/); + assert.ok(grants[0].toLowerCase().includes(`${userInfo().username.toLowerCase()}:(F)`.toLowerCase()), acl); } else { assert.equal(saved.access, 'posix-0600'); assert.equal(statSync(path).mode & 0o777, 0o600); @@ -109,11 +110,38 @@ test('`call --read-only` refuses mutating tools (what remote hosts execute)', () assert.equal(JSON.parse(allowed.stdout).verdict, 'FAIL'); }); -test('Windows hosts-file restriction fails closed when icacls fails', () => { +test('Windows hosts-file restriction removes explicit grants and handles literal Unicode paths', { skip: process.platform !== 'win32' }, async () => { + await withHosts(async (path) => { + // Wildcards and PowerShell expressions are legal filename characters. A + // literal path on stdin must neither expand nor execute any of this text. + const literal = join(dirname(path), "hosts-é [x] $(throw 'path-executed').json"); + saveHost({ name: 'pi', ssh: 'ruv@pi.local' }, literal); + const grant = spawnSync('icacls', [literal, '/grant', '*S-1-1-0:R', '*S-1-5-18:F'], { encoding: 'utf8' }); + assert.equal(grant.status, 0, grant.stderr || grant.stdout); + const before = spawnSync('icacls', [literal], { encoding: 'utf8' }); + assert.equal(before.status, 0, before.stderr); + assert.ok(before.stdout.split('\n').filter(line => /:\(/.test(line)).length >= 3, before.stdout); + assert.equal(restrictToOwner(literal), 'windows-acl-owner-only'); + const after = spawnSync('icacls', [literal], { encoding: 'utf8' }); + assert.equal(after.status, 0, after.stderr); + const grants = after.stdout.split('\n').map(line => line.replace(literal, '').trim()).filter(line => /:\(/.test(line)); + assert.equal(grants.length, 1, after.stdout); + assert.ok(grants[0].toLowerCase().includes(`${userInfo().username}:(F)`.toLowerCase()), after.stdout); + assert.equal(JSON.parse(readFileSync(literal, 'utf8')).hosts[0].name, 'pi'); + }); +}); + +test('Windows hosts-file restriction fails closed and keeps input out of PowerShell source', () => { const calls = []; - const ok = restrictToOwner('C:/x/hosts.json', { platform: 'win32', user: () => 'alice', exec: (cmd, args) => { calls.push([cmd, ...args]); return { status: 0 }; } }); + const path = "C:/x/hosts $(throw 'injected').json"; + const ok = restrictToOwner(path, { platform: 'win32', exec: (cmd, args, options) => { calls.push({ cmd, args, options }); return { status: 0 }; } }); assert.equal(ok, 'windows-acl-owner-only'); - assert.deepEqual(calls[0], ['icacls', 'C:/x/hosts.json', '/inheritance:r', '/grant:r', 'alice:F']); - assert.throws(() => restrictToOwner('C:/x/hosts.json', { platform: 'win32', user: () => 'alice', exec: () => ({ status: 5 }) }), /not made private/); + assert.equal(calls[0].cmd, 'powershell.exe'); + assert.deepEqual(calls[0].args.slice(0, 4), ['-NoLogo', '-NoProfile', '-NonInteractive', '-Command']); + assert.ok(!calls[0].args.at(-1).includes(path)); + assert.deepEqual(JSON.parse(calls[0].options.input), { path }); + for (const result of [{ status: 5 }, { status: null, error: { code: 'ENOENT' } }, { status: null, error: { code: 'ETIMEDOUT' } }]) { + assert.throws(() => restrictToOwner(path, { platform: 'win32', exec: () => result }), /not made private/); + } assert.equal(restrictToOwner('/tmp/h.json', { platform: 'linux', exec: () => { throw new Error('must not run'); } }), 'posix-0600'); });