Skip to content

Commit 5038144

Browse files
mikeraclaude
andcommitted
fix(security): bind UCAN JWT verification to the iss key, not the kid header
UCAN.fromJWT verified the EdDSA signature against the public key named in the attacker-controlled `kid` header, then trusted the `iss` claim without checking the two agree. An attacker could sign a token with their own key, place that key in `kid`, and claim any `iss` (e.g. a trusted root) — an issuer-spoofing authorization bypass. The forged token passed validateJWT / parseTransportUCANs, and via UCANValidator.capabilitiesFor(..., issuer=root) yielded capabilities attributed to the root. Reachable from transport input (e.g. DlfsMcpTools). Fix: derive the verification key from the `iss` DID (did:key encodes the issuer key) and verify against it; the `kid` header is no longer trusted for key selection. Legitimately signed tokens are unaffected (kid == iss key already). The CAD3 path (UCAN.verifySignature / validate) was already correct. Audit of the other JWT verification paths: - PeerAuth.verifySelfIssued: safe — binds sub to the signing key (sub == did:key(kid)) - PeerAuth.verifyPeerSigned / OAuthService (JWKS RS256) / JWKSKeys: safe Added a SECURITY WARNING to the kid-trusting JWT.verifyEdDSA()/verifyPublic(jwt). Adversarial tests (UCANTest): forge iss=ROOT signed by a rogue key and assert rejection at fromJWT, validateJWT, the transport boundary, and in a proof chain; plus a regression that genuine tokens still verify. Confirmed all four fail against the pre-fix kid-based verification. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent ec3ffac commit 5038144

3 files changed

Lines changed: 106 additions & 11 deletions

File tree

convex-core/src/main/java/convex/auth/jwt/JWT.java

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,16 @@ public String getKeyID() {
141141
// ========== Instance verification methods ==========
142142

143143
/**
144-
* Verify this JWT as a self-issued EdDSA token.
145-
* Extracts the public key from the {@code kid} header (multikey format).
144+
* Verify this JWT as a self-issued EdDSA token, taking the public key from the
145+
* {@code kid} header (multikey format).
146+
*
147+
* <p><b>SECURITY WARNING:</b> this trusts the {@code kid} header to supply the
148+
* verification key, so a valid result only proves "signed by whoever is named in
149+
* {@code kid}" — which the sender chooses. Do NOT use this where an identity claim
150+
* ({@code iss}, {@code sub}, ...) is trusted unless you separately bind that claim to
151+
* the signing key (e.g. require {@code sub == did:key(kid)}). For tokens whose identity
152+
* is itself a key (did:key), verify against the key derived from that claim using
153+
* {@link #verifyEdDSA(AccountKey)} instead.</p>
146154
*
147155
* @return true if signature is valid
148156
*/
@@ -374,6 +382,12 @@ public static boolean verifyHS256(AString jwt, byte[] secret) {
374382
* Extracts the public key from the {@code kid} header parameter (multikey format),
375383
* verifies the Ed25519 signature, and returns the parsed claims map.
376384
*
385+
* <p><b>SECURITY WARNING:</b> the {@code kid} header is sender-controlled, so this only
386+
* proves the token was signed by the key named in {@code kid}. Do NOT trust any identity
387+
* claim from the returned map unless you bind it to the signing key. Prefer
388+
* {@link #verifyPublic(AString, AccountKey)} against a key you trust out-of-band, or the
389+
* key derived from the identity claim itself. See {@link #verifyEdDSA()}.</p>
390+
*
377391
* @param jwt The encoded JWT string
378392
* @return Claims map if signature is valid, or null if verification fails
379393
*/

convex-core/src/main/java/convex/auth/ucan/UCAN.java

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -398,20 +398,23 @@ public static UCAN fromJWT(AString jwtString) {
398398
JWT parsed = JWT.parse(jwtString);
399399
if (parsed == null) return null;
400400

401-
// Verify signature via kid header
402-
if (!parsed.verifyEdDSA()) return null;
403-
404401
AMap<AString, ACell> claims = parsed.getClaims();
405402
if (claims == null) return null;
406403

407-
// Extract issuer key for internal use
408-
AString issuerDID = RT.ensureString(claims.get(ISS));
409-
AccountKey issuerKey = fromDIDKey(issuerDID);
404+
// Derive the verification key from the `iss` DID (did:key encodes the issuer's
405+
// public key) and verify the signature against THAT key.
406+
//
407+
// SECURITY: the verification key MUST be bound to the claimed issuer. The JWT `kid`
408+
// header is attacker-controlled and must not select the verification key — otherwise
409+
// an attacker could sign with their own key, name it in `kid`, and forge any `iss`
410+
// (issuer spoofing / authorization bypass). For a did:key UCAN the issuer IS the key,
411+
// so we ignore `kid` and verify directly against the iss key.
412+
AccountKey issuerKey = fromDIDKey(RT.ensureString(claims.get(ISS)));
410413
if (issuerKey == null) return null;
414+
if (!parsed.verifyEdDSA(issuerKey)) return null;
411415

412-
// Build the signature from the JWT's raw bytes
413-
// We store the JWT's signature but note: verifySignature() uses CAD3,
414-
// so for JWT-parsed tokens use verifyJWT() instead
416+
// Build the signature from the JWT's raw bytes. Note: verifySignature() uses CAD3,
417+
// so for JWT-parsed tokens use the JWT verification path, not verifySignature().
415418
ASignature sig = ASignature.fromBlob(Blob.wrap(parsed.getSignatureBytes()));
416419

417420
return new UCAN(claims, sig);

convex-core/src/test/java/convex/auth/ucan/UCANTest.java

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
import org.junit.jupiter.api.Test;
1111

12+
import convex.auth.jwt.JWT;
1213
import convex.core.crypto.AKeyPair;
1314
import convex.core.data.ACell;
1415
import convex.core.data.AMap;
@@ -454,6 +455,83 @@ public void testJWTFromMalformed() {
454455
assertNull(UCAN.fromJWT(Strings.create("")));
455456
}
456457

458+
// ===== Adversarial: issuer spoofing via mismatched kid =====
459+
//
460+
// A did:key UCAN's issuer IS its key, so verification must be bound to the `iss`
461+
// claim — never to the attacker-controlled `kid` header. These tests forge a token
462+
// that claims iss=ROOT but is actually signed by ROGUE (whose key lands in kid), and
463+
// assert it is rejected at every layer. They fail against kid-based verification.
464+
465+
/** A JWT claiming {@code iss=spoofedIssuer} but actually signed by {@code signer} (kid = signer's key). */
466+
private static AString forgeIssuer(AccountKey spoofedIssuer, AKeyPair signer, AccountKey aud,
467+
long exp, AVector<ACell> caps) {
468+
AMap<AString, ACell> claims = UCAN.buildPayload(spoofedIssuer, aud, exp, null, caps, null, null);
469+
return JWT.signPublic(claims, signer); // signs with `signer`; kid header = signer's key
470+
}
471+
472+
@Test
473+
public void testJWTIssuerSpoofRejectedAtParse() {
474+
AString forged = forgeIssuer(ROOT_KP.getAccountKey(), ROGUE_KP, ROGUE_KP.getAccountKey(),
475+
FUTURE_EXPIRY, oneCap("w/secret", Capability.TOP));
476+
assertNull(UCAN.fromJWT(forged),
477+
"JWT claiming iss=ROOT but signed by another key (named in kid) must be rejected");
478+
}
479+
480+
@Test
481+
public void testJWTIssuerSpoofRejectedAtValidate() {
482+
AString forged = forgeIssuer(ROOT_KP.getAccountKey(), ROGUE_KP, ROGUE_KP.getAccountKey(),
483+
FUTURE_EXPIRY, oneCap("w/secret", Capability.TOP));
484+
assertNull(UCANValidator.validateJWT(forged, NOW));
485+
}
486+
487+
@Test
488+
@SuppressWarnings("unchecked")
489+
public void testJWTIssuerSpoofDroppedAtTransportBoundary() {
490+
AString forged = forgeIssuer(ROOT_KP.getAccountKey(), ROGUE_KP, ROGUE_KP.getAccountKey(),
491+
FUTURE_EXPIRY, oneCap("w/secret", Capability.TOP));
492+
// Forgery alone: nothing passes the trust boundary
493+
assertNull(UCANValidator.parseTransportUCANs(Vectors.of(forged)),
494+
"Spoofed-issuer token must not pass the transport trust boundary");
495+
496+
// Mixed with a genuine ROOT->ROGUE grant: only the genuine token survives, and the
497+
// forged ROOT capability is never attributed to ROOT through the end-to-end path.
498+
AString genuine = UCAN.createJWT(ROOT_KP, ROGUE_KP.getAccountKey(), FUTURE_EXPIRY,
499+
oneCap("w/health", Capability.CRUD_READ), null);
500+
AVector<ACell> verified = UCANValidator.parseTransportUCANs(Vectors.of(forged, genuine));
501+
assertNotNull(verified);
502+
assertEquals(1L, verified.count(), "only the genuinely-signed token survives");
503+
504+
AString rogueDID = UCAN.toDIDKey(ROGUE_KP.getAccountKey());
505+
AVector<ACell> caps = UCANValidator.capabilitiesFor(verified, rogueDID, ROOT_DID, NOW);
506+
assertNotNull(caps);
507+
assertEquals(1L, caps.count());
508+
assertFalse(Capability.covers((AMap<AString, ACell>) caps.get(0), "w/secret/x", "crud/write"),
509+
"forged ROOT-issued capability must not appear");
510+
}
511+
512+
@Test
513+
public void testJWTSpoofedProofInChainRejected() {
514+
// A genuine child (AGENT_A -> AGENT_B) whose proof is a forged ROOT token (claims
515+
// iss=ROOT but actually signed by ROGUE). The chain must fail to validate.
516+
AString forgedProof = forgeIssuer(ROOT_KP.getAccountKey(), ROGUE_KP, AGENT_A_KP.getAccountKey(),
517+
FUTURE_EXPIRY, oneCap("w/secret", Capability.TOP));
518+
AString child = UCAN.createJWT(AGENT_A_KP, AGENT_B_KP.getAccountKey(), FUTURE_EXPIRY,
519+
null, Vectors.of(forgedProof));
520+
assertNull(UCANValidator.validateJWT(child, NOW),
521+
"A chain whose proof spoofs its issuer must be rejected");
522+
}
523+
524+
@Test
525+
public void testGenuineJWTStillVerifiesAfterFix() {
526+
// Regression: the fix must not break legitimately-signed tokens (kid == iss key).
527+
AString genuine = UCAN.createJWT(ROOT_KP, AGENT_A_KP.getAccountKey(), FUTURE_EXPIRY,
528+
oneCap("w/health", Capability.CRUD_READ), null);
529+
UCAN parsed = UCAN.fromJWT(genuine);
530+
assertNotNull(parsed);
531+
assertEquals(ROOT_DID, parsed.getIssuer());
532+
assertNotNull(UCANValidator.validateJWT(genuine, NOW));
533+
}
534+
457535
// ===== checkTemporalBounds =====
458536
//
459537
// Post-ingress helper used by callers that have already verified the

0 commit comments

Comments
 (0)