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

Suggestion: Special treatment for unsecured JWTs ("alg": "none") #11

Closed mkauf closed 3 years ago

mkauf commented 3 years ago

The cjose library always reports that tokens with "alg": "none" are invalid, because of the security problems with "alg": "none". See Critical vulnerabilities in JSON Web Token libraries.

So maybe Rhonabwy could do the same?

babelouest commented 3 years ago

Thanks for this, it raised me some questions about the implementation.

I agree using tokens with alg: "none" may be useless, but it could be useful sometimes. Like when an OAuth2 request parameter is sent by a public client in a JWT format, the client has no secret or secret key to sign the message. In that case, a non signed token is useful. https://datatracker.ietf.org/doc/html/rfc7523

Nevertheless, the signature verification must not return RHN_OK if the token is not signed, I'll fix that soon.

babelouest commented 3 years ago

@mkauf , I'm also open to suggestions and feedbacks about that, maybe my interpretation above is not completely accurate. Do you have ideas of improvements concerning the alg: "none"?

babelouest commented 3 years ago

An first update has been committed here: https://github.com/babelouest/rhonabwy/commit/ff9ecad4c9a031c8369acde67ea52d558899e51e , but I'll wait for your feedback before closing this issue.

mkauf commented 3 years ago

I agree that r_jws_verify_signature() should return an error code for "alg": "none".

I don't know whether the use case with OAuth2 that you mentioned is still possible with these changes, though. Would a special error code help, e. g. RHN_ERROR_UNSECURED ?

babelouest commented 3 years ago

I don't know whether the use case with OAuth2 that you mentioned is still possible with these changes

An example of alg:"none" I talk about is here: https://github.com/babelouest/glewlwyd/blob/master/src/plugin/protocol_oidc.c#L4283

If the client is confidential, the algorithm used must match type allowed for the client. If the client is public, the algorithm must be none.

The functions I see where the algorithm should be checked are:

Instead of adding an error value RHN_ERROR_UNSECURED, I have another suggestion:

mkauf commented 3 years ago

I'm fine with the proposed changes. It's a good approach to make it explicit whether unsecured tokens are allowed.

This issue can be closed, commit ff9ecad is already sufficient for my use case.

babelouest commented 3 years ago

@mkauf , I've made the changes in the master branch, the API documentation has been updated too:

Can you verify that those changes don't break your tests?

mkauf commented 3 years ago

I have tested with the master branch, it runs well and our tests have succeeded. Thanks a lot!