1
\$\begingroup\$

I use the following code to store user passwords.

string password = "...";
string user_salt = System.Web.Security.Membership.GeneratePassword(20, 5);
string common_salt = "...";
byte[] hash_target = Encoding.UTF8.GetBytes(password + user_salt + common_salt);
string password_hash = BitConverter.
    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?

\$\endgroup\$
11
  • 3
    \$\begingroup\$ Semi-related: SHA512CryptoServiceProvider implements IDisposable, so it should be assigned to a variable and wrapped in a using block. \$\endgroup\$ Commented May 8, 2012 at 13:41
  • 6
    \$\begingroup\$ @KonradRudolph Personal preference for readability. \$\endgroup\$ Commented May 8, 2012 at 14:51
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ Commented May 8, 2012 at 15:03
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ Commented May 9, 2012 at 8:37
  • 2
    \$\begingroup\$ @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 ""? \$\endgroup\$ Commented May 9, 2012 at 11:16

2 Answers 2

Reset to default
1
\$\begingroup\$

In SQL Server, we store password salts as an int and password hashes as binary. There really is no reason to have the salt as a string value. Also, there is no need to have the hash as a string. You can use the bytes from the computed hash (which is a fixed, known number of bytes) and store the binary value directly in the database. These two things make it very fast to check check against when attempting to verify the password.

See my answer on Stack Overflow here about hashing passwords using SHA-1. You are using SHA-512, which the code can easily be adopted to support.

\$\endgroup\$
1
\$\begingroup\$

Including a secret shared salt, in addition to a per-hash salt, is a decent idea, at the very least it doesn't hurt. I typically put the common salt(which is essentially a key) into a config file instead embedding it in the application code.

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.

\$\endgroup\$
2
  • \$\begingroup\$ Didn't know about PBKDF2. I had looked into bcrypt and scrypt a few months ago and found one or two implementations, but couldn't be sure about their stability. \$\endgroup\$ Commented May 13, 2012 at 20:01
  • \$\begingroup\$ PBKDF2 is the way to go. \$\endgroup\$ Commented May 13, 2012 at 23:04

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.