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();
 }
 }
}

• Caramiriel: Any specific reason to use GetNonZeroBytes and not GetBytes? This limits the output minimally (254 possible values per byte, instead of 255), but I'm just wondering. :)
• an earwig: I originally didn't want null characters in the string, but as @Heslacher pointed out I won't even need to convert to a string.

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;
}

• mafu: What is the rationale behind var? To avoid accidental casting from long to int? Could not happen with the byte[], but I would argue that it is more readable.
• user622505: @mafu: I think that when the type is obvious, using var instead of explicit type is more readable. Consider the following: VeryLongTypeNameFactoryAdapter factoryAdapter = new VeryLongTypeNameFactoryAdapter();. The long name on the left hand side provides no additional information, yet it introduces a lot of useless clutter. Of course the improvement is less clear when you're replacing int or byte[] with var, but it is still more consistent and readable.
• Johnbot: Optional arguments are a maintainability nightmare in libraries as they're implemented as compile-time constants in the calling assembly. Say you have prog.exe using your library lib.dll and you want to replace the 32 with 64 without recompiling prog.exe, this cannot be done. Instead create a GetSalt(int maximumSaltLength) with no default and a GetSalt() { return GetSalt(32); }. Optional arguments are useful when you have several truly optional arguments that will be specified with named arguments and NEVER changed.

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.

• an earwig: Which constructor overload should I use to combat these potential concerns

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!