diff --git a/package.json b/package.json index de9183e..eff401d 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@socioprophet/hellgraph", - "version": "0.4.46", + "version": "0.4.47", "description": "HellGraph \u2014 TypeScript OpenCog-compatible AtomSpace metagraph engine (PLN, ECAN, pattern matcher, SPARQL/Gremlin, SHACL, Atomese, StorageNode federation). Polyglot sibling of the Rust hellgraph crate.", "license": "UNLICENSED", "type": "commonjs", diff --git a/ts/dist/index.d.mts b/ts/dist/index.d.mts index eb636a2..9ce25c2 100644 Binary files a/ts/dist/index.d.mts and b/ts/dist/index.d.mts differ diff --git a/ts/dist/index.d.ts b/ts/dist/index.d.ts index eb636a2..9ce25c2 100644 Binary files a/ts/dist/index.d.ts and b/ts/dist/index.d.ts differ diff --git a/ts/dist/index.js b/ts/dist/index.js index 7c21a6f..baf8e97 100644 Binary files a/ts/dist/index.js and b/ts/dist/index.js differ diff --git a/ts/dist/index.mjs b/ts/dist/index.mjs index 23933a1..df2af0b 100644 Binary files a/ts/dist/index.mjs and b/ts/dist/index.mjs differ diff --git a/ts/src/cypher.ts b/ts/src/cypher.ts index 3c6ba92..f4b287a 100644 --- a/ts/src/cypher.ts +++ b/ts/src/cypher.ts @@ -1,6 +1,7 @@ import { createHash } from 'node:crypto' import { AtomSpace, nodeHandle, linkHandle, type Atom, type Handle } from './atomspace' import { findMatches, V, N, L, type Pattern, type PatternTerm, type Grounding } from './patternMatcher' +import { cloneDict, emptyDict, ownValue, toPlainRow } from './safe-dict.js' /** * Cypher Facade v0.1 — a human/agent-friendly READ query surface over the AtomSpace. @@ -354,8 +355,10 @@ class Parser { } private resolveVal(tok: string): string { - if (tok === '$') return this.params[this.next()] ?? '' - if (tok?.startsWith('$')) return this.params[tok.slice(1)] ?? '' + // The parameter NAME is query-derived and `params` is caller-supplied, so an own-property + // read only: `$constructor` must be an unsupplied parameter, not Object.prototype.constructor. + if (tok === '$') return ownValue(this.params, this.next()) ?? '' + if (tok?.startsWith('$')) return ownValue(this.params, tok.slice(1)) ?? '' return unquote(tok) } } @@ -565,7 +568,9 @@ export function runCypher(as: AtomSpace, query: string, params: Record { - const rr: RRow = {} + // Keyed by Cypher variable names and `var.prop` paths, all query-derived — null-prototype + // so a variable named `__proto__` is stored rather than swallowed (see safe-dict.ts). + const rr: RRow = emptyDict() for (const [v, h] of Object.entries(g)) { if (v.startsWith('_')) continue const atom = as.getAtom(h) @@ -622,7 +627,7 @@ export function runCypher(as: AtomSpace, query: string, params: Record 1 || (pin !== undefined && !as.getAtom(pin))) { groundings = []; break } - if (pin !== undefined) groundings = groundings.map((g) => ({ ...g, [irVar]: pin })) + if (pin !== undefined) groundings = groundings.map((g) => { const n = cloneDict(g); n[irVar] = pin; return n }) else if (!clauseBound.has(irVar)) { scanVars.push({ irVar, labels: e.labels }); continue } for (const label of e.labels) groundings = groundings.filter((g) => atomMatchesLabel(as, as.getAtom(g[irVar]!), label)) } @@ -638,7 +643,9 @@ export function runCypher(as: AtomSpace, query: string, params: Record ({ col: c, ref: { var: c } })) : ast.ret.map((c) => ({ col: c.prop ? `${c.var}.${c.prop}` : c.var, ref: c })) const outCols = outRefs.map((c) => c.col) - let outRows = rows.map((r) => { - const o: Record = {} - for (const { col, ref } of outRefs) { const v = refValue(r, ref); o[col] = v === undefined ? '' : String(v) } - return o - }) + // Column names are query-derived (RETURN aliases). Materialize via toPlainRow so a hostile + // name lands as an own data property instead of hitting the inherited __proto__ setter and + // leaving a DECLARED column missing from every row. + let outRows = rows.map((r) => + toPlainRow(outRefs.map(({ col, ref }) => { + const v = refValue(r, ref) + return [col, v === undefined ? '' : String(v)] as const + })), + ) outRows = outRows.slice(0, Math.min(ast.limit ?? maxRows, maxRows)) opts.onEvidence?.({ queryHash, space: as.id, mode: opts.mode ?? 'operational', columns: outCols, rowCount: outRows.length, evaluatedAtSeq: as.logicalClock, useSpace: ast.useSpace }) diff --git a/ts/src/gremlin.ts b/ts/src/gremlin.ts index 38f5080..f942e8b 100644 --- a/ts/src/gremlin.ts +++ b/ts/src/gremlin.ts @@ -1,5 +1,12 @@ import type { HellGraphStore } from './store' import type { GraphNode, GraphEdge, GremlinResult, PropertyValue } from './types' +import { ownValue } from './safe-dict.js' + +/** Read a property whose KEY came from the traversal text. Own properties only: without this, + * `values("constructor")` / `values("toString")` hand the caller inherited Object.prototype + * FUNCTIONS as if they were stored graph data, and `has("constructor", …)` compares against + * one (js/remote-property-injection — see safe-dict.ts). */ +const prop = (e: GraphNode | GraphEdge, key: string): PropertyValue | undefined => ownValue(e.properties, key) /** * A Gremlin/TinkerPop-style traversal engine over HellGraph's property graph. @@ -29,7 +36,7 @@ export class GraphTraversal { } has(key: string, value: PropertyValue): GraphTraversal { - return this.derive(this.nodes().filter((n) => looseEq(n.properties[key], value))) + return this.derive(this.nodes().filter((n) => looseEq(prop(n, key), value))) } out(label?: string): GraphTraversal { @@ -55,7 +62,7 @@ export class GraphTraversal { // ─── Terminal-ish steps ────────────────────────────────────────────────── values(key: string): GraphTraversal { - const out = this.current.map((t) => (isNode(t) || isEdge(t)) ? t.properties[key] : t).filter((v) => v !== undefined) + const out = this.current.map((t) => (isNode(t) || isEdge(t)) ? prop(t, key) : t).filter((v) => v !== undefined) return this.derive(out as Traverser[]) } @@ -77,8 +84,8 @@ export class GraphTraversal { order(key: string, desc = false): GraphTraversal { const sorted = [...this.current].sort((a, b) => { - const av = isNode(a) || isEdge(a) ? a.properties[key] : a - const bv = isNode(b) || isEdge(b) ? b.properties[key] : b + const av = isNode(a) || isEdge(a) ? prop(a, key) : a + const bv = isNode(b) || isEdge(b) ? prop(b, key) : b const an = Number(av), bn = Number(bv) const cmp = !Number.isNaN(an) && !Number.isNaN(bn) ? an - bn : String(av ?? '').localeCompare(String(bv ?? '')) return desc ? -cmp : cmp diff --git a/ts/src/index.ts b/ts/src/index.ts index b3839e3..cc5341a 100644 --- a/ts/src/index.ts +++ b/ts/src/index.ts @@ -54,6 +54,7 @@ export * from './metta-eval' export * from './vendor-cache' export * from './metrics' export * from './rate-limit' +export * from './log-safe' export * from './discourse' export * from './graph-analytics' export * from './attribute-profile' diff --git a/ts/src/log-safe.ts b/ts/src/log-safe.ts new file mode 100644 index 0000000..05507c1 --- /dev/null +++ b/ts/src/log-safe.ts @@ -0,0 +1,119 @@ +/** + * Log-boundary sanitizer. + * + * Every log line this estate writes is potential evidence: receipts, audit records and + * operator forensics are all read back as fact. A log line an attacker can *shape* is + * therefore worse here than in a typical application — it does not merely add noise, it + * lets an attacker author entries a later reader attributes to the system. + * + * A log record is line-oriented, so the whole attack reduces to "get a line terminator into + * a field". Stripping CR/LF alone is NOT enough. The previous guard in super-peer.ts did + * exactly that — `.replace(/[\r\n\t]+/g, ' ')` — and still let all of these through: + * + * U+000B VT, U+000C FF line breaks to many log viewers and pagers + * U+0085 NEL a C1 line break; a real terminator to Unicode-aware readers + * U+2028 LS, U+2029 PS line/paragraph separators; break lines in JS-based tooling + * U+0000 NUL truncates the record in C-string consumers + * U+001B ESC ANSI CSI: `ESC[2K ESC[1G` erases the real line and rewrites it, + * so an operator tailing the log sees the forged text and never + * sees what it replaced + * + * Breaking the line is not the only way to forge one. A record is only evidence once a HUMAN + * reads it, so anything that changes what the reader sees is in scope too — `\p{Cf}` format + * characters do exactly that without touching a single line terminator: + * + * U+202E RLO the Trojan Source class. `admitted=` + RLO + `resu` renders as + * U+202A-U+202D `admitted=user` while the bytes say something else, so an auditor + * U+2066-U+2069 reading the log and a tool grepping it disagree about its content + * U+200B ZWSP, U+FEFF invisible: splits `admitted` so a search for it never matches + * U+200E LRM, U+200F RLM reorder the remainder of the field + * U+00AD SOFT HYPHEN invisible token split, same effect as ZWSP + * + * The rule is therefore a category allow-list, not a deny-list of specific characters: + * `\p{Cc}` (every C0 control, DEL, every C1 control — so NUL, ESC, VT, FF, NEL are all in), + * `\p{Zl}` / `\p{Zp}` (U+2028 / U+2029), and `\p{Cf}` (every bidi control, every zero-width, + * the BOM) are rendered as a VISIBLE escape. Visible rather than deleted, so a reader can tell + * something was neutralized instead of silently being handed a laundered line. + * + * A category allow-list is the point: it is chosen so that a character nobody thought of is + * covered by the CLASS it belongs to. `\p{Cf}` was the category originally missed — the tests + * did not catch it because their assertion was written from this same set, so it could not + * fail for anything this set omitted. See the note above `UNSAFE_IN_A_LOG_FIELD` in + * security-hardening7.test.ts: that regex is deliberately derived from what a READER can be + * made to misrender, never from what this function happens to strip. + */ + +/** + * Render one control character as a visible, unambiguous escape. + * + * `codePointAt`, not `charCodeAt`: \p{Cf} reaches beyond the BMP (U+13430-U+1343F Egyptian + * format controls, U+1BCA0-U+1BCA3), and the `u`-flagged class matches those as a whole code + * point. `charCodeAt(0)` would report only the HIGH SURROGATE, printing `\ud80d` for U+13430. + * The character is neutralized either way — the replace consumes the whole match — but the + * escape is the forensic record of what was neutralized, and naming the wrong code point + * defeats the reason these are made visible instead of deleted. + */ +function escapeControl(ch: string): string { + const code = ch.codePointAt(0) ?? 0 + const hex = code.toString(16) + if (code <= 0xff) return '\\x' + hex.padStart(2, '0') + return code <= 0xffff ? '\\u' + hex.padStart(4, '0') : '\\u{' + hex + '}' +} + +/** Hard cap so one request cannot flood the log (and so a record stays greppable). */ +export const LOG_FIELD_MAX = 500 + +/** + * Coerce any value to a string WITHOUT being able to throw. + * + * `String(value)` is not total, and both failing cases are reachable from this module's callers: + * + * - `String(Object.create(null))` throws `TypeError: Cannot convert object to primitive value` + * — it has no inherited `toString`/`valueOf`. `safe-dict.ts` hands out exactly such + * dictionaries throughout the engine, so one reaching a log boundary is not hypothetical. + * - a value whose own `toString` throws. + * + * Either would throw from INSIDE `super-peer`'s catch block, replacing the 500 response with an + * unhandled error — the sanitizer would become the outage. A log boundary must never be a + * secondary failure path, and the guarantee below says "any input whatsoever". + * + * `Object.prototype.toString.call` is used for the fallback tag because it reads the internal + * class directly and never invokes user code; the outer guard covers exotic proxies. + */ +function coerceToString(value: unknown): string { + if (typeof value === 'string') return value + try { + return String(value) + } catch { + try { + return `[unstringifiable ${Object.prototype.toString.call(value)}]` + } catch { + return '[unstringifiable]' + } + } +} + +/** + * Render an untrusted value as a single-line, unforgeable log field. + * + * Guarantees, for any input whatsoever: + * - the result contains no character that can terminate or truncate a log line, and none that + * can rewrite one — whether by driving the terminal (ANSI CSI) or by reordering/hiding what + * a human reads (bidi overrides, zero-width characters) + * - the result is at most `max` characters, plus an explicit truncation marker + * - it returns; it never throws, whatever it is handed (see `coerceToString`) + */ +export function sanitizeLogValue(value: unknown, max: number = LOG_FIELD_MAX): string { + const raw = coerceToString(value) + const escaped = raw + // CR and LF first, and individually. Besides being the obvious vector, this exact shape + // — a global replace of a constant string — is the barrier CodeQL's js/log-injection + // recognises. A quantified character class such as /[\r\n\t]+/g is not recognised, which + // is why the previous guard kept alerting even though it did remove newlines. + .replace(/\r/g, '\\r') + .replace(/\n/g, '\\n') + // Everything else that can break, truncate or rewrite a line — including \p{Cf}, which + // rewrites what the READER sees (bidi override, zero-width) without touching a terminator. + .replace(/[\p{Cc}\p{Zl}\p{Zp}\p{Cf}]/gu, escapeControl) + return escaped.length > max ? escaped.slice(0, max) + '[truncated]' : escaped +} diff --git a/ts/src/patternMatcher.ts b/ts/src/patternMatcher.ts index eed05d9..8180ab8 100644 --- a/ts/src/patternMatcher.ts +++ b/ts/src/patternMatcher.ts @@ -1,4 +1,5 @@ import { AtomSpace, nodeHandle, type Atom, type Handle } from './atomspace' +import { cloneDict, emptyDict, toPlainRow } from './safe-dict.js' /** * Pattern Matcher — native hypergraph query over the AtomSpace. @@ -46,7 +47,10 @@ export const L = (type: string, ...outgoing: PatternTerm[]): Extract()] for (const clause of pattern.clauses) { const next: Grounding[] = [] @@ -71,14 +75,17 @@ export function findMatches(as: AtomSpace, pattern: Pattern): MatchResult { }) const variables = pattern.select ?? collectVars(pattern.clauses) - const results = groundings.map((g) => { - const row: Record = {} - for (const v of variables) { + // Output rows are keyed by pattern variable names, which arrive from Cypher query text — so this + // is the same untrusted-key surface as the groundings above, at the boundary rather than inside + // it. Built through toPlainRow (CreateDataProperty) instead of `row[v] = …` on a plain object: + // assigning `__proto__` hits Object.prototype's inherited SETTER, the write is swallowed, and the + // variable stays listed in `variables` while being absent from every row — a declared column that + // silently is not there, which is the WRITE mode described in safe-dict.ts. + const results = groundings.map((g) => + toPlainRow(variables.map((v) => { const atom = g[v] ? as.getAtom(g[v]) : undefined - row[v] = atom ? (atom.name ?? atom.type) : '' - } - return row - }) + return [v, atom ? (atom.name ?? atom.type) : ''] as const + }))) return { variables, results, groundings, evaluatedAtSeq: as.logicalClock } } @@ -103,7 +110,9 @@ function unifyTerm(as: AtomSpace, term: PatternTerm, handle: Handle, binding: Gr const atom = as.getAtom(handle) if (!atom || !as.types.isA(atom.type, term.type)) return null } - return { ...binding, [term.name]: handle } + const bound = cloneDict(binding) + bound[term.name] = handle + return bound } case 'node': return nodeHandle(term.type, term.name) === handle ? binding : null diff --git a/ts/src/safe-dict.ts b/ts/src/safe-dict.ts new file mode 100644 index 0000000..58e77da --- /dev/null +++ b/ts/src/safe-dict.ts @@ -0,0 +1,68 @@ +/** + * Null-prototype dictionaries for untrusted keys. + * + * Every query surface in this engine keys a plain object BY NAMES THAT CAME OFF THE WIRE: + * SPARQL solution bindings by variable name, Cypher rows by variable / RETURN alias, Cypher + * `$params` by parameter name, Gremlin property lookups by property key, pattern-matcher + * groundings by pattern variable. On an ordinary `{}` every such name that collides with a + * member of Object.prototype is a live wire (js/remote-property-injection), in three distinct + * ways — all three are real, and the middle one is the easiest to miss: + * + * WRITE `row['__proto__'] = v` hits the inherited accessor instead of defining a property. + * The column is DECLARED in the response and then absent from every row: a + * silently-wrong result, not an error. + * + * READ `props['constructor']` / `props['toString']` return inherited FUNCTIONS. Those + * escape into query results as if they were data (`g.V().values("constructor")` + * really does hand back the Object constructor), and feed comparisons that then + * match rows no user ever stored. + * + * `in` `'__proto__' in binding` is true on an object that bound nothing. Unification + * then compares the candidate against Object.prototype, fails, and the query + * silently returns ZERO rows. + * + * A null-prototype object has no inherited members, so a variable named ?__proto__ or + * ?constructor is simply an ordinary variable — which is what every one of these query + * languages says it is. That is why this is the fix rather than rejecting such names: + * rejecting them would trade one silently-wrong answer for a different one. + * + * NB: object spread (`{ ...dict }`) and object literals re-attach Object.prototype. Use + * cloneDict / mergeDicts, never a spread, to carry one of these forward. + */ + +/** A dictionary whose keys are untrusted. Structurally a Record; semantically null-prototype. */ +export type SafeDict = Record + +/** A fresh dictionary with no inherited members. */ +export function emptyDict(): SafeDict { + return Object.create(null) as SafeDict +} + +/** Copy `src`'s own entries into a fresh null-prototype dictionary. */ +export function cloneDict(src: SafeDict): SafeDict { + return Object.assign(Object.create(null), src) as SafeDict +} + +/** Merge two dictionaries into a fresh null-prototype one; `b` wins collisions. */ +export function mergeDicts(a: SafeDict, b: SafeDict): SafeDict { + return Object.assign(Object.create(null), a, b) as SafeDict +} + +/** + * Read a key that came from untrusted input, from an object that may have a normal prototype. + * Returns undefined for anything not stored as an OWN property, so `constructor`, `toString` + * and friends read as "absent" rather than as inherited functions. + */ +export function ownValue(src: Record | undefined, key: string): V | undefined { + if (!src) return undefined + return Object.prototype.hasOwnProperty.call(src, key) ? src[key] : undefined +} + +/** + * Materialize an output row for consumers: a NORMAL-prototype object, so callers get a plain + * JSON-shaped value, but built with Object.fromEntries — which uses CreateDataProperty, so even + * a `__proto__` key lands as a real own data property instead of invoking the inherited setter. + */ +export function toPlainRow(entries: Iterable): Record { + return Object.fromEntries(entries) as Record +} diff --git a/ts/src/security-hardening7.test.ts b/ts/src/security-hardening7.test.ts new file mode 100644 index 0000000..8ed431f --- /dev/null +++ b/ts/src/security-hardening7.test.ts @@ -0,0 +1,362 @@ +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { mkdtempSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +process.env['HELLGRAPH_STORE_DIR'] = mkdtempSync(join(tmpdir(), 'hg-sec7-')) + +import { getHellGraph } from './store.js' +import { getAtomSpace, AtomSpace } from './atomspace.js' +import { runSparql } from './sparql.js' +import { runGremlin } from './gremlin.js' +import { runCypher } from './cypher.js' +import { findMatches, V, L } from './patternMatcher.js' +import { sanitizeLogValue, LOG_FIELD_MAX } from './log-safe.js' + +const g = getHellGraph() +g.addNode('n:1', ['Person'], { name: 'Ada', age: 30 }) +g.addNode('n:2', ['Person'], { name: 'Alan', age: 40 }) + +/** Keys that alias a member of Object.prototype — the whole attack surface in one list. */ +const HOSTILE_KEYS = ['__proto__', 'constructor', 'toString', 'valueOf', 'hasOwnProperty'] + +const ownKeys = (o: object): string[] => Object.getOwnPropertyNames(o) +const isOwnData = (o: object, k: string): boolean => { + const d = Object.getOwnPropertyDescriptor(o, k) + return d !== undefined && 'value' in d +} + +// ─── Attack 16: remote property injection through query-derived key names ────────────────── +// CodeQL js/remote-property-injection, alert 34 (ts/src/sparql.ts:628, HIGH). +// A SPARQL variable name is attacker-controlled: the tokenizer accepts /\?[A-Za-z0-9_]+/, so +// `?__proto__` and `?constructor` are expressible verbatim in a query body posted to +// /api/graph/sparql. On a plain-object Binding that reaches Object.prototype three ways — +// a swallowed write, an inherited read, and a spuriously-true `in`. All three are asserted. + +test('SECURITY: GROUP BY ?__proto__ writes an own data property and cannot clobber a prototype', () => { + const r = runSparql(g, 'SELECT ?__proto__ (COUNT(?s) AS ?n) WHERE { ?s ?p ?__proto__ } GROUP BY ?__proto__') + + assert.ok(r.bindings.length > 0, 'a variable named ?__proto__ must still match rows, not silently return zero') + for (const b of r.bindings) { + assert.ok(isOwnData(b, '__proto__'), '__proto__ must be stored as an OWN DATA property, not routed to the setter') + assert.equal(Object.getPrototypeOf(b), Object.prototype, 'the row prototype must be untouched') + assert.ok(JSON.stringify(b).includes('__proto__'), 'a DECLARED variable must appear in the serialized row') + } + // The declared variable list and the row keys must agree — the silent-wrong failure mode is a + // column promised in `variables` and then absent from every row. + for (const v of r.variables) { + assert.ok(r.bindings.every((b) => ownKeys(b).includes(v)), `declared variable ${v} missing from a row`) + } +}) + +test('SECURITY: an aggregate aliased to ?__proto__ lands in the row instead of being swallowed', () => { + const r = runSparql(g, 'SELECT (COUNT(?s) AS ?__proto__) WHERE { ?s ?p ?o }') + assert.deepEqual(r.variables, ['__proto__']) + assert.equal(r.bindings.length, 1) + const row = r.bindings[0]! + assert.ok(isOwnData(row, '__proto__'), 'COUNT(...) AS ?__proto__ must produce a real own property') + assert.equal(typeof (row as Record)['__proto__'], 'number', 'and it must hold the count') +}) + +test('SECURITY: an unbound hostile variable reads as null, never as an inherited function', () => { + for (const key of HOSTILE_KEYS) { + for (const q of [ + `SELECT ?${key} (COUNT(?s) AS ?n) WHERE { ?s ?p ?o } GROUP BY ?${key}`, // aggregate path (line 628) + `SELECT ?${key} WHERE { ?s ?p ?o }`, // plain projection path + ]) { + for (const b of runSparql(g, q).bindings) { + const v = (b as Record)[key] + assert.notEqual(typeof v, 'function', `?${key} leaked an inherited function into a binding (${q})`) + assert.equal(v, null, `?${key} is unbound and must read as null (${q})`) + } + } + } +}) + +test('SECURITY: a hostile variable name still unifies — matchTriple must not see an inherited key', () => { + // The `in` failure mode: `'__proto__' in binding` is true on a plain object that bound nothing, + // so unify() compares the candidate against Object.prototype, fails, and the query returns zero + // rows. `?__proto__` must behave exactly like any other variable name. + const hostile = runSparql(g, 'SELECT ?__proto__ WHERE { ?s ?p ?__proto__ }').bindings.length + const control = runSparql(g, 'SELECT ?ordinary WHERE { ?s ?p ?ordinary }').bindings.length + assert.equal(hostile, control, '?__proto__ must match the same rows as any ordinary variable name') + assert.ok(hostile > 0, 'and that must not be zero') +}) + +test('SECURITY: no query surface pollutes the global Object.prototype', () => { + const probe = () => ({} as Record) + for (const key of HOSTILE_KEYS) { + runSparql(g, `SELECT ?${key} (COUNT(?s) AS ?n) WHERE { ?s ?p ?${key} } GROUP BY ?${key}`) + runSparql(g, `SELECT (COUNT(?s) AS ?${key}) WHERE { ?s ?p ?o }`) + } + assert.equal(probe()['polluted'], undefined, 'Object.prototype must carry no attacker key') + assert.equal(Object.getPrototypeOf({}), Object.prototype) + assert.equal(typeof ({} as Record)['toString'], 'function', 'and must remain intact') +}) + +// ─── Attack 17: the same pattern in the sibling query surfaces ───────────────────────────── + +test('SECURITY: Gremlin values()/has() read own properties only — no inherited function leaks', () => { + for (const key of HOSTILE_KEYS) { + const vals = runGremlin(g, `g.V().values("${key}")`).values as unknown[] + assert.ok( + !vals.some((v) => typeof v === 'function'), + `g.V().values("${key}") handed back an Object.prototype function as if it were graph data`, + ) + assert.equal(vals.length, 0, `no node stores "${key}", so the traversal must yield nothing`) + // has() must not match on an inherited member either. + const matched = runGremlin(g, `g.V().has("${key}","x").values("name")`).values as unknown[] + assert.equal(matched.length, 0, `has("${key}", …) matched a node that never stored it`) + } +}) + +test('SECURITY: Cypher RETURN of a hostile variable yields own data, not a prototype member', () => { + const as = getAtomSpace() + // NB `__proto__` is included for the OWN-PROPERTY assertion only. Its projected value is '' + // because Cypher reserves a leading `_` for internal IR variables (`_leading` behaves + // identically) — that naming convention is pre-existing and unrelated to this alert. + for (const key of ['__proto__', 'constructor', 'toString', 'valueOf']) { + const r = runCypher(as, `MATCH (${key}:Person) RETURN ${key} LIMIT 5`) as { + columns: string[] + rows: Record[] + } + assert.ok(r.rows.length > 0, `MATCH (${key}:Person) must still match — a hostile name is just a name`) + for (const row of r.rows) { + assert.ok(isOwnData(row, key), `a DECLARED column ${key} must be an own data property of the row`) + assert.equal(Object.getPrototypeOf(row), Object.prototype, 'row prototype untouched') + assert.notEqual(typeof row[key], 'function', `${key} leaked an inherited function`) + } + } +}) + +test('SECURITY: an unsupplied Cypher $param named for a prototype member resolves empty', () => { + const as = getAtomSpace() + // Store a node whose name is EXACTLY what Object.prototype.constructor stringifies to. With a + // plain-object params lookup, `$constructor` resolves to that inherited function, stringifies, + // and matches this node — a row the caller never asked for, from a parameter never supplied. + const bait = String(Object) + getHellGraph().addNode('n:bait', ['Person'], { name: bait }) + + const injected = runCypher(as, 'MATCH (n:Person) WHERE n.name = $constructor RETURN n LIMIT 5', {}) as { + rows: Record[] + } + assert.equal(injected.rows.length, 0, 'an unsupplied $param must match nothing, not a stringified prototype function') + + // Control: a supplied parameter of the same name must still work normally. + const supplied = runCypher(as, 'MATCH (n:Person) WHERE n.name = $constructor RETURN n LIMIT 5', { + constructor: bait, + }) as { rows: Record[] } + assert.equal(supplied.rows.length, 1, 'a genuinely supplied parameter named "constructor" must still resolve') +}) + +// ─── Attack 17b: the pattern matcher's OUTPUT row ────────────────────────────────────────── +// The groundings inside findMatches were made null-prototype, but the row it hands back was +// still built as `const row: Record = {}` and keyed by the same query-derived +// variable names. `row['__proto__'] = v` hits Object.prototype's inherited SETTER, so the write +// is swallowed: the variable stays listed in `variables` and is absent from every row. A +// DECLARED column that silently is not there — the WRITE mode safe-dict.ts documents, left +// behind at the boundary of the very file that fixed the interior. + +test('SECURITY: a pattern variable named __proto__ appears as an own column in the result row', () => { + const space = new AtomSpace('sec7-patternmatcher', false) + space.addLink('ListLink', [space.addNode('ConceptNode', 'alice'), space.addNode('ConceptNode', 'bob')]) + + for (const name of HOSTILE_KEYS) { + const r = findMatches(space, { clauses: [L('ListLink', V(name), V('other'))] }) + + assert.ok(r.variables.includes(name), `${name}: precondition — the variable is declared`) + assert.equal(r.results.length, 1, `${name}: precondition — the pattern matches one grounding`) + + const row = r.results[0]! + assert.ok(isOwnData(row, name), + `${name}: declared in variables but not an own data property of the row — the write was swallowed`) + assert.equal(typeof (row as Record)[name], 'string', + `${name}: must read back as the atom label, not an inherited member`) + } +}) + +// ─── Attack 18: log-entry forgery at the log boundary ────────────────────────────────────── +// CodeQL js/log-injection, alert 28 (ts/src/super-peer.ts:318, MEDIUM). Receipts and audit +// records in this estate are read back as evidence, so a log line an attacker can SHAPE lets +// them author entries a later reader attributes to the system. + +const C = String.fromCharCode + +/** + * What a log READER can be made to misrender. + * + * Written as an explicit enumeration, deliberately NOT as the Unicode categories + * `sanitizeLogValue` happens to strip. The first version of this constant was + * `/[\p{Cc}\p{Zl}\p{Zp}]/u` — character-for-character the implementation's own class — so the + * general assertions below could not fail for anything the implementation had omitted. They + * were decoration that looked like coverage, and `\p{Cf}` survived behind them: U+202E and + * U+200B passed the whole suite. + * + * So this list is derived from the consumer's behaviour instead. Each range is here because of + * what it does to a reader or a parser, not because of which category it belongs to. If the + * implementation later drops a class this list still names, these tests go red — which is the + * only arrangement in which they are worth running. + */ +const UNSAFE_IN_A_LOG_FIELD = new RegExp( + '[' + + '\\u0000-\\u001F' + // C0: NUL truncates C-string consumers, ESC drives ANSI, VT/FF/CR/LF break + '\\u007F-\\u009F' + // DEL + C1 controls; U+0085 NEL is a real terminator to Unicode readers + '\\u00AD' + // SOFT HYPHEN — invisible; splits a token so a search never matches it + '\\u061C' + // ARABIC LETTER MARK — bidi control + '\\u200B-\\u200F' + // ZWSP/ZWNJ/ZWJ/LRM/RLM — invisible, or reorder the rest of the field + '\\u2028\\u2029' + // LINE / PARAGRAPH SEPARATOR + '\\u202A-\\u202E' + // bidi embed/override — the Trojan Source class + '\\u2066-\\u2069' + // bidi isolates + '\\uFEFF' + // BOM / ZWNBSP — invisible + ']', 'u') + +/** Kept under the old name so the assertions below read as before. */ +const LINE_BREAKING = UNSAFE_IN_A_LOG_FIELD + +const FORGERY_VECTORS: [string, string][] = [ + ['LF', C(0x0a)], + ['CR', C(0x0d)], + ['CRLF', C(0x0d) + C(0x0a)], + ['VT (vertical tab)', C(0x0b)], + ['FF (form feed)', C(0x0c)], + ['NEL U+0085', C(0x85)], + ['LS U+2028', C(0x2028)], + ['PS U+2029', C(0x2029)], + ['NUL', C(0x00)], + ['ANSI CSI erase-line', C(0x1b) + '[2K' + C(0x1b) + '[1G'], + // Forge the READING of the line rather than the line itself. None of these contain a + // terminator, so every CR/LF-shaped defence lets them straight through. + ['RLO U+202E (Trojan Source)', C(0x202e)], + ['LRO U+202D', C(0x202d)], + ['RLE U+202B', C(0x202b)], + ['bidi isolate U+2066', C(0x2066)], + ['bidi isolate pop U+2069', C(0x2069)], + ['RLM U+200F', C(0x200f)], + ['ZWSP U+200B', C(0x200b)], + ['ZWNJ U+200C', C(0x200c)], + ['SOFT HYPHEN U+00AD', C(0xad)], + ['BOM U+FEFF', C(0xfeff)], + ['ARABIC LETTER MARK U+061C', C(0x61c)], +] + +test('SECURITY: no control character can forge a second log line', () => { + for (const [name, sep] of FORGERY_VECTORS) { + const payload = `benign${sep}[super-peer] audit: admitted=attacker` + const out = sanitizeLogValue(payload) + + assert.ok(!LINE_BREAKING.test(out), `${name}: a line-breaking/control character survived sanitization`) + assert.equal(out.split(/\r|\n/u).length, 1, `${name}: output split into more than one line`) + // The forged text may remain as inert characters — what must NOT survive is the separator + // that turns it into its own record. + assert.ok(out.startsWith('benign'), `${name}: the real field content must be preserved`) + assert.ok(out.includes('\\x') || out.includes('\\u') || out.includes('\\r') || out.includes('\\n'), + `${name}: the neutralized character must be VISIBLE, so a reader can see it was there`) + } +}) + +test('SECURITY: an attacker-shaped SPARQL parse error logs as exactly one line', () => { + // End-to-end shape of alert 28: POST /query with a hostile query, the parse error embeds the + // offending token verbatim, and that message is what super-peer's catch block logs. + for (const [, sep] of FORGERY_VECTORS) { + const query = `SELECT ?s WHERE { ?s ?p ?o } GROUP "x${sep}[super-peer] request error: none"` + let message = '' + try { + runSparql(g, query) + continue // parsed without error; nothing reaches the log for this vector + } catch (e) { + message = (e as Error).message + } + const logged = sanitizeLogValue(message) + assert.ok(!LINE_BREAKING.test(logged), 'the logged line must contain no line-breaking character') + assert.equal(logged.split(/\r|\n/u).length, 1, 'one error must produce exactly one log line') + } +}) + +test('SECURITY: log fields are bounded and non-string input is coerced safely', () => { + const flood = 'A'.repeat(50_000) + C(0x0a) + 'forged' + const out = sanitizeLogValue(flood) + assert.ok(out.length <= LOG_FIELD_MAX + '[truncated]'.length, 'a single field cannot flood the log') + assert.ok(!LINE_BREAKING.test(out)) + + for (const weird of [undefined, null, 42, { toString: () => 'x' + C(0x0a) + 'forged' }]) { + const s = sanitizeLogValue(weird) + assert.equal(typeof s, 'string') + assert.ok(!LINE_BREAKING.test(s), 'coerced non-string input must be sanitized too') + } +}) + +/** + * Copilot review finding on #34. `String(value)` is not total, and this module's own PR is what + * makes the null-prototype case live: safe-dict.ts hands out `Object.create(null)` dictionaries + * throughout the engine, and super-peer calls the sanitizer from INSIDE a catch block — so a + * throw here would swallow the 500 response and turn the log boundary into the outage. + */ +test('SECURITY: the sanitizer never throws, even on values that refuse to stringify', () => { + const throwingToString = { toString() { throw new Error('boom') } } + const nullProto = Object.create(null) + const nullProtoWithProps = Object.assign(Object.create(null), { variable: '?__proto__' }) + + // Precondition: these really do defeat a bare String(). If this ever stops being true the + // test below is no longer exercising anything. + for (const v of [throwingToString, nullProto, nullProtoWithProps]) { + assert.throws(() => String(v), 'precondition: String() must throw for this value') + } + + for (const v of [throwingToString, nullProto, nullProtoWithProps]) { + const out = sanitizeLogValue(v) + assert.equal(typeof out, 'string') + assert.ok(out.length > 0, 'must produce something a reader can see') + assert.ok(!LINE_BREAKING.test(out), 'and it must still be a safe single-line field') + } + + // The exact shape super-peer's catch block passes when a non-Error is thrown. + assert.doesNotThrow(() => sanitizeLogValue(nullProto)) +}) + +/** + * Copilot review finding on #34 (low-confidence channel). A `catch` binding is not an Error — + * `throw 'boom'` and a promise rejected with a non-Error both land here — so `(e as Error).message` + * is `undefined`, and every such failure logs the same contentless line. The whole reason the raw + * value can be handed over instead is `coerceToString`: the sanitizer is total, so passing the + * value is the SAFE option, not the risky one. + */ +test('a non-Error throw keeps its diagnostic content at the log boundary', () => { + const notErrors: unknown[] = ['swarm refused: ECONNREFUSED', 42, { code: 'EAI_AGAIN' }, null] + + for (const e of notErrors) { + // What the bare cast produced: the content is gone. + const viaCast = sanitizeLogValue((e as Error | undefined)?.message) + assert.equal(viaCast, 'undefined', 'precondition: reading .message off a non-Error loses it') + + // What the call site does now: the value itself, coerced safely. + const viaValue = sanitizeLogValue(e instanceof Error ? e.message : e) + assert.notEqual(viaValue, 'undefined', `${String(e)}: must not collapse to "undefined"`) + assert.ok(viaValue.length > 0) + } + + // A real Error still logs its message, not its [object Error] tag. + assert.equal(sanitizeLogValue(new Error('real') instanceof Error ? new Error('real').message : ''), 'real') + + // And the non-Error path is still sanitized — it is untrusted text like any other. + const forged = sanitizeLogValue('swarm' + C(0x0a) + '[superpeer] audit: admitted=attacker') + assert.ok(!LINE_BREAKING.test(forged), 'a non-Error throw must not be able to forge a line either') +}) + +test('SECURITY: an astral-plane format character is escaped as itself, not as its surrogate', () => { + // \p{Cf} extends past the BMP. The `u`-flagged class matches the whole code point, so the + // character is neutralized regardless — but the escape is the forensic record of WHAT was + // neutralized, and charCodeAt would name the high surrogate (\ud80d) instead of U+13430. + for (const cp of [0x13430, 0x1bca0]) { + const out = sanitizeLogValue('benign' + String.fromCodePoint(cp) + 'tail') + assert.ok(!LINE_BREAKING.test(out), `U+${cp.toString(16)} survived sanitization`) + assert.ok(out.includes(`\\u{${cp.toString(16)}}`), + `U+${cp.toString(16)} must be escaped as its own code point, got ${JSON.stringify(out)}`) + assert.ok(!/\\ud8[0-9a-f]{2}/.test(out), 'must not report a bare surrogate') + } +}) + +test('SECURITY: sanitizing leaves ordinary log text untouched', () => { + const ordinary = "SPARQL parse error: expected 'BY', got 'LIMIT' (offset 42) — 100% ok" + assert.equal(sanitizeLogValue(ordinary), ordinary, 'the sanitizer must not mangle normal messages') +}) diff --git a/ts/src/sparql.ts b/ts/src/sparql.ts index 553a478..19194d1 100644 --- a/ts/src/sparql.ts +++ b/ts/src/sparql.ts @@ -1,5 +1,6 @@ import type { HellGraphStore } from './store' import type { Binding, PropertyValue, SparqlResult, Triple } from './types' +import { cloneDict, emptyDict, mergeDicts, toPlainRow } from './safe-dict.js' export type { Binding, SparqlResult } @@ -21,6 +22,17 @@ export type { Binding, SparqlResult } * restriction; then GROUP/aggregate or DISTINCT / ORDER / OFFSET / LIMIT / projection. */ +// ─── Solution bindings ───────────────────────────────────────────────────────── +// A Binding is keyed by SPARQL variable names, which come straight off the wire, so every +// intermediate binding is a null-prototype dictionary and output rows are materialized back +// onto a normal prototype at the boundary. See safe-dict.ts for why (js/remote-property-injection). +// NB: object spread `{ ...binding }` re-attaches Object.prototype — use cloneBinding/mergeBindings. + +const emptyBinding = (): Binding => emptyDict() +const cloneBinding = (b: Binding): Binding => cloneDict(b) +const mergeBindings = (a: Binding, b: Binding): Binding => mergeDicts(a, b) +const toResultRow = (entries: Iterable): Binding => toPlainRow(entries) + // ─── Term model ──────────────────────────────────────────────────────────────── type Term = @@ -412,7 +424,7 @@ function unquote(tok: string): string { // ─── Evaluator ─────────────────────────────────────────────────────────────── function matchTriple(pattern: TriplePattern, triple: Triple, binding: Binding): Binding | null { - const next: Binding = { ...binding } + const next: Binding = cloneBinding(binding) function unify(term: Term, value: PropertyValue): boolean { if (term.kind === 'var') { @@ -511,7 +523,7 @@ function evalPath(pattern: TriplePattern, binding: Binding, triples: Triple[]): for (const from of froms) { for (const to of [...reachFrom(from)].sort()) { if (ov !== null && to !== ov) continue // object bound → must equal - const b: Binding = { ...binding } + const b: Binding = cloneBinding(binding) if (sVar) b[sVar] = from if (oVar) b[oVar] = to results.push(b) @@ -539,12 +551,12 @@ function evalBGP(patterns: TriplePattern[], triples: Triple[], seed: Binding[]): function joinSolutions(a: Binding[], b: Binding[]): Binding[] { const out: Binding[] = [] - for (const x of a) for (const y of b) if (compatible(x, y)) out.push({ ...x, ...y }) + for (const x of a) for (const y of b) if (compatible(x, y)) out.push(mergeBindings(x, y)) return out } function evalGroup(group: GroupGraphPattern, triples: Triple[]): Binding[] { - let solutions = evalBGP(group.patterns, triples, [{}]) + let solutions = evalBGP(group.patterns, triples, [emptyBinding()]) // Lone nested { … } subgroups → natural join. for (const sub of group.subgroups) solutions = joinSolutions(solutions, evalGroup(sub, triples)) @@ -558,7 +570,7 @@ function evalGroup(group: GroupGraphPattern, triples: Triple[]): Binding[] { // VALUES: inline data → join. for (const vb of group.values) { const rows: Binding[] = vb.rows.map((row) => { - const b: Binding = {} + const b: Binding = emptyBinding() vb.vars.forEach((v, i) => { if (row[i] !== null && row[i] !== undefined) b[v] = row[i] as PropertyValue }) return b }) @@ -567,7 +579,11 @@ function evalGroup(group: GroupGraphPattern, triples: Triple[]): Binding[] { // BIND: compute a new variable per solution. for (const bind of group.binds) { - solutions = solutions.map((sol) => ({ ...sol, [bind.var]: evalBind(bind.expr, sol) ?? null } as Binding)) + solutions = solutions.map((sol) => { + const next = cloneBinding(sol) + next[bind.var] = evalBind(bind.expr, sol) ?? null + return next + }) } // OPTIONAL → left join @@ -576,7 +592,7 @@ function evalGroup(group: GroupGraphPattern, triples: Triple[]): Binding[] { for (const sol of solutions) { const matches = evalGroup(opt, triples).filter((m) => compatible(sol, m)) if (matches.length === 0) next.push(sol) - else for (const m of matches) next.push({ ...sol, ...m }) + else for (const m of matches) next.push(mergeBindings(sol, m)) } solutions = next } @@ -624,10 +640,13 @@ function aggregate(solutions: Binding[], groupBy: string[], aggregates: AggSpec[ const variables = [...groupBy, ...aggregates.map((a) => a.as)] const bindings: Binding[] = [] for (const rows of groups.values()) { - const out: Binding = {} - for (const g of groupBy) out[g] = rows[0]?.[g] ?? null - for (const a of aggregates) out[a.as] = computeAgg(a, rows) - bindings.push(out) + // Keys here are GROUP BY variables and aggregate aliases — both query-derived. Collect into + // an entry list and materialize via toResultRow so a name like `__proto__` is stored as an + // own data property instead of being swallowed by the inherited setter. + const out: [string, PropertyValue][] = [] + for (const g of groupBy) out.push([g, rows[0]?.[g] ?? null]) + for (const a of aggregates) out.push([a.as, computeAgg(a, rows)]) + bindings.push(toResultRow(out)) } return { variables, bindings } } @@ -770,13 +789,10 @@ export function runSparql(store: HellGraphStore, queryText: string): SparqlResul ? Array.from(new Set(solutions.flatMap((s) => Object.keys(s)))) : query.projection - let bindings = solutions.map((s) => { - // Build via a Map so projection variable names (query-derived) can't inject - // properties, then materialize a plain Binding (js/remote-property-injection). - const m = new Map() - for (const v of variables) m.set(v, s[v] ?? null) - return Object.fromEntries(m) as Binding - }) + // Projection variable names are query-derived. `s` is null-prototype, so `s[v]` can only ever + // return a value this query actually bound — never an inherited Object.prototype member — and + // toResultRow writes even a hostile name as an own data property. + let bindings = solutions.map((s) => toResultRow(variables.map((v) => [v, s[v] ?? null] as const))) // DISTINCT if (query.distinct) { diff --git a/ts/src/super-peer.ts b/ts/src/super-peer.ts index 2ebe472..8f936ea 100644 --- a/ts/src/super-peer.ts +++ b/ts/src/super-peer.ts @@ -36,6 +36,7 @@ import type { AuditSink } from './policy.js' import { Metrics } from './metrics.js' import { RateLimiter } from './rate-limit.js' import { loadOptionalDep as loadDep } from './optional-dep.js' +import { sanitizeLogValue } from './log-safe.js' /** Per-route scope requirement (enforced only when an auth verifier is configured). */ const ROUTE_SCOPE: Record = { @@ -312,9 +313,11 @@ export class SuperPeer { send(404, { error: 'not found' }) } catch (err) { this.metrics?.inc('hellgraph_errors_total') - // Don't leak internal error/stack detail to the client (js/stack-trace-exposure); - // log a newline-stripped, bounded detail server-side (no CR/LF → no log-injection). - const detail = (err instanceof Error ? err.message : String(err)).replace(/[\r\n\t]+/g, ' ').slice(0, 500) + // Don't leak internal error/stack detail to the client (js/stack-trace-exposure); log a + // bounded, single-line detail server-side. The message is attacker-shaped — a SPARQL parse + // error embeds the offending token verbatim — so it goes through sanitizeLogValue, which + // neutralizes every line terminator and control sequence, not just CR/LF (js/log-injection). + const detail = sanitizeLogValue(err instanceof Error ? err.message : err) console.error('[super-peer] request error:', detail) send(500, { error: 'internal error' }) } diff --git a/ts/src/superpeer-service.ts b/ts/src/superpeer-service.ts index 60846bf..a2cb64e 100644 --- a/ts/src/superpeer-service.ts +++ b/ts/src/superpeer-service.ts @@ -19,6 +19,7 @@ import { HmacTokenVerifier } from './auth.js' import { Metrics } from './metrics.js' import { RateLimiter } from './rate-limit.js' import { InMemoryAuditLog } from './policy.js' +import { sanitizeLogValue } from './log-safe.js' export interface SuperPeerServiceEnv { HELLGRAPH_STORAGE_DIR?: string @@ -78,7 +79,15 @@ export async function startSuperPeerFromEnv(env: SuperPeerServiceEnv = process.e } catch (e) { // Hyperswarm is an optional dependency; without it the node still serves + replicates // over any transport wired directly (e.g. a sidecar). Don't crash the pod. - console.warn(`[superpeer] joinSwarm skipped: ${(e as Error).message}`) + // Same log boundary as super-peer's request handler: this message originates outside the + // process (swarm/DHT peers), so it is untrusted text and must not be able to forge a line. + // + // The VALUE is handed over, not `(e as Error).message`: a rejection need not be an Error, + // and that cast reads `.message` off whatever it is — `undefined` for a thrown string, so + // every non-Error failure logs the identical useless line. `sanitizeLogValue` coerces + // total (see `coerceToString`), which is what makes passing the raw value the safe option + // rather than the risky one. Same shape super-peer's catch block uses. + console.warn(`[superpeer] joinSwarm skipped: ${sanitizeLogValue(e instanceof Error ? e.message : e)}`) } }