Fix nonce comparison to prevent timing

Signed-off-by: maksim.nabokikh <max.nabokih@gmail.com>
This commit is contained in:
maksim.nabokikh
2026-04-08 17:57:07 +02:00
parent 2fb5d78ab7
commit 6189b2085b
4 changed files with 59 additions and 4 deletions
+2 -1
View File
@@ -2,6 +2,7 @@ package server
import (
"context"
"crypto/subtle"
"errors"
"fmt"
"net/http"
@@ -24,7 +25,7 @@ func (s *Server) handleHome(w http.ResponseWriter, r *http.Request) {
if cookie, err := r.Cookie(s.sessionConfig.CookieName); err == nil && cookie.Value != "" {
if userID, connectorID, nonce, err := parseSessionCookie(cookie.Value, s.sessionConfig.CookieEncryptionKey); err == nil {
session, err := s.storage.GetAuthSession(ctx, userID, connectorID)
if err == nil && session.Nonce == nonce {
if err == nil && subtle.ConstantTimeCompare([]byte(session.Nonce), []byte(nonce)) == 1 {
data.LoggedIn = true
data.IPAddress = session.IPAddress
data.UserAgent = session.UserAgent
+3 -2
View File
@@ -2,6 +2,7 @@ package server
import (
"context"
"crypto/subtle"
"errors"
"net/http"
"net/url"
@@ -94,7 +95,7 @@ func (s *Server) handleLogout(w http.ResponseWriter, r *http.Request) {
if cookie, err := r.Cookie(s.sessionConfig.CookieName); err == nil && cookie.Value != "" {
if uid, cid, nonce, err := parseSessionCookie(cookie.Value, s.sessionConfig.CookieEncryptionKey); err == nil {
// Verify the session exists and nonce matches before trusting the cookie.
if session, err := s.storage.GetAuthSession(ctx, uid, cid); err == nil && session.Nonce == nonce {
if session, err := s.storage.GetAuthSession(ctx, uid, cid); err == nil && subtle.ConstantTimeCompare([]byte(session.Nonce), []byte(nonce)) == 1 {
userID = uid
connectorID = cid
s.logger.DebugContext(ctx, "logout: identified user from session cookie",
@@ -178,7 +179,7 @@ func (s *Server) handleLogoutCallback(w http.ResponseWriter, r *http.Request) {
return
}
if session.Nonce != nonce {
if subtle.ConstantTimeCompare([]byte(session.Nonce), []byte(nonce)) != 1 {
s.renderError(r, w, http.StatusBadRequest, "Invalid session.")
return
}
+4 -1
View File
@@ -5,6 +5,7 @@ import (
"crypto/aes"
"crypto/cipher"
"crypto/rand"
"crypto/subtle"
"encoding/base64"
"errors"
"fmt"
@@ -178,7 +179,9 @@ func (s *Server) getValidSession(ctx context.Context, w http.ResponseWriter, r *
}
// Verify nonce to prevent cookie forgery.
if session.Nonce != nonce {
// Use constant-time comparison to prevent timing attacks that could
// allow an attacker to recover the nonce byte-by-byte.
if subtle.ConstantTimeCompare([]byte(session.Nonce), []byte(nonce)) != 1 {
s.logger.DebugContext(ctx, "auth session nonce mismatch")
s.clearSessionCookie(w)
return nil
+50
View File
@@ -1432,6 +1432,56 @@ func TestFinishSessionLogin_MFA(t *testing.T) {
})
}
// TestNonceVerificationRejectsForgedCookie verifies that a session cookie
// with a valid (userID, connectorID) but wrong nonce is rejected.
// The nonce comparison uses constant-time comparison to prevent timing attacks.
func TestNonceVerificationRejectsForgedCookie(t *testing.T) {
ctx := t.Context()
s := newTestSessionServer(t)
now := s.now()
require.NoError(t, s.storage.CreateAuthSession(ctx, storage.AuthSession{
UserID: "user-1", ConnectorID: "mock", Nonce: "real-nonce",
CreatedAt: now.Add(-10 * time.Minute), LastActivity: now.Add(-1 * time.Minute),
AbsoluteExpiry: now.Add(24 * time.Hour), IdleExpiry: now.Add(59 * time.Minute),
}))
tests := []struct {
name string
nonce string
}{
{"wrong nonce", "wrong-nonce"},
{"empty nonce", ""},
{"prefix of real nonce", "real"},
{"real nonce with suffix", "real-nonce-extra"},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
r := sessionCookieRequest("user-1", "mock", tc.nonce)
w := httptest.NewRecorder()
session := s.getValidSession(ctx, w, r)
assert.Nil(t, session, "session with forged nonce %q should be rejected", tc.nonce)
// Cookie should be cleared on nonce mismatch.
for _, c := range w.Result().Cookies() {
if c.Name == "dex_session" {
assert.Equal(t, -1, c.MaxAge, "cookie should be cleared")
}
}
})
}
t.Run("correct nonce accepted", func(t *testing.T) {
r := sessionCookieRequest("user-1", "mock", "real-nonce")
w := httptest.NewRecorder()
session := s.getValidSession(ctx, w, r)
require.NotNil(t, session)
assert.Equal(t, "user-1", session.UserID)
})
}
// TestPromptNone tests the prompt=none silent authentication scenarios.
// These verify the code paths in handleConnectorLogin (handlers.go:444-457)
// where prompt=none requires session-based login without any UI.