From cb1239c5964c7e0d836a80774d8c34778e95c720 Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Thu, 16 Jul 2026 10:59:29 +0200 Subject: [PATCH 1/2] fix: harden Miranda per security review (keychain, relay DoS, LAN, supply chain) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- go/cmd/mir-signal/main.go | 6 + go/internal/agent/lan.go | 21 ++- go/internal/client/keychain.go | 34 ++++- go/internal/client/keychain_test.go | 76 ++++++++++ go/internal/client/lan_locator_test.go | 4 + go/internal/quicmsg/quicmsg.go | 30 ++-- go/internal/quicmsg/quicmsg_test.go | 7 + go/internal/selfupdate/cosign.go | 9 +- go/internal/signal/hardening_test.go | 183 +++++++++++++++++++++++++ go/internal/signal/revocations.go | 22 ++- go/internal/signal/server.go | 54 +++++++- scripts/verify-reproducible.sh | 14 +- 12 files changed, 435 insertions(+), 25 deletions(-) create mode 100644 go/internal/client/keychain_test.go create mode 100644 go/internal/signal/hardening_test.go diff --git a/go/cmd/mir-signal/main.go b/go/cmd/mir-signal/main.go index e7842ea..f9ec76e 100644 --- a/go/cmd/mir-signal/main.go +++ b/go/cmd/mir-signal/main.go @@ -292,6 +292,12 @@ func setStaticSecurityHeaders(w http.ResponseWriter, nonce string) { w.Header().Set("Referrer-Policy", "no-referrer") w.Header().Set("X-Content-Type-Options", "nosniff") w.Header().Set("X-Frame-Options", "DENY") + // HSTS at the origin (defense in depth; the CDN may also set it). This host is + // a passkey/crypto trust root, so pin TLS for future visits. No + // includeSubDomains — this asserts only for the exact host it is served from, + // so it cannot force HSTS onto sibling subdomains. Browsers ignore it over + // plain HTTP (e.g. local dev), so it is safe to send unconditionally. + w.Header().Set("Strict-Transport-Security", "max-age=31536000") w.Header().Set("Permissions-Policy", "camera=(self), microphone=(), geolocation=(), payment=(), usb=(), serial=()") // The SPA currently has unhashed /src and /vendor paths. Prefer freshness over // stale trusted-code delivery until a content-hashed build exists. diff --git a/go/internal/agent/lan.go b/go/internal/agent/lan.go index 633589e..ea256db 100644 --- a/go/internal/agent/lan.go +++ b/go/internal/agent/lan.go @@ -5,6 +5,7 @@ import ( "context" "net" "strconv" + "time" "github.com/grandcat/zeroconf" @@ -16,6 +17,13 @@ import ( // browse for this to discover an agent's ephemeral QUIC port on the local network. const lanService = "_miranda._udp" +// lanPreAuthTimeout bounds the pre-authentication phase of a LAN-direct +// connection — accepting the peer's stream and reading the owner binding +// (frame 0). A peer that opens a QUIC connection but then stalls must not hold an +// admit() slot indefinitely; it fails here and releases the slot. The full Noise +// session that follows runs under the agent's long-lived context, not this one. +const lanPreAuthTimeout = 10 * time.Second + // startLAN opens a QUIC listener for LAN-direct attach and advertises it over mDNS. // Returns the bound address (for callers/tests) and a stop func. Each connection runs // the same binding-gated authenticated session as the relay path. @@ -86,7 +94,18 @@ func (rt *Runtime) lanAccept(ctx context.Context, conn *quicmsg.Conn) { } defer rt.release() - bindingJSON, err := conn.Recv(ctx) // frame 0 + // Bound the whole pre-auth phase: accepting the peer's stream and reading the + // owner binding. A silent peer fails here rather than parking on an admit() + // slot forever. serveAuthenticated below uses ctx (not authCtx) so an + // authenticated session is not cut off after the pre-auth window. + authCtx, cancel := context.WithTimeout(ctx, lanPreAuthTimeout) + defer cancel() + + if err := conn.Establish(authCtx); err != nil { + return // peer never opened a stream in time + } + + bindingJSON, err := conn.Recv(authCtx) // frame 0 if err != nil { return } diff --git a/go/internal/client/keychain.go b/go/internal/client/keychain.go index 5ff6efb..ebd4c87 100644 --- a/go/internal/client/keychain.go +++ b/go/internal/client/keychain.go @@ -80,14 +80,23 @@ func (s commandSecretStore) Get(ref string) ([]byte, error) { func (s commandSecretStore) Put(ref string, secret []byte) error { var cmd *exec.Cmd + var input []byte if runtime.GOOS == "darwin" { - // With -w last, security reads the password from stdin. It never appears in - // argv (and therefore not in ps output, shell history, or diagnostics). - cmd = exec.Command(s.path, "add-generic-password", "-a", ref, "-s", keychainService, "-U", "-w") + // See darwinPutInput: `add-generic-password ... -w` (flag last, no value) + // does NOT read the secret from piped stdin — security prompts via + // readpassphrase and silently stores an EMPTY secret when stdin is a pipe. + // Drive interactive command mode so the whole command, secret included, + // travels on stdin — never argv (no ps exposure), never a tty prompt. + in, err := darwinPutInput(ref, keychainService, secret) + if err != nil { + return fmt.Errorf("%s: could not store owner secret (plaintext fallback is disabled)", s.kind) + } + cmd = exec.Command(s.path, "-i") + input = in } else { cmd = exec.Command(s.path, "store", "--label=Miranda owner identity", "service", keychainService, "owner", ref) + input = append(append([]byte(nil), secret...), '\n') } - input := append(append([]byte(nil), secret...), '\n') defer zeroBytes(input) cmd.Stdin = bytes.NewReader(input) cmd.Stdout = io.Discard @@ -113,6 +122,23 @@ func (s commandSecretStore) Delete(ref string) error { return nil } +// darwinPutInput builds the stdin fed to `security -i` for a keychain write. +// The secret must be safe to place on the interactive command line (which lives +// entirely on stdin, never argv): no whitespace that would end the token early +// and no quote/backslash that `security`'s tokenizer would treat specially. The +// owner secret is hex, so this always holds; the guard is fail-closed defense in +// depth against a caller passing raw bytes. Returned bytes hold the secret — the +// caller must zero them. +func darwinPutInput(ref, service string, secret []byte) ([]byte, error) { + if !secretRefRE.MatchString(ref) || !secretRefRE.MatchString(service) { + return nil, fmt.Errorf("invalid keychain reference") + } + if len(secret) == 0 || bytes.ContainsAny(secret, " \t\r\n\"'\\") { + return nil, fmt.Errorf("secret is not command-line safe") + } + return []byte(fmt.Sprintf("add-generic-password -a %s -s %s -U -w %s\n", ref, service, secret)), nil +} + // fileSecretStore is test-only; platformSecretStore refuses to select it from a // production binary even if the environment variable is present. type fileSecretStore struct{ dir string } diff --git a/go/internal/client/keychain_test.go b/go/internal/client/keychain_test.go new file mode 100644 index 0000000..7f54562 --- /dev/null +++ b/go/internal/client/keychain_test.go @@ -0,0 +1,76 @@ +package client + +import ( + "bytes" + "strings" + "testing" +) + +// TestDarwinPutInputCarriesSecretInBand is the regression guard for the macOS +// keychain bug where `security add-generic-password -w` (flag last, no value) +// read the secret from a tty prompt instead of piped stdin and silently stored +// an EMPTY secret with exit 0 — breaking every `mir list`/`up`/`attach` with +// "owner secret is empty". The fix drives `security -i`, carrying the whole +// command (secret included) on stdin. This must stay true. +func TestDarwinPutInputCarriesSecretInBand(t *testing.T) { + ref := "owner-0123456789abcdef0123456789abcdef" + secret := []byte("a1b2c3d4e5f60718293a4b5c6d7e8f90a1b2c3d4e5f60718293a4b5c6d7e8f90") // 64 hex + + in, err := darwinPutInput(ref, keychainService, secret) + if err != nil { + t.Fatalf("darwinPutInput returned error for valid input: %v", err) + } + got := string(in) + // The secret must appear verbatim after `-w` (the store bug produced input + // that stored nothing) and the command must end with a newline so `security + // -i` executes it. + if !strings.Contains(got, "-w "+string(secret)+"\n") { + t.Fatalf("secret not carried in-band after -w; input=%q", got) + } + if !strings.HasPrefix(got, "add-generic-password -a "+ref+" -s "+keychainService+" -U -w ") { + t.Fatalf("unexpected command shape: %q", got) + } +} + +func TestDarwinPutInputRejectsUnsafeSecret(t *testing.T) { + ref := "owner-0123456789abcdef0123456789abcdef" + // A secret with whitespace/quotes/backslash could break the command line and + // must be refused (fail-closed) rather than silently mangled. + for _, bad := range [][]byte{ + []byte("has space"), + []byte("has\nnewline"), + []byte("has\"quote"), + []byte("has\\backslash"), + []byte{}, // empty + } { + if _, err := darwinPutInput(ref, keychainService, bad); err == nil { + t.Fatalf("expected rejection for unsafe secret %q", bad) + } + } +} + +func TestDarwinPutInputRejectsBadRef(t *testing.T) { + secret := []byte("deadbeef") + for _, bad := range []string{"", "has space", "has/slash", "a;rm -rf", strings.Repeat("x", 200)} { + if _, err := darwinPutInput(bad, keychainService, secret); err == nil { + t.Fatalf("expected rejection for bad ref %q", bad) + } + } +} + +// TestDarwinPutInputSecretNotInArgv documents the security property: the secret +// travels only via the returned stdin bytes, so it never reaches the process +// argv (ps/shell-history exposure). This is a shape assertion, not a live exec. +func TestDarwinPutInputSecretNotInArgv(t *testing.T) { + ref := "owner-0123456789abcdef0123456789abcdef" + secret := []byte("cafebabecafebabe") + in, err := darwinPutInput(ref, keychainService, secret) + if err != nil { + t.Fatal(err) + } + // The command is `security -i`; argv holds no secret. Confirm the secret is + // present in the stdin payload only. + if !bytes.Contains(in, secret) { + t.Fatal("secret missing from stdin payload") + } +} diff --git a/go/internal/client/lan_locator_test.go b/go/internal/client/lan_locator_test.go index 71b6bdf..6bcc44a 100644 --- a/go/internal/client/lan_locator_test.go +++ b/go/internal/client/lan_locator_test.go @@ -44,6 +44,10 @@ func TestLANLocatorDialSendsBinding(t *testing.T) { gotErr <- err return } + if err := conn.Establish(actx); err != nil { + gotErr <- err + return + } frame, err := conn.Recv(actx) if err != nil { gotErr <- err diff --git a/go/internal/quicmsg/quicmsg.go b/go/internal/quicmsg/quicmsg.go index eb32ecb..1a1965c 100644 --- a/go/internal/quicmsg/quicmsg.go +++ b/go/internal/quicmsg/quicmsg.go @@ -226,25 +226,37 @@ func Listen(addr string) (*Listener, error) { // the ephemeral port). func (l *Listener) Addr() net.Addr { return l.ln.Addr() } -// Accept waits for the next QUIC connection, accepts its first bidirectional -// stream, consumes the open-nudge frame Dial sent, and wraps it in a *Conn. +// Accept waits for the next QUIC connection and wraps it in a *Conn. The Conn is +// NOT yet usable for Send/Recv: the caller must call Establish (under a deadline, +// off the accept path) to accept the peer's stream and consume the open-nudge +// frame. Splitting these two phases is deliberate — doing AcceptStream inline +// here would let a peer that completes the QUIC handshake but never opens a +// stream block the entire accept loop indefinitely (head-of-line DoS). Accept +// only blocks on the genuine "wait for the next connection" step. func (l *Listener) Accept(ctx context.Context) (*Conn, error) { conn, err := l.ln.Accept(ctx) if err != nil { return nil, err } - stream, err := conn.AcceptStream(ctx) + return &Conn{conn: conn}, nil +} + +// Establish accepts the peer's single bidirectional stream and consumes the +// open-nudge frame Dial sent, making the Conn usable for Send/Recv. It honors +// ctx: pass a deadline so a peer that connects but never opens a stream fails +// here (releasing whatever pre-auth budget the caller reserved) instead of +// parking forever. Safe to call exactly once per Conn returned by Accept. +func (c *Conn) Establish(ctx context.Context) error { + stream, err := c.conn.AcceptStream(ctx) if err != nil { - _ = conn.CloseWithError(0, "") - return nil, err + return err } - c := &Conn{conn: conn, stream: stream} + c.stream = stream // Consume the empty open-nudge frame Dial wrote to flush the stream open. if _, err := c.Recv(ctx); err != nil { - _ = c.Close() - return nil, err + return err } - return c, nil + return nil } // Close closes the listener (does not close already-accepted connections). diff --git a/go/internal/quicmsg/quicmsg_test.go b/go/internal/quicmsg/quicmsg_test.go index 3447d0f..40e270d 100644 --- a/go/internal/quicmsg/quicmsg_test.go +++ b/go/internal/quicmsg/quicmsg_test.go @@ -36,6 +36,9 @@ func TestConnRoundTripFrames(t *testing.T) { accepted := make(chan acceptResult, 1) go func() { c, err := ln.Accept(ctx) + if err == nil { + err = c.Establish(ctx) + } accepted <- acceptResult{c, err} }() @@ -119,6 +122,10 @@ func TestRecvRespectsContext(t *testing.T) { accepted <- nil return } + if err := c.Establish(dialCtx); err != nil { + accepted <- nil + return + } accepted <- c }() diff --git a/go/internal/selfupdate/cosign.go b/go/internal/selfupdate/cosign.go index 9f74895..37a5d84 100644 --- a/go/internal/selfupdate/cosign.go +++ b/go/internal/selfupdate/cosign.go @@ -51,10 +51,11 @@ func (c *Client) verifyChecksumsSignature(rel *Release, sums []byte, note func(s return fmt.Errorf("cosign is required to verify Miranda releases") } - // A release cut before signing was introduced carries no .sig/.pem. cosign - // being installed cannot conjure them — fall back rather than hard-fail so - // upgrading FROM an old release still works. (The next signed tag is the - // first one that will actually exercise verification.) + // Fail closed: a release that carries no .sig/.pem is refused. Do NOT soften + // this into a fallback — accepting an unsigned release would let anyone who + // can publish (or MITM) a GitHub release strip the signature and push a + // malicious binary, which is exactly the attack cosign verification exists to + // stop. This matches SECURITY.md's stated guarantee. if rel.ChecksumsSigURL == "" || rel.ChecksumsCertURL == "" { return fmt.Errorf("release has no cosign signature; refusing update") } diff --git a/go/internal/signal/hardening_test.go b/go/internal/signal/hardening_test.go new file mode 100644 index 0000000..3d91ddc --- /dev/null +++ b/go/internal/signal/hardening_test.go @@ -0,0 +1,183 @@ +package signal + +import ( + "bytes" + "context" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/coder/websocket" +) + +// waitAgentCount polls the live-agent map until it reaches want, or fails. +func waitAgentCount(t *testing.T, s *Server, want int) { + t.Helper() + for i := 0; i < 100; i++ { + s.mu.Lock() + n := len(s.agents) + s.mu.Unlock() + if n == want { + return + } + time.Sleep(20 * time.Millisecond) + } + s.mu.Lock() + n := len(s.agents) + s.mu.Unlock() + t.Fatalf("agent count = %d, want %d", n, want) +} + +// TestAgentCapBoundsNewRegistrations is the regression guard for the unbounded +// agent-registration memory DoS: without a cap, an attacker opens a registration +// per (owner, machine) and holds each socket (with a retained registry blob). +// New slots must be refused past the cap, while replacing an existing slot +// (routine on restart) must still succeed. +func TestAgentCapBoundsNewRegistrations(t *testing.T) { + s := New() + s.SetMaxAgents(2) + srv := httptest.NewServer(s.Handler()) + defer srv.Close() + + a0 := registerAgentWithRegistry(t, srv.URL, "o", "m0", "blob0") + defer a0.CloseNow() + a1 := registerAgentWithRegistry(t, srv.URL, "o", "m1", "blob1") + defer a1.CloseNow() + waitAgentCount(t, s, 2) + + // A brand-new third slot is refused: the upgrade succeeds but the relay closes + // the socket before sending Ready, and the map never grows past the cap. + third := dialJSON(t, wsURL(srv.URL, "/agent/signal", map[string]string{"owner_id": "o", "machine_id": "m2"})) + defer third.CloseNow() + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + if _, _, err := third.Read(ctx); err == nil { + t.Fatal("third registration past the cap should be refused, but the socket stayed open") + } + s.mu.Lock() + _, m2Live := s.agents[key("o", "m2")] + n := len(s.agents) + s.mu.Unlock() + if m2Live { + t.Fatal("m2 was admitted despite the cap") + } + if n != 2 { + t.Fatalf("agent count = %d, want 2 (cap holds)", n) + } + + // Replacing an already-registered slot must still work at the cap — it does + // not grow the map. + replace := registerAgentWithRegistry(t, srv.URL, "o", "m0", "blob0b") + defer replace.CloseNow() + waitAgentCount(t, s, 2) +} + +// TestOversizedRegistryBlobDropped guards the per-agent registry memory bound: +// a blob larger than maxRegistryBlobBytes (but under the 256 KiB signal limit) is +// dropped rather than retained, removing the memory-amplification an attacker +// would otherwise get. +func TestOversizedRegistryBlobDropped(t *testing.T) { + srv := httptest.NewServer(New().Handler()) + defer srv.Close() + + oversized := strings.Repeat("A", maxRegistryBlobBytes+1) + a := registerAgentWithRegistry(t, srv.URL, "wallet-Z", "big", oversized) + defer a.CloseNow() + // Give the reader a moment to process (and drop) the blob. + time.Sleep(200 * time.Millisecond) + entries := getRegistry(t, srv.URL, "wallet-Z", http.StatusOK) + if len(entries) != 0 { + t.Fatalf("oversized blob was retained: %+v", entries) + } + + // Control: a normally-sized blob under the same owner is still served. + small := registerAgentWithRegistry(t, srv.URL, "wallet-Z", "small", "tiny-blob") + defer small.CloseNow() + time.Sleep(200 * time.Millisecond) + entries = getRegistry(t, srv.URL, "wallet-Z", http.StatusOK) + if len(entries) != 1 || entries[0].MachineID != "small" { + t.Fatalf("small blob not served alongside dropped oversized blob: %+v", entries) + } +} + +// TestDuplicateRevocationSkipsPersist proves the fsync is skipped for an exact +// replay: after the first POST persists the file, the store directory is made +// unwritable so any real persist would fail with 500. A byte-identical replay +// must still return 204 — which is only possible if it never touches disk. +func TestDuplicateRevocationSkipsPersist(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "revocations.json") + s := New() + if err := s.LoadRevocations(path); err != nil { + t.Fatal(err) + } + srv := httptest.NewServer(s.Handler()) + defer srv.Close() + + record := signedRevocation(t, bytes.Repeat([]byte{0x71}, 32), "machine-dup") + + resp := postRevocation(t, srv.URL, record) + io.Copy(io.Discard, resp.Body) + resp.Body.Close() + if resp.StatusCode != http.StatusNoContent { + t.Fatalf("first POST status = %d, want 204", resp.StatusCode) + } + + // Make the store dir unwritable: a real persist (CreateTemp + rename) would now + // fail. Restore perms on cleanup so t.TempDir removal succeeds. + if err := os.Chmod(dir, 0o500); err != nil { + t.Fatal(err) + } + defer os.Chmod(dir, 0o700) + + dup := postRevocation(t, srv.URL, record) + io.Copy(io.Discard, dup.Body) + dup.Body.Close() + if dup.StatusCode != http.StatusNoContent { + t.Fatalf("duplicate POST status = %d, want 204 (persist must be skipped, not attempted)", dup.StatusCode) + } +} + +// TestRegistryAndRevocationsNoStore ensures a caching intermediary cannot serve a +// stale registry or revocation list (a stale empty revocation list could let a +// client attach to a revoked machine). +func TestRegistryAndRevocationsNoStore(t *testing.T) { + srv := httptest.NewServer(New().Handler()) + defer srv.Close() + + for _, u := range []string{"/registry?owner_id=w", "/revocations?owner_id=w"} { + resp, err := http.Get(srv.URL + u) + if err != nil { + t.Fatalf("GET %s: %v", u, err) + } + got := resp.Header.Get("Cache-Control") + io.Copy(io.Discard, resp.Body) + resp.Body.Close() + if got != "no-store" { + t.Fatalf("GET %s Cache-Control = %q, want no-store", u, got) + } + } +} + +// TestAgentRegistrationRejectsPipeInIdentifiers guards the owner|machine slot +// namespace: '|' in an identifier must be refused so an attacker cannot inject a +// bogus entry into another owner's registry listing. +func TestAgentRegistrationRejectsPipeInIdentifiers(t *testing.T) { + srv := httptest.NewServer(New().Handler()) + defer srv.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + _, resp, err := websocket.Dial(ctx, wsURL(srv.URL, "/agent/signal", map[string]string{"owner_id": "victim|evil", "machine_id": "m"}), nil) + if err == nil { + t.Fatal("expected the dial to be rejected") + } + if resp == nil || resp.StatusCode != http.StatusBadRequest { + t.Fatalf("status = %v, want 400", resp) + } +} diff --git a/go/internal/signal/revocations.go b/go/internal/signal/revocations.go index a231097..fdb6702 100644 --- a/go/internal/signal/revocations.go +++ b/go/internal/signal/revocations.go @@ -85,6 +85,10 @@ func (s *Server) getRevocations(w http.ResponseWriter, r *http.Request) { } s.mu.Unlock() sort.Slice(out, func(i, j int) bool { return out[i].MachineID < out[j].MachineID }) + // Revocation freshness is security-relevant: a cached (pre-revocation) empty + // list could let a client that relies on the relay fetch still attach to a + // revoked machine. Forbid caching intermediaries from serving a stale list. + w.Header().Set("Cache-Control", "no-store") w.Header().Set("Content-Type", "application/json") _ = json.NewEncoder(w).Encode(out) } @@ -110,12 +114,24 @@ func (s *Server) postRevocation(w http.ResponseWriter, r *http.Request) { slot := key(record.OwnerID, record.MachineID) s.mu.Lock() - if _, exists := s.revoked[slot]; !exists && s.maxRevocations > 0 && len(s.revoked) >= s.maxRevocations { + existing, exists := s.revoked[slot] + if exists && existing == record { + // Exact replay of an already-recorded tombstone changes nothing. Skip the + // whole-file marshal + fsync entirely: without this, resending one + // observed, validly-signed revocation forces an O(n) rewrite under the + // global broker lock on every request — a cheap way to stall all + // signaling. (Revocation is permanent, so a byte-identical record is a + // pure no-op.) + s.mu.Unlock() + s.logf("event=machine_revoked owner=%s machine=%s duplicate=true ip=%s", shortID(record.OwnerID), shortID(record.MachineID), s.remoteIP(r)) + w.WriteHeader(http.StatusNoContent) + return + } + if !exists && s.maxRevocations > 0 && len(s.revoked) >= s.maxRevocations { s.mu.Unlock() http.Error(w, capacityReason, http.StatusServiceUnavailable) return } - _, existed := s.revoked[slot] s.revoked[slot] = record ac := s.agents[slot] delete(s.agents, slot) @@ -129,7 +145,7 @@ func (s *Server) postRevocation(w http.ResponseWriter, r *http.Request) { http.Error(w, "could not persist revocation", http.StatusInternalServerError) return } - s.logf("event=machine_revoked owner=%s machine=%s duplicate=%t ip=%s", shortID(record.OwnerID), shortID(record.MachineID), existed, s.remoteIP(r)) + s.logf("event=machine_revoked owner=%s machine=%s duplicate=%t ip=%s", shortID(record.OwnerID), shortID(record.MachineID), exists, s.remoteIP(r)) w.WriteHeader(http.StatusNoContent) } diff --git a/go/internal/signal/server.go b/go/internal/signal/server.go index 340bfa4..1042c5c 100644 --- a/go/internal/signal/server.go +++ b/go/internal/signal/server.go @@ -48,6 +48,20 @@ const ( defaultMaxRateEntries = 8192 defaultMaxRevocations = 100000 capacityReason = "server capacity reached" + + // defaultMaxAgents caps the number of concurrently registered agent + // sockets. Without a cap, an attacker can open a registration per + // (owner_id, machine_id) — the non-32-byte owner_id path is unauthenticated — + // and hold each socket open with a retained registry blob, growing relay + // memory without bound. The cap turns an unbounded memory-exhaustion DoS into + // a bounded slot-exhaustion that only affects new registrations. + defaultMaxAgents = 4096 + + // maxRegistryBlobBytes caps the opaque device blob an agent may publish on + // its live registration. A registry blob is a small AEAD-sealed device list; + // bounding it well below maxSignalMessageBytes (256 KiB) removes the 256 KiB + // per-agent memory amplification an attacker would otherwise get for free. + maxRegistryBlobBytes = 16 << 10 ) const ( @@ -104,6 +118,7 @@ type Server struct { revocationsFile string maxRevocations int + maxAgents int // Logf records one structured line per relay event (register, replace, // reject, gone, attach, flap, stats). It is never nil at runtime: New() @@ -265,6 +280,16 @@ func New() *Server { maxPairRooms: defaultMaxPairRooms, limiter: newRequestLimiter(defaultMaxRateEntries), maxRevocations: defaultMaxRevocations, + maxAgents: defaultMaxAgents, + } +} + +// SetMaxAgents overrides the concurrent-agent-registration cap. A value <= 0 +// leaves the default in place. Use it to size the relay to the host's memory: +// worst-case retained registry memory is roughly maxAgents * maxRegistryBlobBytes. +func (s *Server) SetMaxAgents(n int) { + if n > 0 { + s.maxAgents = n } } @@ -401,6 +426,9 @@ func (s *Server) handleRegistry(w http.ResponseWriter, r *http.Request) { } } s.mu.Unlock() + // A stale registry listing must never be served: no-store keeps any caching + // intermediary (CDN/proxy) from pinning an old view of an owner's live agents. + w.Header().Set("Cache-Control", "no-store") w.Header().Set("Content-Type", "application/json") _ = json.NewEncoder(w).Encode(out) // [] when none; encode never fails the relay } @@ -438,6 +466,14 @@ func (s *Server) handleAgent(w http.ResponseWriter, r *http.Request) { http.Error(w, "missing owner_id/machine_id", http.StatusBadRequest) return } + // '|' is the internal owner|machine slot separator. Forbidding it in the + // identifiers keeps an attacker from registering owner_id="VICTIM|x" and + // thereby injecting a bogus entry into VICTIM's GET /registry listing (the + // blob still fails the victim's AEAD, but the namespace must stay clean). + if strings.ContainsRune(owner, '|') || strings.ContainsRune(machine, '|') { + http.Error(w, "invalid owner_id/machine_id", http.StatusBadRequest) + return + } proof := r.Header.Get(AgentRegistrationSecretHeader) authorization := r.Header.Get(AgentRegistrationAuthHeader) ip := s.remoteIP(r) @@ -490,6 +526,16 @@ func (s *Server) handleAgent(w http.ResponseWriter, r *http.Request) { return } prev := s.agents[k] + // Bound total live registrations. Replacing an existing slot (prev != nil) is + // always allowed — it is routine on agent restart and does not grow the map — + // but a brand-new slot is refused once the cap is reached so a flood of + // distinct (owner, machine) pairs cannot exhaust memory. + if prev == nil && s.maxAgents > 0 && len(s.agents) >= s.maxAgents { + s.mu.Unlock() + s.logf("event=agent_reject reason=capacity owner=%s machine=%s ip=%s", ownerS, machineS, ip) + c.Close(websocket.StatusTryAgainLater, capacityReason) + return + } s.proofs.learn(k, proof) s.agents[k] = ac // Track replacements while holding s.mu so the flap counter is consistent @@ -552,7 +598,13 @@ func (s *Server) handleAgent(w http.ResponseWriter, r *http.Request) { if m.Type == TypeRegistry { // The agent publishes its opaque encrypted device blob on the // live registration. The relay holds it in-memory (no persist, - // no decrypt) and serves it via GET /registry. + // no decrypt) and serves it via GET /registry. An oversized blob + // is dropped: a real registry blob is a small AEAD-sealed device + // list, so anything larger is only useful for memory + // amplification. + if len(m.Registry) > maxRegistryBlobBytes { + continue + } ac.setRegistry(m.Registry) continue } diff --git a/scripts/verify-reproducible.sh b/scripts/verify-reproducible.sh index a221211..712c5b3 100755 --- a/scripts/verify-reproducible.sh +++ b/scripts/verify-reproducible.sh @@ -10,9 +10,17 @@ trap 'rm -rf "$TMP"' EXIT VERSION="${VERSION:-$(git -C "$ROOT" describe --tags --always --dirty)}" VERSION="${VERSION#v}" -COMMIT="$(git -C "$ROOT" rev-parse --short=8 HEAD)" -DATE="$(git -C "$ROOT" show -s --format=%cI HEAD)" -export SOURCE_DATE_EPOCH="$(git -C "$ROOT" show -s --format=%ct HEAD)" +# These -X values are baked into the binary, so to reproduce a *published* +# release the recipe must derive them EXACTLY as GoReleaser does (.goreleaser.yaml): +# .ShortCommit -> first 7 chars of the full SHA (not `--short=8`, and not git's +# variable-length abbreviation). +# .CommitDate -> the commit time in UTC RFC3339 with a literal Z (not `%cI`, +# which emits the local-timezone offset and made the hash both +# wrong vs. the release AND dependent on the reviewer's timezone). +# With the old 8-char commit / local-tz date, this script could only ever prove +# self-consistency (build twice locally); it never matched an honest release. +COMMIT="$(git -C "$ROOT" rev-parse HEAD | cut -c1-7)" +DATE="$(TZ=UTC0 git -C "$ROOT" show -s --date=format-local:%Y-%m-%dT%H:%M:%SZ --format=%cd HEAD)" build_set() { local output="$1" cache="$2" From b6f798175c505bd2db2e27a3ad44482e15004ba4 Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Thu, 16 Jul 2026 17:42:10 +0200 Subject: [PATCH 2/2] fix: complete the deferred security-review follow-ups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/test.yml | 2 + go/internal/signal/hardening_test.go | 55 +++++++++++++++++++++++++++ go/internal/signal/revocations.go | 47 +++++++++++++++++++---- go/internal/signal/server.go | 10 +++++ web/src/app.js | 30 +++++++++++---- web/tools/verify-vendor.sh | 56 ++++++++++++++++++++++++++++ 6 files changed, 185 insertions(+), 15 deletions(-) create mode 100755 web/tools/verify-vendor.sh diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 20a4f10..b97a1f0 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -57,6 +57,8 @@ jobs: run: npm test - working-directory: web run: npm audit --omit=dev + - name: vendored crypto bundles match pinned packages + run: ./web/tools/verify-vendor.sh # The Go and web jobs together are the byte-identical crypto gate: go test # writes/asserts testdata/ vectors and npm test asserts the JS side matches the diff --git a/go/internal/signal/hardening_test.go b/go/internal/signal/hardening_test.go index 3d91ddc..009166b 100644 --- a/go/internal/signal/hardening_test.go +++ b/go/internal/signal/hardening_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" "time" @@ -143,6 +144,60 @@ func TestDuplicateRevocationSkipsPersist(t *testing.T) { } } +// TestConcurrentRevocationsAllPersist exercises the lock-free, versioned +// persister: many distinct revocations posted concurrently must all survive to +// disk. The stale-snapshot skip is only safe if a higher version is always a +// superset — this asserts no write is lost. +func TestConcurrentRevocationsAllPersist(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "revocations.json") + s := New() + if err := s.LoadRevocations(path); err != nil { + t.Fatal(err) + } + srv := httptest.NewServer(s.Handler()) + defer srv.Close() + + const n = 12 + records := make([]struct { + root []byte + machine string + }, n) + for i := range records { + records[i].root = bytes.Repeat([]byte{byte(0x40 + i)}, 32) + records[i].machine = "machine-conc-" + string(rune('a'+i)) + } + + var wg sync.WaitGroup + for i := range records { + wg.Add(1) + go func(i int) { + defer wg.Done() + rec := signedRevocation(t, records[i].root, records[i].machine) + resp := postRevocation(t, srv.URL, rec) + io.Copy(io.Discard, resp.Body) + resp.Body.Close() + if resp.StatusCode != http.StatusNoContent { + t.Errorf("POST %d status = %d, want 204", i, resp.StatusCode) + } + }(i) + } + wg.Wait() + + // Reload from disk in a fresh server: every concurrently-posted tombstone must + // be present and signature-valid. + reloaded := New() + if err := reloaded.LoadRevocations(path); err != nil { + t.Fatalf("reload: %v", err) + } + reloaded.mu.Lock() + got := len(reloaded.revoked) + reloaded.mu.Unlock() + if got != n { + t.Fatalf("persisted %d revocations, want %d (a concurrent write was lost)", got, n) + } +} + // TestRegistryAndRevocationsNoStore ensures a caching intermediary cannot serve a // stale registry or revocation list (a stale empty revocation list could let a // client attach to a revoked machine). diff --git a/go/internal/signal/revocations.go b/go/internal/signal/revocations.go index fdb6702..f3827e3 100644 --- a/go/internal/signal/revocations.go +++ b/go/internal/signal/revocations.go @@ -133,14 +133,19 @@ func (s *Server) postRevocation(w http.ResponseWriter, r *http.Request) { return } s.revoked[slot] = record + s.revVersion++ + version := s.revVersion + snapshot := s.snapshotRevocationsLocked() ac := s.agents[slot] delete(s.agents, slot) - err := s.persistRevocationsLocked() s.mu.Unlock() + // Enforcement (tearing down the live agent) has already happened under s.mu; + // durability is best-effort and now runs off the broker lock so a slow fsync + // cannot stall signaling. if ac != nil { ac.teardown() } - if err != nil { + if err := s.persistRevocations(version, snapshot); err != nil { s.logf("event=revocation_persist_error owner=%s machine=%s", shortID(record.OwnerID), shortID(record.MachineID)) http.Error(w, "could not persist revocation", http.StatusInternalServerError) return @@ -149,12 +154,10 @@ func (s *Server) postRevocation(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) } -// persistRevocationsLocked writes the complete signed tombstone set atomically. -// s.mu must be held so concurrent POSTs cannot reorder whole-file snapshots. -func (s *Server) persistRevocationsLocked() error { - if s.revocationsFile == "" { - return nil - } +// snapshotRevocationsLocked returns a stable, sorted copy of the tombstone set. +// s.mu must be held. The copy is what the (lock-free) persister writes, so a slow +// fsync never touches s.revoked or blocks the broker lock. +func (s *Server) snapshotRevocationsLocked() []identity.Revocation { list := make([]identity.Revocation, 0, len(s.revoked)) for _, record := range s.revoked { list = append(list, record) @@ -165,6 +168,34 @@ func (s *Server) persistRevocationsLocked() error { } return list[i].OwnerID < list[j].OwnerID }) + return list +} + +// persistRevocations writes a versioned snapshot to disk under persistMu (NOT the +// broker lock). Writers are serialized here, and a snapshot whose version is +// already <= revPersisted is skipped: because every version bump happens under +// s.mu together with the map write, a higher-versioned snapshot is always a +// superset, so dropping the stale write still leaves a superset on disk. +func (s *Server) persistRevocations(version uint64, list []identity.Revocation) error { + if s.revocationsFile == "" { + return nil + } + s.persistMu.Lock() + defer s.persistMu.Unlock() + if version <= s.revPersisted { + return nil // a newer (superset) snapshot already reached disk + } + if err := s.writeRevocationsFile(list); err != nil { + return err + } + s.revPersisted = version + return nil +} + +// writeRevocationsFile atomically writes the given snapshot. It reads no shared +// state, so it holds neither s.mu nor (beyond the caller) persistMu longer than +// the disk write. Callers pass a snapshot taken under s.mu. +func (s *Server) writeRevocationsFile(list []identity.Revocation) error { data, err := json.MarshalIndent(revocationFile{V: 1, Revocations: list}, "", " ") if err != nil { return err diff --git a/go/internal/signal/server.go b/go/internal/signal/server.go index 1042c5c..77d2c2e 100644 --- a/go/internal/signal/server.go +++ b/go/internal/signal/server.go @@ -120,6 +120,16 @@ type Server struct { maxRevocations int maxAgents int + // Revocation durability is serialized on persistMu, NOT the broker lock s.mu, + // so the whole-file fsync no longer stalls all signaling. Each in-memory + // change (under s.mu) bumps revVersion and snapshots the set; the persister + // (under persistMu) writes that snapshot and records revPersisted. A snapshot + // whose version is already <= revPersisted is skipped — a newer snapshot is a + // superset, so it is safe to drop the stale write. + persistMu sync.Mutex + revVersion uint64 + revPersisted uint64 + // Logf records one structured line per relay event (register, replace, // reject, gone, attach, flap, stats). It is never nil at runtime: New() // installs a no-op so the broker code can call it unconditionally; the diff --git a/web/src/app.js b/web/src/app.js index d9a2b3d..f6f7f88 100644 --- a/web/src/app.js +++ b/web/src/app.js @@ -21,20 +21,36 @@ import jsQR from '/vendor/jsqr.js'; const te = new TextEncoder(); const td = new TextDecoder(); -const DEFAULT_STUN = 'stun:stun.l.google.com:19302'; - -// iceServers builds the ICE config: a default STUN plus ephemeral TURN creds -// fetched from the signaling server (for symmetric-NAT / cellular reachability). -// TURN only ever relays ciphertext — Noise keeps content end-to-end. +// Default STUN server for server-reflexive candidate discovery. Overridable via +// window.MIRANDA_STUN (set it to '' to disable) so a privacy-conscious deployment +// can point at its own STUN instead of a third party. A bare STUN request leaks +// the client IP to whoever runs the server, so this default is only ever used as +// a last resort — see iceServers. +const DEFAULT_STUN = (typeof window !== 'undefined' && typeof window.MIRANDA_STUN === 'string') + ? window.MIRANDA_STUN + : 'stun:stun.l.google.com:19302'; + +// iceServers builds the ICE config. It prefers the relay's own ephemeral TURN +// credentials: a TURN server already yields a server-reflexive candidate via its +// built-in STUN, so when TURN is available we do NOT add a standalone third-party +// STUN server — that would leak the client IP to a third party on every attach +// for no connectivity gain. Only when the relay offers no TURN do we fall back to +// the (configurable) default STUN. TURN, when used, relays ciphertext only — +// Noise keeps content end-to-end. async function iceServers(signalURL) { - const list = [{ urls: DEFAULT_STUN }]; + const list = []; + let haveTurn = false; try { const r = await fetch(signalURL.replace(/\/$/, '') + '/turn-credentials'); if (r.ok) { const t = await r.json(); - if (t.urls && t.urls.length) list.push({ urls: t.urls, username: t.username, credential: t.password }); + if (t.urls && t.urls.length) { + list.push({ urls: t.urls, username: t.username, credential: t.password }); + haveTurn = true; + } } } catch {} + if (!haveTurn && DEFAULT_STUN) list.unshift({ urls: DEFAULT_STUN }); return list; } diff --git a/web/tools/verify-vendor.sh b/web/tools/verify-vendor.sh new file mode 100755 index 0000000..5e0ece5 --- /dev/null +++ b/web/tools/verify-vendor.sh @@ -0,0 +1,56 @@ +#!/usr/bin/env bash +# Regenerate the vendored @noble crypto bundles from the lockfile-pinned packages +# and fail if they differ from what is committed under web/vendor/. +# +# Why: the Go<->JS interop vectors validate the @noble code in node_modules, but +# the browser SPA imports the *vendored* bundles via the importmap in index.html. +# Without this check a vendored crypto bundle could drift from the pinned @noble +# version — accidental staleness, or a malicious edit to web/vendor/*.js — and +# still pass the whole test suite, the interop vectors, and npm audit. This closes +# that gap: it re-bundles each @noble entry point with a pinned esbuild and +# byte-compares the result against the committed file. +# +# Scope: the six @noble crypto bundles only. They reproduce byte-identically. +# xterm/@xterm-addon-fit/jsqr are deliberately NOT gated here — they are UI, not +# crypto, and their bundles carry an esbuild-version-dependent CJS wrapper that is +# not byte-stable across esbuild releases. +# +# Prerequisite: `npm ci` has been run in web/ so node_modules/@noble exists. +set -euo pipefail + +# Pin esbuild: the byte-for-byte output is deterministic per esbuild version, so +# this version must match what the committed bundles were built with. Bump it only +# together with a fresh regeneration + commit of web/vendor/*. +ESBUILD_VERSION="0.28.1" + +ROOT="$(cd "$(dirname "$0")/.." && pwd)" # web/ +cd "$ROOT" + +names=(noble-curves-ed25519 noble-ciphers-chacha noble-hashes-sha2 noble-hashes-hmac noble-hashes-hkdf noble-hashes-utils) +specs=("@noble/curves/ed25519" "@noble/ciphers/chacha" "@noble/hashes/sha2" "@noble/hashes/hmac" "@noble/hashes/hkdf" "@noble/hashes/utils") + +TMP="$(mktemp -d)" +ENTRY="$ROOT/.vendor-verify-entry.js" # must live under web/ so esbuild resolves @noble from web/node_modules +trap 'rm -rf "$TMP" "$ENTRY"' EXIT + +fail=0 +for i in "${!names[@]}"; do + name="${names[$i]}" + spec="${specs[$i]}" + printf "export * from '%s';\n" "$spec" >"$ENTRY" + npx --yes "esbuild@${ESBUILD_VERSION}" "$ENTRY" \ + --bundle --minify --format=esm --outfile="$TMP/$name.js" >/dev/null 2>&1 + if ! cmp -s "$TMP/$name.js" "vendor/$name.js"; then + echo "VENDOR DRIFT: vendor/$name.js does not match a fresh esbuild of $spec" + fail=1 + fi +done + +if [ "$fail" -ne 0 ]; then + echo + echo "One or more vendored @noble crypto bundles differ from the pinned packages." + echo "Regenerate with esbuild@${ESBUILD_VERSION} (--bundle --minify --format=esm) and commit," + echo "or investigate why web/vendor/ was edited out of band." + exit 1 +fi +echo "vendored @noble crypto bundles match the lockfile-pinned packages"