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)