Salt generation in C#

Question

I have designed a small method to generate a salt for a password. All this does is create a random salt and does nothing to add it to the password. I have a few questions regarding my very simple method:

1. Is this a secure generator?
2. Currently, it encodes in base64. Is this an issue in any way?
3. Are there any other potential issues? How could I improve this in terms of security, speed, etc...

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using System.Text;
using System.Security.Cryptography;

namespace Test
{
    public class Program
    {
        // Here is a method to generate a random password salt
        private static string getSalt()
        {
            var random = new RNGCryptoServiceProvider();

            // Maximum length of salt
            int max_length = 32;

            // Empty salt array
            byte[] salt = new byte[max_length];

            // Build the random bytes
            random.GetNonZeroBytes(salt);

            // Return the string encoded salt
            return Convert.ToBase64String(salt);
        }

        static void Main(string[] args)
        {
            System.Console.WriteLine(getSalt());
            System.Console.ReadKey();
        }
    }
}

Answer 1

var

You should use them with max_length and salt too. If the type is obvious from the right hand side of the assignment you should use var.

From here

Under what circumstances is it necessary for a variable's type to be clearly understood when reading the code? Only when the mechanism of the code -- the "how it works" -- is more important to the reader than the semantics -- the "what its for".

So basically in a line like

byte[] salt = new byte[max_length];  

or

int max_length = 32;  

the type of the variables does not add any value to the code. It is too obvious what the assigned type is, using the type instead of var only adds noise to the code without any real value.

Disposing

If an object implements IDisposable you should either call its Dispose() method or enclose it in an using block.

Naming

Also variables local to a method aren't mentioned in the naming guidelines I would suggest to use camelCase casing for naming them instead of snake_case casing.

For naming private methods you should use PascalCase casing.

Comments

Comments should only describe why something is done. Let the code itself explain what is done by using meaningful and readable names.

A very good answer about comments can be found here: https://codereview.stackexchange.com/a/90113/29371

getSalt()

You should allow to pass the max_length to the method instead of hardcoding it. This has the advantage that a change to this value won't need the class class/method to be changed. If you make it an optional parameter you can still call it like it whould have no parameter.

Edit

Based on the valid comment from Johnbot you should better use a overloaded GetSalt() method instead of using an optional parameter.

I don't see the need for converting to base64. Encryption algorithms use byte[] arrays, so you should better just return the byte[].

---

Applying the above would lead to

private static int saltLengthLimit = 32;
private static byte[] GetSalt()
{
    return GetSalt(saltLengthLimit);
}
private static byte[] GetSalt(int maximumSaltLength)
{
    var salt = new byte[maximumSaltLength];
    using (var random = new RNGCryptoServiceProvider())
    {
        random.GetNonZeroBytes(salt);
    }

    return salt;
}

Answer 2

If you are going to call getSalt() more than once while your application is running, you should keep a single static instance of RNGCryptoServiceProvider in your class instead of creating a new one in every invocation of getSalt().

This is because the 0-argument RNGCryptoServiceProvider constructor is not guaranteed to get its entropy from a cryptographic-quality source. You could end up with predictable sequences of salts or even repetitions of the same salt.

Answer 3

I'm in no way experienced with C#, however I did some looking at the documentation online and did some research on cryptography in said language.

As far as I can tell, it looks like you've taken the correct procedures to secure your code! Your use of RNGCryptoServiceProvider looks well structured and without holes. The length of your salt (32 bytes, correct?) seems good. 16 should be your minimum (that's not a rule, just a suggestion), but you could go higher than 32 if you wanted. The mindset should not be: the longest salt is the strongest, rather: the longer the salt, the less likely it will be to guess it. You can find more detail on that over at Sec.SE.

Overall, it appears good. I'm no expert, so if I've got something wrong I hope someone comments or chimes in here!