fix(datalog): reject forged dl-program handles instead of dereferencing them - #438
Conversation
…ng them dl-program wrapped a raw dl_program_t* in a plain -RAY_I64 atom with no tag, and dl_unwrap_program reinterpreted ANY i64 as that pointer. So a one-line expression — (dl-free 1), (dl-stratify 1), (dl-query 1 'x), (dl-eval 1), (dl-add-edb 1 ...) — dereferenced an attacker-chosen address and crashed with SIGSEGV. On a server that evaluates client input over IPC this is a remotely-triggerable crash, and a chosen integer is an arbitrary-pointer free/deref (memory corruption), not merely a DoS. Tag the handle atom with RAY_ATTR_DLPROG (mirroring the RAY_ATTR_GRAPH / RAY_ATTR_HNSW handle scheme) and verify it in dl_unwrap_program, so a plain integer or an arithmetic copy (which does not carry attrs) is rejected with a type error. dl-free clears the tag and zeroes the pointer on free, keeping its idempotent-false contract for a genuine double-free while rejecting forged handles. The bit reuses 0x20 (RAY_ATTR_SORTED / ATTR_QUOTED), which is vector / -RAY_SYM scoped and never read on a -RAY_I64 atom, and no free-path touches 0x20 — so there is no finalizer collision. Also closes the arithmetic-copy double-free (a copy loses the tag, so only the original frees) and the snapshot/ser-de-restored stale-handle crash (the restored atom loses the tag and is rejected). Adds forged/copied-handle coverage to datalog_coverage.rfl.
singaraiona
left a comment
There was a problem hiding this comment.
Reviewed with an adversarial runtime pass (ASan/UBSan, repros run on both head and pre-PR master). The fix is correct and lands a real security win — approving. Pre-PR, (dl-free 1) (a forged integer handle from one line of client input) hits a UBSan "misaligned address 0x1" and SIGSEGVs — an arbitrary-pointer deref. Post-PR, (dl-free 1), (dl-stratify 1), (dl-query 1 'x), (dl-eval 1) are all cleanly rejected with type errors. That's the whole point of the PR and it works.
I did chase down four concerns hard (the tag reuses attr bit 0x20, which is also RAY_ATTR_SORTED, and for a -RAY_I64 atom len aliases i64 in the union — both confirmed). None is a blocker after runtime testing:
- Double-free via block copy (the scary one): does NOT reproduce. The aliasing mechanism is real (
ray_retain_owned_refshas HNSW/GRAPH branches but no DLPROG one), but no rfl surface produces such a copy:(set G H)shares the atom (rc bump, not a copy) so the firstdl-freeclears the shared tag and the second returnsfalse;(alter 'H set 0 0)type-errors on theray_is_vec/LISTguard beforeray_cowand before the bounds check, so the union-aliasing path is unreachable too;(+ H 0)/(first (enlist H))produce untagged copies thatdl-freerejects — which is exactly the arithmetic-copy escape the PR closes. No ASan double-free or UAF from any variant. - Pointer disclosure via
.attr.drop: real but negligible.(.attr.drop H)does return the raw pointer via the unguarded SORTED branch — but a-RAY_I64handle already prints as its own pointer value, pre- and post-PR alike, so nothing is disclosed beyond evaluating the handle. ASan-clean (the returned copy is untagged, unre-freeable). - Leak on last-ref drop without
dl-free: real, memory-safe. Confirmed via.mem.ts(LSan can't see it —dl_program_newuses the ray buddy pool, not malloc): dropping a handle without freeing leaks ~2MB (net-bytes: 2097152), vsnet-bytes: 0withdl-free, while a GRAPH handle dropped without.graph.freeisnet-bytes: 0. So the free-path asymmetry (finalizers for HNSW/GRAPH, none for DLPROG) is genuine.
Recommended follow-up hardening (non-blocking — I'd take them in a separate commit or PR):
- Add DLPROG branches to
ray_retain_owned_refs, the copy-detach path, and an rc→0 finalizer, mirroring GRAPH/HNSW. This closes both the latent double-free (should a generic deep-copy-over-atoms builtin ever be added) and the finding-3 leak at once. - Guard
ray_attr_drop_fnagainst-RAY_I64handles (add theray_is_veccheck the other SORTED readers have). dl-freeopen-codes theattrs & RAY_ATTR_DLPROGpredicate instead of routing throughdl_unwrap_program(datalog.c:4530) — the one free-calling path drifting from the accessor is how the next gate-strengthening gets missed; peer frees reuse their unwrap.- Update the bit-allocation table in heap.h (~55) to record
0x20 / -RAY_I64 / RAY_ATTR_DLPROG— right now the reuse is documented only in the#defineblock, so the next handle type could claim the same bit.
Approving and merging — the security fix is sound; the above are all defense-in-depth on top of it.
How it looks from the user's side
A single line — a typo, or a stray value from an IPC client — takes the whole process down.
Before (unpatched
dev), a user types(dl-free 1):The value
1is dereferenced as adl_program_t*. Everydl-*builtin unwraps its handle argument, so(dl-stratify 1),(dl-query 1 'r),(dl-eval 1),(dl-add-edb 1 …)crash the same way. On a server evaluating client input over IPC (-U/-i) this is remotely triggerable — one client kills every connected session — and since the integer is used directly as a pointer tofree()/dereference, an attacker-chosen value is an arbitrary-pointer free/deref (memory corruption), not merely a DoS.After (this PR) — the same inputs return a clean type error and the process keeps running:
Fix
Tag the handle atom with
RAY_ATTR_DLPROG, mirroring the existingRAY_ATTR_GRAPH/RAY_ATTR_HNSWhandle scheme, and verify it indl_unwrap_program:dl-freeclears the tag and zeroes the pointer on free, so it keeps its idempotent-falsecontract for a genuine double-free (datalog_coverage.rflline 542) while rejecting a forged handle with a type error.Bit choice:
RAY_ATTR_DLPROGreuses0x20(same value asRAY_ATTR_SORTEDon vectors /ATTR_QUOTEDon-RAY_SYM). Those meanings are vector- /-RAY_SYM-scoped and are never read on a-RAY_I64atom, and — unlikeHAS_INDEX(0x08) /SLICE(0x10) /ARENA(0x80) — no heap free-path or finalizer reads0x20, so there is no collision on GC. This is the same context-reuse the codebase already does (RAY_ATTR_HNSWandRAY_ATTR_HAS_LINKshare0x04).Also fixed by the same tag
(+ h 0)produces a freshi64without the tag, so a copy can no longer be freed (which would double-free the original). The original still frees exactly once.ser→derestored stale handle: serialization strips atom attrs, so a restored handle loses the tag and is rejected instead of dereferencing an address from the previous process. Verified:(dl-free (de (ser (dl-program))))→ type error, no crash.Out of scope (separate follow-ups): a dropped handle still leaks (needs a heap finalizer entry), and restricted-IPC clients can still call
dl-free(needsRAY_FN_RESTRICTED).Tests
test/rfl/datalog/datalog_coverage.rfl: forged-handle rejection fordl-free/dl-stratify/dl-eval/dl-query/dl-add-edb, and the arithmetic-copy double-free case. The existing handle-lifecycle claims (free → idempotent-false→ use-after-free type error) are unchanged.Full ASan+UBSan suite green:
3708 of 3709 passed (1 skipped, 0 failed).