5
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ What exactly do you want to use this code for? How do you intend to verify the hashed value? \$\endgroup\$ – Roland Illig May 13 '18 at 16:03
  • \$\begingroup\$ 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. \$\endgroup\$ – Alejandro May 14 '18 at 1:02
4
\$\begingroup\$
  • 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;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. Just wondering, from a security standpoint, how safe is this method of generating salted hashes? \$\endgroup\$ – Ioan Thomas May 13 '18 at 20:31
  • \$\begingroup\$ 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. \$\endgroup\$ – Heslacher May 13 '18 at 20:35
  • \$\begingroup\$ @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. \$\endgroup\$ – Sinjai May 20 '18 at 21:11
  • \$\begingroup\$ I meant overwriting the passed array. \$\endgroup\$ – Heslacher May 22 '18 at 5:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.