diff --git a/packages/business/src/config-dir-trust.mjs b/packages/business/src/config-dir-trust.mjs new file mode 100644 index 000000000..50b2ee7e2 --- /dev/null +++ b/packages/business/src/config-dir-trust.mjs @@ -0,0 +1,2 @@ +// Old path of packages/organization/src/config-dir-trust.mjs (rename design r2, stage B). +export * from "../../organization/src/config-dir-trust.mjs"; diff --git a/packages/cli/tests/accounts.test.mjs b/packages/cli/tests/accounts.test.mjs index 5bb5cb876..1b2dae7a6 100644 --- a/packages/cli/tests/accounts.test.mjs +++ b/packages/cli/tests/accounts.test.mjs @@ -79,9 +79,25 @@ test('M3 CLI integration refuses foreign effective uid before any account mutati writeFileSync(f.env.MOSAIC_CONFIG,JSON.stringify(config)); const prelude=join(root,'uid.mjs'); const cli=new URL('../src/cli.mjs',import.meta.url).pathname; const run=args=>spawnSync(process.execPath,['--import',prelude,cli,'accounts',...args],{env:{...process.env,...f.env},encoding:'utf8',timeout:10000}); - writeFileSync(prelude,`process.geteuid=()=>${process.geteuid()+1};`); const refused=run(['init']); assert.equal(refused.status,3,refused.stderr); assert.match(refused.stderr,/config directory must belong to uid/); + writeFileSync(prelude,`process.geteuid=()=>${process.geteuid()+1};`); const refused=run(['init']); assert.equal(refused.status,3,refused.stderr); assert.match(refused.stderr,/accounts-unsafe: (a config directory ancestor|config directory) must belong to (root or )?uid \d+/); for(const name of ['accounts.json','accounts.lock','accounts-receipts.jsonl']) assert.equal(existsSync(join(root,'config',name)),false); // Different real uid, same effective uid: the effective identity is recorded. writeFileSync(prelude,`process.getuid=()=>${process.geteuid()+2};`); const ok=run(['init']); assert.equal(ok.status,0,ok.stderr); const receipt=JSON.parse(readFileSync(join(root,'config/accounts-receipts.jsonl'),'utf8')); assert.equal(receipt.uid,process.geteuid()); }); + +// org-acct-anchor (#1873): `mosaic accounts init` refuses under an ancestor another user could +// rename, before any lock or write, and the refusal names the rule, never the path. +test('org-acct-anchor: init, list and remove refuse a 0777 ancestor before any lock; 1777 passes', async t => { + const top = mkdtempSync(join(tmpdir(), 'a-')); t.after(() => { chmodSync(top, 0o700); rmSync(top, { recursive: true, force: true }); }); + const parent = join(top, 'sk-test-NOT-A-SECRET-1234'), configDir = join(parent, 'cfg'); mkdirSync(configDir, { recursive: true, mode: 0o700 }); + let output = ''; const io = { env: {}, stdout: { write: s => output += s }, stderr: { write: () => {} } }; + const deps = { configDir, instance: 'test', system: { console: { sso: { providers } } } }; + chmodSync(parent, 0o777); + for (const args of [['init'], ['list'], ['user', 'remove', 'u-000000000001', '--revision', '1']]) { + await assert.rejects(accountsCommand(args, io, deps), e => e.exitCode === 3 && /^refused: accounts-unsafe: a config directory ancestor must belong to root or uid \d+ and not be writable by others without the sticky bit$/.test(e.message) && !e.message.includes('sk-test') && !e.message.includes('/')); + for (const name of ['accounts.json', 'accounts.lock', 'accounts-receipts.jsonl', 'org-runtime.lock']) assert.equal(existsSync(join(configDir, name)), false, name); + } + chmodSync(parent, 0o1777); + await accountsCommand(['init'], io, deps); assert.match(output, /accounts init: u-[0-9a-f]{12}, revision 1/); +}); diff --git a/packages/organization/src/accounts.mjs b/packages/organization/src/accounts.mjs index 36ba0b375..deb86f38d 100644 --- a/packages/organization/src/accounts.mjs +++ b/packages/organization/src/accounts.mjs @@ -1,9 +1,10 @@ // Phase M2 (#1873): instance accounts, independent of Console sessions and bus authority. -import { constants, lstatSync } from 'node:fs'; +import { constants } from 'node:fs'; import * as fs from 'node:fs/promises'; -import { dirname, join, parse, resolve } from 'node:path'; +import { dirname, join } from 'node:path'; import { randomBytes } from 'node:crypto'; import { OrgError, readRegistryLocked } from './org-registry.mjs'; +import { assertTrustedConfigDir } from './config-dir-trust.mjs'; import { readJsonFile, requireObject, rejectUnknownKeys, requireString, requirePositiveInt, requireId, requireDistinctList, deepFreeze, canonicalJson } from './util.mjs'; import { RIGHTS, ROLE_RIGHTS, normalizeRights } from './org-rights.mjs'; @@ -128,23 +129,16 @@ export function accountsRepairs(doc) { return result; } -// Check the host boundary before any read or lock. Every ancestor must be a real -// directory; configDir itself belongs to this uid and is not publicly writable. +// Check the host boundary before any read or lock: every ancestor a real directory +// that only root or this uid can change, and configDir this uid's and not publicly +// writable (config-dir-trust.mjs). Refusals name the rule and uid, never the path. export function assertAccountsOwner(configDir, { uid = process.geteuid() } = {}) { - if (typeof configDir !== 'string' || resolve(configDir) !== configDir) fail('accounts-unsafe', 'config directory must be an absolute normalized path'); - let current = parse(configDir).root; - for (const part of configDir.slice(current.length).split('/').filter(Boolean)) { - current = join(current, part); const st = lstatSync(current); - if (!st.isDirectory() || st.isSymbolicLink()) fail('accounts-unsafe', 'config path contains a symlink or non-directory'); - } - const st = lstatSync(configDir); - if (st.uid !== uid) fail('accounts-unsafe', `config directory must belong to uid ${uid}: ${configDir}`); - if (st.mode & 0o022) fail('accounts-unsafe', 'config directory must not be writable by group or other'); + assertTrustedConfigDir(configDir, { uid, code: 'accounts-unsafe' }); } export function loadAccounts(file, context, { uid = process.geteuid() } = {}) { assertAccountsOwner(dirname(file), { uid }); const doc = readJsonFile(file, 'accounts file', st => { - if (st.uid !== uid) fail('accounts-unsafe', `accounts file must belong to uid ${uid}: ${file}`); + if (st.uid !== uid) fail('accounts-unsafe', `accounts file must belong to uid ${uid}`); if (st.mode & 0o022) fail('accounts-unsafe', 'accounts file must not be writable by group or other'); }); return checkAccounts(doc, context); diff --git a/packages/organization/src/agent-levels-write.mjs b/packages/organization/src/agent-levels-write.mjs index 7badc3589..dc1ff1870 100644 --- a/packages/organization/src/agent-levels-write.mjs +++ b/packages/organization/src/agent-levels-write.mjs @@ -11,11 +11,12 @@ // No refusal and no receipt carries a value from the file or the request: a var value can be a // secret typed into the wrong place. Messages name the file (agents.json) and the key, never a // host path. -import { constants, lstatSync } from 'node:fs'; +import { constants } from 'node:fs'; import * as fs from 'node:fs/promises'; import { createHash, randomBytes } from 'node:crypto'; -import { join, parse, resolve } from 'node:path'; +import { join } from 'node:path'; import { OrgError } from './org-registry.mjs'; +import { assertTrustedConfigDir } from './config-dir-trust.mjs'; import { ROOT_FILE, checkProjectAgents, checkRootAgents, loadRootAgents, projectUnion, rootAgentsPath } from './agent-levels.mjs'; import { canonicalJson, isPlainObject } from './util.mjs'; import { ID_PATTERN } from './vocabulary.mjs'; @@ -38,27 +39,9 @@ export function requireRootOwner(actor, owner) { fail('human-required', 'only the signed-in owner writes the root agent file'); } -// Every ancestor a real directory that only root or this uid can change: owned by one of them, and -// group or other writable only with the sticky bit (as /tmp is), so no other user can rename or -// replace configDir between these checks and the path-based lock, temp file and rename. configDir -// itself is this uid's and not group or other writable. -function assertConfigDir(configDir, uid) { - if (typeof configDir !== 'string' || resolve(configDir) !== configDir) fail('agents-unsafe', 'config directory must be an absolute normalized path'); - let current = parse(configDir).root; - const trusted = st => (st.uid === 0 || st.uid === uid) && (!(st.mode & 0o022) || (st.mode & 0o1000)); - try { if (!trusted(lstatSync(current))) fail('agents-unsafe', `a config directory ancestor must belong to root or uid ${uid} and not be writable by others without the sticky bit`); } - catch (error) { if (error instanceof OrgError) throw error; fail('agents-unsafe', 'config directory is missing'); } - for (const part of configDir.slice(current.length).split('/').filter(Boolean)) { - current = join(current, part); - let st; - try { st = lstatSync(current); } catch { fail('agents-unsafe', 'config directory is missing'); } - if (!st.isDirectory() || st.isSymbolicLink()) fail('agents-unsafe', 'config path contains a symbolic link or non-directory'); - if (current !== configDir && !trusted(st)) fail('agents-unsafe', `a config directory ancestor must belong to root or uid ${uid} and not be writable by others without the sticky bit`); - } - const st = lstatSync(configDir); - if (st.uid !== uid) fail('agents-unsafe', `config directory must belong to uid ${uid}`); - if (st.mode & 0o022) fail('agents-unsafe', 'config directory must not be writable by group or other'); -} +// The shared config directory check (config-dir-trust.mjs): every ancestor root's or this uid's and +// not writable by others without the sticky bit, configDir this uid's and not group or other writable. +const assertConfigDir = (configDir, uid) => assertTrustedConfigDir(configDir, { uid, code: 'agents-unsafe' }); // The reader's descriptor checks (loadRootAgents), plus one link: a regular file, no symbolic link, // this uid, no group or other write. Missing is an empty root level at ABSENT_REVISION. diff --git a/packages/organization/src/config-dir-trust.mjs b/packages/organization/src/config-dir-trust.mjs new file mode 100644 index 000000000..7d44a0580 --- /dev/null +++ b/packages/organization/src/config-dir-trust.mjs @@ -0,0 +1,33 @@ +// #1873: the config directory check shared by the accounts store (accounts.mjs) and the root agent +// file's writer (agent-levels-write.mjs). Both open, lock, write and rename files by path under +// configDir, so the path must stay this uid's from the check to the last write. +// +// Every ancestor is a real directory that only root or this uid can change: owned by one of them, and +// group or other writable only with the sticky bit (as /tmp is). Otherwise another user could rename +// configDir out from under the check and put their own file in its place. configDir itself belongs to +// this uid and is not group or other writable. +// +// Each caller keeps its own refusal code. No message quotes a path: a refusal names the rule and the +// uid only (the redaction rule, PR #1916). +import { lstatSync } from 'node:fs'; +import { join, parse, resolve } from 'node:path'; +import { OrgError } from './org-registry.mjs'; + +export function assertTrustedConfigDir(configDir, { uid, code }) { + const fail = message => { throw new OrgError(code, message); }; + if (typeof configDir !== 'string' || resolve(configDir) !== configDir) fail('config directory must be an absolute normalized path'); + const ancestor = `a config directory ancestor must belong to root or uid ${uid} and not be writable by others without the sticky bit`; + const trusted = st => (st.uid === 0 || st.uid === uid) && (!(st.mode & 0o022) || (st.mode & 0o1000)); + const stat = path => { try { return lstatSync(path); } catch { return fail('config directory is missing'); } }; + let current = parse(configDir).root; + if (!trusted(stat(current))) fail(ancestor); + for (const part of configDir.slice(current.length).split('/').filter(Boolean)) { + current = join(current, part); + const st = stat(current); + if (!st.isDirectory() || st.isSymbolicLink()) fail('config path contains a symbolic link or non-directory'); + if (current !== configDir && !trusted(st)) fail(ancestor); + } + const st = stat(configDir); + if (st.uid !== uid) fail(`config directory must belong to uid ${uid}`); + if (st.mode & 0o022) fail('config directory must not be writable by group or other'); +} diff --git a/packages/organization/tests/accounts.test.mjs b/packages/organization/tests/accounts.test.mjs index 2f1c97825..f298bfe5c 100644 --- a/packages/organization/tests/accounts.test.mjs +++ b/packages/organization/tests/accounts.test.mjs @@ -1,9 +1,9 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, chmodSync, symlinkSync, unlinkSync, existsSync, readdirSync, rmSync, statSync } from 'node:fs'; +import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, chmodSync, symlinkSync, unlinkSync, existsSync, readdirSync, rmSync, statSync, lstatSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; -import { checkAccounts, loadAccounts, accountsRepairs, withAccounts, withAccountsLock, withAccountsLocked } from '../src/accounts.mjs'; +import { checkAccounts, loadAccounts, accountsRepairs, assertAccountsOwner, withAccounts, withAccountsLock, withAccountsLocked } from '../src/accounts.mjs'; import { templateRender } from '../src/org-service.mjs'; import { loadBusiness } from '../src/organization-config.mjs'; import { REPO_ROLES } from './helpers.mjs'; @@ -178,3 +178,75 @@ test('review P3-6: a real business file and accounts file load with the same hum const s = setup(t), doc = await templateRender('acme'); doc.human = OP; mkdirSync(join(s.configDir, 'businesses')); writeFileSync(join(s.configDir, 'businesses', 'acme.json'), JSON.stringify(doc), { mode: 0o600 }); assert.equal(loadBusiness('acme', { dir: s.configDir, rolesDir: REPO_ROLES }).human, OP); assert.equal(loadAccounts(s.file, s).users[0].id, OP); }); + +// org-acct-anchor (#1873, A2 review P2-1): every ancestor of the config directory must belong to root +// or this uid, and be writable by others only with the sticky bit. Otherwise another user could rename +// the directory away and put their own accounts.json in its place. No refusal quotes the path. +const SECRETISH = 'sk-test-NOT-A-SECRET-1234'; +function nested(t) { + const top = mkdtempSync(join(tmpdir(), 'a-')); t.after(() => { chmodSync(top, 0o700); rmSync(top, { recursive: true, force: true }); }); + const parent = join(top, SECRETISH), configDir = join(parent, 'cfg'); mkdirSync(configDir, { recursive: true, mode: 0o700 }); chmodSync(parent, 0o700); + const accountsFile = join(configDir, 'accounts.json'); writeFileSync(accountsFile, JSON.stringify(document()), { mode: 0o600 }); + return { top, parent, configDir, instance: 'test', ...context, file: accountsFile }; +} +const unsafe = rule => e => e.code === 'accounts-unsafe' && rule.test(e.message) && !e.message.includes(SECRETISH) && !e.message.includes('/'); +const ANCESTOR = /ancestor must belong to root or uid \d+ and not be writable by others without the sticky bit/; + +test('org-acct-anchor: a 0777 ancestor refuses load, init and the lock; 1777 passes', async t => { + const s = nested(t), initDir = join(s.parent, 'new'); mkdirSync(initDir, { mode: 0o700 }); + const init = { ...s, configDir: initDir, file: join(initDir, 'accounts.json') }; + for (const dir of [s.parent, s.top]) { + chmodSync(dir, 0o777); + try { + assert.throws(() => loadAccounts(s.file, s), unsafe(ANCESTOR)); + await assert.rejects(withAccounts(init, { revision: 0, verb: 'init', init: true }, d => d.users.push(user())), unsafe(ANCESTOR)); + let entered = false; + await assert.rejects(withAccountsLock(s, async () => { entered = true; }), unsafe(ANCESTOR)); + assert.equal(entered, false); + for (const c of [s, init]) for (const name of ['accounts.lock', 'accounts-receipts.jsonl']) assert.equal(existsSync(join(c.configDir, name)), false, name); + assert.equal(existsSync(init.file), false); + chmodSync(dir, 0o1777); + assert.equal(loadAccounts(s.file, s).revision, 1); + await withAccountsLock(s, async () => { entered = true; }); assert.equal(entered, true); + await withAccounts(init, { revision: 0, verb: 'init', init: true }, d => d.users.push(user())); + assert.equal(loadAccounts(init.file, init).revision, 1); + rmSync(init.file); rmSync(join(init.configDir, 'accounts-receipts.jsonl')); + } finally { chmodSync(dir, 0o700); } + } + // A group-writable ancestor without the sticky bit refuses too. + chmodSync(s.parent, 0o770); + try { assert.throws(() => loadAccounts(s.file, s), unsafe(ANCESTOR)); } finally { chmodSync(s.parent, 0o700); } +}); + +test('org-acct-anchor: an ancestor owned by another uid refuses; root-owned ancestors pass', t => { + const s = nested(t), other = process.geteuid() + 1; + // Simulated as the agent-levels tests do: as another uid, this uid's directories are foreign. + assert.throws(() => assertAccountsOwner(s.configDir, { uid: other }), unsafe(new RegExp(`ancestor must belong to root or uid ${other} `))); + assert.throws(() => loadAccounts(s.file, s, { uid: other }), unsafe(ANCESTOR)); + // The real ancestors above the scratch root include root-owned ones (at least /), which pass. + assert.equal(lstatSync('/').uid, 0); + assertAccountsOwner(s.configDir); + assert.equal(loadAccounts(s.file, s).revision, 1); +}); + +test('org-acct-anchor: a symlinked ancestor refuses, and leaf refusals name no path', async t => { + const s = nested(t), link = join(s.top, 'link'); symlinkSync(s.parent, link); + const via = { ...s, configDir: join(link, 'cfg'), file: join(link, 'cfg', 'accounts.json') }; + assert.throws(() => loadAccounts(via.file, via), unsafe(/symbolic link or non-directory/)); + await assert.rejects(withAccountsLock(via, async () => {}), unsafe(/symbolic link or non-directory/)); + assert.equal(existsSync(join(s.configDir, 'accounts.lock')), false); + // The leaf checks are unchanged, but no longer quote the path. + const other = process.geteuid() + 1; + for (const mode of [0o720, 0o702]) { + chmodSync(s.configDir, mode); + try { assert.throws(() => loadAccounts(s.file, s), unsafe(/^config directory must not be writable by group or other$/)); } finally { chmodSync(s.configDir, 0o700); } + } + // As another uid the ancestors refuse first; the leaf's uid rule is the same no-path form (source check below). + assert.throws(() => assertAccountsOwner(s.configDir, { uid: process.geteuid() + 1 }), e => !e.message.includes(SECRETISH) && !e.message.includes('/')); + const source = readFileSync(new URL('../src/config-dir-trust.mjs', import.meta.url), 'utf8') + readFileSync(new URL('../src/accounts.mjs', import.meta.url), 'utf8'); + assert.match(source, /`config directory must belong to uid \$\{uid\}`/); + assert.match(source, /`accounts file must belong to uid \$\{uid\}`/); + assert.doesNotMatch(source, /must belong to uid \$\{uid\}: \$\{/); + assert.throws(() => loadAccounts(join(s.configDir, 'missing', 'accounts.json'), s), unsafe(/^config directory is missing$/)); + assert.throws(() => assertAccountsOwner(`${s.configDir}/../cfg`), unsafe(/absolute normalized/)); +});