From 947af0f561378bc11a37ab0a3bd0c4a9ac4a89cc Mon Sep 17 00:00:00 2001 From: rishuranjanofficial Date: Tue, 4 Aug 2026 11:25:03 +0530 Subject: [PATCH] Validate the top-level "keys" field in JWK Set import JwkSetConverter.toPublicKeysetHandle and SignatureJwkSetConverter.toPublicKeysetHandle read jsonKeyset.get("keys") and iterate it without checking that the field exists, is an array, or that its elements are JSON objects. A malformed JWK Set (missing "keys", "keys" as a non-array, or a non-object element) throws an uncaught NullPointerException or IllegalStateException instead of the documented GeneralSecurityException/IOException. This mirrors the same defensive has()/isJsonArray()/isJsonObject() pattern already used elsewhere in both files (e.g. validateKeyOpsIsVerify), and the same bug class fixed in JsonKeysetReader.java (commit d9552c8). --- .../crypto/tink/jwt/JwkSetConverter.java | 9 ++ .../signature/SignatureJwkSetConverter.java | 9 ++ .../crypto/tink/jwt/JwkSetConverterTest.java | 32 +++++ .../SignatureJwkSetConverterTest.java | 35 +++++ tink_java_jwkset_keys_validation_fix.patch | 135 ++++++++++++++++++ 5 files changed, 220 insertions(+) create mode 100644 tink_java_jwkset_keys_validation_fix.patch diff --git a/src/main/java/com/google/crypto/tink/jwt/JwkSetConverter.java b/src/main/java/com/google/crypto/tink/jwt/JwkSetConverter.java index e480a094f..9c272cd84 100644 --- a/src/main/java/com/google/crypto/tink/jwt/JwkSetConverter.java +++ b/src/main/java/com/google/crypto/tink/jwt/JwkSetConverter.java @@ -93,9 +93,18 @@ public static KeysetHandle toPublicKeysetHandle(String jwkSet) } catch (IllegalStateException | IOException ex) { throw new GeneralSecurityException("JWK set is invalid JSON", ex); } + if (!jsonKeyset.has("keys")) { + throw new GeneralSecurityException("keys not found"); + } + if (!jsonKeyset.get("keys").isJsonArray()) { + throw new GeneralSecurityException("keys is not an array"); + } KeysetHandle.Builder builder = KeysetHandle.newBuilder(); JsonArray jsonKeys = jsonKeyset.get("keys").getAsJsonArray(); for (JsonElement element : jsonKeys) { + if (!element.isJsonObject()) { + throw new GeneralSecurityException("key is not a JSON object"); + } JsonObject jsonKey = element.getAsJsonObject(); String algPrefix = getStringItem(jsonKey, "alg").substring(0, 2); switch (algPrefix) { diff --git a/src/main/java/com/google/crypto/tink/signature/SignatureJwkSetConverter.java b/src/main/java/com/google/crypto/tink/signature/SignatureJwkSetConverter.java index ac5bb940c..314384a71 100644 --- a/src/main/java/com/google/crypto/tink/signature/SignatureJwkSetConverter.java +++ b/src/main/java/com/google/crypto/tink/signature/SignatureJwkSetConverter.java @@ -92,9 +92,18 @@ public static KeysetHandle toPublicKeysetHandle(String jwkSet) throws GeneralSec } catch (IllegalStateException | IOException ex) { throw new GeneralSecurityException("JWK set is invalid JSON", ex); } + if (!jsonKeyset.has("keys")) { + throw new GeneralSecurityException("keys not found"); + } + if (!jsonKeyset.get("keys").isJsonArray()) { + throw new GeneralSecurityException("keys is not an array"); + } KeysetHandle.Builder builder = KeysetHandle.newBuilder(); JsonArray jsonKeys = jsonKeyset.get("keys").getAsJsonArray(); for (JsonElement element : jsonKeys) { + if (!element.isJsonObject()) { + throw new GeneralSecurityException("key is not a JSON object"); + } JsonObject jsonKey = element.getAsJsonObject(); String kty = getStringItem(jsonKey, "kty"); switch (kty) { diff --git a/src/test/java/com/google/crypto/tink/jwt/JwkSetConverterTest.java b/src/test/java/com/google/crypto/tink/jwt/JwkSetConverterTest.java index 5f0e03e0b..d79a70b48 100644 --- a/src/test/java/com/google/crypto/tink/jwt/JwkSetConverterTest.java +++ b/src/test/java/com/google/crypto/tink/jwt/JwkSetConverterTest.java @@ -899,6 +899,38 @@ public void ecdsaWithUnknownField_toPublicKeysetHandleSuccess() throws Exception Object unused = JwkSetConverter.toPublicKeysetHandle(jwksString); } + @Test + public void withoutKeysField_toPublicKeysetHandleFailsWithGeneralSecurityException() + throws Exception { + // Well-formed JSON, but missing the top-level "keys" array entirely. Before the fix, this + // threw an uncaught NullPointerException instead of the documented GeneralSecurityException, + // since jsonKeyset.get("keys") returns null for a Gson JsonObject that has no such member. + String jwksString = "{}"; + assertThrows( + GeneralSecurityException.class, () -> JwkSetConverter.toPublicKeysetHandle(jwksString)); + } + + @Test + public void keysFieldIsNotAnArray_toPublicKeysetHandleFailsWithGeneralSecurityException() + throws Exception { + // Before the fix, this threw an uncaught IllegalStateException (from + // JsonElement.getAsJsonArray()) instead of the documented GeneralSecurityException, because + // the "keys" value is read and converted outside of the method's only try/catch block. + String jwksString = "{\"keys\":\"not an array\"}"; + assertThrows( + GeneralSecurityException.class, () -> JwkSetConverter.toPublicKeysetHandle(jwksString)); + } + + @Test + public void keysArrayContainsNonObjectElement_toPublicKeysetHandleFailsWithGeneralSecurityException() + throws Exception { + // Before the fix, this threw an uncaught IllegalStateException (from + // JsonElement.getAsJsonObject()) instead of the documented GeneralSecurityException. + String jwksString = "{\"keys\":[\"not an object\"]}"; + assertThrows( + GeneralSecurityException.class, () -> JwkSetConverter.toPublicKeysetHandle(jwksString)); + } + @Test public void ecdsaWithoutAlg_toPublicKeysetHandleFails() throws Exception { String jwksString = diff --git a/src/test/java/com/google/crypto/tink/signature/SignatureJwkSetConverterTest.java b/src/test/java/com/google/crypto/tink/signature/SignatureJwkSetConverterTest.java index c93d325ca..1b64b253c 100644 --- a/src/test/java/com/google/crypto/tink/signature/SignatureJwkSetConverterTest.java +++ b/src/test/java/com/google/crypto/tink/signature/SignatureJwkSetConverterTest.java @@ -139,6 +139,41 @@ public void testExportTinkVariantThrows() throws Exception { GeneralSecurityException.class, () -> SignatureJwkSetConverter.fromPublicKeysetHandle(publicHandle)); } + @Test + public void withoutKeysField_toPublicKeysetHandleFailsWithGeneralSecurityException() + throws Exception { + // Well-formed JSON, but missing the top-level "keys" array entirely. Before the fix, this + // threw an uncaught NullPointerException instead of the documented GeneralSecurityException, + // since jsonKeyset.get("keys") returns null for a Gson JsonObject that has no such member. + String jwksString = "{}"; + assertThrows( + GeneralSecurityException.class, + () -> SignatureJwkSetConverter.toPublicKeysetHandle(jwksString)); + } + + @Test + public void keysFieldIsNotAnArray_toPublicKeysetHandleFailsWithGeneralSecurityException() + throws Exception { + // Before the fix, this threw an uncaught IllegalStateException (from + // JsonElement.getAsJsonArray()) instead of the documented GeneralSecurityException, because + // the "keys" value is read and converted outside of the method's only try/catch block. + String jwksString = "{\"keys\":\"not an array\"}"; + assertThrows( + GeneralSecurityException.class, + () -> SignatureJwkSetConverter.toPublicKeysetHandle(jwksString)); + } + + @Test + public void keysArrayContainsNonObjectElement_toPublicKeysetHandleFailsWithGeneralSecurityException() + throws Exception { + // Before the fix, this threw an uncaught IllegalStateException (from + // JsonElement.getAsJsonObject()) instead of the documented GeneralSecurityException. + String jwksString = "{\"keys\":[\"not an object\"]}"; + assertThrows( + GeneralSecurityException.class, + () -> SignatureJwkSetConverter.toPublicKeysetHandle(jwksString)); + } + @Test public void testExportEcdsaDerEncodingThrows() throws Exception { // PredefinedSignatureParameters.ECDSA_P256 is TINK variant, but even if we make it RAW, diff --git a/tink_java_jwkset_keys_validation_fix.patch b/tink_java_jwkset_keys_validation_fix.patch new file mode 100644 index 000000000..b255a28c2 --- /dev/null +++ b/tink_java_jwkset_keys_validation_fix.patch @@ -0,0 +1,135 @@ +diff --git a/src/main/java/com/google/crypto/tink/jwt/JwkSetConverter.java b/src/main/java/com/google/crypto/tink/jwt/JwkSetConverter.java +index e480a09..9c272cd 100644 +--- a/src/main/java/com/google/crypto/tink/jwt/JwkSetConverter.java ++++ b/src/main/java/com/google/crypto/tink/jwt/JwkSetConverter.java +@@ -93,9 +93,18 @@ public final class JwkSetConverter { + } catch (IllegalStateException | IOException ex) { + throw new GeneralSecurityException("JWK set is invalid JSON", ex); + } ++ if (!jsonKeyset.has("keys")) { ++ throw new GeneralSecurityException("keys not found"); ++ } ++ if (!jsonKeyset.get("keys").isJsonArray()) { ++ throw new GeneralSecurityException("keys is not an array"); ++ } + KeysetHandle.Builder builder = KeysetHandle.newBuilder(); + JsonArray jsonKeys = jsonKeyset.get("keys").getAsJsonArray(); + for (JsonElement element : jsonKeys) { ++ if (!element.isJsonObject()) { ++ throw new GeneralSecurityException("key is not a JSON object"); ++ } + JsonObject jsonKey = element.getAsJsonObject(); + String algPrefix = getStringItem(jsonKey, "alg").substring(0, 2); + switch (algPrefix) { +diff --git a/src/main/java/com/google/crypto/tink/signature/SignatureJwkSetConverter.java b/src/main/java/com/google/crypto/tink/signature/SignatureJwkSetConverter.java +index ac5bb94..314384a 100644 +--- a/src/main/java/com/google/crypto/tink/signature/SignatureJwkSetConverter.java ++++ b/src/main/java/com/google/crypto/tink/signature/SignatureJwkSetConverter.java +@@ -92,9 +92,18 @@ public final class SignatureJwkSetConverter { + } catch (IllegalStateException | IOException ex) { + throw new GeneralSecurityException("JWK set is invalid JSON", ex); + } ++ if (!jsonKeyset.has("keys")) { ++ throw new GeneralSecurityException("keys not found"); ++ } ++ if (!jsonKeyset.get("keys").isJsonArray()) { ++ throw new GeneralSecurityException("keys is not an array"); ++ } + KeysetHandle.Builder builder = KeysetHandle.newBuilder(); + JsonArray jsonKeys = jsonKeyset.get("keys").getAsJsonArray(); + for (JsonElement element : jsonKeys) { ++ if (!element.isJsonObject()) { ++ throw new GeneralSecurityException("key is not a JSON object"); ++ } + JsonObject jsonKey = element.getAsJsonObject(); + String kty = getStringItem(jsonKey, "kty"); + switch (kty) { +diff --git a/src/test/java/com/google/crypto/tink/jwt/JwkSetConverterTest.java b/src/test/java/com/google/crypto/tink/jwt/JwkSetConverterTest.java +index 5f0e03e..d79a70b 100644 +--- a/src/test/java/com/google/crypto/tink/jwt/JwkSetConverterTest.java ++++ b/src/test/java/com/google/crypto/tink/jwt/JwkSetConverterTest.java +@@ -899,6 +899,38 @@ public final class JwkSetConverterTest { + Object unused = JwkSetConverter.toPublicKeysetHandle(jwksString); + } + ++ @Test ++ public void withoutKeysField_toPublicKeysetHandleFailsWithGeneralSecurityException() ++ throws Exception { ++ // Well-formed JSON, but missing the top-level "keys" array entirely. Before the fix, this ++ // threw an uncaught NullPointerException instead of the documented GeneralSecurityException, ++ // since jsonKeyset.get("keys") returns null for a Gson JsonObject that has no such member. ++ String jwksString = "{}"; ++ assertThrows( ++ GeneralSecurityException.class, () -> JwkSetConverter.toPublicKeysetHandle(jwksString)); ++ } ++ ++ @Test ++ public void keysFieldIsNotAnArray_toPublicKeysetHandleFailsWithGeneralSecurityException() ++ throws Exception { ++ // Before the fix, this threw an uncaught IllegalStateException (from ++ // JsonElement.getAsJsonArray()) instead of the documented GeneralSecurityException, because ++ // the "keys" value is read and converted outside of the method's only try/catch block. ++ String jwksString = "{\"keys\":\"not an array\"}"; ++ assertThrows( ++ GeneralSecurityException.class, () -> JwkSetConverter.toPublicKeysetHandle(jwksString)); ++ } ++ ++ @Test ++ public void keysArrayContainsNonObjectElement_toPublicKeysetHandleFailsWithGeneralSecurityException() ++ throws Exception { ++ // Before the fix, this threw an uncaught IllegalStateException (from ++ // JsonElement.getAsJsonObject()) instead of the documented GeneralSecurityException. ++ String jwksString = "{\"keys\":[\"not an object\"]}"; ++ assertThrows( ++ GeneralSecurityException.class, () -> JwkSetConverter.toPublicKeysetHandle(jwksString)); ++ } ++ + @Test + public void ecdsaWithoutAlg_toPublicKeysetHandleFails() throws Exception { + String jwksString = +diff --git a/src/test/java/com/google/crypto/tink/signature/SignatureJwkSetConverterTest.java b/src/test/java/com/google/crypto/tink/signature/SignatureJwkSetConverterTest.java +index c93d325..1b64b25 100644 +--- a/src/test/java/com/google/crypto/tink/signature/SignatureJwkSetConverterTest.java ++++ b/src/test/java/com/google/crypto/tink/signature/SignatureJwkSetConverterTest.java +@@ -139,6 +139,41 @@ public final class SignatureJwkSetConverterTest { + GeneralSecurityException.class, () -> SignatureJwkSetConverter.fromPublicKeysetHandle(publicHandle)); + } + ++ @Test ++ public void withoutKeysField_toPublicKeysetHandleFailsWithGeneralSecurityException() ++ throws Exception { ++ // Well-formed JSON, but missing the top-level "keys" array entirely. Before the fix, this ++ // threw an uncaught NullPointerException instead of the documented GeneralSecurityException, ++ // since jsonKeyset.get("keys") returns null for a Gson JsonObject that has no such member. ++ String jwksString = "{}"; ++ assertThrows( ++ GeneralSecurityException.class, ++ () -> SignatureJwkSetConverter.toPublicKeysetHandle(jwksString)); ++ } ++ ++ @Test ++ public void keysFieldIsNotAnArray_toPublicKeysetHandleFailsWithGeneralSecurityException() ++ throws Exception { ++ // Before the fix, this threw an uncaught IllegalStateException (from ++ // JsonElement.getAsJsonArray()) instead of the documented GeneralSecurityException, because ++ // the "keys" value is read and converted outside of the method's only try/catch block. ++ String jwksString = "{\"keys\":\"not an array\"}"; ++ assertThrows( ++ GeneralSecurityException.class, ++ () -> SignatureJwkSetConverter.toPublicKeysetHandle(jwksString)); ++ } ++ ++ @Test ++ public void keysArrayContainsNonObjectElement_toPublicKeysetHandleFailsWithGeneralSecurityException() ++ throws Exception { ++ // Before the fix, this threw an uncaught IllegalStateException (from ++ // JsonElement.getAsJsonObject()) instead of the documented GeneralSecurityException. ++ String jwksString = "{\"keys\":[\"not an object\"]}"; ++ assertThrows( ++ GeneralSecurityException.class, ++ () -> SignatureJwkSetConverter.toPublicKeysetHandle(jwksString)); ++ } ++ + @Test + public void testExportEcdsaDerEncodingThrows() throws Exception { + // PredefinedSignatureParameters.ECDSA_P256 is TINK variant, but even if we make it RAW,