Your password is too good
-
@morbiuswilters said:
Would you be so kind as to provide to non-experts an explanation on what is wrong? Is it because of the use of a fixed salt instead of a nonce?@Aaron said:
@Jaime said:
Standard .Net setup of an AES encrytor/decryptor:
string password = "passwordofanylengthwillworkhere";
RijndaelManaged alg = new RijndaelManaged();
byte[ salt = Encoding.ASCII.GetBytes("salttomakerainbowtableslesseffective");
Rfc2898DeriveByes key = new Rfc2898DeriveBytes(password, salt);
alg.Key = key.GetBytes(alg.KeySize / 8);
alg.IV = key.GetBytes(alg.BlockSize / 8);Jeff Atwood strikes again. I think he single-handedly set the world of computer security back 20 years with one stupid blog post.
LOL, I didn't see that until you pointed it out.
-
@Zecc said:
Would you be so kind as to provide to non-experts an explanation on what is wrong? Is it because of the use of a fixed salt instead of a nonce?
Aye.
-
@Lingerance said:
@Zecc said:
Would you be so kind as to provide to non-experts an explanation on what is wrong? Is it because of the use of a fixed salt instead of a nonce?
Aye.Of course, the example also had a fixed password. Obviously I wasn't suggesting that every encryption event should use the same password. Similarly, I only hardcoded the salt to simplify the example. The corrected code should be:
[code]
byte[16] salt;
RandomNumberGenerator.Create().GetBytes(salt);
[/code]However, now we have to worry about specifing that the salt has to be stored somewhere, but the example is purposely trimmed down to just the initialization of the encryptor. I could expand it to an entire application, but I'm sure someone will find something else that was left out, assume the stupidest possible thing to be in there, and blame me for writing it.
-
@Lingerance said:
...I was told that .NET was supposed to make things less verbose, that example looks needlessly complicated. Wouldn't a simple:
string salt = binStreamToHexString(fread("/dev/urandom", 128)); string hashed_password = HASH_ALGO(salt, password);be clearer? (Replace the functions with whatever does what the name says it does)
Unfortunately, making the code shorter limits flexibility. Your example above works, but embeds the password-to-key translation in the method call. There will be many uses of AES that use keys that are derived from much stronger sources than a password and pre-shared. Also, /dev/random should be used for a nonce instead of /dev/urandom.Additionally, your code uses a library pattern instead of an OO pattern. It is very typical in .Net code to first create an instance of an object and then call its methods. There are some advantages to creating a stateful instance first, such as the option to create the instance during initialization according to configuration settings and then having that instance used in various parts of the application. Where statefulness is unnecessary, static methods can be used which don't require the extra step of instantiating the object.
-
@The_Assimilator said:
@Someone You Know said:
Care to provide a link to said blog post?
I used to be a fan of Jeff, until he tried to be a security expert. Now I just read Raymond Chen's blog; at least he doesn't step outside his area of expertise whenever he posts.
That's the one. To Jeff's dubious credit, he had the sense to link to a follow-up article that actually explained the issue properly, but unfortunately much of his horde of tl;dr readers didn't make it that far down the post and didn't read any of the comments. This is the result - a bunch of incompetent code monkeys yelling "I'm secure because I append the same 20 characters to every plaintext password so no cr4xx0r can rainbow-table me!"
The very idea of a rainbow table with bcrypt is laughable. Rainbow tables exist because MD5 is cheap to compute, and even then they're largely ineffective because every major password-protection scheme for the past 30 years has used salts. It would take decades to create a bcrypt rainbow table on ultra-modern distributed hardware, and by that time people can just add a few more log-rounds to the computation. Unless and until a weakness is found in blowfish itself - and the smartest minds in crypto haven't been able to do it for 15 years - then it's not getting cracked, period.
SHA-512, on the other hand, is garbage as a password-hashing function. Like every other hashing function, it's designed to be fast. It really doesn't matter how many bits the hash is, what the collision rate is, if that's your only protection and someone decides to hardware-accelerate the hashing function up to trillions per second.
-
@Jaime said:
<font face="Lucida Console" size="2">
</font>However, now we have to worry about specifing that the salt has to be stored somewhere
No, you don't - once again demonstrating that your source is a bunch of crappy blog posts and Google. The salt is not a secret. You store it with the password. This is not a "worry", it is a trivial implementation detail.
-
@Jaime said:
Additionally, your code uses a library pattern instead of an OO pattern. It is very typical in .Net code to first create an instance of an object and then call its methods.
Why is it even a class though? You really want to initialize an object just to call a single member? You realize that even though you're using OOP, you don't have to use OOP for every single problem right? It's the same issue with Java, every thing has to be part of a class.
@Jaime said:Where statefulness is unnecessary, static methods can be used which don't require the extra step of instantiating the object.
Which was sort of my point. It's a hash function. Salt and plaintext go in, hash comes out. Same with the salt generation, nothing goes in, something random comes out.
-
@Lingerance said:
Why is it even a class though? You really want to initialize an object just to call a single member? You realize that even though you're using OOP, you don't have to use OOP for every single problem right? It's the same issue with Java, every thing has to be part of a class.
Yes, everything has to be part of a class, but C# has static classes which never need to be (and in fact never can be) instantiated.
And in fact, the bcrypt.NET hashing methods are static - you don't need to instantiate a class to use them. It's simply "BCrypt.HashPassword(password, BCrypt.GenerateSalt())". The static methods do, however, internally instantiate a class, because the bcrypt algorithm is pretty complex and requires a lot of internal state.
One of the reasons not to rely too much on static methods or classes in non-trivial production applications is dependency injection and subsequent testability. Static classes (or methods) cannot implement interfaces.
-
@Aaron said:
That's the one. To Jeff's dubious credit, he had the sense to link to a follow-up article that actually explained the issue properly, but unfortunately much of his horde of tl;dr readers didn't make it that far down the post and didn't read any of the comments. This is the result - a bunch of incompetent code monkeys yelling "I'm secure because I append the same 20 characters to every plaintext password so no cr4xx0r can rainbow-table me!"
I remember that. I cringed through the entire post. It was almost as bad as that episode of The Office where Michael promises the poor, ethnic third grades he will pay their college tuition if they graduate high school a decade before and then he has to face all of the almost-grads and tell them he can't afford it. What I liked about Atwood wasn't his insight or knowledge, but his enthusiasm at learning new things. He was like a young coder documenting every cool new thing he found and it was a good blog up until the day he decided he had learned enough.
@Aaron said:
The very idea of a rainbow table with bcrypt is laughable.
To be fair, the idea of a rainbow table with salted MD5 is laughable, too. Salts are what defeat rainbow tables, not expensive algos. Where bcrypt is superior to MD5/SHA is in dictionary attacks. Modern botnets or a Beowulf cluster of PS3s can brute force MD5/SHA a million dictionary entries with a 16-bit salt in no time. There are three (maybe more, but I'm drunk and 3 sounds right) basic goals to a password hashing scheme: 1) hash should not be reversable, MD5 and SHA are good enough at this; 2) the hashing should introduce some true randomness so that pre-computed attacks are too difficult, salts solve this and 3) the execution time for the hashing should be very, very long, to make dictionary attacks infeasible.
I can tell from you post you understand this, but it bears noting that if you are only looking for resiliance to pre-computation attacks, MD5 + salt fits.
-
@morbiuswilters said:
LOL, I didn't see that until you pointed it out.
This is the worst part. Neither did I, and one of my big things is security. It's one of those fuckups that's so pervasive (and which you are typically "absolutely not" allowed to change - because then they'd have to get all their users to reregister - or implement a complicated grandfathering scheme) that even those of us who know better start to gloss over it- which means it's only a matter of time before we start making the same damned mistake (Oh, wait, no it's not, because I write security code exactly once for every language I use, and drag that around via copypasta)
-
@Weng said:
@morbiuswilters said:
LOL, I didn't see that until you pointed it out.
This is the worst part. Neither did I, and one of my big things is security. It's one of those fuckups that's so pervasive (and which you are typically "absolutely not" allowed to change - because then they'd have to get all their users to reregister - or implement a complicated grandfathering scheme) that even those of us who know better start to gloss over it- which means it's only a matter of time before we start making the same damned mistake (Oh, wait, no it's not, because I write security code exactly once for every language I use, and drag that around via copypasta)I've done the grandfathering thing in the past; it's really not that hard. Just added code to check for the new password columns in the user table, if they didn't exist it check against the old plain-jane MD5 column and if that succeeded it hashed the password using the new scheme and updated the new columns. Leave that in place for 4 months or so, and you will get all of your regular users. Then just send out an email letting everyone who doesn't have a new password know that they need to login and update their password to something new. After a week or two, take the remainder of the accounts and generate a new password and mail them a "forgot password" type email with the new password. Then cut over completely. They can always use the "forgot password" feature if they didn't get your original emails. Then you can drop the old MD5 column and the old password validation code.
It takes more time to explain than it does to implement; it's less than 100 lines of PHP for all the pieces. I've done it quite successfully a few times with 100k+ users and never had complaints. Of course, if your application is badly designed and has hard-coded queries for the password everywhere, it will take more time, but then you can use the opportunity to properly encapsulate the password validation, right?
