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/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) { 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)