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/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..009166b --- /dev/null +++ b/go/internal/signal/hardening_test.go @@ -0,0 +1,238 @@ +package signal + +import ( + "bytes" + "context" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync" + "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) + } +} + +// 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). +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..f3827e3 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,35 +114,50 @@ 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 + 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 } - 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) } -// 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) @@ -149,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 340bfa4..77d2c2e 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,17 @@ type Server struct { revocationsFile string 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() @@ -265,6 +290,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 +436,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 +476,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 +536,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 +608,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" 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"