From 0273855f649d381485b0644a766705826a4b4e3c Mon Sep 17 00:00:00 2001 From: Paul Latzelsperger Date: Wed, 15 May 2024 15:29:03 +0200 Subject: [PATCH 1/4] fix: prevent JWT attacks by spoofed keyIDs --- .../jwt/JwtPresentationVerifier.java | 16 ++- .../jwt/rules/IssuerKeyIdValidationRule.java | 45 ++++++++ .../jwt/JwtPresentationVerifierTest.java | 108 ++++++++++++++++++ 3 files changed, 164 insertions(+), 5 deletions(-) create mode 100644 extensions/common/crypto/jwt-verifiable-credentials/src/main/java/org/eclipse/edc/verifiablecredentials/jwt/rules/IssuerKeyIdValidationRule.java diff --git a/extensions/common/crypto/jwt-verifiable-credentials/src/main/java/org/eclipse/edc/verifiablecredentials/jwt/JwtPresentationVerifier.java b/extensions/common/crypto/jwt-verifiable-credentials/src/main/java/org/eclipse/edc/verifiablecredentials/jwt/JwtPresentationVerifier.java index 2bca39cb0c1..d647508a4e6 100644 --- a/extensions/common/crypto/jwt-verifiable-credentials/src/main/java/org/eclipse/edc/verifiablecredentials/jwt/JwtPresentationVerifier.java +++ b/extensions/common/crypto/jwt-verifiable-credentials/src/main/java/org/eclipse/edc/verifiablecredentials/jwt/JwtPresentationVerifier.java @@ -26,6 +26,7 @@ import org.eclipse.edc.token.spi.TokenValidationRule; import org.eclipse.edc.token.spi.TokenValidationRulesRegistry; import org.eclipse.edc.token.spi.TokenValidationService; +import org.eclipse.edc.verifiablecredentials.jwt.rules.IssuerKeyIdValidationRule; import java.text.ParseException; import java.util.ArrayList; @@ -110,8 +111,11 @@ public Result verify(String serializedJwt, VerifierContext context) { try { // obtain the actual JSON structure var signedJwt = SignedJWT.parse(serializedJwt); + var keyId = signedJwt.getHeader().getKeyID(); if (isCredential(signedJwt)) { - return tokenValidationService.validate(serializedJwt, publicKeyResolver, tokenValidationRulesRegistry.getRules(JWT_VC_TOKEN_CONTEXT)) + var rules = new ArrayList<>(tokenValidationRulesRegistry.getRules(JWT_VC_TOKEN_CONTEXT)); + rules.add(new IssuerKeyIdValidationRule(keyId)); + return tokenValidationService.validate(serializedJwt, publicKeyResolver, rules) .mapTo(); } @@ -120,7 +124,7 @@ public Result verify(String serializedJwt, VerifierContext context) { } //we can be sure to have a presentation token - verificationResult = tokenValidationService.validate(serializedJwt, publicKeyResolver, vpValidationRules(context.getAudience())); + verificationResult = tokenValidationService.validate(serializedJwt, publicKeyResolver, vpValidationRules(context.getAudience(), keyId)); var vpClaim = (Map) signedJwt.getJWTClaimsSet().getClaim(VP_CLAIM); @@ -145,16 +149,18 @@ public Result verify(String serializedJwt, VerifierContext context) { return verificationResult.mapTo(); } - private List vpValidationRules(String audience) { + private List vpValidationRules(String audience, String keyId) { + var rules = new ArrayList<>(tokenValidationRulesRegistry.getRules(JWT_VP_TOKEN_CONTEXT)); + rules.add(new IssuerKeyIdValidationRule(keyId)); return Optional.ofNullable(audience) .map(aud -> { - List r = new ArrayList<>(tokenValidationRulesRegistry.getRules(JWT_VP_TOKEN_CONTEXT)); + List r = new ArrayList<>(rules); var audRule = new AudienceValidationRule(audience); r.add(audRule); return r; }) - .orElse(tokenValidationRulesRegistry.getRules(JWT_VP_TOKEN_CONTEXT)); + .orElse(rules); } diff --git a/extensions/common/crypto/jwt-verifiable-credentials/src/main/java/org/eclipse/edc/verifiablecredentials/jwt/rules/IssuerKeyIdValidationRule.java b/extensions/common/crypto/jwt-verifiable-credentials/src/main/java/org/eclipse/edc/verifiablecredentials/jwt/rules/IssuerKeyIdValidationRule.java new file mode 100644 index 00000000000..6492ab2fb3f --- /dev/null +++ b/extensions/common/crypto/jwt-verifiable-credentials/src/main/java/org/eclipse/edc/verifiablecredentials/jwt/rules/IssuerKeyIdValidationRule.java @@ -0,0 +1,45 @@ +/* + * Copyright (c) 2024 Bayerische Motoren Werke Aktiengesellschaft (BMW AG) + * + * This program and the accompanying materials are made available under the + * terms of the Apache License, Version 2.0 which is available at + * https://www.apache.org/licenses/LICENSE-2.0 + * + * SPDX-License-Identifier: Apache-2.0 + * + * Contributors: + * Bayerische Motoren Werke Aktiengesellschaft (BMW AG) - initial API and implementation + * + */ + +package org.eclipse.edc.verifiablecredentials.jwt.rules; + +import org.eclipse.edc.spi.iam.ClaimToken; +import org.eclipse.edc.spi.result.Result; +import org.eclipse.edc.token.spi.TokenValidationRule; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import java.util.Map; + +/** + * Asserts that the {@code kid} header of a JWT contains the {@code iss}: + * 
+ *     kid := iss # key-id
+ *
+ * where {@code iss} usually are DIDs, and the {@code key-id} is an arbitrary string. + */ +public class IssuerKeyIdValidationRule implements TokenValidationRule { + private final String keyId; + + public IssuerKeyIdValidationRule(String tokenKeyIdHeader) { + this.keyId = tokenKeyIdHeader; + } + + @Override + public Result checkRule(@NotNull ClaimToken toVerify, @Nullable Map additional) { + var iss = toVerify.getStringClaim("iss"); + // keyID MUST be a composite of the issuer and the key-id in the form # + return keyId.matches("%s#.*".formatted(iss)) ? Result.success() : Result.failure("kid header '%s' expected to correlate to 'iss' claim ('%s'), but it did not.".formatted(keyId, iss)); + } +} diff --git a/extensions/common/crypto/jwt-verifiable-credentials/src/test/java/org/eclipse/edc/verifiablecredentials/jwt/JwtPresentationVerifierTest.java b/extensions/common/crypto/jwt-verifiable-credentials/src/test/java/org/eclipse/edc/verifiablecredentials/jwt/JwtPresentationVerifierTest.java index c5d7a683a65..539edeabfa5 100644 --- a/extensions/common/crypto/jwt-verifiable-credentials/src/test/java/org/eclipse/edc/verifiablecredentials/jwt/JwtPresentationVerifierTest.java +++ b/extensions/common/crypto/jwt-verifiable-credentials/src/test/java/org/eclipse/edc/verifiablecredentials/jwt/JwtPresentationVerifierTest.java @@ -18,9 +18,14 @@ import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; import com.nimbusds.jose.JOSEException; +import com.nimbusds.jose.JWSAlgorithm; +import com.nimbusds.jose.JWSHeader; +import com.nimbusds.jose.crypto.ECDSASigner; import com.nimbusds.jose.jwk.Curve; import com.nimbusds.jose.jwk.ECKey; import com.nimbusds.jose.jwk.gen.ECKeyGenerator; +import com.nimbusds.jwt.JWTClaimsSet; +import com.nimbusds.jwt.SignedJWT; import org.eclipse.edc.iam.identitytrust.spi.verification.VerifierContext; import org.eclipse.edc.jsonld.util.JacksonJsonLd; import org.eclipse.edc.junit.annotations.ComponentTest; @@ -34,7 +39,10 @@ import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; +import java.sql.Date; +import java.time.Instant; import java.util.Map; +import java.util.UUID; import static org.eclipse.edc.junit.assertions.AbstractResultAssert.assertThat; import static org.eclipse.edc.spi.result.Result.success; @@ -48,6 +56,7 @@ import static org.eclipse.edc.verifiablecredentials.jwt.TestConstants.VP_CONTENT_TEMPLATE; import static org.eclipse.edc.verifiablecredentials.jwt.TestConstants.VP_HOLDER_ID; import static org.eclipse.edc.verifiablecredentials.jwt.TestFunctions.createPublicKey; +import static org.mockito.ArgumentMatchers.endsWith; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -241,6 +250,105 @@ void verifyPresentation_wrongAudience() { assertThat(result).isFailed().detail().contains("Token audience claim (aud -> [invalid-vp-audience]) did not contain expected audience: did:web:myself"); } + @Test + void verifyPresentation_jwtVpHasSpoofedKidClaim() throws JOSEException { + + var spoofedKey = new ECKeyGenerator(Curve.P_256).keyID("did:web:attacker#violating-key").generate(); + + when(publicKeyResolverMock.resolveKey(endsWith("violating-key"))).thenReturn(success(createPublicKey(spoofedKey.toPublicJWK()))); + + // create first VC-JWT (signed by the central issuer) + var vcJwt1 = JwtCreationUtils.createJwt(vcSigningKey, CENTRAL_ISSUER_DID, "degreeSub", VP_HOLDER_ID, Map.of("vc", VC_CONTENT_DEGREE_EXAMPLE)); + + var vpContent = VP_CONTENT_TEMPLATE.formatted("\"" + vcJwt1 + "\""); + + + String vpJwt; + try { + var signer = new ECDSASigner(spoofedKey.toECPrivateKey()); + + // Prepare JWT with claims set + var now = Date.from(Instant.now()); + var claimsSet = new JWTClaimsSet.Builder() + .issuer(VP_HOLDER_ID) + .subject("testSub") + .issueTime(now) + .audience(MY_OWN_DID) + .notBeforeTime(now) + .claim("jti", UUID.randomUUID().toString()) + .expirationTime(Date.from(Instant.now().plusSeconds(60))); + + Map.of("vp", asMap(vpContent)).forEach(claimsSet::claim); + + var signedJwt = new SignedJWT(new JWSHeader.Builder(JWSAlgorithm.ES256).keyID(spoofedKey.getKeyID()).build(), claimsSet.build()); + + signedJwt.sign(signer); + + vpJwt = signedJwt.serialize(); + + } catch (JOSEException e) { + throw new RuntimeException(e); + } + var context = VerifierContext.Builder.newInstance() + .verifier(verifier) + .audience(MY_OWN_DID) + .build(); + var result = verifier.verify(vpJwt, context); + + assertThat(result) + .isFailed() + .detail() + .isEqualTo("kid header 'did:web:attacker#violating-key' expected to correlate to 'iss' claim ('did:web:test-issuer'), but it did not."); + } + + @Test + void verifyCredential_jwtVpHasSpoofedKidClaim() throws JOSEException { + + var spoofedKey = new ECKeyGenerator(Curve.P_256).keyID("did:web:attacker#violating-key").generate(); + + when(publicKeyResolverMock.resolveKey(endsWith("violating-key"))).thenReturn(success(createPublicKey(spoofedKey.toPublicJWK()))); + + // create first VC-JWT (signed by the central issuer) + + String vcJwt1; + try { + var signer = new ECDSASigner(spoofedKey.toECPrivateKey()); + + // Prepare JWT with claims set + var now = Date.from(Instant.now()); + var claimsSet = new JWTClaimsSet.Builder() + .issuer(CENTRAL_ISSUER_DID) + .subject("degreeSub") + .issueTime(now) + .audience(VP_HOLDER_ID) + .notBeforeTime(now) + .claim("jti", UUID.randomUUID().toString()) + .expirationTime(Date.from(Instant.now().plusSeconds(60))); + + Map.of("vc", VC_CONTENT_DEGREE_EXAMPLE).forEach(claimsSet::claim); + + var signedJwt = new SignedJWT(new JWSHeader.Builder(JWSAlgorithm.ES256).keyID(spoofedKey.getKeyID()).build(), claimsSet.build()); + + signedJwt.sign(signer); + + vcJwt1 = signedJwt.serialize(); + + } catch (JOSEException e) { + throw new RuntimeException(e); + } + var context = VerifierContext.Builder.newInstance() + .verifier(verifier) + .audience(MY_OWN_DID) + .build(); + var result = verifier.verify(vcJwt1, context); + + assertThat(result) + .isFailed() + .detail() + .isEqualTo("kid header 'did:web:attacker#violating-key' expected to correlate to 'iss' claim ('did:web:some-official-issuer'), but it did not."); + } + + private Map asMap(String rawContent) { try { return mapper.readValue(rawContent, new TypeReference<>() { From fe6ad6dfbbd5b5b6240c87d227af67945e5f42e6 Mon Sep 17 00:00:00 2001 From: Paul Latzelsperger Date: Wed, 15 May 2024 15:56:33 +0200 Subject: [PATCH 2/4] DEPENDENCIES --- DEPENDENCIES | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/DEPENDENCIES b/DEPENDENCIES index 9bfed1bddc9..8dae39c857f 100644 --- a/DEPENDENCIES +++ b/DEPENDENCIES @@ -125,7 +125,7 @@ maven/mavencentral/io.netty/netty-tcnative-classes/2.0.56.Final, Apache-2.0, app maven/mavencentral/io.netty/netty-transport-native-unix-common/4.1.86.Final, Apache-2.0 AND BSD-3-Clause AND MIT, approved, CQ20926 maven/mavencentral/io.netty/netty-transport/4.1.86.Final, Apache-2.0 AND BSD-3-Clause AND MIT, approved, CQ20926 maven/mavencentral/io.opentelemetry.instrumentation/opentelemetry-instrumentation-annotations/1.32.0, Apache-2.0, approved, #11684 -maven/mavencentral/io.opentelemetry.proto/opentelemetry-proto/1.3.1-alpha, None, restricted, #14688 +maven/mavencentral/io.opentelemetry.proto/opentelemetry-proto/1.3.1-alpha, Apache-2.0, approved, #14688 maven/mavencentral/io.opentelemetry/opentelemetry-api/1.32.0, Apache-2.0, approved, #11682 maven/mavencentral/io.opentelemetry/opentelemetry-context/1.32.0, Apache-2.0, approved, #11683 maven/mavencentral/io.prometheus/simpleclient/0.16.0, Apache-2.0, approved, clearlydefined @@ -305,18 +305,22 @@ maven/mavencentral/org.jetbrains/annotations/24.1.0, Apache-2.0, approved, clear maven/mavencentral/org.junit-pioneer/junit-pioneer/2.2.0, EPL-2.0, approved, #11857 maven/mavencentral/org.junit.jupiter/junit-jupiter-api/5.10.1, EPL-2.0, approved, #9714 maven/mavencentral/org.junit.jupiter/junit-jupiter-api/5.10.2, EPL-2.0, approved, #9714 +maven/mavencentral/org.junit.jupiter/junit-jupiter-api/5.9.2, EPL-2.0, approved, #3133 maven/mavencentral/org.junit.jupiter/junit-jupiter-engine/5.10.1, EPL-2.0, approved, #9711 maven/mavencentral/org.junit.jupiter/junit-jupiter-engine/5.10.2, EPL-2.0, approved, #9711 +maven/mavencentral/org.junit.jupiter/junit-jupiter-engine/5.9.2, EPL-2.0, approved, #3125 maven/mavencentral/org.junit.jupiter/junit-jupiter-params/5.10.1, EPL-2.0, approved, #9708 maven/mavencentral/org.junit.jupiter/junit-jupiter-params/5.10.2, EPL-2.0, approved, #9708 +maven/mavencentral/org.junit.jupiter/junit-jupiter-params/5.9.2, EPL-2.0, approved, #3134 maven/mavencentral/org.junit.platform/junit-platform-commons/1.10.1, EPL-2.0, approved, #9715 maven/mavencentral/org.junit.platform/junit-platform-commons/1.10.2, EPL-2.0, approved, #9715 +maven/mavencentral/org.junit.platform/junit-platform-commons/1.9.2, EPL-2.0, approved, #3130 maven/mavencentral/org.junit.platform/junit-platform-engine/1.10.1, EPL-2.0, approved, #9709 maven/mavencentral/org.junit.platform/junit-platform-engine/1.10.2, EPL-2.0, approved, #9709 +maven/mavencentral/org.junit.platform/junit-platform-engine/1.9.2, EPL-2.0, approved, #3128 maven/mavencentral/org.junit.platform/junit-platform-launcher/1.10.1, EPL-2.0, approved, #9704 -maven/mavencentral/org.junit.platform/junit-platform-launcher/1.10.2, EPL-2.0, approved, #9704 +maven/mavencentral/org.junit.platform/junit-platform-launcher/1.9.2, EPL-2.0, approved, #3132 maven/mavencentral/org.junit/junit-bom/5.10.1, EPL-2.0, approved, #9844 -maven/mavencentral/org.junit/junit-bom/5.10.2, EPL-2.0, approved, #9844 maven/mavencentral/org.junit/junit-bom/5.9.2, EPL-2.0, approved, #4711 maven/mavencentral/org.jvnet.mimepull/mimepull/1.9.15, CDDL-1.1 OR GPL-2.0-only WITH Classpath-exception-2.0, approved, CQ21484 maven/mavencentral/org.latencyutils/LatencyUtils/2.0.3, BSD-2-Clause, approved, CQ17408 From e2a9c2f1a9d87f21c02c686388060d9911905847 Mon Sep 17 00:00:00 2001 From: Paul Latzelsperger Date: Wed, 15 May 2024 15:58:39 +0200 Subject: [PATCH 3/4] use constant --- .../jwt/rules/IssuerKeyIdValidationRule.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/extensions/common/crypto/jwt-verifiable-credentials/src/main/java/org/eclipse/edc/verifiablecredentials/jwt/rules/IssuerKeyIdValidationRule.java b/extensions/common/crypto/jwt-verifiable-credentials/src/main/java/org/eclipse/edc/verifiablecredentials/jwt/rules/IssuerKeyIdValidationRule.java index 6492ab2fb3f..7e674b9dc4e 100644 --- a/extensions/common/crypto/jwt-verifiable-credentials/src/main/java/org/eclipse/edc/verifiablecredentials/jwt/rules/IssuerKeyIdValidationRule.java +++ b/extensions/common/crypto/jwt-verifiable-credentials/src/main/java/org/eclipse/edc/verifiablecredentials/jwt/rules/IssuerKeyIdValidationRule.java @@ -14,6 +14,7 @@ package org.eclipse.edc.verifiablecredentials.jwt.rules; +import org.eclipse.edc.jwt.spi.JwtRegisteredClaimNames; import org.eclipse.edc.spi.iam.ClaimToken; import org.eclipse.edc.spi.result.Result; import org.eclipse.edc.token.spi.TokenValidationRule; @@ -38,7 +39,7 @@ public IssuerKeyIdValidationRule(String tokenKeyIdHeader) { @Override public Result checkRule(@NotNull ClaimToken toVerify, @Nullable Map additional) { - var iss = toVerify.getStringClaim("iss"); + var iss = toVerify.getStringClaim(JwtRegisteredClaimNames.ISSUER); // keyID MUST be a composite of the issuer and the key-id in the form # return keyId.matches("%s#.*".formatted(iss)) ? Result.success() : Result.failure("kid header '%s' expected to correlate to 'iss' claim ('%s'), but it did not.".formatted(keyId, iss)); } From 8244efa4cd0a9d820530831e46dbfcb3852beafd Mon Sep 17 00:00:00 2001 From: Paul Latzelsperger Date: Wed, 15 May 2024 16:50:59 +0200 Subject: [PATCH 4/4] DEPENDENCIES --- DEPENDENCIES | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/DEPENDENCIES b/DEPENDENCIES index 8dae39c857f..b7040091014 100644 --- a/DEPENDENCIES +++ b/DEPENDENCIES @@ -305,22 +305,18 @@ maven/mavencentral/org.jetbrains/annotations/24.1.0, Apache-2.0, approved, clear maven/mavencentral/org.junit-pioneer/junit-pioneer/2.2.0, EPL-2.0, approved, #11857 maven/mavencentral/org.junit.jupiter/junit-jupiter-api/5.10.1, EPL-2.0, approved, #9714 maven/mavencentral/org.junit.jupiter/junit-jupiter-api/5.10.2, EPL-2.0, approved, #9714 -maven/mavencentral/org.junit.jupiter/junit-jupiter-api/5.9.2, EPL-2.0, approved, #3133 maven/mavencentral/org.junit.jupiter/junit-jupiter-engine/5.10.1, EPL-2.0, approved, #9711 maven/mavencentral/org.junit.jupiter/junit-jupiter-engine/5.10.2, EPL-2.0, approved, #9711 -maven/mavencentral/org.junit.jupiter/junit-jupiter-engine/5.9.2, EPL-2.0, approved, #3125 maven/mavencentral/org.junit.jupiter/junit-jupiter-params/5.10.1, EPL-2.0, approved, #9708 maven/mavencentral/org.junit.jupiter/junit-jupiter-params/5.10.2, EPL-2.0, approved, #9708 -maven/mavencentral/org.junit.jupiter/junit-jupiter-params/5.9.2, EPL-2.0, approved, #3134 maven/mavencentral/org.junit.platform/junit-platform-commons/1.10.1, EPL-2.0, approved, #9715 maven/mavencentral/org.junit.platform/junit-platform-commons/1.10.2, EPL-2.0, approved, #9715 -maven/mavencentral/org.junit.platform/junit-platform-commons/1.9.2, EPL-2.0, approved, #3130 maven/mavencentral/org.junit.platform/junit-platform-engine/1.10.1, EPL-2.0, approved, #9709 maven/mavencentral/org.junit.platform/junit-platform-engine/1.10.2, EPL-2.0, approved, #9709 -maven/mavencentral/org.junit.platform/junit-platform-engine/1.9.2, EPL-2.0, approved, #3128 maven/mavencentral/org.junit.platform/junit-platform-launcher/1.10.1, EPL-2.0, approved, #9704 -maven/mavencentral/org.junit.platform/junit-platform-launcher/1.9.2, EPL-2.0, approved, #3132 +maven/mavencentral/org.junit.platform/junit-platform-launcher/1.10.2, EPL-2.0, approved, #9704 maven/mavencentral/org.junit/junit-bom/5.10.1, EPL-2.0, approved, #9844 +maven/mavencentral/org.junit/junit-bom/5.10.2, EPL-2.0, approved, #9844 maven/mavencentral/org.junit/junit-bom/5.9.2, EPL-2.0, approved, #4711 maven/mavencentral/org.jvnet.mimepull/mimepull/1.9.15, CDDL-1.1 OR GPL-2.0-only WITH Classpath-exception-2.0, approved, CQ21484 maven/mavencentral/org.latencyutils/LatencyUtils/2.0.3, BSD-2-Clause, approved, CQ17408