Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent JWT attacks by spoofed keyIDs #4186

Merged
merged 4 commits into from
May 16, 2024
Merged

fix: prevent JWT attacks by spoofed keyIDs #4186

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0273855
fix: prevent JWT attacks by spoofed keyIDs
paullatzelsperger May 15, 2024
fe6ad6d
DEPENDENCIES
paullatzelsperger May 15, 2024
e2a9c2f
use constant
paullatzelsperger May 15, 2024
8244efa
DEPENDENCIES
paullatzelsperger May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions DEPENDENCIES
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
maven/mavencentral/com.apicatalog/carbon-did/0.3.0, Apache-2.0, approved, clearlydefined

Check failure on line 1 in DEPENDENCIES

View workflow job for this annotation

GitHub Actions / check / Dash-Verify-Licenses

Dependencies outdated 

The DEPENDENCIES file was outdated and must be regenerated. Check the output of this job for more information
maven/mavencentral/com.apicatalog/copper-multibase/0.5.0, Apache-2.0, approved, #14501
maven/mavencentral/com.apicatalog/copper-multicodec/0.1.1, Apache-2.0, approved, #14500
maven/mavencentral/com.apicatalog/iron-ed25519-cryptosuite-2020/0.14.0, Apache-2.0, approved, #14503
Expand Down Expand Up @@ -125,7 +125,7 @@
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
Expand Down Expand Up @@ -305,18 +305,22 @@
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
Expand Down
16 changes: 11 additions & 5 deletions ...ials/src/main/java/org/eclipse/edc/verifiablecredentials/jwt/JwtPresentationVerifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -110,8 +111,11 @@ public Result<Void> 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();
}

Expand All @@ -120,7 +124,7 @@ public Result<Void> 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<String, Object>) signedJwt.getJWTClaimsSet().getClaim(VP_CLAIM);

Expand All @@ -145,16 +149,18 @@ public Result<Void> verify(String serializedJwt, VerifierContext context) {
return verificationResult.mapTo();
}

private List<TokenValidationRule> vpValidationRules(String audience) {
private List<TokenValidationRule> 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<TokenValidationRule> r = new ArrayList<>(tokenValidationRulesRegistry.getRules(JWT_VP_TOKEN_CONTEXT));
List<TokenValidationRule> r = new ArrayList<>(rules);
var audRule = new AudienceValidationRule(audience);
r.add(audRule);
return r;
})
.orElse(tokenValidationRulesRegistry.getRules(JWT_VP_TOKEN_CONTEXT));
.orElse(rules);

}

Expand Down
45 changes: 45 additions & 0 deletions .../main/java/org/eclipse/edc/verifiablecredentials/jwt/rules/IssuerKeyIdValidationRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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}:
* <pre>
* kid := iss # key-id
* </pre>
* 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<Void> checkRule(@NotNull ClaimToken toVerify, @Nullable Map<String, Object> additional) {
var iss = toVerify.getStringClaim("iss");
// keyID MUST be a composite of the issuer and the key-id in the form <ISSUER>#<keyID>
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));
}
}
108 changes: 108 additions & 0 deletions .../src/test/java/org/eclipse/edc/verifiablecredentials/jwt/JwtPresentationVerifierTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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<String, Object> asMap(String rawContent) {
try {
return mapper.readValue(rawContent, new TypeReference<>() {
Expand Down
Loading