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

Use bcrypt instead of SHA1 with a global salt #8

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
@jneen
Copy link

jneen commented Oct 20, 2011

Among other things, this makes it harder to run brute-force dictionary attacks against the database.

[Edit] updated title

@codahale

This comment has been minimized.

Copy link

codahale commented Oct 20, 2011

Use bcrypt.

2 similar comments
@ieure

This comment has been minimized.

Copy link

ieure commented Oct 20, 2011

Use bcrypt.

@b

This comment has been minimized.

Copy link

b commented Oct 20, 2011

Use bcrypt.

@btmerr

This comment has been minimized.

Copy link

btmerr commented Oct 20, 2011

Fuck it, use rot13.

@JeremyGrosser

This comment has been minimized.

Copy link

JeremyGrosser commented Oct 20, 2011

Use bcrypt.

@bitprophet

This comment has been minimized.

Copy link

bitprophet commented Oct 20, 2011

Use bcrypt! :)

@antirez

This comment has been minimized.

Copy link
Owner

antirez commented Oct 20, 2011

For this application SHA1 or SHA2 is the same.

bcrypt makes a difference, but only for wake passwords.
bcrypt uses the Blowfish algorithm. Blowfish was specifically designed to have a slow key scheduling stage. This way it is hard to test a lot of passwords per second, so attacking weak passwords is hard.

However attacking a strong password is not feasible even if SHA1 is used.

An alternatie to bcrypt is to just use SHA1 nested, like this:

SHA1(SHA1(SHA1(SHA1(... password ... )))))

This works since you can't compute it in another direct way as far as we can tell, so you just turned SHA1 in something slower, but in this way you can make it a lot slower than bcrypt, gaining more security.

I love when security is discussed ;) as without details just resorting to best practice can be dangerous.

@jneen

This comment has been minimized.

Copy link

jneen commented Oct 20, 2011

using bcrypt, patch forthcoming. Thanks all :)

@jneen

This comment has been minimized.

Copy link

jneen commented Oct 20, 2011

also, I highly recommend some kind of gem-management tool like bundler. At this point I can only assume the production servers will have coda's gem installed.

@garybernhardt

This comment has been minimized.

Copy link

garybernhardt commented Oct 20, 2011

Or, instead of manually reimplementing stretching, you could just increase the bcrypt work factor. Bcrypt is as slow as you want it to be.

@b

This comment has been minimized.

Copy link

b commented Oct 20, 2011

You don't solve this problem by insisting users only use "strong" passwords to make up for your misuse of cryptographic primitives. bcrypt (and scrypt) are designed to solve exactly this problem. Use one of them.

@KirinDave

This comment has been minimized.

Copy link

KirinDave commented Oct 20, 2011

P.S. Use bcrypt.

@PaulMcMillan

This comment has been minimized.

Copy link

PaulMcMillan commented Oct 20, 2011

@antirez: nested SHA1 like that doesn't work because a cracker can re-use the work by chaining guesses. What you're looking for is the algorithm used in PBKDF2, which does essentially that, but it uses HMAC and part of the previous output for each round, so you can't chain your guesses.

If you're set on using a SHA family algorithm, at least vary the salt on a per-user basis.

@micrypt

This comment has been minimized.

Copy link
Contributor

micrypt commented Oct 20, 2011

... or just use bcrypt.

app.rb Outdated
@@ -599,10 +598,11 @@ def create_user(username,password)
return nil if $r.exists("username.to.id:#{username.downcase}")
id = $r.incr("users.count")
auth_token = get_rand
salt = get_rand

This comment has been minimized.

@titanous

titanous Oct 20, 2011

Bcrypt has salting built in.

This comment has been minimized.

@jneen

jneen Oct 20, 2011

Oops, forgot to remove that. You'll note that it doesn't get saved in the user hash.

@antirez

This comment has been minimized.

Copy link
Owner

antirez commented Oct 20, 2011

@paul: exactly, you can not "vanilla nest" SHA1, but you can use it in counter mode for instance, or any other mode where it is not possible to precompute previous steps. Thanks for being skilled. It is very sad that when the topic is security people can just repeat the sentence they know, but are not open to any other solution, or understanding of the problem.

@ieure

This comment has been minimized.

Copy link

ieure commented Oct 20, 2011

Use bcrypt.

@afeinberg

This comment has been minimized.

Copy link

afeinberg commented Oct 20, 2011

use bcrypt

@ice799

This comment has been minimized.

Copy link

ice799 commented Oct 20, 2011

Use an bcrypt.

@antirez

This comment has been minimized.

Copy link
Owner

antirez commented Oct 20, 2011

p.s. Not understanding is very bad for one reason. If you are for instance implementing a serious password system where there are concerns (like no bcrypt available) if you understand what you are doing you can do this without problems, otherwise you don't have a clue.

Bcrypt is the pre-cooked solution, but what it does under the hoods is to use a (slow) crypto primitive that you can reiterate at your wish, in a way so that you always need to compute the previous output.

It would be cool if we stress the concept instead of the best practice. After all we are programmers!

@b

This comment has been minimized.

Copy link

b commented Oct 20, 2011

"After all we are programmers!"

Yes, but not cryptographers. Use bcrypt.

@garybernhardt

This comment has been minimized.

Copy link

garybernhardt commented Oct 20, 2011

"Yes, but not cryptographers." <- THIS

@antirez

This comment has been minimized.

Copy link
Owner

antirez commented Oct 20, 2011

@b: if you fail understanding how you can safely combine crypto primitives, you will fail anyway.

The idea of cryptographers 15 years ago at least, was that cryptographers design crypto algorithms and protocols, but programmers should learn how they more or less work, what are the properties, and how they can be combined.

Applied Cryptography was a lot a book targeted to programmers for instance, and followed this idea.
In the latest years I see crypto to become more and more a dogma instead, and this is not good.
To solve complex problems you are going to combine crypto stuff in some way even if you don't want, so better to create knowledge, dogmas never help.

@raggi

This comment has been minimized.

Copy link

raggi commented Oct 20, 2011

Or just limit the number of connections per second allowed. At least then a DoS attempt only DoS's disconnected clients, whereas bcrypt in slow mode would DoS ALL clients.

@raggi

This comment has been minimized.

Copy link

raggi commented Oct 20, 2011

next up, people will start saying "USE PARALLEL THREADED BCRYPT", which is a total fucking fail.

@antirez

This comment has been minimized.

Copy link
Owner

antirez commented Oct 20, 2011

@raggi: good point but this does not help if your database will be stolen. You need a way to turn a password into an hashed password that has a few properties, especially: it should be very hard computationally to return back to the password, of course. Also you want that it should be slow to try a lot of password to check if one will hash to the one you see in the database. And many other properties. But limiting the API requests does not help once the DB is out of control.

@codahale

This comment has been minimized.

Copy link

codahale commented Oct 21, 2011

"For this crew programmers are just stupid drones applying guidelines, that can't in any way reason about cryptography."

Seems rather unambiguous, @antirez.

@antirez

This comment has been minimized.

Copy link
Owner

antirez commented Oct 21, 2011

@codahale no, maybe it is my English, but you are misunderstanding here.

Just following YOUR advice of avoid thinking and just use bcrypt means to act as stupid drones (we, the programmers, following YOUR advice). At this point you see I'm direct, no point in not telling you what I meant.

@voodootikigod

This comment has been minimized.

Copy link

voodootikigod commented Oct 21, 2011

Everyone calm the fuck down, this has turned into a complete shit show.

@coda your point is valid. BCrypt has been through various stringent cryptanalysis and has held up for many years and for most use cases the overplayed response of "use bcrypt" on this thread is great advice. SHA256 and SHA512 are not as strong these days but still has the benefit of abuse - that said, there is a reason they are hold the SHA3 competition. So your point about the cryptographic strength of each implementation is made.

In the end, a better argument would be don't use lamernews or any other site that doesn't implement strong cryptographic security - which you made in your initial tweet but it could have been 1000x better done in a less flamebait manner and probably far better received. Also I fear that would encompass about 90% of the internet, including most financial institutions (obviously not square, as mentioned), most "secure" sites, and anything else.

@antirez In the end, this boils down to your decision if you want to implement your own cryptographic security or utilize something that is vetted and tested and continually maintained by another group that are focused on security. Coda's arguments, while a bit aggressive, do have merit. Also because of this debacle the site does have a ridiculous amount of attention and arguably since this is an "example implementation" designed for others to fork and grow/learn, you should use something simple and obvious and most importantly well documented so you aren't handling requests like this. Less magic is always better. In the end, its up to you.

Both of you are at the point of diminishing returns and could have spent the last couple hours of your life doing something better.

If you are not @coda, @antirez, @jayferd, or have at least a year of cryptanalysis study, please stay off this thread. Mindless +1 just clouds the actual discussion.

@codahale

This comment has been minimized.

Copy link

codahale commented Oct 21, 2011

Ah, so more like "In the eyes of this crew, …". That is better, thanks for the clarification.

Far better to have my views radically misrepresented than to be called dumb.

@jrecursive

This comment has been minimized.

Copy link

jrecursive commented Oct 21, 2011

@voodootikigod

This comment has been minimized.

Copy link

voodootikigod commented Oct 21, 2011

@jrecursive see above.

@codahale

This comment has been minimized.

Copy link

codahale commented Oct 21, 2011

@voodootikigod: I'm confused -- why do you care?

@antirez

This comment has been minimized.

Copy link
Owner

antirez commented Oct 21, 2011

@voodootikigod this stuff happen it's not a big issue. After all I think that:

  1. I learned that even in trivial web projects I should ship secure hashing by default.
  2. We learned that even if bcrypt is more famous there are other ways to get the same effect, and with NIST approved primitives, that in some environment may be mandatory.
  3. I hope @codahale learned that it is possible to advice people without being so much absolute.
  4. I learned that the next time I'll not let me get involved in such kind of fights.

So maybe this is not as bad as it seems.

@raggi

This comment has been minimized.

Copy link

raggi commented Oct 21, 2011

I wonder, has anyone tried to attack this with a redis injection? You know, a viable attack?

@antirez

This comment has been minimized.

Copy link
Owner

antirez commented Oct 21, 2011

@raggi: redis injection is not possible at all. The protocol is designed against it.

@raggi

This comment has been minimized.

Copy link

raggi commented Oct 21, 2011

troll defense ++

@codahale

This comment has been minimized.

Copy link

codahale commented Oct 21, 2011

So we gonna merge this bad boy or what?

@antirez

This comment has been minimized.

Copy link
Owner

antirez commented Oct 21, 2011

@codehale: I guess it's not worth merging given that it does not fix properly the problem.
Now I could like to use PBKDF2, but what I'm thinking is the following: if I use a pure Ruby implementation, as I would like to do, it is going to be a lot slower compared to an optimized C implementation, since there are more steps including a pure Ruby string xor primitive in the implementation.

I'll benchmark it a bit to see if it is fast enough that the client is not too much slower compared to the attacker.
But what can be done is to set an high number of rounds by default and relax this a bit in the deployment if the VPS is too small to compute the iterations in a reasonable amount of time.

However the auth cookie is set to like... 20 years in the future, so it is unlikely that there will be many authentications per second. Probably I'm doing a premature optimization here...

@jneen

This comment has been minimized.

Copy link

jneen commented Oct 21, 2011

From a technical perspective, I believe we're done here. I'm not a cryptographer, but I feel strongly that a Ruby web app is not the place to be implementing password-hashes. The responsibility of this app is to glue together a couple of libraries and create a compelling experience for the end user - password hashing should be delegated to an external library. Which library in particular doesn't matter much to me, as long as I can be assured that it does what it says it does, and well. Also, a single SHA1 with a global salt, we can all agree, is insufficient for any app. @codahale, thanks for pointing this out so we can all move past it.

On a meta-technical note, I strongly disagree with this:

Tactfully? No, but fuck tact.

My fly has been down a few times before (figuratively and otherwise), and, for the most part, instead of publicly calling me out, knowledgeable people in the community have informed me that my fly is down, and given me the means to fix it. As I said above, that's the community that I want.

[Edit]: Oops, I missed @antirez's last comment while writing that. I stand by what I said above, which is that IMO a Ruby web app is the wrong level of abstraction to be implementing password hashes, so I'd much prefer to use an external library. And one which uses a C extension is probably appropriate, too - if it's slow, you want it to be slow for the attacker too :). If someone tries to DOS you based on your slow password-hashing library, there are other ways to protect against that (rate-limiting as @raggi mentioned, CAPTCHAs after two or three login attempts, etc.).

@antirez

This comment has been minimized.

Copy link
Owner

antirez commented Oct 21, 2011

@jayferd: we are moving forward, now the point is how to fix it, what library to use, if it is instead sensible to force users to pick passwords of a given length and with non alphanum chars and so on. Just talking about how fixing this stuff.

@jneen

This comment has been minimized.

Copy link

jneen commented Oct 21, 2011

Cool. I'd much rather use a better hashing system than force users to pick any particular kind of password (aside from some simple UI checks, like >=4 chars). I think our intended userbase (i.e. hackers) will appreciate that.

@jneen

This comment has been minimized.

Copy link

jneen commented Oct 22, 2011

BTW, I know some reasons were listed above, but I wonder if you could recap specifically the arguments against just applying the patch as is? As far as I see it, bcrypt is as secure as anything else, and someone else has already written the library. I also love reinventing the wheel, but it's all about choosing the right place. IMO a Ruby web app isn't the place to reinvent the wheel of password hashing.

@melo

This comment has been minimized.

Copy link

melo commented Oct 22, 2011

@jaYFRED I believe I read @antirez state on several places (including the project readme) that we would like to keep dependencies down for this Redis demo. As such, if he can stay with something that uses SHA1 only, we would prefer to do so.

He particularly mentioned (I'm sorry I don't recall exactly where, maybe in the blog post) that bcrypt might not be installed everywhere, at least not as much deployed as SHA1.

@antirez

This comment has been minimized.

Copy link
Owner

antirez commented Oct 22, 2011

Hello, finally fixed it here:

d23a38b

With the default 10000 iterations it is pretty slow, and an authentication requires 0.3 seconds in my macbook air i7, but after all the cookie is set a lot in the future so authentications should be rare. However there is a constant to tune the number of iterations if needed.

The original PBKDF2 library was modified in order to use the ruby-hmac gem instead of OpenSSL as the former is a pure-Ruby gem trivial to install. I also did benchmarks and strange but true switching between the two does not make any difference. Probably ruby-hmac is able to sense the presence of OpenSSL and use it?

Well, finally the issue is fixed and I'm closing it.

@antirez antirez closed this Oct 22, 2011

@jneen

This comment has been minimized.

Copy link

jneen commented Oct 22, 2011

@melo honestly, this is what Bundler is for. It's pretty standard in the Ruby community these days.

@melo

This comment has been minimized.

Copy link

melo commented Oct 22, 2011

@jaYFRED sorry, not a ruby user, so I'm not sure exactly what Bundler does. I'm assuming its a dependency manager for gems.

But you missed my point, I was not condoning or condemning @antirez choice about the number of dependencies. I'm a Perl guy, I love dependencies from CPAN :), less work for me. I was just pointing out that it was a design goal for him to limit the number of dependencies.

Sometimes there are good reasons to do so. I'm not qualified to say if this is a case for it or not, given my near-zero knowledge of the Ruby ecosystem.

Best regards,

@tylerkahn

This comment has been minimized.

Copy link

tylerkahn commented Oct 22, 2011

So to be clear, now instead of just bcrypt-ruby being a dependency, hmac-sha1 and openssl are and now there is a implementation of PBKDF2 as part of the source tree.

@jneen

This comment has been minimized.

Copy link

jneen commented Oct 23, 2011

Basically, bundler is a way of automatically tracking dependencies so that anyone can install all dependencies in one go. In terms of deployment, it's commonly used to ensure that the same versions of gems are running in production and development. So from a Ruby dev's perspective, you have this:

Extra gem dependencies: super easy
Extra external library dependencies (openssl, etc): super hard.
Reimplementing external libraries in-app (or copying code): seems easy at first, but a management nightmare eventually.

@antirez

This comment has been minimized.

Copy link
Owner

antirez commented Oct 23, 2011

@tkahn6 we only depend on hmac-sha1 that is a pure-Ruby gem (trivial to install in every system out there), but openssl if available will improve performances of authentication. After our long chat I decided that bcrypt from my point of view is not a good enough pick as something built on a NIST approved primitive (SHA1) and a very well understood protocol like PBKDF2.

@antirez

This comment has been minimized.

Copy link
Owner

antirez commented Oct 23, 2011

p.s. also in my opinion if a system really cares about providing security for an user it should not limit the care to the hashing function as still there are two categories of passwords that are easily breakable: very short passwords, and guessable passwords. So that system should also:

  • Force the user to a minimum password length.
  • Don't accept passwords that are in a (big) wordlist contained in the authentication system.
  • Don't accept trivial variations from the wordlist (up/down case, or 3 instead of "e" and so forth).

So anyway the current status with the lamernews code base is not optimal. Given the spirit of the code probably the best thing would be to add authentication as an object with methods to check the authentication, create a new salt, check if a password is acceptable, and so forth, and ship just the lame SHA1+salt that I used in the first release as example about this plugin system (warning the user it is not the most secure pick at all), and other examples of stronger authentication as "optionals".

@jneen

This comment has been minimized.

Copy link

jneen commented Oct 23, 2011

We can't predict the needs of the users, and we really want to avoid sticky-note security (where everyone's password is so complicated they write it on sticky-notes attached to their monitors).

If we're set on PBKDF2, the right way to do it would be to write a C extension that implements PBKDF2 in a way that is API-compatible with bcrypt, and package it as a gem. I'd be happy to do all the yak-shaving :)

@ieure

This comment has been minimized.

Copy link

ieure commented Jun 7, 2012

Use bcrypt.

@sorenmacbeth

This comment has been minimized.

Copy link

sorenmacbeth commented Jun 7, 2012

Why hasn't anyone suggested using something like bcrypt?

@ieure

This comment has been minimized.

Copy link

ieure commented Nov 6, 2014

Use bcrypt.

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