From b6fee154f6a823b8395f9a806f085c79c1628613 Mon Sep 17 00:00:00 2001 From: yulia-ivashko Date: Fri, 7 Aug 2026 13:27:32 +0300 Subject: [PATCH] Restrict workspace state on Windows, where the declared modes do nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The store creates 0o700 directories and 0o600 files to keep container endpoint tokens to one account, and Windows implements neither: chmod is close to a no-op and every file reports 0o666. Under a default profile the store is private anyway, by inheritance — which is why this went unnoticed. Point OPENCHAMBER_WORKSPACE_STATE_DIR at a second drive or a shared folder and it inherits that location's permissions instead, and nothing objects. Windows states this with an access list, which Node cannot set, so the state root gets one through icacls and everything created beneath it inherits the result. Once per root, not once per write. Three details are load-bearing. SYSTEM and Administrators are named by security identifier, because their names are localised and "Administrators" does not exist on a Ukrainian install. Both system tools are named by absolute path, because Git for Windows ships a POSIX whoami that rejects /user and usually comes first on PATH — the same shadowing that made its tar read C:\ as a remote host. And the list is emptied before it is rebuilt: /inheritance:r drops what the parent contributed and /grant:r replaces only the principals it names, so an entry someone added explicitly for a third party would otherwise survive being "restricted". It runs through child_process rather than the shared runner. Redaction and provider attribution mean nothing for an OS primitive, and provider suites replace that runner wholesale — routing through it left the store unable to write a file in every test that mocks a container CLI. The state store test no longer skips itself on Windows. It asks each platform about the mechanism that actually restricts the store there, and it opens its own root to Everyone first: a directory made under %TEMP% is already private, so without that the assertion passes whether or not the code protects anything. Verified by disabling the protection and watching both suites fail. --- src/state-store.js | 8 +++ src/state-store.test.js | 29 ++++++--- src/windows-acl.js | 127 ++++++++++++++++++++++++++++++++++++++++ src/windows-acl.test.js | 90 ++++++++++++++++++++++++++++ 4 files changed, 247 insertions(+), 7 deletions(-) create mode 100644 src/windows-acl.js create mode 100644 src/windows-acl.test.js 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); + }); +});