Replace homegrown crypto with safer constructs #33
Conversation
1. Default, non-random secrets are *always* a bad idea. 2. MD5 is not a good passphrase hashing function. 3. MD5, while not *yet* vulnerable to preimage attacks (which would allow forging "spinners") will only get weaker over time, and should not be used in new projects. 4. Raw hash functions are not suitable for use as message authenticators. That said, this PR makes a few changes: 1. The default secret is removed, so users must specify them explicitly. 2. All uses of MD5 have been removed. 3. Concatenated strings as authenticators have been replaced with HMAC constructions. To elaborate on the change to HMACs: Simply concatenating a secret to a message and hashing it does not constitute a secure construction. [HMAC](http://en.wikipedia.org/wiki/Hash-based_message_authentication_code)s are used for this purpose, and are specifically designed to avoid things like length extension attacks as well as maintain some security even in the face of collisions in the unkeyed hash function. Additionally, strings can't simply be concatenated to form the message. This can allow attackers to [manipulate message boundaries](http://security.stackexchange.com/questions/2202/lessons-learned-and-misconceptions-regarding-encryption-and-cryptology/2212#2212). You attempted to use hyphens, but the output of `Time.now` already includes them and so they cannot be reliably used to delineate message boundaries. Length prefixes are a sufficient solution (as suggested in the linked article). Full disclosure: I have not run the tests for this change. I edited it in the GitHub editor. You should probably run the tests. I porblaby hvae a tpyo soemwheer. As one final concern, timestamps are being used to create unique "session keys" for each user. Timestamps are predictable, and predictability is not a good idea here. You should be using randomly-generated nonces. As this requires a change across multiple files (to change the parameter name), I haven't submitted it as part of this edit.
|
I think this is mostly good. Let me think about it for a bit and test it out and whatnot. One thing that will have to change is that this breaks backwards compatibility by changing the method signature for
How would you expire nonces? The point of the timestamp is to provide a maximum amount of replayability if they had a human hand-identify form fields and then a bot perform multiple submissions with this knowledge. Retargeting for image captchas is already A Thing. |
|
A combination of nonce and timestamp is probably a good idea. If you're assuming Rails, you could use |
That said, this PR makes a few changes:
To elaborate on the change to HMACs: Simply concatenating a secret to a message and hashing it does not constitute a secure construction. HMACs are used for this purpose, and are specifically designed to avoid things like length extension attacks as well as maintain some security even in the face of collisions in the unkeyed hash function.
Additionally, strings can't simply be concatenated to form the message. This can allow attackers to manipulate message boundaries. You attempted to use hyphens, but the output of
Time.nowalready includes them and so they cannot be reliably used to delineate message boundaries. Length prefixes are a sufficient solution (as suggested in the linked article).Full disclosure: I have not run the tests for this change. I edited it in the GitHub editor. You should probably run the tests. I porblaby hvae a tpyo soemwheer.
As one final concern, timestamps are being used to create unique "session keys" for each user. Timestamps are predictable, and predictability is not a good idea here. You should be using randomly-generated nonces. As this requires a change across multiple files (to change the parameter name), I haven't submitted it as part of this edit.