diff --git a/backend/internal/service/oidc_service.go b/backend/internal/service/oidc_service.go index a8134f5c..a224a214 100644 --- a/backend/internal/service/oidc_service.go +++ b/backend/internal/service/oidc_service.go @@ -3,6 +3,7 @@ package service import ( "context" "crypto/sha256" + "crypto/subtle" "crypto/tls" "encoding/base64" "encoding/json" @@ -394,7 +395,7 @@ func (s *OidcService) createTokenFromAuthorizationCode(ctx context.Context, inpu // If the client is public or PKCE is enabled, the code verifier must match the code challenge if client.IsPublic || client.PkceEnabled { - if !s.validateCodeVerifier(input.CodeVerifier, *authorizationCodeMetaData.CodeChallenge, *authorizationCodeMetaData.CodeChallengeMethodSha256) { + if !validateCodeVerifier(input.CodeVerifier, *authorizationCodeMetaData.CodeChallenge, *authorizationCodeMetaData.CodeChallengeMethodSha256) { return CreatedTokens{}, &common.OidcInvalidCodeVerifierError{} } } @@ -1083,13 +1084,20 @@ func (s *OidcService) createAuthorizationCode(ctx context.Context, clientID stri return randomString, nil } -func (s *OidcService) validateCodeVerifier(codeVerifier, codeChallenge string, codeChallengeMethodSha256 bool) bool { +func validateCodeVerifier(codeVerifier, codeChallenge string, codeChallengeMethodSha256 bool) bool { if codeVerifier == "" || codeChallenge == "" { return false } if !codeChallengeMethodSha256 { - return codeVerifier == codeChallenge + return subtle.ConstantTimeCompare([]byte(codeVerifier), []byte(codeChallenge)) == 1 + } + + // Base64 URL decode the challenge + // If it's not valid base64url, fail the operation + codeChallengeBytes, err := base64.RawURLEncoding.DecodeString(codeChallenge) + if err != nil { + return false } // Compute SHA-256 hash of the codeVerifier @@ -1097,10 +1105,7 @@ func (s *OidcService) validateCodeVerifier(codeVerifier, codeChallenge string, c h.Write([]byte(codeVerifier)) codeVerifierHash := h.Sum(nil) - // Base64 URL encode the verifier hash - encodedVerifierHash := base64.RawURLEncoding.EncodeToString(codeVerifierHash) - - return encodedVerifierHash == codeChallenge + return subtle.ConstantTimeCompare(codeVerifierHash, codeChallengeBytes) == 1 } func (s *OidcService) getCallbackURL(client *model.OidcClient, inputCallbackURL string, tx *gorm.DB, ctx context.Context) (callbackURL string, err error) { diff --git a/backend/internal/service/oidc_service_test.go b/backend/internal/service/oidc_service_test.go index cb6d674b..5d4d04df 100644 --- a/backend/internal/service/oidc_service_test.go +++ b/backend/internal/service/oidc_service_test.go @@ -5,6 +5,8 @@ import ( "crypto/ecdsa" "crypto/elliptic" "crypto/rand" + "crypto/sha256" + "encoding/base64" "encoding/json" "net/http" "testing" @@ -510,3 +512,28 @@ func TestOidcService_verifyClientCredentialsInternal(t *testing.T) { }) }) } + +func TestValidateCodeVerifier_Plain(t *testing.T) { + require.False(t, validateCodeVerifier("", "", false)) + require.False(t, validateCodeVerifier("", "", true)) + + t.Run("plain", func(t *testing.T) { + require.False(t, validateCodeVerifier("", "challenge", false)) + require.False(t, validateCodeVerifier("verifier", "", false)) + require.True(t, validateCodeVerifier("plainVerifier", "plainVerifier", false)) + require.False(t, validateCodeVerifier("plainVerifier", "otherVerifier", false)) + }) + + t.Run("SHA 256", func(t *testing.T) { + codeVerifier := "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" + hash := sha256.Sum256([]byte(codeVerifier)) + codeChallenge := base64.RawURLEncoding.EncodeToString(hash[:]) + + require.True(t, validateCodeVerifier(codeVerifier, codeChallenge, true)) + require.False(t, validateCodeVerifier("wrongVerifier", codeChallenge, true)) + require.False(t, validateCodeVerifier(codeVerifier, "!", true)) + + // Invalid base64 + require.False(t, validateCodeVerifier("NOT!VALID", codeChallenge, true)) + }) +}