From f4a500caf68169dccb0b54cb90523e68ee1ac2be Mon Sep 17 00:00:00 2001 From: Milos Gajdos Date: Sat, 1 Feb 2025 15:30:18 -0800 Subject: [PATCH 1/3] Fix registry token authentication bug When a JWT contains a JWK header without a certificate chain, the original code only checked if the KeyID (kid) matches one of the trusted keys, but doesn't verify that the actual key material matches. As a result, if an attacker guesses the kid, they can inject an untrusted key which would then be used to grant access to protected data. This fixes the issue such as only the trusted key is verified. Signed-off-by: Milos Gajdos --- registry/auth/token/token.go | 5 +-- registry/auth/token/token_test.go | 54 +++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/registry/auth/token/token.go b/registry/auth/token/token.go index 55d0b7f6f..e1660d176 100644 --- a/registry/auth/token/token.go +++ b/registry/auth/token/token.go @@ -219,11 +219,12 @@ func verifyJWK(header jose.Header, verifyOpts VerifyOptions) (signingKey crypto. // Check to see if the key includes a certificate chain. if len(jwk.Certificates) == 0 { // The JWK should be one of the trusted root keys. - if _, trusted := verifyOpts.TrustedKeys[jwk.KeyID]; !trusted { + trustedKey, trusted := verifyOpts.TrustedKeys[jwk.KeyID] + if !trusted { return nil, errors.New("untrusted JWK with no certificate chain") } // The JWK is one of the trusted keys. - return + return trustedKey, nil } opts := x509.VerifyOptions{ diff --git a/registry/auth/token/token_test.go b/registry/auth/token/token_test.go index b982ab14b..ba9f073cb 100644 --- a/registry/auth/token/token_test.go +++ b/registry/auth/token/token_test.go @@ -646,3 +646,57 @@ func TestNewAccessControllerPemBlock(t *testing.T) { t.Fatal("accessController has the wrong number of certificates") } } + +// This test makes sure the untrusted key can not be used in token verification. +func TestVerifyJWKWithTrustedKey(t *testing.T) { + // Generate a test key pair + privKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + pubKey := privKey.Public() + + // Create a JWK with no certificates + jwk := &jose.JSONWebKey{ + Key: privKey, + KeyID: "test-key-id", + Use: "sig", + Algorithm: string(jose.ES256), + } + + // Create verify options with our public key as trusted + verifyOpts := VerifyOptions{ + TrustedKeys: map[string]crypto.PublicKey{ + "test-key-id": pubKey, + }, + } + + // Create test header + header := jose.Header{ + JSONWebKey: jwk, + } + + // Test the verifyJWK function + returnedKey, err := verifyJWK(header, verifyOpts) + if err != nil { + t.Fatalf("Expected no error, got: %v", err) + } + + // Verify the returned key matches our trusted key + if returnedKey != pubKey { + t.Error("Returned key does not match the trusted key") + } + + // Test with untrusted key + verifyOpts.TrustedKeys = map[string]crypto.PublicKey{ + "different-key-id": pubKey, + } + + _, err = verifyJWK(header, verifyOpts) + if err == nil { + t.Error("Expected error for untrusted key, got none") + } + if err.Error() != "untrusted JWK with no certificate chain" { + t.Errorf("Expected 'untrusted JWK with no certificate chain' error, got: %v", err) + } +} From 53c382641c9223aaa2b79793b05d444bebff0587 Mon Sep 17 00:00:00 2001 From: Milos Gajdos Date: Wed, 5 Feb 2025 21:26:23 -0800 Subject: [PATCH 2/3] Remove named returns and fix linting woes Signed-off-by: Milos Gajdos --- registry/auth/token/token.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/registry/auth/token/token.go b/registry/auth/token/token.go index e1660d176..6061e5c1f 100644 --- a/registry/auth/token/token.go +++ b/registry/auth/token/token.go @@ -212,9 +212,8 @@ func verifyCertChain(header jose.Header, roots *x509.CertPool) (signingKey crypt return } -func verifyJWK(header jose.Header, verifyOpts VerifyOptions) (signingKey crypto.PublicKey, err error) { +func verifyJWK(header jose.Header, verifyOpts VerifyOptions) (crypto.PublicKey, error) { jwk := header.JSONWebKey - signingKey = jwk.Key // Check to see if the key includes a certificate chain. if len(jwk.Certificates) == 0 { @@ -246,9 +245,8 @@ func verifyJWK(header jose.Header, verifyOpts VerifyOptions) (signingKey crypto. if err != nil { return nil, err } - signingKey = getCertPubKey(chains) - return + return getCertPubKey(chains), nil } func getCertPubKey(chains [][]*x509.Certificate) crypto.PublicKey { From 6ed60b0f4892685fc9bc5924ff2e2788d7dbbab7 Mon Sep 17 00:00:00 2001 From: Milos Gajdos Date: Thu, 6 Feb 2025 17:43:28 +0000 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Sebastiaan van Stijn Signed-off-by: Milos Gajdos --- registry/auth/token/token.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/registry/auth/token/token.go b/registry/auth/token/token.go index 6061e5c1f..0773d5307 100644 --- a/registry/auth/token/token.go +++ b/registry/auth/token/token.go @@ -218,12 +218,12 @@ func verifyJWK(header jose.Header, verifyOpts VerifyOptions) (crypto.PublicKey, // Check to see if the key includes a certificate chain. if len(jwk.Certificates) == 0 { // The JWK should be one of the trusted root keys. - trustedKey, trusted := verifyOpts.TrustedKeys[jwk.KeyID] + key, trusted := verifyOpts.TrustedKeys[jwk.KeyID] if !trusted { return nil, errors.New("untrusted JWK with no certificate chain") } // The JWK is one of the trusted keys. - return trustedKey, nil + return key, nil } opts := x509.VerifyOptions{