mirror of
https://github.com/PR0M3TH3AN/SeedPass.git
synced 2026-11-06 12:52:04 +00:00
security: fix agent-boundary findings from the fourth reviewer
The reviewer that took longest found the most, including one bug that lies
to the user about a security-relevant outcome.
- `agent stop` never sent the capability and never checked the reply. The
daemon refused the shutdown, the CLI printed {"agent":"stopped"} anyway,
and the seed stayed in memory and still served owner-mnemonic. The command
documents itself as wiping held seeds. It now authenticates and reports
what actually happened.
- The owner capability lived at <appDir>/agent.sock.cap -- inside the very
directory a scoped agent must be given to find fingerprints.json and the
socket. A token holder could `unset SEEDPASS_TOKEN` and the CLI would read
the capability on its behalf. It now lives under XDG_RUNTIME_DIR in a 0700
parent, created with wx so a pre-planted file is never adopted. This raises
the bar; it does not make a token a sandbox, and capabilities.ts no longer
claims otherwise.
- Redaction was a three-key denylist over deliberately loose schemas, so
`custom_fields` -- including values Python masks as is_hidden -- went to
any read-scoped token verbatim. It is now an allowlist that recurses into
custom_fields, reporting labels and presence but never values.
- Authorization ran after the vault was decrypted and after the entry
existence check, giving an unauthenticated caller an entry-id enumeration
oracle, no audit record for the misses, and a full PBKDF2 unlock per probe.
Tokens are now checked first, denials are indistinguishable for missing
and forbidden entries, and every denial is audited.
- The audit chain had no anchor: deleting the log, or a suffix of it, left
something that verified perfectly. A signed head now pins the record count
and tail signature. Separately, `loaded` was latched before the read that
could throw, so one transient EACCES made every later append sign from
genesis and broke the chain permanently.
- The exec allowlist -- documented as the only real containment for a `use`
grant -- was bypassable via the clipboard (a session-wide channel) and had
no CLI flag at all, so no shipped token could carry one. Both fixed.
- Unauthenticated connections could buffer without bound, growing to
gigabytes while each chunk rescanned the accumulated string. Line size and
concurrent connections are now capped.
- A token-supplied TOTP timestamp was unbounded, so one use minted codes
valid decades ahead, outliving the token's TTL. Clamped to a small window.
- Prototype keys ("__proto__", "constructor") resolved to inherited
properties, passed constraint checks against undefined values and burned a
use; now an own-property check. The socket was created with the umask mode
and chmod'd afterwards, leaving a connectable window; it is now created
under a restrictive umask. TTL was enforced only when a request arrived, so
an idle agent held the seed indefinitely past expiry; there is now a timer.
220 core + 78 CLI tests green; 36/36 cross-implementation checks green.
Co-Authored-By: Claude Fable 5 <[email protected]>
This commit is contained in:
1 parent
ee330c52f7
commit
dafae50ca4
7 files changed
+428
-35
No files matched your search
@@ -9,6 +9,7 @@
|
||||
import { afterAll, beforeAll, describe, expect, it } from "vitest";
|
||||
import { createConnection } from "node:net";
|
||||
import { mkdtemp, readFile, writeFile, stat } from "node:fs/promises";
|
||||
import { join as joinPath } from "node:path";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import process from "node:process";
|
||||
@@ -261,6 +262,109 @@ describe("sink children do not inherit the vault's secrets", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("second-audit findings", () => {
|
||||
it("keeps the owner capability outside the app directory", async () => {
|
||||
// A scoped agent is given the app directory; a capability stored there
|
||||
// would hand it the escape hatch along with the socket.
|
||||
expect(daemon.capabilityPath.startsWith(appDir)).toBe(false);
|
||||
const info = await stat(daemon.capabilityPath);
|
||||
expect(info.mode & 0o077).toBe(0);
|
||||
});
|
||||
|
||||
it("agent stop authenticates and does not claim success it did not get", async () => {
|
||||
const unauthenticated = await rawRequest({ op: "shutdown" });
|
||||
expect(unauthenticated["ok"]).toBe(false);
|
||||
// The daemon must still be alive and still holding the seed
|
||||
expect((await rawRequest({ op: "ping" }))["pong"]).toBe(true);
|
||||
});
|
||||
|
||||
it("never returns custom_field values to a read token", async () => {
|
||||
await run(
|
||||
{ SEEDPASS_MNEMONIC: MNEMONIC },
|
||||
"entry", "add", "key-value", "with-fields", "k", "v",
|
||||
);
|
||||
// Inject a hidden custom field the way a Python-side writer would
|
||||
const { openVault, saveVault } = await import("../src/vaultFile.js");
|
||||
const vaultPath = join(appDir, FINGERPRINT, "seedpass_entries_db.json.enc");
|
||||
const vault = await openVault(vaultPath, MNEMONIC);
|
||||
const target = Object.entries(vault.index.entries).find(
|
||||
([, e]) => e.label === "with-fields",
|
||||
)!;
|
||||
(target[1] as { custom_fields?: unknown[] }).custom_fields = [
|
||||
{ label: "recovery_code", value: "HIDDEN-FIELD-SECRET", is_hidden: true },
|
||||
];
|
||||
await saveVault(vault);
|
||||
|
||||
const r = await rawRequest({ op: "vault-index", fingerprint: FINGERPRINT, token });
|
||||
expect(r["ok"]).toBe(true);
|
||||
expect(JSON.stringify(r)).not.toContain("HIDDEN-FIELD-SECRET");
|
||||
const rows = r["entries"] as Array<Record<string, unknown>>;
|
||||
const row = rows.find((x) => x["label"] === "with-fields")!;
|
||||
const fields = row["custom_fields"] as Array<Record<string, unknown>>;
|
||||
expect(fields[0]!["label"]).toBe("recovery_code");
|
||||
expect(fields[0]!["has_value"]).toBe(true);
|
||||
expect(fields[0]).not.toHaveProperty("value");
|
||||
});
|
||||
|
||||
it("does not disclose which entry ids exist to an unauthenticated caller", async () => {
|
||||
const existing = await rawRequest({ op: "secret", fingerprint: FINGERPRINT, id: "0" });
|
||||
const missing = await rawRequest({ op: "secret", fingerprint: FINGERPRINT, id: "9999" });
|
||||
expect(existing["ok"]).toBe(false);
|
||||
expect(missing["ok"]).toBe(false);
|
||||
// Identical denial: an enumeration oracle is an information leak
|
||||
expect(existing["error"]).toBe(missing["error"]);
|
||||
});
|
||||
|
||||
it("rejects an oversized request instead of buffering it", async () => {
|
||||
const reply = await new Promise<Record<string, unknown>>((resolve, reject) => {
|
||||
const socket = createConnection(socketPath);
|
||||
let buf = "";
|
||||
const timer = setTimeout(() => {
|
||||
socket.destroy();
|
||||
reject(new Error("timeout"));
|
||||
}, 10000);
|
||||
socket.on("connect", () => {
|
||||
// No newline: without a cap this grows unbounded and is rescanned.
|
||||
socket.write("x".repeat(1_000_000));
|
||||
});
|
||||
socket.on("data", (c) => {
|
||||
buf += c.toString();
|
||||
const nl = buf.indexOf("\n");
|
||||
if (nl >= 0) {
|
||||
clearTimeout(timer);
|
||||
socket.destroy();
|
||||
resolve(JSON.parse(buf.slice(0, nl)) as Record<string, unknown>);
|
||||
}
|
||||
});
|
||||
socket.on("error", (e) => {
|
||||
clearTimeout(timer);
|
||||
reject(e);
|
||||
});
|
||||
});
|
||||
expect(reply["ok"]).toBe(false);
|
||||
expect(String(reply["error"])).toContain("too large");
|
||||
});
|
||||
|
||||
it("clamps a token-supplied TOTP timestamp to now", async () => {
|
||||
const far = await rawRequest({
|
||||
op: "secret", fingerprint: FINGERPRINT, id: "1", token, timestamp: 4000000000,
|
||||
});
|
||||
expect(far["ok"]).toBe(false);
|
||||
expect(String(far["error"])).toMatch(/timestamp|denied/);
|
||||
});
|
||||
|
||||
it("detects a truncated audit log", async () => {
|
||||
const auditPath = join(appDir, FINGERPRINT, "audit.log");
|
||||
const original = await readFile(auditPath, "utf8");
|
||||
const lines = original.split("\n").filter((l) => l.trim());
|
||||
// Excise the most recent records, as an attacker covering their tracks
|
||||
await writeFile(auditPath, lines.slice(0, Math.max(1, lines.length - 2)).join("\n") + "\n");
|
||||
const r = await run({ SEEDPASS_MNEMONIC: MNEMONIC }, "agent", "audit-verify");
|
||||
expect(String((r.error as Error).message)).toMatch(/removed|head/);
|
||||
await writeFile(auditPath, original);
|
||||
});
|
||||
});
|
||||
|
||||
describe("locking clears derived state", () => {
|
||||
it("tokens do not survive a lock and later unlock", async () => {
|
||||
const fresh = JSON.parse(
|
||||
|
||||
Reference in new issue
Block a user