From 930182b54a6ab25583c99be1b42d56ae486f8180 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Tue, 10 Nov 2026 22:21:06 -0600 Subject: [PATCH 1/2] fix(organization): accounts config directory checks every ancestor (#1873) assertAccountsOwner refused a symlink or non-directory on the config path, but checked owner and mode only on the leaf. An ancestor owned by another user, or writable by others without the sticky bit, let that user rename the directory away and put their own accounts.json in its place. Since M3 that file decides root authority on a deployed Console. One shared helper, config-dir-trust.mjs, now holds A2's rule: every ancestor belongs to root or this uid and is group or other writable only with the sticky bit; configDir itself is this uid's and not group or other writable. The accounts store (load, init, lock, locked write, mosaic accounts) and the root agent file's writer both call it, each with its own refusal code. A2's messages are unchanged. Accounts refusals no longer quote a path: the config directory and accounts file uid messages name the rule and the uid only (redaction rule, #1916). A missing config directory now refuses accounts-unsafe instead of a raw ENOENT. --- packages/cli/tests/accounts.test.mjs | 18 ++++- packages/organization/src/accounts.mjs | 22 ++---- .../organization/src/agent-levels-write.mjs | 29 ++----- .../organization/src/config-dir-trust.mjs | 33 ++++++++ packages/organization/tests/accounts.test.mjs | 76 ++++++++++++++++++- 5 files changed, 138 insertions(+), 40 deletions(-) create mode 100644 packages/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/)); +}); -- 2.54.0 From 57052c53dd6c2e81313a874ec98e16cc72849013 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Tue, 10 Nov 2026 22:26:46 -0600 Subject: [PATCH 2/2] fix(organization): old-path re-export for config-dir-trust.mjs (#1873) rename-b-identity requires one packages/business/src shim per packages/organization/src module. --- packages/business/src/config-dir-trust.mjs | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 packages/business/src/config-dir-trust.mjs 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"; -- 2.54.0