From 93c08f9de13ce60aad8388e4689da7802de47e4d Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 6 Aug 2026 03:23:49 +0400 Subject: [PATCH 1/2] netaddr: greet a tcp peer off the accept path (V-581) A peer that connected and then said nothing froze the whole seam. The token handshake ran inline in Listener.Accept, so the five seconds of handshakeTimeout the silent peer was owed were five seconds no other connection could be accepted. One unauthenticated stranger holding a socket open was a denial of service on every daemon behind a tcp seam, which is the path V-515 is about to put mavwaked and mavenclient on. Accept now takes authorized connections off a channel. A background loop pulls from the wrapped listener and greets each connection in its own goroutine, so a slow greeting costs only its own connection. Listener.Close releases anything still waiting to be handed over. A unix seam delegates straight to the wrapped listener and grows no machinery, because it has no handshake to run. Co-Authored-By: Claude Opus 5 --- internal/netaddr/netaddr.go | 80 +++++++++++++++++++++++++++----- internal/netaddr/netaddr_test.go | 36 ++++++++++++++ 2 files changed, 105 insertions(+), 11 deletions(-) diff --git a/internal/netaddr/netaddr.go b/internal/netaddr/netaddr.go index 11bb4bd..11e1403 100644 --- a/internal/netaddr/netaddr.go +++ b/internal/netaddr/netaddr.go @@ -31,6 +31,7 @@ import ( "os" "path/filepath" "strings" + "sync" "time" "golang.org/x/sys/unix" @@ -154,31 +155,78 @@ func clientHandshake(c net.Conn, token string) error { // Listener wraps a net.Listener so Accept performs the token check for a tcp // seam. A connection that fails the check is closed and never surfaces, so // the protocol above this layer only ever sees authorized peers. +// +// A unix seam takes none of that machinery: Accept delegates straight to the +// wrapped listener, which is what it did before the token existed. type Listener struct { net.Listener addr Addr + + start sync.Once + closeOnce sync.Once + conns chan net.Conn + errc chan error // buffered 1, re-armed so every Accept sees the error + done chan struct{} } // Accept returns the next authorized connection. Unauthorized peers are // dropped and Accept keeps waiting: a bad token is a rejected stranger, not a // reason to stop serving. +// +// Each tcp handshake runs in its own goroutine rather than inline here. A peer +// that connects and then says nothing holds its greeting open for +// handshakeTimeout, and inline that peer stalls every other connection for +// five seconds — one silent stranger was enough to freeze the seam. func (l *Listener) Accept() (net.Conn, error) { + if l.addr.IsUnix() { + return l.Listener.Accept() + } + l.start.Do(func() { go l.acceptLoop() }) + select { + case c := <-l.conns: + return c, nil + case err := <-l.errc: + l.errc <- err + return nil, err + } +} + +// acceptLoop takes connections off the wrapped listener and greets each one +// concurrently. It ends on the first listener error, which every later Accept +// then reports. +func (l *Listener) acceptLoop() { for { c, err := l.Listener.Accept() if err != nil { - return nil, err + select { + case l.errc <- err: + case <-l.done: + } + return } - if l.addr.IsUnix() { - return c, nil - } - if err := serverHandshake(c, l.addr.Token); err != nil { - _ = c.Close() - continue - } - return c, nil + go l.greet(c) } } +func (l *Listener) greet(c net.Conn) { + if err := serverHandshake(c, l.addr.Token); err != nil { + _ = c.Close() + return + } + select { + case l.conns <- c: + case <-l.done: + _ = c.Close() + } +} + +// Close stops the listener and releases any connection still waiting to be +// handed to Accept. +func (l *Listener) Close() error { + l.closeOnce.Do(func() { close(l.done) }) + return l.Listener.Close() +} + // Addr reports the parsed seam address this listener was built from. func (l *Listener) SeamAddr() Addr { return l.addr } @@ -236,7 +284,7 @@ func Listen(a Addr) (*Listener, error) { if err != nil { return nil, err } - return &Listener{Listener: ln, addr: a}, nil + return wrap(ln, a), nil } if a.Token == "" { return nil, fmt.Errorf("netaddr: listen %s: tcp seam requires a token", a) @@ -245,7 +293,17 @@ func Listen(a Addr) (*Listener, error) { if err != nil { return nil, fmt.Errorf("netaddr: listen %s: %w", a, err) } - return &Listener{Listener: ln, addr: a}, nil + return wrap(ln, a), nil +} + +func wrap(ln net.Listener, a Addr) *Listener { + return &Listener{ + Listener: ln, + addr: a, + conns: make(chan net.Conn), + errc: make(chan error, 1), + done: make(chan struct{}), + } } func listenUnix(path string) (net.Listener, error) { diff --git a/internal/netaddr/netaddr_test.go b/internal/netaddr/netaddr_test.go index d0414d8..273e0bb 100644 --- a/internal/netaddr/netaddr_test.go +++ b/internal/netaddr/netaddr_test.go @@ -5,6 +5,7 @@ import ( "net" "path/filepath" "testing" + "time" ) // A scheme-less address must stay unix. Every deploy in the tree writes a bare @@ -140,6 +141,41 @@ func TestTCPUngreetedPeerDoesNotKillTheListener(t *testing.T) { } } +// A peer that connects and never speaks must not hold the seam. The greeting +// it owes is bounded by handshakeTimeout, so serving it on the accept path +// costs every later connection those five seconds. +func TestTCPSilentPeerDoesNotStallTheSeam(t *testing.T) { + ln, addr := listenLoopback(t, "s3cret") + defer ln.Close() + go echoOnce(ln) + + mute, err := net.Dial("tcp", addr.Address) + if err != nil { + t.Fatalf("mute dial: %v", err) + } + defer mute.Close() + + done := make(chan string, 1) + go func() { + c, err := Dial(addr) + if err != nil { + done <- "dial: " + err.Error() + return + } + defer c.Close() + done <- roundTrip(t, c, "still here") + }() + + select { + case got := <-done: + if got != "still here" { + t.Fatalf("got %q", got) + } + case <-time.After(handshakeTimeout / 2): + t.Fatal("a silent peer stalled the listener") + } +} + // A tcp seam with no token is a misconfiguration, and it must fail at bind // rather than serve the owner's turns to anyone who connects. func TestTCPListenRequiresToken(t *testing.T) { From 94c273780a481d286e5d37bf2ccf412924785c86 Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 6 Aug 2026 03:24:00 +0400 Subject: [PATCH 2/2] webauthn: lock the challenge maps and take a challenge once (V-581) The RP kept its two in-flight challenge maps bare, and mavweb serves the four passkey endpoints from HTTP handlers. Two browsers beginning a challenge at once were a concurrent map write, which is a fatal runtime error rather than a recovered panic, so it takes the daemon down. The endpoint that reaches it answers before any credential is proven. Every read and write of regs and asserts is now under a mutex. Lookup and delete moved into takeReg and takeAssert so they happen under one hold, which is what makes a challenge single-use: separately, two replays of the same response both found it before either deleted it. The challenge in clientDataJSON is compared in constant time. It is the one secret in that blob, 32 bytes of crypto/rand the browser has to echo back, and a byte-at-a-time compare is the shape that leaks a guessed prefix. Also corrected the comment over ipc.codeOf, which claimed an unmatched error keeps its text server-side. rpcErr ships that text deliberately, and on a tcp seam it leaves the box. Co-Authored-By: Claude Opus 5 --- internal/ipc/wire.go | 12 ++++-- internal/webauthn/webauthn.go | 66 ++++++++++++++++++++++++++---- internal/webauthn/webauthn_test.go | 48 ++++++++++++++++++++++ 3 files changed, 114 insertions(+), 12 deletions(-) diff --git a/internal/ipc/wire.go b/internal/ipc/wire.go index f671316..4b6ac78 100644 --- a/internal/ipc/wire.go +++ b/internal/ipc/wire.go @@ -131,9 +131,15 @@ const ( codeInternal = "internal" ) -// codeOf maps a server-side sentinel to its wire code. Anything not matched -// is codeInternal — we never leak internal Go error text to a module; it -// gets a generic "internal" and the daemon logs the real error server-side. +// codeOf maps a server-side sentinel to its wire code. Anything not matched is +// codeInternal. +// +// This used to claim the text of an unmatched error stays server-side. It does +// not: rpcErr below ships err.Error() for codeInternal and codeBadParams, +// deliberately, because on those two codes the text is the whole diagnostic and +// a module has no other way to see it. Worth knowing before putting a secret in +// an error string, and worth knowing twice on a tcp seam, where that string +// leaves the box. func codeOf(err error) string { switch { case err == nil: diff --git a/internal/webauthn/webauthn.go b/internal/webauthn/webauthn.go index 0a366a4..7da5612 100644 --- a/internal/webauthn/webauthn.go +++ b/internal/webauthn/webauthn.go @@ -14,11 +14,13 @@ import ( "crypto/elliptic" "crypto/rand" "crypto/sha256" + "crypto/subtle" "encoding/base64" "encoding/binary" "encoding/json" "fmt" "math/big" + "sync" "time" ) @@ -56,8 +58,16 @@ type credentialAssertion struct { // RP — the relying party instance. Holds config and transient challenge state. // A single-user daemon has one RP. +// +// mavweb serves the four passkey endpoints from its HTTP handlers, so the two +// challenge maps are reached concurrently even on a single-user box: a browser +// retrying an assertion while another tab begins one is enough. A concurrent +// map write is a fatal runtime error, not a recovered panic, so it would take +// the whole daemon down from an endpoint that answers before any credential is +// proven. Every read and write of regs and asserts is under mu. type RP struct { cfg Config + mu sync.Mutex regs map[string]*credentialRegistration asserts map[string]*credentialAssertion challengeTTL time.Duration @@ -75,6 +85,13 @@ func NewRP(cfg Config) *RP { // CleanExpired removes challenges older than the TTL. func (rp *RP) CleanExpired() { + rp.mu.Lock() + defer rp.mu.Unlock() + rp.cleanExpired() +} + +// cleanExpired is CleanExpired for a caller that already holds mu. +func (rp *RP) cleanExpired() { now := time.Now() for k, r := range rp.regs { if now.Sub(r.CreatedAt) > rp.challengeTTL { @@ -97,12 +114,14 @@ func (rp *RP) CreationOptions(userID []byte, userName string) (map[string]any, s } challengeB64 := base64.RawURLEncoding.EncodeToString(challenge) - rp.CleanExpired() + rp.mu.Lock() + rp.cleanExpired() rp.regs[challengeB64] = &credentialRegistration{ Challenge: challengeB64, UserID: userID, CreatedAt: time.Now(), } + rp.mu.Unlock() return map[string]any{ "rp": map[string]string{ @@ -136,12 +155,10 @@ func (rp *RP) CreationOptions(userID []byte, userName string) (map[string]any, s // FinishRegistration parses the browser's response and stores the credential. func (rp *RP) FinishRegistration(save CredentialSaver, challengeB64 string, resp map[string]any) (string, error) { - rp.CleanExpired() - reg, ok := rp.regs[challengeB64] + reg, ok := rp.takeReg(challengeB64) if !ok { return "", fmt.Errorf("webauthn: unknown or expired challenge") } - delete(rp.regs, challengeB64) credID := rawString(resp, "id") if credID == "" { @@ -188,11 +205,13 @@ func (rp *RP) AssertionOptions() (map[string]any, string, error) { } challengeB64 := base64.RawURLEncoding.EncodeToString(challenge) - rp.CleanExpired() + rp.mu.Lock() + rp.cleanExpired() rp.asserts[challengeB64] = &credentialAssertion{ Challenge: challengeB64, CreatedAt: time.Now(), } + rp.mu.Unlock() return map[string]any{ "challenge": challengeB64, @@ -215,11 +234,9 @@ func (rp *RP) AssertionOptions() (map[string]any, string, error) { // FinishAssertion verifies the browser's assertion response and returns the // verified credential ID. func (rp *RP) FinishAssertion(lookup CredentialLookup, updateSignCount SignCountUpdater, challengeB64 string, resp map[string]any) (string, error) { - rp.CleanExpired() - if _, ok := rp.asserts[challengeB64]; !ok { + if !rp.takeAssert(challengeB64) { return "", fmt.Errorf("webauthn: unknown or expired challenge") } - delete(rp.asserts, challengeB64) credID := rawString(resp, "id") if credID == "" { @@ -298,6 +315,34 @@ func (rp *RP) FinishAssertion(lookup CredentialLookup, updateSignCount SignCount return credID, nil } +// takeReg removes and returns the in-flight registration for challengeB64. +// Taking under one lock is what makes a challenge single-use: looking it up +// and deleting it separately lets two replays of the same response both find +// it before either deletes. +func (rp *RP) takeReg(challengeB64 string) (*credentialRegistration, bool) { + rp.mu.Lock() + defer rp.mu.Unlock() + rp.cleanExpired() + reg, ok := rp.regs[challengeB64] + if ok { + delete(rp.regs, challengeB64) + } + return reg, ok +} + +// takeAssert removes the in-flight assertion for challengeB64 and reports +// whether it was there. Single-use for the same reason takeReg is. +func (rp *RP) takeAssert(challengeB64 string) bool { + rp.mu.Lock() + defer rp.mu.Unlock() + rp.cleanExpired() + if _, ok := rp.asserts[challengeB64]; !ok { + return false + } + delete(rp.asserts, challengeB64) + return true +} + func verifyClientDataBytes(clientDataJSON []byte, expectedType, expectedChallenge, expectedOrigin string) error { var cdj struct { Type string `json:"type"` @@ -310,7 +355,10 @@ func verifyClientDataBytes(clientDataJSON []byte, expectedType, expectedChalleng if cdj.Type != expectedType { return fmt.Errorf("webauthn: unexpected type %q", cdj.Type) } - if cdj.Challenge != expectedChallenge { + // Constant time, because the challenge is the one secret in clientDataJSON: + // it is 32 bytes of crypto/rand the browser has to echo back, and a + // byte-at-a-time compare is the shape that leaks a guessed prefix. + if subtle.ConstantTimeCompare([]byte(cdj.Challenge), []byte(expectedChallenge)) != 1 { return fmt.Errorf("webauthn: challenge mismatch") } if cdj.Origin != expectedOrigin { diff --git a/internal/webauthn/webauthn_test.go b/internal/webauthn/webauthn_test.go index a5b0bdf..67ed2a4 100644 --- a/internal/webauthn/webauthn_test.go +++ b/internal/webauthn/webauthn_test.go @@ -8,6 +8,7 @@ import ( "encoding/base64" "encoding/binary" "encoding/json" + "sync" "testing" ) @@ -179,6 +180,53 @@ func TestRegisterAssertRoundTrip(t *testing.T) { } } +// The passkey endpoints are HTTP handlers, so two browsers beginning a +// challenge at once reach the same RP. Under -race this fails on the bare maps +// it used to keep, and in production a concurrent map write is fatal. +func TestChallengeMapsAreConcurrencySafe(t *testing.T) { + rp := NewRP(Config{Origin: testOrigin, RPID: testRPID, RPName: "maven"}) + lookup := func(string) ([]byte, int64, error) { return nil, 0, nil } + upd := func(string, int64) error { return nil } + + var wg sync.WaitGroup + for i := 0; i < 16; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 32; j++ { + _, chal, err := rp.AssertionOptions() + if err != nil { + t.Error(err) + return + } + _, _ = rp.FinishAssertion(lookup, upd, chal, map[string]any{}) + if _, _, err := rp.CreationOptions([]byte("u"), "user"); err != nil { + t.Error(err) + return + } + rp.CleanExpired() + } + }() + } + wg.Wait() +} + +// A challenge is single-use: the second presentation of one already spent is +// unknown, whichever goroutine gets there first. +func TestAssertionChallengeIsSingleUse(t *testing.T) { + rp := NewRP(Config{Origin: testOrigin, RPID: testRPID, RPName: "maven"}) + _, chal, err := rp.AssertionOptions() + if err != nil { + t.Fatal(err) + } + if !rp.takeAssert(chal) { + t.Fatal("first take of a fresh challenge failed") + } + if rp.takeAssert(chal) { + t.Fatal("a spent challenge was accepted twice") + } +} + // TestAssertRejectsWrongOrigin — a phished assertion from another origin fails. func TestAssertRejectsWrongOrigin(t *testing.T) { err := verifyClientDataBytes(clientData("webauthn.get", "abc", "https://evil.test"), "webauthn.get", "abc", testOrigin)