Skip to content

Commit 8d1e57e

Browse files
committed
oidc: verify the ID Token's signature before processing claims
This change updates the verification logic of this library to first validate the ID Token instead of parsing claims. This hopefully makes it harder for a malicious client to provide an invalid token for validation that's crafted to cause this package to over-allocate memory. See the associated bug and CVE-2025-27144. Fixes #463
1 parent a7c457e commit 8d1e57e

File tree

3 files changed

+49
-63
lines changed

3 files changed

+49
-63
lines changed

oidc/jwks_test.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"encoding/json"
1212
"errors"
1313
"fmt"
14+
"io"
1415
"net/http"
1516
"net/http/httptest"
1617
"strconv"
@@ -151,11 +152,8 @@ func TestKeyVerifyContextCanceled(t *testing.T) {
151152
t.Fatal(err)
152153
}
153154

154-
ch := make(chan struct{})
155-
defer close(ch)
156-
157155
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
158-
<-ch
156+
io.WriteString(w, "{}")
159157
}))
160158
defer s.Close()
161159

oidc/verify.go

Lines changed: 41 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,11 @@
11
package oidc
22

33
import (
4-
"bytes"
54
"context"
6-
"encoding/base64"
75
"encoding/json"
8-
"errors"
96
"fmt"
107
"io"
118
"net/http"
12-
"strings"
139
"time"
1410

1511
jose "github.com/go-jose/go-jose/v4"
@@ -145,18 +141,6 @@ func (p *Provider) newVerifier(keySet KeySet, config *Config) *IDTokenVerifier {
145141
return NewVerifier(p.issuer, keySet, config)
146142
}
147143

148-
func parseJWT(p string) ([]byte, error) {
149-
parts := strings.Split(p, ".")
150-
if len(parts) < 2 {
151-
return nil, fmt.Errorf("oidc: malformed jwt, expected 3 parts got %d", len(parts))
152-
}
153-
payload, err := base64.RawURLEncoding.DecodeString(parts[1])
154-
if err != nil {
155-
return nil, fmt.Errorf("oidc: malformed jwt payload: %v", err)
156-
}
157-
return payload, nil
158-
}
159-
160144
func contains(sli []string, ele string) bool {
161145
for _, s := range sli {
162146
if s == ele {
@@ -219,12 +203,49 @@ func resolveDistributedClaim(ctx context.Context, verifier *IDTokenVerifier, src
219203
//
220204
// token, err := verifier.Verify(ctx, rawIDToken)
221205
func (v *IDTokenVerifier) Verify(ctx context.Context, rawIDToken string) (*IDToken, error) {
222-
// Throw out tokens with invalid claims before trying to verify the token. This lets
223-
// us do cheap checks before possibly re-syncing keys.
224-
payload, err := parseJWT(rawIDToken)
206+
var supportedSigAlgs []jose.SignatureAlgorithm
207+
for _, alg := range v.config.SupportedSigningAlgs {
208+
supportedSigAlgs = append(supportedSigAlgs, jose.SignatureAlgorithm(alg))
209+
}
210+
if len(supportedSigAlgs) == 0 {
211+
// If no algorithms were specified by both the config and discovery, default
212+
// to the one mandatory algorithm "RS256".
213+
supportedSigAlgs = []jose.SignatureAlgorithm{jose.RS256}
214+
}
215+
if v.config.InsecureSkipSignatureCheck {
216+
// "none" is a required value to even parse a JWT with the "none" algorithm
217+
// using go-jose.
218+
supportedSigAlgs = append(supportedSigAlgs, "none")
219+
}
220+
221+
// Parse and verify the signature first. This at least forces the user to have
222+
// a valid, signed ID token before we do any other processing.
223+
jws, err := jose.ParseSigned(rawIDToken, supportedSigAlgs)
225224
if err != nil {
226225
return nil, fmt.Errorf("oidc: malformed jwt: %v", err)
227226
}
227+
switch len(jws.Signatures) {
228+
case 0:
229+
return nil, fmt.Errorf("oidc: id token not signed")
230+
case 1:
231+
default:
232+
return nil, fmt.Errorf("oidc: multiple signatures on id token not supported")
233+
}
234+
sig := jws.Signatures[0]
235+
236+
var payload []byte
237+
if v.config.InsecureSkipSignatureCheck {
238+
// Yolo mode.
239+
payload = jws.UnsafePayloadWithoutVerification()
240+
} else {
241+
// The JWT is attached here for the happy path to avoid the verifier from
242+
// having to parse the JWT twice.
243+
ctx = context.WithValue(ctx, parsedJWTKey, jws)
244+
payload, err = v.keySet.VerifySignature(ctx, rawIDToken)
245+
if err != nil {
246+
return nil, fmt.Errorf("failed to verify signature: %v", err)
247+
}
248+
}
228249
var token idToken
229250
if err := json.Unmarshal(payload, &token); err != nil {
230251
return nil, fmt.Errorf("oidc: failed to unmarshal claims: %v", err)
@@ -254,6 +275,7 @@ func (v *IDTokenVerifier) Verify(ctx context.Context, rawIDToken string) (*IDTok
254275
AccessTokenHash: token.AtHash,
255276
claims: payload,
256277
distributedClaims: distributedClaims,
278+
sigAlgorithm: sig.Header.Algorithm,
257279
}
258280

259281
// Check issuer.
@@ -306,45 +328,6 @@ func (v *IDTokenVerifier) Verify(ctx context.Context, rawIDToken string) (*IDTok
306328
}
307329
}
308330

309-
if v.config.InsecureSkipSignatureCheck {
310-
return t, nil
311-
}
312-
313-
var supportedSigAlgs []jose.SignatureAlgorithm
314-
for _, alg := range v.config.SupportedSigningAlgs {
315-
supportedSigAlgs = append(supportedSigAlgs, jose.SignatureAlgorithm(alg))
316-
}
317-
if len(supportedSigAlgs) == 0 {
318-
// If no algorithms were specified by both the config and discovery, default
319-
// to the one mandatory algorithm "RS256".
320-
supportedSigAlgs = []jose.SignatureAlgorithm{jose.RS256}
321-
}
322-
jws, err := jose.ParseSigned(rawIDToken, supportedSigAlgs)
323-
if err != nil {
324-
return nil, fmt.Errorf("oidc: malformed jwt: %v", err)
325-
}
326-
327-
switch len(jws.Signatures) {
328-
case 0:
329-
return nil, fmt.Errorf("oidc: id token not signed")
330-
case 1:
331-
default:
332-
return nil, fmt.Errorf("oidc: multiple signatures on id token not supported")
333-
}
334-
sig := jws.Signatures[0]
335-
t.sigAlgorithm = sig.Header.Algorithm
336-
337-
ctx = context.WithValue(ctx, parsedJWTKey, jws)
338-
gotPayload, err := v.keySet.VerifySignature(ctx, rawIDToken)
339-
if err != nil {
340-
return nil, fmt.Errorf("failed to verify signature: %v", err)
341-
}
342-
343-
// Ensure that the payload returned by the square actually matches the payload parsed earlier.
344-
if !bytes.Equal(gotPayload, payload) {
345-
return nil, errors.New("oidc: internal error, payload parsed did not match previous payload")
346-
}
347-
348331
return t, nil
349332
}
350333

oidc/verify_test.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -580,9 +580,14 @@ func (v verificationTest) runGetToken(t *testing.T) (*IDToken, error) {
580580
if v.signKey != nil {
581581
token = v.signKey.sign(t, []byte(v.idToken))
582582
} else {
583-
token = base64.RawURLEncoding.EncodeToString([]byte(`{alg: "none"}`))
583+
// "none" still uses a second "." character, but "...MUST use the empty octet
584+
// sequence as its JWS Signature value."
585+
//
586+
// https://datatracker.ietf.org/doc/html/rfc7518#section-3.6
587+
token = base64.RawURLEncoding.EncodeToString([]byte(`{"alg": "none"}`))
584588
token += "."
585589
token += base64.RawURLEncoding.EncodeToString([]byte(v.idToken))
590+
token += "."
586591
}
587592

588593
ctx, cancel := context.WithCancel(context.Background())

0 commit comments

Comments
 (0)