Disable the support for embedded JWKs or allow to disable it

[p-]

Hello @babelouest, thanks for creating rhonabwy! I noticed that rhonabwy seems to support embedded JWKs in JWE and JWS:

• in jwe.c: https://github.com/babelouest/rhonabwy/blob/4480465efa3201ee27a9112507f3740294db01c9/src/jwe.c#L1991

• in jws.c: https://github.com/babelouest/rhonabwy/blob/4480465efa3201ee27a9112507f3740294db01c9/src/jws.c#L85

I'm not sure yet whether this could lead to a vulnerability in this context or not, but I think it would be good if this feature was disabled by default or if it were possible to disable that feature (e.g. like it's possible to disable remote certificates with R_FLAG_IGNORE_REMOTE).

In an OIDC context it makes sense to disable these features anyway, as the OpenID Connect Core 1.0 spec says:

ID Tokens SHOULD NOT use the JWS or JWE x5u, x5c, jku, or jwk Header Parameter fields. Instead, references to keys used are communicated in advance using Discovery and Registration parameters

[babelouest]

Hello @p- , thanks for the feedback!

It is true that the jwk parameter in a JWS header could lead to forged signatures and bypass a signature verification if not properly used.

About the jwk parameter, the JWS RFC simply states that:

   The "jwk" (JSON Web Key) Header Parameter is the public key that
   corresponds to the key used to digitally sign the JWS.  This key is
   represented as a JSON Web Key [JWK].  Use of this Header Parameter is
   OPTIONAL.

And the parameters x5c and x5u have similar meanings.

One way to make sure the JWS signature verification isn't fooled by a false public key is to explicitly specify the public key when using r_jws_verify_signature. The simple example in the readme file shows a secure signature verification. Even if the parsed token has a jwk or x5c header parameter, the signature verification specifies the public key so no error is possible in this case:

[...]
  const char token[] = "eyJ0eXAiOiJKV1QiLCJhbGciOiJFUzI1NiIsImtpZCI6IjEifQ."
  "eyJzdHIiOiJwbG9wIiwiaW50Ijo0Miwib2JqIjp0cnVlfQ."
  "ooXNEt3JWFGMuvkGUM-szUOU1QTu4DvyC3qQP64UGeeJQuMGupBCVATnGkiqNLiPSJ9uBsjZbyUrWe8z7Iag_A";

  const char jwk_pubkey_ecdsa_str[] = "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"MKBCTNIcKUSDii11ySs3526iDZ8AiTo7Tu6KPAqv7D4\","\
                                      "\"y\":\"4Etl6SRW2YiLUrN5vfvVHuhp7x8PxltmWWlbbM4IFyM\",\"use\":\"enc\",\"kid\":\"1\",\"alg\":\"ES256\"}";

[...]
  if (r_jwk_init(&jwk) == RHN_OK) {
    if (r_jwt_init(&jwt) == RHN_OK) {
      if (r_jwk_import_from_json_str(jwk, jwk_pubkey_ecdsa_str) == RHN_OK) {
[...]
          if (r_jwt_parse(jwt, token, 0) == RHN_OK) {
            if (r_jwt_verify_signature(jwt, jwk, 0) == RHN_OK) { // jwk is used to verify the signature
[...]

To improve the security when needed, I can add a r_jws_parse_advanced -or something like that- function with the following prototype:

/**
 * parse_flags can have multiple concatenated values like
 * R_PARSE_NO_HEADER_KEY
 * R_PARSE_ALL_HEADER_KEY
 * R_PARSE_HEADER_JWK
 * R_PARSE_HEADER_X5C
 * R_PARSE_HEADER_X5U
 * R_PARSE_UNSIGNED
 * R_PARSE_ALL
 */
int r_jws_parse_advanced(jws_t * jws, const char * token, int x5u_flags, uint parse_flags);

The use of the parse_flag makes it easier for the future to add more flags.

What do you think?

[p-]

Hi @babelouest

One way to make sure the JWS signature verification isn't fooled by a false public key is to explicitly specify the public key when using r_jws_verify_signature. The simple example in the readme file shows a secure signature verification.

That's good to know!

To improve the security when needed, I can add a r_jws_parse_advanced -or something like that- function with the following prototype:

Yeah, I think something like this would be nice. So that a user of a library can explicitly disable the parts he doesn't want to use. Altough, I could also imagine a variant where those parts have to be enabled explicitly. (I suppose most library users get by without parsing the jku, x5u and jwk headers.)

[babelouest]

@p- , I've added new functions to parse tokens, can you give me feedbacks?

/**
 * Parses a serialized JWT
 * If the JWT is signed only, the claims will be available
 * If the JWT is encrypted, the claims will not be accessible until
 * r_jwt_decrypt or r_jwt_decrypt_verify_signature_nested is succesfull
 * @param token: the token to parse into a JWT, must end with a NULL string terminator
 * @param parse_flags: Flags to set or unset options
 * Flags available are
 * - R_PARSE_NONE
 * - R_PARSE_HEADER_JWK
 * - R_PARSE_HEADER_JKU
 * - R_PARSE_HEADER_X5C
 * - R_PARSE_HEADER_X5U
 * - R_PARSE_HEADER_ALL
 * - R_PARSE_UNSIGNED
 * - R_PARSE_ALL
 * @param x5u_flags: Flags to retrieve x5u certificates
 * pointed by x5u if necessary, could be 0 if not needed
 * Flags available are 
 * - R_FLAG_IGNORE_SERVER_CERTIFICATE: ignrore if web server certificate is invalid
 * - R_FLAG_FOLLOW_REDIRECT: follow redirections if necessary
 * - R_FLAG_IGNORE_REMOTE: do not download remote key, but the function may return an error
 * @return a new jwt_t * on success, NULL on error
 */

jwt_t * r_jwt_quick_parse(const char * token, uint32_t parse_flags, int x5u_flags);
/**
 * Parses a serialized JWT
 * If the JWT is signed only, the claims will be available
 * If the JWT is encrypted, the claims will not be accessible until
 * r_jwt_decrypt or r_jwt_decrypt_verify_signature_nested is succesfull
 * @param jwt: the jwt that will contain the parsed token
 * @param token: the token to parse into a JWT, must end with a NULL string terminator
 * @param parse_flags: Flags to set or unset options
 * Flags available are
 * - R_PARSE_NONE
 * - R_PARSE_HEADER_JWK
 * - R_PARSE_HEADER_JKU
 * - R_PARSE_HEADER_X5C
 * - R_PARSE_HEADER_X5U
 * - R_PARSE_HEADER_ALL
 * - R_PARSE_UNSIGNED
 * - R_PARSE_ALL
 * @param x5u_flags: Flags to retrieve x5u certificates
 * pointed by x5u if necessary, could be 0 if not needed
 * Flags available are 
 * - R_FLAG_IGNORE_SERVER_CERTIFICATE: ignrore if web server certificate is invalid
 * - R_FLAG_FOLLOW_REDIRECT: follow redirections if necessary
 * - R_FLAG_IGNORE_REMOTE: do not download remote key, but the function may return an error
 * @return RHN_OK on success, an error value on error
 */
int r_jwt_advanced_parse(jwt_t * jwt, const char * token, uint32_t parse_flags, int x5u_flags);

[p-]

Hi @babelouest,

I've added new functions to parse tokens, can you give me feedbacks?

Uuh, that was fast! Nice! You probably know best how these functionalities match into the library, so you can take my comments with a grain of salt (because they represent my opinion, which might not be based on the whole picture). 😉

r_jwt_advanced_parse

So I think r_jwt_advanced_parse looks alright for me with one question mark. I took the text from the documentation in API.md. Where the documentation reads:

Parses the serialized JWT in all modes (compact, flattened or general)

A JWT (as I understand it) always uses the compact serialization format. As per RFC-7519:

JWTs are always represented using the JWS Compact Serialization or the JWE Compact Serialization.

So in my opinion it's not necessary to parse a JWT in any other format than the compact one. (less is more)

r_jwt_quick_parse

I don't get the use case of r_jwt_quick_parse yet. (again I might simply not understand it.) Specifically when would a user use this function? First of: The word quick for example doesn't gain my trust when reading it 😉 What I might understand (but I'm not saying that it's necessarily a better idea) would be a simple_parse method. But the simple parse method would not behave the same as the quick parse method, but rather be a dumbed down version of it: reading none of those properties (jku, jwk, x5u, x5c) and not accessing any remote URLs (admittedly that's implied when not looking at jku and x5u). So this method could for example be used for tokens used in OIDC. Again: only an idea.

[babelouest]

Parses the serialized JWT in all modes (compact, flattened or general)

A JWT (as I understand it) always uses the compact serialization format. As per RFC-7519:

Indeed, that was a mistake in the API.md file due to an early ^C^V, the rhonabwy.h file was right but not the API doc:

https://github.com/babelouest/rhonabwy/blob/master/include/rhonabwy.h#L3177

This is fixed in the last commit

I don't get the use case of r_jwt_quick_parse yet.

It's a way to make parsing more simple by embedding r_jwt_init and r_advanced_parse in a function and return a newly alocated jwt_t *, see the code, maybe the term quick isn't right though, if you have a better idea.

The sample code in the readme file would looke like this with the quick functions:

/**
 * To compile this program run:
 * gcc -o demo_rhonabwy demo_rhonabwy.c -lrhonabwy
 */
#include <stdio.h>
#include <rhonabwy.h>

int main(void) {
  const char token[] = "eyJ0eXAiOiJKV1QiLCJhbGciOiJFUzI1NiIsImtpZCI6IjEifQ."
  "eyJzdHIiOiJwbG9wIiwiaW50Ijo0Miwib2JqIjp0cnVlfQ."
  "ooXNEt3JWFGMuvkGUM-szUOU1QTu4DvyC3qQP64UGeeJQuMGupBCVATnGkiqNLiPSJ9uBsjZbyUrWe8z7Iag_A";

  const char jwk_pubkey_ecdsa_str[] = "{\"kty\":\"EC\",\"crv\":\"P-256\",\"x\":\"MKBCTNIcKUSDii11ySs3526iDZ8AiTo7Tu6KPAqv7D4\","\
                                      "\"y\":\"4Etl6SRW2YiLUrN5vfvVHuhp7x8PxltmWWlbbM4IFyM\",\"use\":\"enc\",\"kid\":\"1\",\"alg\":\"ES256\"}";

  unsigned char output[2048];
  size_t output_len = 2048;
  jwk_t * jwk = NULL;
  jwt_t * jwt = NULL;
  char * claims;

  if ((jwk = r_jwk_quick_import(R_IMPORT_JSON_STR, jwk_pubkey_ecdsa_str)) != NULL && (jwt = r_jwt_quick_parse(token, R_PARSE_NONE, 0))) {
    if (r_jwk_export_to_pem_der(jwk, R_FORMAT_PEM, output, &output_len, 0) == RHN_OK) {
      printf("Exported key:\n%.*s\n", (int)output_len, output);
      if (r_jwt_verify_signature(jwt, jwk, 0) == RHN_OK) {
        claims = r_jwt_get_full_claims_str(jwt);
        printf("Verified payload:\n%s\n", claims);
        r_free(claims);
      } else {
        fprintf(stderr, "Error r_jwt_verify_signature\n");
      }
    } else {
      fprintf(stderr, "Error r_jwk_export_to_pem_der\n");
    }
  } else {
    fprintf(stderr, "Error parsing\n");
  }
  r_jwk_free(jwk);
  r_jwt_free(jwt);

  return 0;
}

Also, I could add a simple_parse set of functions but you can easily use r_jwt_quick_parse(token, R_PARSE_NONE, 0) like in the example below. I don't mind adding simple_parse functions but I wouldn't like adding too much functions for the same purpose, when the one already present are supposed to be for all use cases. Also, the new parameter uint32_t parse_flags has room for other options I don't think about yet.

[p-]

Indeed, that was a mistake in the API.md file due to an early ^C^V, the rhonabwy.h file was right but not the API doc:

Ah I see, confused me for a moment 😉

It's a way to make parsing more simple by embedding r_jwt_init and r_advanced_parse in a function and return a newly >alocated jwt_t *, see the code, maybe the term quick isn't right though, if you have a better idea.

Ok, I get it. I was coming from another direction. Unfortunately, I don't have a better name right now.

I don't mind adding simple_parse functions but I wouldn't like adding too much functions for the same purpose, when the one already present are supposed to be for all use cases.

Yeah, that's alright. I don't think the need is that big yet. What I like though are functions, that are hard to misuse. With JWTs you have to be careful as a developer.

@babelouest I'm closing this issue if that's right for you?

Thanks again!

[babelouest]

Hello @p- ,

Happy to read that! The version 1.1.0 will be released soon then.