Missing HMAC_SECRET When HMAC Encoding Not Raising Proper Error

[jonmchan]

According to the documentation, when utilizing the HMAC algorithm, a JWT::DecodeError should be raised if the hmac_secret is not provided.

https://github.com/jwt/ruby-jwt#hmac

# The secret must be a string. A JWT::DecodeError will be raised if it isn't provided.
hmac_secret = 'my$ecretK3y'

As it stands, there is inconsistent behavior between what version of Openssl is installed on the system. For example, for the following systems:

irb(main):001:0> puts OpenSSL::OPENSSL_VERSION
OpenSSL 1.1.1f  31 Mar 2020
=> nil                                                 
irb(main):002:0> JWT.encode( {user: 'test'}, nil, 'HS256')
=> "eyJhbGciOiJIUzI1NiJ9.eyJ1c2VyIjoidGVzdCJ9.J6fk_CtqWW8QMxkcNc1olC3YI_C-B2FGpIcHegIaZ08"
irb(main):003:0> JWT.encode( {user: 'test'}, '', 'HS256')
=> "eyJhbGciOiJIUzI1NiJ9.eyJ1c2VyIjoidGVzdCJ9.J6fk_CtqWW8QMxkcNc1olC3YI_C-B2FGpIcHegIaZ08"

This should not be succeeding. Not sure what is being used to generate the HMAC.

However, on newer versions of OpenSSL, we observe the following:

irb(main):001:0> puts OpenSSL::OPENSSL_VERSION
OpenSSL 3.0.5 5 Jul 2022
=> nil 
irb(main):003:0> JWT.encode( {user: 'test'}, '', 'HS256')
/Users/jmc/.rbenv/versions/3.1.2/lib/ruby/3.1.0/openssl/hmac.rb:36:in `initialize': EVP_PKEY_new_mac_key: malloc failure (OpenSSL::HMACError)
irb(main):003:0> JWT.encode( {user: 'test'}, nil, 'HS256')
/Users/jmc/.rbenv/versions/3.1.2/lib/ruby/3.1.0/openssl/hmac.rb:36:in `initialize': EVP_PKEY_new_mac_key: malloc failure (OpenSSL::HMACError)

Neither of these is the proper JWT::DecodeError as defined by the documentation. Catching this properly would be good.

[anakinj]

Agree that it should be handled better. Think the behaviour has changed somewhere along the way.

[jonmchan]

I'd be happy to submit a PR if I can get some guidance on what is expected.

Would the following be appropriate to raise in https://github.com/jwt/ruby-jwt/blob/main/lib/jwt/algos/hmac.rb?

sign:

raise EncodeError, "hmac_secret is required for hmac encoding" unless key.class == String && !key.strip.empty?

verify:

raise DecodeError, "hmac_secret is required for hmac decoding" unless key.class == String && !key.strip.empty?

[excpt]

@jonmchan
A test should be provided with the changes that ensures that the behaviour is kept in future versions.

[jonmchan]

@excpt yes, I will also write the tests too. Is the error and behavior I describe above acceptable?

[anakinj]

Now thinking some more about this I think the behaviour is intended (#312).

Also to be noted that the behaviour (EVP_PKEY_new_mac_key error) with an empty key is is now fixed for the openssl gem and OpenSSL 3.0. ruby/openssl#538

Also maybe the behaviour should be:

• Raise if the key is nil
• Do not raise if the key is an empty string or a string with whitespaces (whitespaces are characters too)

But this behaviour change probably needs to happen in a major version with documented breaking changes.

[jonmchan]

@anakinj I would argue that as it stands, it is very inconsistent behavior. We had a hard time tracking this error down because I wasn't getting any errors on my linux machine while my coworkers on mac with different versions of openssl were seeing the error.

I believe this can be extremely frustrating for developers to track down. This is already "broken" and I would argue a minor version fix would be appropriate because if someone was using nil values and they upgrade OpenSSL, it would magically break for them at that point. This at least needs to be documented, which I suppose is already accomplished by this issue.

I am still amicable to submit a PR if we can have some consensus on what the behavior should be and you guys can release it in whatever version you deem fit.

[anakinj]

I agree with you that the current way it behaves with different OpenSSL versions is not the most optimal.

The challenge is what the underlying OpenSSL 3.0 functionality in Ruby is supporting. There are other similar changes in OpenSSL 3.0 that are affecting what you can and cannot do via the openssl gem interfaces. For example generating RSA keys from parameters is very restricted currently in the openssl gem combined with OpenSSL 3.0 (#523).

I would not like to change the current behaviour with OpenSSL 1.1 and signing with a empty string as the key in a minor version of this gem. The unfortunate thing is that currently it's not possible to do have the exact same with certain OpenSSL 3.x versions, apparently the behaviour will be different depending on what openssl version you are using.

Maybe the best would be to change the behaviour for everyone and not allow signing with a empty key, as suggested.
It's not a secure approach anyway. Also think this kind of behaviour change should happen in a major version bump.

We have a history breaking stuff doing these kind of "trivial" changes so I therefore I would like to play it safe.

[jonmchan]

I totally can respect that.

What about this proposal for a minor fix:

Rescue the EVP_PKEY_new_mac_key: malloc failure (OpenSSL::HMACError) error and raise a DecodeError or EncodeError stating: OpenSSL 3.0 does not support nil or empty hmac_secret; update openssl gem > 3.0.1 as a workaround.

Pros:

• For old versions of OpenSSL, no change happens; they continue to allow nil or empty hmac
• Newer versions of openssl or projects that use the openssl gem that has bypassed this issue will work with nil or empty.
• Systems with the version of openssl that has this issue no longer receive a cryptic exception but one that clearly informs the developer what the issue was. (I thought malloc failure was indicative of a bad binary or something much bigger wrong than just passing nil or blank to hmac_secret).

[anakinj]

That is a great idea. Better feedback for OpenSSL 3.0 users.