diff --git a/AUDIT.md b/AUDIT.md index 71bfa52..62bcf82 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -227,7 +227,7 @@ The REST `handleTokenIssue` and gRPC `IssueServiceToken` both revoke the existin | Yes | F-11 | MEDIUM | Missing security headers on UI responses | Small | | No | F-05 | LOW | No `nbf` claim in issued JWTs | Trivial | | No | F-06 | LOW | `HasRole` uses non-constant-time comparison | Trivial | -| No | F-07 | LOW | Dummy Argon2 hash timing mismatch | Small | +| Yes | F-07 | LOW | Dummy Argon2 hash timing mismatch | Small | | No | F-08 | LOW | No account lockout after repeated failures | Medium | | No | F-09 | LOW | `synchronous=NORMAL` risks audit data loss | Trivial | | No | F-10 | LOW | No maximum token expiry validation | Small | diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 2008d4c..64a8650 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -24,6 +24,7 @@ import ( "math" "strconv" "strings" + "sync" "time" "golang.org/x/crypto/argon2" @@ -54,6 +55,38 @@ func DefaultArgonParams() ArgonParams { } } +// dummyHashOnce ensures the dummy hash is computed exactly once at first use. +// Security: computing it lazily (rather than at init time) keeps startup fast; +// using sync.Once makes it safe to call from concurrent goroutines. +var ( + dummyHashOnce sync.Once + dummyHashVal string +) + +// DummyHash returns a pre-computed Argon2id PHC hash of a fixed dummy password +// using DefaultArgonParams. It is computed once on first call and cached. +// +// Security (F-07): using a real hash with the exact same parameters as +// production password verification ensures that dummy operations (run for +// unknown users or inactive accounts to prevent timing-based enumeration) +// take as long as real verifications, regardless of parameter changes. +// The previous hardcoded string used a 6-byte salt and 6-byte hash which +// was faster to verify than a real 16-byte-salt / 32-byte-hash record. +func DummyHash() string { + dummyHashOnce.Do(func() { + h, err := HashPassword("dummy-password-for-timing-only", DefaultArgonParams()) + if err != nil { + // This should be unreachable in production — HashPassword only fails + // if crypto/rand fails or the password is empty, neither of which + // applies here. Panic so the misconfiguration surfaces immediately + // rather than silently degrading security. + panic("auth: DummyHash: failed to pre-compute dummy hash: " + err.Error()) + } + dummyHashVal = h + }) + return dummyHashVal +} + // HashPassword hashes a password using Argon2id and returns a PHC-format string. // A random 16-byte salt is generated via crypto/rand for each call. // diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index b5e2cd8..8a53233 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -214,3 +214,51 @@ func TestDefaultArgonParams(t *testing.T) { t.Errorf("default Threads=%d < 1", p.Threads) } } + +// TestDummyHashIsValidPHC verifies DummyHash returns a non-empty string that +// is a valid Argon2id PHC hash verifiable by VerifyPassword. +func TestDummyHashIsValidPHC(t *testing.T) { + h := DummyHash() + if h == "" { + t.Fatal("DummyHash returned empty string") + } + // Must parse as a valid PHC string with OWASP-compatible parameters. + // VerifyPassword with the correct password must succeed. + ok, err := VerifyPassword("dummy-password-for-timing-only", h) + if err != nil { + t.Fatalf("VerifyPassword against DummyHash: %v", err) + } + if !ok { + t.Error("VerifyPassword against DummyHash returned false (hash mismatch)") + } +} + +// TestDummyHashIsCached verifies that consecutive calls return the same string +// (sync.Once caching). +func TestDummyHashIsCached(t *testing.T) { + h1 := DummyHash() + h2 := DummyHash() + if h1 != h2 { + t.Errorf("DummyHash returned different values on consecutive calls:\n first: %q\n second: %q", h1, h2) + } +} + +// TestDummyHashMatchesDefaultParams verifies the embedded parameters in the +// returned PHC string match DefaultArgonParams (m, t, p). +func TestDummyHashMatchesDefaultParams(t *testing.T) { + h := DummyHash() + params, _, _, err := parsePHC(h) + if err != nil { + t.Fatalf("parsePHC(DummyHash()): %v", err) + } + def := DefaultArgonParams() + if params.Memory != def.Memory { + t.Errorf("Memory = %d, want %d", params.Memory, def.Memory) + } + if params.Time != def.Time { + t.Errorf("Time = %d, want %d", params.Time, def.Time) + } + if params.Threads != def.Threads { + t.Errorf("Threads = %d, want %d", params.Threads, def.Threads) + } +} diff --git a/internal/grpcserver/auth.go b/internal/grpcserver/auth.go index ed23042..1165aba 100644 --- a/internal/grpcserver/auth.go +++ b/internal/grpcserver/auth.go @@ -40,14 +40,14 @@ func (a *authServiceServer) Login(ctx context.Context, req *mciasv1.LoginRequest acct, err := a.s.db.GetAccountByUsername(req.Username) if err != nil { // Security: run dummy Argon2 to equalise timing for unknown users. - _, _ = auth.VerifyPassword("dummy", "$argon2id$v=19$m=65536,t=3,p=4$dGVzdHNhbHQ$dGVzdGhhc2g") + _, _ = auth.VerifyPassword("dummy", auth.DummyHash()) a.s.db.WriteAuditEvent(model.EventLoginFail, nil, nil, ip, //nolint:errcheck // audit failure is non-fatal fmt.Sprintf(`{"username":%q,"reason":"unknown_user"}`, req.Username)) return nil, status.Error(codes.Unauthenticated, "invalid credentials") } if acct.Status != model.AccountStatusActive { - _, _ = auth.VerifyPassword("dummy", "$argon2id$v=19$m=65536,t=3,p=4$dGVzdHNhbHQ$dGVzdGhhc2g") + _, _ = auth.VerifyPassword("dummy", auth.DummyHash()) a.s.db.WriteAuditEvent(model.EventLoginFail, &acct.ID, nil, ip, `{"reason":"account_inactive"}`) //nolint:errcheck return nil, status.Error(codes.Unauthenticated, "invalid credentials") } diff --git a/internal/server/server.go b/internal/server/server.go index 200feae..14cb3a8 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -174,7 +174,7 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { if err != nil { // Security: return a generic error whether the user exists or not. // Always run a dummy Argon2 check to prevent timing-based user enumeration. - _, _ = auth.VerifyPassword("dummy", "$argon2id$v=19$m=65536,t=3,p=4$dGVzdHNhbHQ$dGVzdGhhc2g") + _, _ = auth.VerifyPassword("dummy", auth.DummyHash()) s.writeAudit(r, model.EventLoginFail, nil, nil, fmt.Sprintf(`{"username":%q,"reason":"unknown_user"}`, req.Username)) middleware.WriteError(w, http.StatusUnauthorized, "invalid credentials", "unauthorized") return @@ -183,7 +183,7 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { // Security: Check account status before credential verification to avoid // leaking whether the account exists based on timing differences. if acct.Status != model.AccountStatusActive { - _, _ = auth.VerifyPassword("dummy", "$argon2id$v=19$m=65536,t=3,p=4$dGVzdHNhbHQ$dGVzdGhhc2g") + _, _ = auth.VerifyPassword("dummy", auth.DummyHash()) s.writeAudit(r, model.EventLoginFail, &acct.ID, nil, `{"reason":"account_inactive"}`) middleware.WriteError(w, http.StatusUnauthorized, "invalid credentials", "unauthorized") return diff --git a/internal/ui/ui.go b/internal/ui/ui.go index 071fcd7..6bf69b1 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -27,6 +27,7 @@ import ( "sync" "time" + "git.wntrmute.dev/kyle/mcias/internal/auth" "git.wntrmute.dev/kyle/mcias/internal/config" "git.wntrmute.dev/kyle/mcias/internal/db" "git.wntrmute.dev/kyle/mcias/internal/model" @@ -96,13 +97,11 @@ func (u *UIServer) consumeTOTPNonce(nonce string) (int64, bool) { return pl.accountID, true } -// dummyHash returns a hardcoded Argon2id PHC string used for constant-time -// dummy password verification when the account is unknown or inactive. -// Security: the dummy hash uses OWASP-recommended parameters (m=65536,t=3,p=4) -// to match real verification timing. F-07 will replace this with a -// sync.Once-computed real hash for exact parameter parity. +// dummyHash returns the pre-computed Argon2id PHC hash for constant-time dummy +// verification when an account is unknown or inactive (F-07). +// Delegates to auth.DummyHash() which uses sync.Once for one-time computation. func (u *UIServer) dummyHash() string { - return "$argon2id$v=19$m=65536,t=3,p=4$dGVzdHNhbHQ$dGVzdGhhc2g" + return auth.DummyHash() } // New constructs a UIServer, parses all templates, and returns it.