I use the following code to store user passwords.

string password = "...";
string common_salt = "...";
byte[] hash_target = Encoding.UTF8.GetBytes(password + user_salt + common_salt);
ToString(new SHA512CryptoServiceProvider().ComputeHash(hash_target)).
Replace("-", string.Empty).
ToUpper();


The user salt is stored together with the hashed password in the database. Additionally, every time a user's password is changed, the user salt is regenerated.

The reasoning behind a common salt and a user salt is: access to the database alone isn't enough to figure out how the hashing works. And then in case someone manages to disassemble the code, the malicious user cannot make use of a single rainbow table.

Is this a good method?

• Semi-related: SHA512CryptoServiceProvider implements IDisposable, so it should be assigned to a variable and wrapped in a using block. May 8, 2012 at 13:41
• @Stijn I’ve never understood these preferences. How is that more readable? Sorry for the flame bait, I’m just seriously baffled and find it a horrid practice. May 8, 2012 at 15:03
• @KonradRudolph "" is a value and string.Empty is a constant declared for this value. This is of course a very well known value but still can be considered magic.
– Den
May 9, 2012 at 8:37
• @KonradRudolph I find string.Empty not only more readable but safer because as soon as you start coding string literals, you introduce potential for a fat-finger typo. I take it you haven't hunted down a bug that turned out to be " " instead of ""? May 9, 2012 at 11:16

On the other hand SHA-512 is completely wrong, since it's fast, and thus cheap to brute-force. You should use a slow key derivation function, such as PBKDF2, bcrypt, or scrypt. The only one of these built into .net is PBKDF2-HMAC-SHA-1, exposed through the Rfc2898DeriveBytes class. Choose the number of iterations as high as possible without hurting the performance of your application.