babelouest / rhonabwy

Javascript Object Signing and Encryption (JOSE) library - JWK, JWKS, JWS, JWE and JWT
https://babelouest.github.io/rhonabwy/
GNU Lesser General Public License v2.1
45 stars 21 forks source link

Flawed check that the "kty" member is present in JWKs #10

Closed mkauf closed 3 years ago

mkauf commented 3 years ago

JWKs must have a "kty" member. From RFC 7517:

The "kty" value is a case-sensitive string. This member MUST be present in a JWK.

In the function r_jwk_is_valid() in jwk.c, there are some checks:

I don't understand the reasoning behind this. "kty" should always be present, and this should be checked.

Current code:

// Validate if required parameters are present and consistent
if (ret == RHN_OK) {
  if (!has_kty) {
    if (!has_alg) {
      y_log_message(Y_LOG_LEVEL_ERROR, "r_jwk_is_valid - Invalid data");
      ret = RHN_ERROR_PARAM;
    }
  } else {
    if (has_kty && !has_pubkey_parameters) {
      y_log_message(Y_LOG_LEVEL_ERROR, "r_jwk_is_valid - public key parameters missing");
      ret = RHN_ERROR_PARAM;
    }
  }
}
babelouest commented 3 years ago

Hello @mkauf ,

Thanks for the report. Indeed there are bugs in the jwk validation functions. Fixing the kty bug made me realize other parts had some bugs too (I'm talking to you both x5c and x5u!).

Also, some tests were simply wrong, like this one: https://github.com/babelouest/rhonabwy/blob/master/test/jwk_import.c#L685

I'll soon push a fix about that, shouldn't take long.

babelouest commented 3 years ago

The bug has been fixed in the master branch, including some refactoring of the functions r_jwk_is_valid and r_jwk_key_type.

mkauf commented 3 years ago

Thank you very much for the bugfix!