Support encrypted tokens (with JWT fallback)

[benthorner]

https://trello.com/c/FwB5bALg/336-double-opt-in-fix-data-leak

Previously we supported verification of a subscriber via the signature
of a JWT token, for which the payload isn't encrypted. This adds support
for an encrypted+signed token using ActiveSupport's MessageVerifier.

We expect to switch all tokens to use this new mechanism and remove
support for JWT tokens after about a week from deployment, since the
tokens would be invalid anyway after that time.

Since the associated secret is only intended for this purpose, we use a
fixed, empty salt ("") when generating a key of the appropriate length
for the MessageVerifier.

[kevindew]

Great catch - I did not realise that the data in JWT wasn't encrypted and expect that this is a very common misconception.

[kevindew]

Do we want to specify an algorithm so that if Rails defaults change and our apps don't change at the same time this doesn't break?

[barrucadu]

That sounds like a good idea to me

[benthorner]

Sure. Turns out the default stated in the docs is wrong and is not the default in the code. It's supposed to be more secure, though, so I've switched to using it.

[kevindew]

Are we sure there are no risks or clues given if we don't have a salt? we could always add one as an environment variable?

[barrucadu]

If we specify a salt in an environment variable, would we have to change it frequently? Does a static salt add much security over a nonexistent salt?

[kevindew]

I don't personally really understand the role of salt in symmetric encryption 🤔 It makes a lot of sense in hashing.

[barrucadu]

Looks like ActiveSupport::KeyGenerator uses PBKD2F, and the role of the salt there is to prevent rainbow table attacks to reverse-engineer the password

[kevindew]

Oh I see - so it's used on a one way hash on the key. Thanks 👍. I can understand why it may be of no value in this situation.

[kevindew]

@benthorner by the way we already have a precedence for using email in plain text in https://github.com/alphagov/email-alert-api/blob/bd3988428c72e83b1c93c3ca8a0d239272cb0d69/spec/builders/message_email_builder_spec.rb#L103 which may also want to be changed if this has been identified as a concern.

[barrucadu]

PBKDF2 also takes the number of iterations as a parameter. We probably want to specify that here. OWASP recommends at least 10,000 iterations.

---

Using a static salt is called "peppering", and it is a thing people do, so fortunately we're not totally missing the point. However, it seems that we don't actually need to transmit the salt in cleartext to the token consumer, it's included in the ciphertext in some way, as seen from this example:

len   = ActiveSupport::MessageEncryptor.key_len
salt  = SecureRandom.random_bytes(len)
key   = ActiveSupport::KeyGenerator.new('password').generate_key(salt, len) # => "\x89\xE0\x156\xAC..."
crypt = ActiveSupport::MessageEncryptor.new(key)                            # => #<ActiveSupport::MessageEncryptor ...>
encrypted_data = crypt.encrypt_and_sign('my secret data')                   # => "NlFBTTMwOUV5UlA1QlNEN2xkY2d6eThYWWh..."
crypt.decrypt_and_verify(encrypted_data)                                    # => "my secret data"

Seeing this, I think we should generate a random salt.

[kevindew]

PBKDF2 also takes the number of iterations as a parameter. We probably want to specify that here. OWASP recommends at least 10,000 iterations.

Using a static salt is called "peppering", and it is a thing people do, so fortunately we're not totally missing the point. However, it seems that we don't actually need to transmit the salt in cleartext to the token consumer, it's included in the ciphertext in some way, as seen from this example:

len   = ActiveSupport::MessageEncryptor.key_len
salt  = SecureRandom.random_bytes(len)
key   = ActiveSupport::KeyGenerator.new('password').generate_key(salt, len) # => "\x89\xE0\x156\xAC..."
crypt = ActiveSupport::MessageEncryptor.new(key)                            # => #<ActiveSupport::MessageEncryptor ...>
encrypted_data = crypt.encrypt_and_sign('my secret data')                   # => "NlFBTTMwOUV5UlA1QlNEN2xkY2d6eThYWWh..."
crypt.decrypt_and_verify(encrypted_data)                                    # => "my secret data"

Seeing this, I think we should generate a random salt.

Oh cool - yes I thought there were a number of algorithms that included the salt in the text but was unable to find evidence / convince Ben of this haha. Nice work.

[benthorner]

@kevindew @barrucadu thanks for looking into this a bit more. I'm struggling to follow the comments about the use of the salt. If it is included in the ciphertext, it's not clear how we would extract it in order for it to be part of the shared secret between the two apps. We could use a non-empty salt, but that would require more work to set it in environment variables.

As a summary question: are we concerned that not specifying a salt is a security risk? Is this risk greater than our previous implementation with JWT?

[kevindew]

@kevindew @barrucadu thanks for looking into this a bit more. I'm struggling to follow the comments about the use of the salt. If it is included in the ciphertext, it's not clear how we would extract it in order for it to be part of the shared secret between the two apps. We could use a non-empty salt, but that would require more work to set it in environment variables.

I believe that the purpose is just that the same non-secret input produces different output so you can't determine which data is the same purely by the output. If my understanding is right you don't need the salt to decrypt.

As a summary question: are we concerned that not specifying a salt is a security risk? Is this risk greater than our previous implementation with JWT?

I wouldn't say it's a concern. But then I don't think this whole thing is a security issue as the information we're sharing isn't particularly secret - just undesirable for logs and 3rd parties. The risk factor I'd identify is that by doing this message encryption we're setting a precedent and often these things are copied and used again without concern. So my preference on cracking the salt stuff is not so much for the risks now but more for our commitment towards best practices.

[barrucadu]

I just tried this out in a rails console to confirm you don't need the salt to decrypt:

Encrypt data with a random salt:

irb(main):002:0> len   = ActiveSupport::MessageEncryptor.key_len
irb(main):003:0> salt  = SecureRandom.random_bytes(len)
=> "\x95\xD1p\e\xCA\xC4\xA7\x18&\xC6\xB1\x0EO\x83e(\x0Eru\xCC_\xD1\xC98\xC9\x1DI\x86\xAA\xBF\x18\xAE"
irb(main):004:0> key   = ActiveSupport::KeyGenerator.new('password').generate_key(salt, len)
irb(main):005:0> crypt = ActiveSupport::MessageEncryptor.new(key)
irb(main):006:0> encrypted_data = crypt.encrypt_and_sign('my secret data')
=> "bjhGOEZtcjREYXBpd25teWM5bUNjMDVjUWpsVXRoTHlpeHF4TUZlcDF3QT0tLS9ZSlVYUkRLUmJsVzkrWS9BUUlVZnc9PQ==--6a7b2aedcbaae87aa8b17884941c7e97845ca865"

Decrypt data with a static salt:

irb(main):007:0> key2   = ActiveSupport::KeyGenerator.new('password').generate_key("no salt", len)
irb(main):008:0> crypt2 = ActiveSupport::MessageEncryptor.new(key)
irb(main):009:0> crypt2.decrypt_and_verify(encrypted_data)
=> "my secret data"

[benthorner]

@kevindew I agree we should try to set a good precedent here, if possible. My understanding is still that we can't use a salt without sharing it between the two apps, which means it has to be static for all uses and is effectively the same as not having a salt. Although @barrucadu's example indicates sharing a random salt isn't necessary, that's because line 008 should be key2, not key.

Trying this myself...

irb(main):007:0> crypt2.decrypt_and_verify(encrypted_data)
Traceback (most recent call last):
        1: from (irb):7
ActiveSupport::MessageVerifier::InvalidSignature (ActiveSupport::MessageVerifier::InvalidSignature)

I think the scenario where a salt is appropriate is when it can be stored in memory, or associated with some other stored data, such as a user ID. We could opt to use the subscription ID as the salt for double opt-in, since this is shared between the apps. But I think that would just make the mechanism more complicated and harder to re-use for the existing management flow.

A reasonable counter argument might be: we were comfortable with not using a (random) salt with JWT, with the assumption that it too was doing encryption.

Does that address your concerns a little?

[barrucadu]

Although @barrucadu's example indicates sharing a random salt isn't necessary, that's because line 008 should be key2, not key.

🤦‍♂

[benthorner]

@barrucadu @kevindew do you have any further comments/changes on this?