fix: harden Miranda per security review (keychain, relay DoS, LAN, supply chain)#53
Open
frahlg wants to merge 2 commits into
Open
fix: harden Miranda per security review (keychain, relay DoS, LAN, supply chain)#53frahlg wants to merge 2 commits into
frahlg wants to merge 2 commits into
Conversation
…pply chain) Fixes surfaced by a full security + E2E review of the v0.7.0 restructure. macOS keychain (functional, high) — commandSecretStore.Put drove `security add-generic-password -w` reading the secret from piped stdin, but that flag prompts via readpassphrase and silently stored an EMPTY secret, breaking every `mir list/up/attach` on macOS with "owner secret is empty". The unit tests never caught it because they use the file-backed store. Drive `security -i` so the whole command (secret included) travels on stdin — never argv, never a tty prompt. Extracted darwinPutInput + regression tests. Relay registration/registry DoS (high) — the agents map was unbounded and the non-32-byte owner_id path is unauthenticated, so an attacker could open a registration per (owner, machine) and hold each socket with a retained 256 KiB registry blob until OOM. Cap concurrent registrations (defaultMaxAgents, SetMaxAgents), always allow replacing an existing slot, and cap the retained registry blob to 16 KiB. Revocation replay stall (medium) — every /revocations POST, including an exact replay of a validly-signed record, marshalled + fsync'd the whole file under the global broker lock. Skip the persist entirely when the record is byte-identical to the stored one. LAN-direct head-of-line DoS (medium) — the accept loop did AcceptStream inline, so a peer that completed the QUIC handshake but never opened a stream blocked all LAN-direct attach, and the frame-0 read had no pre-auth deadline. Split quicmsg Accept (connection only) from Establish (stream + nudge, off the accept path), and bound the pre-auth phase with lanPreAuthTimeout. Reproducible-release recipe (medium) — verify-reproducible.sh baked different ldflags than GoReleaser (8-char commit, local-tz date), so it could never reproduce an honest release. Match .ShortCommit (7 chars) and .CommitDate (UTC RFC3339); verified it now reproduces the released binary's embedded Commit/Date. Lower-severity hardening — Cache-Control: no-store on /registry and /revocations (stale revocation list could let a client attach to a revoked machine); reject '|' in owner_id/machine_id (registry-listing injection); HSTS at the origin; corrected the cosign fail-closed comment that described the opposite (soft-fallback) behavior. Verified: go test ./... (+ -race), 111 web tests, byte-identical crypto vectors, reproducibility script, and full local E2E (pair → encrypted run over relay and LAN-direct) all green. Co-Authored-By: Claude Opus 4.8 <[email protected]>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb1239c596
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+117
to
+118
| existing, exists := s.revoked[slot] | ||
| if exists && existing == record { |
There was a problem hiding this comment.
When durable revocations are enabled and the first POST reaches persistRevocationsLocked() but the write fails, s.revoked[slot] has already been updated and is not rolled back. A client retrying the exact same signed tombstone will now hit this duplicate fast path, return 204, and never retry the disk write, so the revocation is lost on relay restart even though the retry appeared successful.
Useful? React with 👍 / 👎.
The three items called out as follow-ups in the hardening PR. Revocation fsync off the broker lock (medium) — /revocations POST previously marshalled + fsync'd the whole file while holding s.mu, the single lock every signaling request takes, so a slow disk stalled all signaling. Persistence now runs on its own mutex: each in-memory change (under s.mu) bumps a version and snapshots the set, and a lock-free persister writes the snapshot, skipping any snapshot whose version is already superseded (a higher version is always a superset, so the skip is safe). Added a concurrent-persist test. CI gate for browser-shipped crypto (medium) — the Go<->JS interop vectors only validated the @noble code in node_modules, not the vendored bundles the SPA actually imports via the importmap. web/tools/verify-vendor.sh re-bundles each @noble entry point with a pinned esbuild and byte-compares against web/vendor/, wired into the web CI job. Scoped to the six @noble crypto bundles (which reproduce byte-identically); xterm/jsqr are UI and carry a non-byte-stable esbuild wrapper, so they are intentionally out of scope. STUN client-IP privacy (low) — the web client hardcoded Google STUN and sent it on every attach, leaking the client IP to a third party. It now prefers the relay's own TURN (which already yields a server-reflexive candidate, so no separate STUN is needed) and only falls back to a default STUN when no TURN is offered — and that default is overridable via window.MIRANDA_STUN (set '' to disable). Verified: go test ./... (+ -race on signal), 111 web tests, verify-vendor.sh, and a live revocation E2E (local enforcement + relay publish + durable persist through the refactored lock-free persister). Co-Authored-By: Claude Opus 4.8 <[email protected]>
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
1 participant
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.
Full security + E2E review of the v0.7.0 restructure. Core architecture and invariants held (relay never sees plaintext, prf-rooted identity, byte-identical Go/JS crypto) — no critical findings. This PR fixes every actionable one, each with a regression test.
Fixes
macOS keychain — functional, high.
commandSecretStore.Putdrovesecurity add-generic-password -wreading the secret from piped stdin, but that flag prompts viareadpassphraseand silently stored an empty secret — breaking everymir list/up/attachon macOS with"owner secret is empty". The unit tests never caught it because they use the file-backed store, never the real keychain. Now drivessecurity -iso the whole command (secret included) travels on stdin — never argv (nopsexposure), never a tty prompt. Found via E2E, not tests.Relay registration/registry DoS — high. The
agentsmap was unbounded and the non-32-byteowner_idpath is unauthenticated, so an attacker could open a registration per(owner, machine)and hold each socket with a retained 256 KiB registry blob → OOM on the 512 MB relay box. Cap concurrent registrations (defaultMaxAgents+SetMaxAgents), always allow replacing an existing slot, and cap the retained blob to 16 KiB.Revocation replay stall + fsync under the broker lock — medium. Every
/revocationsPOST — including an exact replay of a validly-signed record — marshalled + fsync'd the whole file unders.mu, the single lock every signaling request takes. Now: an exact replay is a no-op (skips persist), and persistence runs on its own mutex with a versioned snapshot (a lock-free persister writes off the broker lock; stale snapshots are safely skipped because a higher version is always a superset).LAN-direct head-of-line DoS — medium. The accept loop did
AcceptStreaminline, so a peer that completed the QUIC handshake but never opened a stream blocked all LAN-direct attach; the frame-0 read had no pre-auth deadline. Splitquicmsg.Accept(connection only) fromEstablish(stream + nudge, off the accept path) and bound the pre-auth phase withlanPreAuthTimeout.Reproducible-release recipe — medium.
verify-reproducible.shbaked different ldflags than GoReleaser (8-char commit, local-tz date), so it could never reproduce an honest release — the reproducibility claim in SECURITY.md was structurally broken. Now matches.ShortCommit(7 chars) and.CommitDate(UTC RFC3339); verified it reproduces the released binary's embeddedCommit/Dateexactly.Browser-shipped crypto now gated by CI — medium. The Go↔JS interop vectors only validated the
@noblecode innode_modules, not the vendored bundles the SPA imports via the importmap.web/tools/verify-vendor.shre-bundles each@nobleentry point with a pinned esbuild and byte-compares againstweb/vendor/, wired into the web CI job. Scoped to the six@noblecrypto bundles (which reproduce byte-identically); xterm/jsqr are UI and carry a non-byte-stable esbuild wrapper, so they are out of scope.STUN client-IP privacy — low. The web client hardcoded Google STUN and sent it on every attach, leaking the client IP to a third party. It now prefers the relay's own TURN (which already yields a server-reflexive candidate) and only falls back to a default STUN when no TURN is offered — and that default is overridable via
window.MIRANDA_STUN(set''to disable).Lower-severity hardening.
Cache-Control: no-storeon/registry+/revocations(a stale empty revocation list could let a client attach to a revoked machine); reject|inowner_id/machine_id(registry-listing injection); HSTS at the origin; corrected the cosign fail-closed comment that described the opposite (soft-fallback) behavior.Verification
go test ./...+-race: greenverify-reproducible.sh(matches the released binary's embedded fields) andverify-vendor.sh: greenmir list→ encryptedmir runover both the relay path and LAN-direct; plus a revocation E2E (local enforcement + relay publish + durable persist through the lock-free persister): green🤖 Generated with Claude Code