New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ApplicationSecretGenerator doesn't gather sufficient entropy #2879

Closed
alsmola opened this Issue May 18, 2014 · 6 comments

Comments

Projects
None yet
4 participants
@alsmola

alsmola commented May 18, 2014

The ApplicationSecretGenerator.generateSecret function uses the SecureRandom.nextInt function to generate 64 characters with an ASCII code between 48 (0) and 122 (Z). While this generates a readable 64 character UTF-8 string with about 400 bits of entropy, only the first sixteen characters of the secret are used in the Crypto.encryptAES function, yielding about 99 bits of entropy, rather than the optimal 128 bits.

Something like the following code could be used to preserve entropy:

val keyBytes = new Array[Byte](32)
random.nextBytes(keyBytes)
new String(keyBytes, "UTF-8")

However, this would not necessarily be readable in console output. A cleaner approach might be to convert the secret to bytes and take 16, rather than taking a 16 character substring and then converting to bytes.

@huntc

This comment has been minimized.

Show comment
Hide comment
@huntc

huntc May 18, 2014

Collaborator

Nice find. I couldn't interest you in a PR could I? :-)

Collaborator

huntc commented May 18, 2014

Nice find. I couldn't interest you in a PR could I? :-)

@jroper

This comment has been minimized.

Show comment
Hide comment
@jroper

jroper May 19, 2014

Member

I would suggest actually md5ing the secret, since md5 produces 128 bits, that way all the bits of the secret are used, so even if each character only has a very limited amount of entropy, this won't matter for a sufficiently long secret.

It's important though in a solution that we provide a method of preserving the old behaviour (via a configuration option and/or different method implementations), to make it straight forward for people using the existing behaviour to get their data back.

Member

jroper commented May 19, 2014

I would suggest actually md5ing the secret, since md5 produces 128 bits, that way all the bits of the secret are used, so even if each character only has a very limited amount of entropy, this won't matter for a sufficiently long secret.

It's important though in a solution that we provide a method of preserving the old behaviour (via a configuration option and/or different method implementations), to make it straight forward for people using the existing behaviour to get their data back.

@jroper

This comment has been minimized.

Show comment
Hide comment
@jroper

jroper May 19, 2014

Member

Also, I think we should provide a new encryptAES that takes a secret of type Array[Byte] instead of String, and deprecate the one that takes String. The one that takes String should remain unchanged, and the one that uses the application secret should use the configurable secret generation as mentioned above.

Member

jroper commented May 19, 2014

Also, I think we should provide a new encryptAES that takes a secret of type Array[Byte] instead of String, and deprecate the one that takes String. The one that takes String should remain unchanged, and the one that uses the application secret should use the configurable secret generation as mentioned above.

@alsmola

This comment has been minimized.

Show comment
Hide comment
@alsmola

alsmola May 20, 2014

I think the simplest fix here is to use the code I linked above, with the caveat about display problems. Is that no good?

I'm not sure what the advantage of using an MD5 in the key generator is, since you're already guaranteed random bits from SecureRandom. The question to me is how these bits are stored in the config file. It could be hex, or Base 64, or UTF-8. UTF-8 is obviously backwards compatible, but, again, it includes non-ASCII characters when maintaining full entropy.

If you were to change the secret format, I think you'd need a secret version, which could be something like:

case class Secret(key: Array[Byte], version: Int)
private[play] def secret: Secret = {

Am I understanding the issue correctly? Happy to listen to more suggestions.

alsmola commented May 20, 2014

I think the simplest fix here is to use the code I linked above, with the caveat about display problems. Is that no good?

I'm not sure what the advantage of using an MD5 in the key generator is, since you're already guaranteed random bits from SecureRandom. The question to me is how these bits are stored in the config file. It could be hex, or Base 64, or UTF-8. UTF-8 is obviously backwards compatible, but, again, it includes non-ASCII characters when maintaining full entropy.

If you were to change the secret format, I think you'd need a secret version, which could be something like:

case class Secret(key: Array[Byte], version: Int)
private[play] def secret: Secret = {

Am I understanding the issue correctly? Happy to listen to more suggestions.

@jroper

This comment has been minimized.

Show comment
Hide comment
@jroper

jroper May 20, 2014

Member

The problem is that UTF-8 doesn't give 128 bits of entropy from 16 bytes, because there are a number of byte combinations that are not valid UTF-8 characters, so it's impossible to get them when converting a UTF-8 string to bytes. ASCII gives 112 bits (7 bits per byte), but the Play config file doesn't support all ascii characters. Also, multibyte UTF-8 characters are quite restricted in what is valid and what isn't.

The nice thing about md5ing the secret is that you get a secret that is exactly the right width for AES, plus you use the whole secret. So for example, if I set my secret to:

application.secret = "this starts off not so secret but then becomes very secret j/p8T;LOf?O?Xm=v0:8NSAV[D<FU>f:`4e?o/t>G1YlB?BubCyh^1[h09AoVlJQU"

That's a perfectly fine secret, as long as all the bytes of it are used. But if we just take the first 16 bytes, as we're doing, then we get something with very low, or in fact, no entropy. By md5ing it, we ensure that every bit of entropy in the original secret impacts the secret being used in the AES encryption.

So what this means is that the secret in the file can be in any format, whether it's a UTF-8 String, base64 encoded binary, hex encoded binary, even binary encoded binary, ie 128 random 1's and 0's - as long as there's 128 total bits of entropy, the secret will be sufficient.

Member

jroper commented May 20, 2014

The problem is that UTF-8 doesn't give 128 bits of entropy from 16 bytes, because there are a number of byte combinations that are not valid UTF-8 characters, so it's impossible to get them when converting a UTF-8 string to bytes. ASCII gives 112 bits (7 bits per byte), but the Play config file doesn't support all ascii characters. Also, multibyte UTF-8 characters are quite restricted in what is valid and what isn't.

The nice thing about md5ing the secret is that you get a secret that is exactly the right width for AES, plus you use the whole secret. So for example, if I set my secret to:

application.secret = "this starts off not so secret but then becomes very secret j/p8T;LOf?O?Xm=v0:8NSAV[D<FU>f:`4e?o/t>G1YlB?BubCyh^1[h09AoVlJQU"

That's a perfectly fine secret, as long as all the bytes of it are used. But if we just take the first 16 bytes, as we're doing, then we get something with very low, or in fact, no entropy. By md5ing it, we ensure that every bit of entropy in the original secret impacts the secret being used in the AES encryption.

So what this means is that the secret in the file can be in any format, whether it's a UTF-8 String, base64 encoded binary, hex encoded binary, even binary encoded binary, ie 128 random 1's and 0's - as long as there's 128 total bits of entropy, the secret will be sufficient.

@v6ak

This comment has been minimized.

Show comment
Hide comment
@v6ak

v6ak May 29, 2015

Contributor

According to the source, it seems to be fixed in 2.4 for new formats, see

private def secretKeyWithSha256(privateKey: String, algorithm: String) = {
and its usage.

Contributor

v6ak commented May 29, 2015

According to the source, it seems to be fixed in 2.4 for new formats, see

private def secretKeyWithSha256(privateKey: String, algorithm: String) = {
and its usage.

@jroper jroper added this to the 2.4.0 milestone Jun 4, 2015

@jroper jroper closed this Jun 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment