CODEWITHSUNDEEP
c#asp.net-mvcencryptionpasswords
bigmac
bigmac

Reputation: 2553

Is my MVC3 Password Handling Adequate

I have read a ton of articles about how to store and compare entered passwords, some old and some new, but all different. Therefore, I took what I thought to be the best options and implemented the following.

Note that I created my own authentication provider and I am not using nor inheriting from the standard MS MembershipProvider (too bloated). This is for a lighter volume site that does not need to be "uber-secure", but I would like to follow best practices. I would just like some validation that I am not doing anything glaring wrong or opening up security holes. Also, I looked at per-user password salting, but it seemed overly complex for my needs. Is that a valid assumption?

First, I put the following appSettings key in my web.config, which is returned by my configurationProvider class.

<add key="PasswordEncryptionKey" value="qTnY9lf...40 more random characters here" />

Here is my code that publically Authenticates a user and privatly Checks the stored password against what what was entered as well as Encodes the password when saving. I have not shown the add or update password methods as they use the same private methods.

public bool Authenticate(string emailAddress, string password, bool setAuthCookie = false)
{
  bool isAuthenticated = false;
  var member = _memberRepository.Find(m => m.Email == emailAddress).SingleOrDefault();
  if (member != null)
  {
    if (CheckPassword(password, member.Password))
    {
      isAuthenticated = true;
      FormsAuthentication.SetAuthCookie(emailAddress, setAuthCookie);
    }
  }
  return isAuthenticated;
}

private bool CheckPassword(string providedPassword, string storedPassword)
{
  return EncodePassword(providedPassword) == storedPassword;
}

private string EncodePassword(string password)
{
  var hash = new HMACSHA1
               {
                 Key = Encoding.ASCII.GetBytes(_configurationProvider.PasswordEncryptionKey)
               };
  string encodedPassword = Convert.ToBase64String(hash.ComputeHash(Encoding.Unicode.GetBytes(password)));
  return encodedPassword;
}

My only rules are:

With this, is there anything glaring that I'm missing?

Upvotes: 3

Views: 403

Answers (2)

atanamir
atanamir

Reputation: 4953

I believe your scheme is adequate, but it can be improved upon a little bit.

It seems to me that you have the beginnings of a salting scheme here, with your EncryptionKey that you have in your app.config file. However, for best security practices, generally people use a different salt for each password, and store the salt alongside the hash in the database. 

class MyAuthClass {
  private const int SaltSize = 40;
  private ThreadLocal<HashAlgorithm> Hasher;

  public MyAuthClass ()
  {
    // This is 'ThreadLocal' so your methods which use this are thread-safe.
    Hasher = new ThreadLocal<HashAlgorithm>( 
      () => new HMACSHA256(Encoding.ASCII.GetBytes(_configurationProvider.PasswordEncryptionKey)
    );
  } 

  public User CreateUser(string email, string password) {
    var rng = new RNGCryptoServiceProvider();
    var pwBytes = Encoding.Unicode.GetBytes(password);
    var salt = new byte[SaltSize];
    rng.GetBytes(salt);

    var hasher = Hasher.Value;
    hasher.TransformBlock(salt, 0, SaltSize, salt, 0);
    hasher.TransformFinalBlock(pwBytes, 0, pwBytes.Length);
    var finalHash = hasher.Hash;
    return new User { UserName = email, PasswordHash = finalHash, Salt = salt };
  }

With a scheme such as this, your passwords are made more complex because if an attacker happened to get the hashes, he'd have to also guess the salt during a brute-force attack.

It's the same philosophy as your EncodingKey in your configuration file, but more secure since each hash has its own salt. Checking entered passwords is similar:

  public bool IsPasswordCorrect(User u, string attempt) 
  {
    var hasher = Hasher.Value;
    var pwBytes = Encoding.Unicode.GetBytes(attempt);
    hasher.TransformBlock(u.Salt, 0, u.Salt.Length, Salt, 0);
    hasher.TransformFinalBlock(pwBytes, 0, pwBytes.Length);
    // LINQ method that checks element equality.
    return hasher.Hash.SequenceEqual(u.PasswordHash);  
  }
} // end MyAuthClass

Of course, if you'd rather store the hashes as strings rather than byte arrays, you're welcome to do so.

Just my 2 cents!

Upvotes: 2

David Hoerster
David Hoerster

Reputation: 28701

A few thoughts...

  1. You should use HMACSHA256 or HMACSHA512 instead of HMACSHA1. .NET4's default hashing algorithm is SHA256 now instead of SHA1.
  2. When you get the bytes of your encryption key, you use the ASCII encoding, but you're computing the hash using the Unicode encoding. Keep them consistent - use Unicode.
  3. Consider salting your hash.
  4. You say encryption, but you're really hashing.

Upvotes: 1

Related Questions