Skip to content

Validate the top-level "keys" field in JWK Set import - #76

Open
rishuranjanofficial wants to merge 1 commit into
tink-crypto:mainfrom
rishuranjanofficial:fix/jwkset-keys-validation
Open

Validate the top-level "keys" field in JWK Set import#76
rishuranjanofficial wants to merge 1 commit into
tink-crypto:mainfrom
rishuranjanofficial:fix/jwkset-keys-validation

Conversation

@rishuranjanofficial

Copy link
Copy Markdown

Problem

JwkSetConverter.toPublicKeysetHandle(String jwkSet) and
SignatureJwkSetConverter.toPublicKeysetHandle(String jwkSet) both wrap only
the initial JSON parse in a try/catch block:

jsonKeyset = JsonParser.parse(jwkSet).getAsJsonObject();
// catches IllegalStateException | IOException

Immediately after, both methods read the top-level "keys" field and
iterate its contents with no further validation, outside of that
try/catch:

JsonArray jsonKeys = jsonKeyset.get("keys").getAsJsonArray();
for (JsonElement element : jsonKeys) {
  JsonObject jsonKey = element.getAsJsonObject();

A well-formed but unexpected JSON input triggers an uncaught
RuntimeException instead of the documented GeneralSecurityException in
three cases:

  1. No "keys" field present (input is "{}"): JsonObject.get("keys") returns
    null, and calling .getAsJsonArray() on it throws
    NullPointerException.
  2. "keys" present but not an array (e.g. {"keys":"not an array"}):
    JsonElement.getAsJsonArray() throws IllegalStateException.
  3. An element of "keys" is not a JSON object (e.g.
    {"keys":["not an object"]}): JsonElement.getAsJsonObject() throws
    IllegalStateException.

Both methods declare a checked-exception contract (GeneralSecurityException,
plus IOException on the JWT variant for backward compatibility), so any
caller written against that contract will not catch these exceptions.

Root cause

Both files already apply a defensive has() / isJsonArray() / isJsonObject()
check before extracting every other field from untrusted JSON, for
example getStringItem() and validateKeyOpsIsVerify() in both files. The
one place this pattern is missing is the very first thing each method
does with the parsed JSON: reading the top-level "keys" array.

This is the same bug class fixed in JsonKeysetReader.java in commit
d9552c8 ("Report malformed JSON keyset fields as IOException instead of
an uncaught exception"), which documents the identical mechanism for a
different accessor (getAsString() rather than getAsJsonArray() /
getAsJsonObject()). That fix did not extend to these two JWK Set
converters.

Fix

Add the missing validation, mirroring the existing
has()/isJsonArray()/isJsonObject() pattern already used elsewhere in both
files, before the "keys" field is read or iterated in each converter.

Testing

Added three regression tests to each of JwkSetConverterTest.java and
SignatureJwkSetConverterTest.java, covering a missing "keys" field, a
non-array "keys" field, and a non-object element inside "keys". All three
previously threw an uncaught RuntimeException and now correctly throw
GeneralSecurityException.

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).
@rishuranjanofficial

Copy link
Copy Markdown
Author

@tholenst

I've submitted this pull request and would appreciate a review when you have some bandwidth. Please let me know if any changes or additional context are needed.

Thank you for your time and consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant