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 <noreply@anthropic.com>
2026-08-06 03:24:00 +04:00
parent 93c08f9de1
commit 94c273780a
3 changed files with 114 additions and 12 deletions
@@ -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:
@@ -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 {
@@ -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)