harden: remove capability that should never have shipped - #1344
Merged
Merged
Conversation
Microsoft's ML flagged the rc.1 release binaries. The decisive evidence is that the SAME sha256 went from 0/62 clean to Microsoft-detected in about an hour with no byte change, so the verdict lives partly in scanner-side state and no code change can promise a clean result. What code CAN do is stop shipping things that have no business in a release artifact, which is worth doing on its own merits and incidentally widens the classifier margin. Every claim below is verified against a built binary by the new gate, not by reading source. Executable stack (the worst of the findings). vendored/nomic/code_vectors_blob.S is the only assembly in the build and carried no .note.GNU-stack. An unannotated object makes ld assume the worst for the whole link, so EVERY Linux release we have ever shipped had GNU_STACK RWE. Adds the note (cause) plus ELF-only -Wl,-z,noexecstack (outcome); the gate fails the release if it returns. Test seams are now opt-in, never opt-out. TEST_SEAMS=1 defines CBM_ENABLE_TEST_SEAMS; without it the crash-orphan probe -- which forks a child that ignores SIGTERM and loops forever, then writes its pid to a caller-supplied path -- and the lease-ownership marker compile to trivial stubs, so call sites are untouched and the binary holds no fork, no signal handler and no env-var string. Opt-IN is the point: forgetting the flag yields a clean binary rather than a leaky one. scripts/test.sh requests it in the leg that consumes it, and tests/test_worker_watchdog.sh now asserts the capability up front instead of dying later with an opaque "Killed: 9". The daemon's background version check is gone. It spawned curl against github.com/ghapi/repos/.../releases/latest on the first eligible session of every run to say "a newer version exists" -- a release URL and an outbound request in every shipped binary, for something the install scripts already report. The INJECTABLE SEAM survives: update_ops is still honoured, the fakes in tests/test_daemon_application.c still cover notice/ownership/cancellation/replay, and with no provider application_update_subscribe_locked returns early so no generation ever starts. "No network request by default" is now structural. Dead capability out of release builds. The tar.gz/zip extraction block (gzip_decompress through cbm_extract_binary_from_zip, plus its cli.h declarations) moves under CBM_CLI_ENABLE_TEST_API -- verified self-contained, zero uses of any helper outside it, only callers the excluded updater and tests/test_cli.c. Downloading an archive, decompressing it, picking an executable out of it and marking it executable is the canonical dropper composite; it is now absent rather than merely unreachable. SQLite is built with -DSQLITE_OMIT_LOAD_EXTENSION (no caller of load_extension anywhere in src/ or internal/), removing that API surface and part of the dlopen/dlsym surface. Temp files and environment scanning (S2/S3). Predictable paths in mcp.c, artifact.c and diagnostics.c are created privately and exclusively and written through the returned descriptor; pass_envscan.c no longer descends symlinked directories out of the project root, and its fixed 512-byte path buffers no longer truncate into pointer arithmetic that could land outside the buffer. Build-time entropy. mimalloc's version banner baked __DATE__/__TIME__ into every binary, so two builds of identical source seconds apart could never share a hash and no release could inherit a false-positive determination made about its predecessor. Local patch removes it (marked to survive refreshes), -Wdate-time makes any future use a build error, and -Wl,--no-insert-timestamp stops the PE header carrying the link clock. scripts/ci/check-binary-composition.sh is the proof that each removal stays removed, wired into package-release.sh after strip so the local artifact-flow smoke enforces exactly what CI does. It asserts absences plus a CANARY string, so handing it a compressed, stubbed or empty file fails instead of passing vacuously, and a missing tool is a hard error -- a skipped assertion must never look like a satisfied one. Two build-system traps found by that gate, both of which had silently defeated a fix: the product binary is compiled in one shot from sources, so a flag flip did not rebuild it (now tracked by a .build-config stamp that also removes the binary, making it independent of mtime granularity); and prod_sqlite3.o / prod_mimalloc.o depended on a single named source, so SQLITE_OMIT_LOAD_EXTENSION and the mimalloc patch BOTH compiled to nothing on the first incremental build. Source review would have called them done. Deliberately NOT changed. Three seams stay in release artifacts because scripts/smoke-test.sh runs against the real artifact and needs them: CBM_TEST_CRASH_ON and CBM_TEST_HANG_ON inject the faults that prove supervisor recovery, and CBM_TEST_WINDOWS_USER_PATH_RUN_ID is what keeps the PATH smoke from writing the tester's actual PATH. The gate treats those as an allowlist, so a NOVEL seam still fails. The true no-UI standard build is deferred rather than rushed: src/ui/* is in PROD_SRCS and four files outside src/ui reference UI symbols, including the daemon that serves the UI, so that assertion reports instead of failing until the split lands -- a gate everyone knows is red teaches people to ignore gates. No grammar is removed. ObjectScript accounts for essentially all binary growth since the last provably-clean release (+21.4MB rodata, +1.1MB text from two four-line shims), which made it the obvious ablation candidate, but a dry run performed twelve real Defender endpoint scans across standard/UI and amd64/arm64 with ObjectScript, the daemon and the expanded hooks all present and every scan was clean. Nothing there is a deterministic trigger, so cutting a community-contributed language would spend a real feature on unproven margin. Lean is not a candidate either: at 99.6MB of source it is by far the largest grammar, but it shipped in v0.9.0 which scanned 20/20 clean, so removing it would produce a novel unscanned profile instead of restoring a known-good one. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The update handoff moved out of the product on every platform, but two loose ends were left behind. README was actively WRONG, not merely incomplete. It still documented in-place updates for macOS/Linux, and it promised "The MCP server also checks for updates on startup and notifies on the first tool call" -- a claim that died with the MCP update thread and then the daemon's curl check. The section now states the one contract that holds everywhere (`update` validates flags and prints the install script command), why it works that way (on Windows a running image cannot replace itself; on POSIX a deliberate choice, because an in-process updater is structurally a downloader), and the affirmative property that replaced it: cbm makes no network request of its own accord and the archives carry no download URLs at all. `uninstall` now REPORTS the install script it finds beside the binary, printing the path and the `rm` command, and says "would leave" under --dry-run. It does not delete it. install.sh places itself there so `update` has something to hand off to, but that file may equally be the user's own copy, a symlink into a checkout, or package-manager managed -- an uninstaller must not delete a file it cannot prove it owns, and one printed line leaves the decision where it belongs. Windows checks install.ps1 too and tolerates either path separator. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Removes capability that should never have shipped, and adds a release gate that proves it stays removed. Driven by a private static audit of the rc.1 artifacts after Microsoft's ML flagged them.
The framing matters, because it sets what this PR can and cannot claim. The same sha256 (
9e2f0f12…, linux-amd64) went from 0/62 clean to Microsoft-detected in about an hour with no byte change — our own run-134 log versus a later rescan of the identical file. So part of that verdict lives in scanner-side state, and no code change can promise a clean scan. What code can do is stop shipping malware-shaped surface that has no business in a release artifact. Every item below stands on its own security merit; a wider classifier margin is a side effect, not the justification.The findings
Executable stack on every Linux binary we have ever shipped.
vendored/nomic/code_vectors_blob.Sis the only assembly in the build and carried no.note.GNU-stack. An unannotated object makesldassume the worst for the whole link, so every release hadGNU_STACK RWE. Fixed at the cause (the note) and the outcome (ELF-only-Wl,-z,noexecstack). Verified on a real Linux ELF:RWE→RW.A test probe that forks a SIGTERM-ignoring immortal child and writes its pid to a caller-supplied path shipped in production (
src/main.c). Test seams are now opt-in (TEST_SEAMS=1→CBM_ENABLE_TEST_SEAMS); disabled builds compile a trivial success stub so call sites are untouched and the binary holds no fork, no signal handler and no env-var string. Opt-in deliberately: forgetting the flag now yields a clean binary rather than a leaky one.The daemon spawned
curlagainst the GitHub releases API on the first eligible session of every run, to say "a newer version exists". That put a release URL and an outbound request in every shipped binary. Deleted. The injectable seam survives —update_opsis still honoured and the fakes intests/test_daemon_application.cstill cover notice / ownership / cancellation / generation-replay — and with no providerapplication_update_subscribe_lockedreturns early, so no generation ever starts. "No network request by default" is now structural, not a promise.Dead capability. The tar.gz/zip extraction block (verified self-contained, zero uses of any helper outside it, only callers the already-excluded updater and
tests/test_cli.c) moves underCBM_CLI_ENABLE_TEST_API. Download → decompress → pick an executable → mark it executable is the canonical dropper composite; it is now absent, not merely unreachable. SQLite is built-DSQLITE_OMIT_LOAD_EXTENSION(no caller anywhere insrc/orinternal/).Temp files and env scanning. Predictable
/tmp/cbm_search_<pid>.pat-style paths opened with plainfopen→ privatecbm_mkdtempdirectory +cbm_mkstempexclusive creation, written through the returned descriptor and never reopened by name.pass_envscan.cno longer descends symlinked directories out of the project root (lstaton POSIX, reparse-point screening on Windows wherelstatdoes not exist), and its fixed 512-byte buffers no longer truncate into pointer arithmetic that could land outside the buffer.Build-time entropy. mimalloc's banner baked
__DATE__/__TIME__into every binary, so two builds of identical source seconds apart could never share a hash — and no release could ever inherit a false-positive determination made about its predecessor. Local patch removes it (marked to survive refreshes),-Wdate-timemakes any future use a build error,-Wl,--no-insert-timestampstops the PE header carrying the link clock.The gate
scripts/ci/check-binary-composition.sh, wired intopackage-release.shafter strip (the last byte-changing step) so the local artifact-flow smoke enforces exactly what the release does. Asserts absences — test seams, updater URLs, SQLite load-extension, UI/HTTP in standard artifacts, non-executable stack — plus a canary string, because an absence check aimed at a compressed, stubbed or empty file would otherwise pass vacuously and read green. A missing tool is a hard error: a skipped assertion must never look like a satisfied one.It earned its keep immediately. Both
SQLITE_OMIT_LOAD_EXTENSIONand the mimalloc patch silently compiled to nothing on the first rebuild, becauseprod_sqlite3.o/prod_mimalloc.odepended on a single named source and make never recompiled them. Source review would have called both "done" — this is very likely why earlier fixes in this saga appeared to work for one release and then stopped. Now fixed: those objects depend onMakefile.cbm(and mimalloc, an amalgamation, on its included sources), plus a.build-configstamp that removes the binary on any config change so the guarantee does not rest on mtime granularity.Deliberately NOT done
CBM_TEST_CRASH_ON,CBM_TEST_HANG_ON,CBM_TEST_WINDOWS_USER_PATH_RUN_ID.scripts/smoke-test.shruns against the real release artifact and needs them — the run-id is what stops the PATH smoke writing the tester's actual PATH. Removing them would trade genuine release-artifact coverage for a cosmetic win. The gate treats them as an allowlist, so a novel seam still fails.src/ui/*is inPROD_SRCSand four files outsidesrc/uireference UI symbols (src/main.c,src/daemon/host.c,src/daemon/application.c,src/mcp/index_supervisor.c), including the daemon that serves the UI. That assertion reports INFO until the split lands (CBM_CHECK_UI_ABSENT=1enforces): a gate everyone knows is red teaches people to ignore gates. All six UI needles were verified unique tosrc/ui/so it will bite correctly when enabled.Also in this PR
README.mdwas actively wrong, not merely incomplete: it documented in-place updates for macOS/Linux and promised "The MCP server also checks for updates on startup and notifies on the first tool call" — a claim that died with the MCP update thread and then the daemon's curl check. Rewritten to state the one contract that holds everywhere, why it works that way, and the affirmative property that replaced it.uninstallnow reports the install script beside the binary (path +rmcommand, "would leave" under--dry-run) but does not delete it: that file may be the user's own copy, a symlink into a checkout, or package-manager managed, and an uninstaller must not delete what it cannot prove it owns.Verification
RWE → RWconfirmed against the flagged artifact.test_windows_update_handoff.py("update handed off to install.ps1 without touching its own image"). One reported failure was an invalid suite name in my own runner script, re-run clean with names verified against--list-suites.tests/test_worker_watchdog.sh: it asserts the seam is present and says how to get it, instead of dying later with an opaqueKilled: 9.Risk
Behaviour-preserving by construction for the removals (dead or test-only code). The two real behaviour changes are the daemon no longer performing a background version check (intended, documented) and
uninstallprinting one extra section. The gate is release-blocking, which is the point — but it fails closed on a missing tool or an unreadable file, so a broken toolchain surfaces as a red gate rather than a silent pass.