search

erik-megarad / negative-captcha

A plugin to make the process of creating a negative captcha in Rails much less painful
MIT License
791 stars 72 forks source link

Replace homegrown crypto with safer constructs #33

Open stouset opened 11 years ago

stouset commented 11 years ago
  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. 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.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.

erik-megarad commented 11 years ago

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 #initialize.

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.

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.

stouset commented 11 years ago

A combination of nonce and timestamp is probably a good idea.

If you're assuming Rails, you could use Rails.cache to store each seen nonce.