diff --git a/src/state-store.js b/src/state-store.js index 6917746..f664b72 100644 --- a/src/state-store.js +++ b/src/state-store.js @@ -4,6 +4,7 @@ import { homedir } from 'node:os'; import { dirname, join } from 'node:path'; import { randomBytes } from 'node:crypto'; import { StateStoreError } from './errors.js'; +import { protectDirectoryForCurrentUser } from './windows-acl.js'; const LOCK_STALE_MS = 5 * 60_000; const LOCK_WAIT_MS = 30_000; @@ -123,6 +124,13 @@ export async function atomicWrite(path, content) { } async function secureDirectory(path) { + const root = stateRoot(); + await mkdir(root, { recursive: true, mode: 0o700 }); + await chmod(root, 0o700); + // Windows accepts both of the above and honours neither. The root carries an + // inheritable ACL instead, so every directory and file created below it is restricted + // without a call of its own. + await protectDirectoryForCurrentUser(root); await mkdir(path, { recursive: true, mode: 0o700 }); await chmod(path, 0o700); } diff --git a/src/state-store.test.js b/src/state-store.test.js index a253978..1a3ea7d 100644 --- a/src/state-store.test.js +++ b/src/state-store.test.js @@ -2,16 +2,27 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { mkdtemp, readFile, rm, stat, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; +import { run } from './process.js'; +import { resetWindowsAclCache } from './windows-acl.js'; import { readWorkspaceSecret, readWorkspaceState, withWorkspaceLock, workspaceStateDirectory, writeWorkspaceSecret, writeWorkspaceState } from './state-store.js'; +const icacls = `${process.env.SystemRoot || 'C:\\Windows'}\\System32\\icacls.exe`; + describe('workspace state store', () => { let root; beforeEach(async () => { root = await mkdtemp(join(tmpdir(), 'workspace-state-test-')); process.env.OPENCHAMBER_WORKSPACE_STATE_DIR = root; + if (process.platform === 'win32') { + // A profile's temporary directory is already private, so a store created there + // would look protected whether or not the code protects anything. This one is + // opened to everyone first, and the test can then observe it being closed. + await run(icacls, [root, '/grant', '*S-1-1-0:(OI)(CI)F', '/q']); + } }); afterEach(async () => { delete process.env.OPENCHAMBER_WORKSPACE_STATE_DIR; + resetWindowsAclCache(); await rm(root, { recursive: true, force: true }); }); @@ -21,13 +32,17 @@ describe('workspace state store', () => { await writeWorkspaceSecret(id, 'endpoint-token', 'secret'); expect(await readWorkspaceState(id)).toMatchObject({ lifecycle: 'ready' }); expect(await readWorkspaceSecret(id, 'endpoint-token')).toBe('secret'); - // POSIX modes are how this store restricts its state and secrets, and Windows does - // not implement them: `chmod` is close to a no-op there and every file reports 0o666. - // Asserting the modes on Windows would only restate that, so the check is skipped — - // but the protection genuinely is absent there, standing only on ACLs inherited from - // wherever the data directory happens to live. Enforcing it explicitly on Windows is - // outstanding work, not a platform difference that can be waved through. - if (process.platform !== 'win32') { + // Each platform is asked about the mechanism that actually restricts the store there: + // POSIX modes where they are implemented, and the inherited ACL on Windows, which + // accepts a chmod and honours none of it. + if (process.platform === 'win32') { + const secret = join(workspaceStateDirectory(id), 'secrets', 'endpoint-token'); + const icacls = `${process.env.SystemRoot || 'C:\\Windows'}\\System32\\icacls.exe`; + const { stdout } = await run(icacls, [secret]); + const granted = stdout.split(/\r?\n/).filter((line) => line.includes(':(')).join('\n'); + expect(granted).not.toMatch(/Everyone|BUILTIN\\Users|Authenticated Users/i); + expect(granted).toContain(process.env.USERNAME); + } else { expect((await stat(workspaceStateDirectory(id))).mode & 0o777).toBe(0o700); expect((await stat(join(workspaceStateDirectory(id), 'state.json'))).mode & 0o777).toBe(0o600); expect((await stat(join(workspaceStateDirectory(id), 'secrets', 'endpoint-token'))).mode & 0o777).toBe(0o600); diff --git a/src/windows-acl.js b/src/windows-acl.js new file mode 100644 index 0000000..dffe3f1 --- /dev/null +++ b/src/windows-acl.js @@ -0,0 +1,127 @@ +import { execFile } from 'node:child_process'; +import { promisify } from 'node:util'; +import { StateStoreError } from './errors.js'; + +/** + * Setting an access list is an operating-system primitive, not a provider command, so it + * runs through `child_process` rather than the shared runner. The runner carries argument + * redaction and provider attribution that mean nothing here, and — the reason that + * matters — provider tests replace it wholesale, which would leave the state store unable + * to write a file in any suite that mocks a container CLI. + */ +const exec = promisify(execFile); + +/** + * The state store declares `0o700` directories and `0o600` files, and Windows implements + * neither — `chmod` is close to a no-op there and every file reports `0o666`. Under the + * default data directory the store is nonetheless private, because it inherits the + * profile's permissions; move it to a second drive, a shared folder, or anywhere else an + * operator points `OPENCHAMBER_WORKSPACE_STATE_DIR` and it inherits that location's + * instead. What the store holds is container endpoint tokens, so the protection has to be + * stated rather than inherited. + * + * Windows states it with an ACL, which Node cannot set, so this shells out to `icacls` + * once per state root: inheritance is removed and full control granted to this account, + * SYSTEM, and Administrators — the last two by SID, because their names are localised and + * `Administrators` does not exist on a Ukrainian or German install. + */ + +const SYSTEM_SID = '*S-1-5-18'; +const ADMINISTRATORS_SID = '*S-1-5-32-544'; + +const protectedRoots = new Set(); +let accountSid; + +/** + * Both of these are named by absolute path rather than looked up on PATH. Git for Windows + * ships a POSIX `whoami` that takes no `/user`, and it usually comes first — the same way + * its `tar` shadows the system one and misreads `C:\…` as a remote host. Whichever tool + * answers should not depend on what else happens to be installed. + */ +function system32(binary) { + return `${process.env.SystemRoot || 'C:\\Windows'}\\System32\\${binary}`; +} + +export function resetWindowsAclCache() { + protectedRoots.clear(); + accountSid = undefined; +} + +/** Parses `whoami /user /fo csv /nh`, which prints `"DOMAIN\user","S-1-5-21-…"`. */ +export function parseAccountSid(output) { + const match = /S-1-[0-9-]+/.exec(String(output ?? '')); + return match ? match[0] : null; +} + +async function currentAccountSid() { + if (accountSid) return accountSid; + let result; + try { + result = await exec(system32('whoami.exe'), ['/user', '/fo', 'csv', '/nh'], { timeout: 15_000, windowsHide: true }); + } catch (error) { + throw new StateStoreError('Unable to identify the current Windows account, so workspace state cannot be restricted to it.', { cause: error }); + } + const sid = parseAccountSid(result.stdout); + if (!sid) throw new StateStoreError('Windows reported no security identifier for the current account, so workspace state cannot be restricted to it.'); + accountSid = sid; + return sid; +} + +/** + * Lists the principals holding an entry on a path. `icacls` puts the path and the first + * entry on one line and indents the rest: + * + * C:\…\workspaces Everyone:(OI)(CI)(F) + * NT AUTHORITY\SYSTEM:(I)(OI)(CI)(F) + * DESKTOP-CP9J2I4\Bohdan Triapitsyn:(I)(OI)(CI)(F) + * + * The path has to come off first: it contains the drive's colon, and a principal may + * contain spaces, so there is no separator that tells them apart on that first line. + */ +export function parseGrantedPrincipals(output, path) { + const principals = []; + const prefix = String(path ?? ''); + for (const raw of String(output ?? '').split(/\r?\n/)) { + const line = prefix && raw.toLowerCase().startsWith(prefix.toLowerCase()) ? raw.slice(prefix.length) : raw; + const match = /^\s*(\S.*?):\([A-Z]+\)/.exec(line); + if (match) principals.push(match[1].trim()); + } + return principals; +} + +/** + * Restricts a directory and everything created beneath it to this account. Children + * inherit the entries, so this runs once per root rather than once per write. + * + * Neither switch does the whole job on its own. `/inheritance:r` drops what the parent + * contributed, and `/grant:r` replaces the permissions of the principals it names — but + * an entry someone added explicitly for a third party is touched by neither and simply + * stays. So the list is emptied first and rebuilt second, rather than granting over + * whatever was already there; the owner may always rewrite its own list, which is why the + * directory does not lock this process out in between. + */ +export async function protectDirectoryForCurrentUser(path, { platform = process.platform } = {}) { + if (platform !== 'win32') return false; + if (protectedRoots.has(path)) return false; + const sid = await currentAccountSid(); + const icacls = system32('icacls.exe'); + try { + const options = { timeout: 30_000, windowsHide: true }; + await exec(icacls, [path, '/inheritance:r', '/q'], options); + const { stdout } = await exec(icacls, [path], options); + for (const principal of parseGrantedPrincipals(stdout, path)) { + await exec(icacls, [path, '/remove:g', principal, '/q'], options); + } + await exec(icacls, [ + path, + '/grant:r', `*${sid}:(OI)(CI)F`, + '/grant:r', `${SYSTEM_SID}:(OI)(CI)F`, + '/grant:r', `${ADMINISTRATORS_SID}:(OI)(CI)F`, + '/q', + ], options); + } catch (error) { + throw new StateStoreError(`Unable to restrict workspace state to the current account: ${path}`, { cause: error }); + } + protectedRoots.add(path); + return true; +} diff --git a/src/windows-acl.test.js b/src/windows-acl.test.js new file mode 100644 index 0000000..ae9c2b0 --- /dev/null +++ b/src/windows-acl.test.js @@ -0,0 +1,90 @@ +import { afterEach, describe, expect, it } from 'vitest'; +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { parseAccountSid, parseGrantedPrincipals, protectDirectoryForCurrentUser, resetWindowsAclCache } from './windows-acl.js'; +import { run } from './process.js'; + +const icacls = `${process.env.SystemRoot || 'C:\\Windows'}\\System32\\icacls.exe`; + +describe('windows state protection', () => { + const created = []; + + /** The principal lines of an access list, joined so a test can match across them. */ + async function granted(path) { + const { stdout } = await run(icacls, [path]); + return stdout.split(/\r?\n/).filter((line) => line.includes(':(')).join('\n'); + } + + /** A directory anyone on the machine can read, so removing that grant is observable. */ + async function openDirectory() { + const directory = await mkdtemp(join(tmpdir(), 'workspace-acl-test-')); + created.push(directory); + await run(icacls, [directory, '/grant', '*S-1-1-0:(OI)(CI)F', '/q']); + return directory; + } + afterEach(async () => { + resetWindowsAclCache(); + await Promise.all(created.splice(0).map((path) => rm(path, { recursive: true, force: true }))); + }); + + it('reads every principal out of a list, including one whose name has a space', () => { + const path = 'C:\\Users\\BOHDAN~1\\AppData\\Local\\Temp\\acl-probe'; + const output = [ + `${path} Everyone:(OI)(CI)(F)`, + ' NT AUTHORITY\\SYSTEM:(I)(OI)(CI)(F)', + ' BUILTIN\\Administrators:(I)(OI)(CI)(F)', + ' DESKTOP-CP9J2I4\\Bohdan Triapitsyn:(I)(OI)(CI)(F)', + '', + 'Successfully processed 1 files; Failed processing 0 files', + ].join('\r\n'); + expect(parseGrantedPrincipals(output, path)).toEqual([ + 'Everyone', + 'NT AUTHORITY\\SYSTEM', + 'BUILTIN\\Administrators', + 'DESKTOP-CP9J2I4\\Bohdan Triapitsyn', + ]); + }); + + it('reads the account identifier out of what whoami prints', () => { + expect(parseAccountSid('"CORP\\\\yulia","S-1-5-21-1004336348-1177238915-682003330-512"\r\n')) + .toBe('S-1-5-21-1004336348-1177238915-682003330-512'); + expect(parseAccountSid('')).toBeNull(); + }); + + it('does nothing where POSIX modes already restrict the store', async () => { + const directory = await mkdtemp(join(tmpdir(), 'workspace-acl-test-')); + created.push(directory); + expect(await protectDirectoryForCurrentUser(directory, { platform: 'linux' })).toBe(false); + }); + + it.runIf(process.platform === 'win32')('removes a grant that was there before it, not only inherited ones', async () => { + const directory = await openDirectory(); + // %TEMP% is already private on a normal profile, so a directory made there cannot + // show whether anything was actually taken away. This one is opened to Everyone + // first — an explicit entry, which `/grant:r` alone would leave untouched. + expect(await granted(directory)).toMatch(/Everyone/i); + + expect(await protectDirectoryForCurrentUser(directory)).toBe(true); + + expect(await granted(directory)).not.toMatch(/Everyone/i); + expect(await granted(directory)).toContain(process.env.USERNAME); + }); + + it.runIf(process.platform === 'win32')('passes the restriction down to files created afterwards', async () => { + const directory = await openDirectory(); + await protectDirectoryForCurrentUser(directory); + const secret = join(directory, 'endpoint-token'); + await writeFile(secret, 'secret'); + + expect(await granted(secret)).not.toMatch(/Everyone/i); + expect(await granted(secret)).toContain(process.env.USERNAME); + }); + + it.runIf(process.platform === 'win32')('asks the operating system once per root, not once per write', async () => { + const directory = await mkdtemp(join(tmpdir(), 'workspace-acl-test-')); + created.push(directory); + expect(await protectDirectoryForCurrentUser(directory)).toBe(true); + expect(await protectDirectoryForCurrentUser(directory)).toBe(false); + }); +});