avoid key mixup for JWT access token validation

[zandbelt]

As someone pointed out to me, lua-resty-openidc in its default setup seems vulnerable to https://www.chosenplaintext.ca/2015/03/31/jwt-algorithm-confusion.html

It appears one can create a symmetric key "derived" from a public RSA key and use that "crafted" symmetric key to sign a JWT with alg HS256 and present that token to openidc_load_jwt_and_verify_crypto(opts, access_token, opts.secret, opts.secret, opts.token_signing_alg_values_expected, …) at: https://github.com/zmartzone/lua-resty-openidc/blob/v1.6.1/lib/resty/openidc.lua#L1435
This would validate the HS256 access token because it would treat the public key stored in opt.secret as a symmetric key value.

This is only possible when opts.token_signing_alg_values_expected is not set so an attacker can confuse the validation function. Since this option is not set by default, lua-resty-openidc is vulnerable by default to the attack described above and we can do better.

Note that this won't work for the id_token in an OIDC RP setup as the id_token is only returned from backchannel call to a trusted OP.

I believe we should split the opts.secret configuration option into an explicit opts.symmetric_key and opts.public_key value, probably providing some backwards compatibility with a warning.

@bodewig what do you think?

[bodewig]

This discussion sounds familiar #122 (comment) :-)

Back then we didn't see a big difference and kept what looked like the smaller change. No problem with making the difference explicit for opts as well but this probably cannot be done in a backwards compatible way.

[zandbelt]

@bodewig can you confirm that the new log message shows up in your unit tests? Strangely I don't seem to be able to see it. https://github.com/zmartzone/lua-resty-openidc/compare/avoid-key-confusion

[bodewig]

Yes, I see it.

2018/09/14 16:54:49 [warn] 29#29: *3 [lua] openidc.lua:866: openidc_load_jwt_and_verify_crypto(): using deprecated option `opts.secret` for asymmetric key; switch to `opts.public_key` instead, client: 127.0.0.1, server: , request: "GET /verify_bearer_token HTTP/1.1", host: "127.0.0.1"

It is a bit tricky, I've removed all tests except for "when using a statically configured RSA public key" from bearer_token_verification_spec, started the container with /bin/bash as command and looked at the log file.

I'll look into adding an assertion and a new test case that verifies the warning is not there when using the new keys and push it to the branch.

[bodewig]

see a71f2d1