Salted hash generator

Question

I've had a go at creating a small class in C# that can generated salted hashes from text. I would like to know how this could be improved (in terms of code style) and whether or not this code is secure enough that it could be used in a professional environment (it won't be).

Hasher.cs

using System.Security.Cryptography;
namespace HashAndSaltTest
{
 public static class Hasher
 {
 private static readonly int MaxSaltLength = 32;
 public static byte[] GenerateSaltedHash(byte[] plainText)
 {
 HashAlgorithm algorithm = new SHA256Managed();
 byte[] salt = GenerateSalt();
 byte[] saltedText = new byte[plainText.Length + salt.Length];
 for (int i = 0; i < plainText.Length; i++)
 saltedText[i] = plainText[i];
 for (int i = 0; i < salt.Length; i++)
 saltedText[plainText.Length + i] = salt[i];
 return algorithm.ComputeHash(saltedText);
 }
 private static byte[] GenerateSalt()
 {
 byte[] salt = new byte[MaxSaltLength];
 using (RandomNumberGenerator random = new RNGCryptoServiceProvider())
 random.GetNonZeroBytes(salt);
 return salt;
 }
 }
}

• Roland Illig: What exactly do you want to use this code for? How do you intend to verify the hashed value?
• Alejandro: If "plainttext" is ever going to be a password, this is not secure. SHA256 is not designed to handle passwords (use PBKDF2 or bCrypt instead). If you're not using it with passwords, I don't get the purpose of the salt. A bit more context of what kind of input is expected and what'll you use the hashes for is relevant when speaking about security.

Answer 1

• Because HashAlgorithm implements IDisposable you should enclose the usage of it in an using block as well.

• Omitting braces {} although they might be optional can lead to hidden and therfor hard to find bugs. I would like to encourage you to always use braces.

• Instead of using a for loop to copy plainText to saltedText and salt to saltedText you may take advantage of the Array.CopyTo() method.

• Public methods should validate passed parameters.

• Because you don't return the salt you can only generate a hash but you can't verify the hash.

Applying the mentioned points can lead to

public static byte[] GenerateSaltedHash(byte[] plainText)
{
 if (plainText == null) { throw new ArgumentNullException("plainText"); }
 if (plainText.Length == 0) { throw new ArgumentException("Length may not be zero", "plainText"); }
 using (HashAlgorithm algorithm = new SHA256Managed())
 {
 byte[] salt = GenerateSalt();
 byte[] saltedText = new byte[plainText.Length + salt.Length];
 plainText.CopyTo(saltedText, 0);
 salt.CopyTo(saltedText, plainText.Length);
 return algorithm.ComputeHash(saltedText);
 }
}
private static byte[] GenerateSalt()
{
 using (RandomNumberGenerator random = new RNGCryptoServiceProvider())
 {
 byte[] salt = new byte[MaxSaltLength];
 random.GetNonZeroBytes(salt);
 return salt;
 }
}

• user162687: Thanks for the feedback. Just wondering, from a security standpoint, how safe is this method of generating salted hashes?
• Heslacher: IMO its safe enough at least if the plainText isn't containing security relevant information. If plainText contains security relevant information you should override it inside this method.
• Sinjai: @Heslacher, could you elaborate on you should override it inside this method a little, and/or show an example? I'm assumingsecurity relevant information would be passwords and such.
• Heslacher: I meant overwriting the passed array.