- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
hash is violation of WYSIWYG principle
Admin
And also terrible UX.
Admin
Note the commented line directly above
bRet = s.ComputeHash(bIn);.Admin
Fortunately the encrypted passwords were still salted... if they ever used them.
Admin
TRWTF is the parameter passwordFormat. Format 1 seems to be the healthy one with a drastic reduction in sodium intake. ;-)
Admin
I hate to defend this code, but...if passwordFormat is anything but 0 or 1, it does EncryptPassword on the correctly-salted array. And if it's 0 it just stores the password in plaintext.
This looks like the place originally stored passwords in plaintext, then moved to hashing the password, and then changed again to adding salt. The purpose of this code is to allow all three of these systems to coexist in the same database via versioning the encryption algorithm.
The correct thing to do would be to reset everyone's passwords when you change encryption algorithm, but if management said "no" to that, this wouldn't be a totally asinine way to resolve it.
Admin
Never apply reason to a Daily WTF. You are of course correct, although I'd go one further and posit the existence of a huge number of unit/functional tests written against the database app that would be expensive to change (thus making versions 0 and 1 a very sensible choice).
Sadly, these days, we trend towards the Meta-WTF. It's too hard to think about the possible reasons for the existence of the odd-looking code, and we're never given much in the way of context. (Unless it's Eric Gern, in which case we're given Context From Plan 9.) But who cares? Tee hee hee, silly people, I'd never do that, next story please.
Admin
The correct way to do it is to transparently re-encrypt the password when someone with an old account successfully logs in. It's a bit tricky to do this without exposing too much information, but it's not difficult to find a correct method online instead of letting old sins live on forever.
Admin
Hm – The correct code is in the source
bRet = s.ComputeHash(bAll);, but it's put into a comment. This was not an accident! Translates into "management decision pending" or "overruled by management decision". So the TRWFT in using your own password solution is it providing the means for interfering (management) decisions in the first place ... (Otherwise: "Oooh, this is not how this library works. Sorry.")Admin
Even if you're going to transparently fix old passwords you still need to be able to validate them. Thus this code actually makes sense--it's run against the user-entered password before comparing it to what's in the database. This is a WTF environment, not WTF code.
Admin
Actually, this code is taken directly from SqlMembershipProvider, that is, if you were to uncomment the line hashing bAll, and remove the line hashing bIn. https://referencesource.microsoft.com/#System.Web/Security/SQLMembershipProvider.cs,1900 (Microsoft's original code has been modified since to include handling of KeyedHashAlgorithm, but, prior to that, it was identical to how I described it). In this case, format of 0 is to store the passwords in plain text, format of 1 is to apply a hash to the password with the salt to create a one way encryption of the password, while a format of 2 was to use reversible encryption (I believe with the machine key, thus, if you had the machine key, you could decrypt the encrypted passwords). The value used for passwordFormat was chosen by the web.config settings when creating a new password, and was read from the database when verifying a password.
Admin
I think we need more people with access to the "moderated" button :/ Not a fan of waiting for longer than the discussion's useful lifetime (i.e. the next day's article being posted) before seeing that three or four people might have had the same response to my post.
Admin
There's no reason the plaintext passwords can't be upgraded immediately. It's hard to see why that's still in there, maybe for testing? But, otherwise your scenario sounds plausible.
Even if management agreed to reset everyone's password, it's not necessarily the best thing to do. Besides possibly prompting people to flip out over nothing, you're sending a reset link over other people's email servers.
And a mass reset is quite an ordeal. You'd have to ask people to reset their password, support them through that, and then you'll have to go and ask people again, probably forcibly reset the last few people who ignored the emails, and deal with them complaining.
Admin
Yep, I've done something similar before, under much the circumstances you describe. Given requirements over the years to improve password security without forcing mass resets, using some kind of "password encoding version" field is the way to go - validate against the current version on login, then knowing that you have the correct plaintext, re-hash it with the latest strength.
It's actually kind of handy for testing, too... if you don't know the password for an account in the test DB, just use SQL to set the version to 0 and password to "password".
Admin
That works in the production database, too!
Um, hold on a second ...
Admin
Yeah, only that won't help either, because you still need to have the old code into all eternity, until the prof that retired fifteen years ago, doesn't use the service at all, but eff-no, you canNOT deprovision the service from him, actually logs in.
Admin
Or add a rule in your adblocker to filter out all .comment-moderation divs
Admin
The old versions should still be covered by your cryptography library anyway (you are using one and not rolling your own, right?) There's a reason why e.g., glibc-crypt style password hashes include algorithm version information.
Admin
That wouldn't actually allow Chronomium to read the moderated posts in due time.
Admin
Or just go through the database and encrypt every password.
Admin
Do what some resource providers do, and issue a message if an account has not been touched for several years, to say: if you do not respond to this email saying "yes I would like to continue with this subscription," we will reset your password and require that you create a new one. And if that user does respond, send him to a "change password" screen which will allow him to do just that.
Admin
"Or just go through the database and encrypt every password"
In any slightly sane environment, you just DON'T have the password in the database. You have either a hash of the password or an encrypted copy (which is a bit of an WTF in itself, but as I said it's still slightly sane because at least it's not plaintext). The actually correct way is to simply store the hash.
And since you don't have the password, how exactly should one follow your recommendation?
Admin
If posting without the link to reference source works, the main point I made is that the code presented, if you were to remove the line computing the hash of bIn, and add the line back computing the hash of bAll, the code is completely identical to ASP.NET's SqlMembershipProvider in .NET 2.0, before the added support for keyed hashes. As this needed to be generic enough to work with many different unrelated web applications, it was made capable of storing passwords in three different formats: plain text, hashed, and reversible encryption, with hashed being recommended.
This code, to me, appears to be a case where someone couldn't understand or didn't like the idea of salted hashes, so he took the code either from reference source or from reflector and made the modifications he wanted to remove salting from hashes.
Admin
Maybe logged in will allow me to post. The code above is taken directly from ASP.NET 2.0 SqlMembershipProvider, modified to only hash the password rather than the salted password. The built in SqlMembershipProvider needs to support various websites, so it has the option for plain text, hashed, and reversible encrypted passwords, numbered as 0, 1, and 2. This code was likely pulled from reference source or reflector by someone who either doesn't understand or doesn't like salts in passwords.
Admin
If your having a bad user experience with hash, you're smoking it wrong.
Admin
Actually I would implement the login algorithm for the new way of hash, then old way of hash. If the user can login with password in old way of hash, update the password field to the hash generated in new way of hashing. This will smoothly transparently help you migrate the users to use the new algorithm.
Admin
Actually I would implement the login algorithm for the new way of hash, then old way of hash. If the user can login with password in old way of hash, update the password field to the hash generated in new way of hashing. This will smoothly transparently help you migrate the users to use the new algorithm.
Admin
Wait until the user logs in, and see if they pass the old-style password check. At this point you have their username and their plaintext password, since they had to give you that to hand to the password checker.
If their password is correct, blow away the old one and replace it with the correctly-salted one.
Admin
Pro-tip: try reading the entire conversation, rather than just taking one sentence out of context and thinking you're so clever for suggesting an idea that's already been brought up about a hundred times.