Mfcttrf

I need to generate semi-complex throw-able password (just one use) on my application. I want it to be:

• at least 8 char long;


• contains at least 1 digit, 1 [a-z] char, 1 [A-Z] char;

I generate a few passwords a day, I don't mind about speed. I'm more looking for suggestions about code being maintainable (or not) and possible huge security flaw. Here's the code:

public static string GenerateSimplePassword(int length = 8)

 length = Math.Max(length, 8);

 var allowedLower = new[] 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'j', 'k', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z' ;
 var allowedDigits = new[] '1', '2', '3', '4', '5', '6', '7', '8', '9' ;
 var allowedUpper = allowedLower.Select(x => char.ToUpper(x)).ToArray();

 var r = new Random();
 var generatedCode = "";
 generatedCode += allowedLower[r.Next(0, allowedLower.Length)];

 generatedCode += allowedDigits[r.Next(0, allowedDigits.Length)];

 generatedCode += allowedUpper[r.Next(0, allowedUpper.Length)];

 var arrays = new[] allowedLower, allowedUpper, allowedDigits ;
 for (var i = 0; i < length - 3; i++)
 
 var array = arrays[r.Next(0, arrays.Length)];
 generatedCode += array[r.Next(0, array.Length)];
 

 return String.Join("", generatedCode.OrderBy(x => r.Next()));

When calling your method like this:

 for (int i = 0; i < 10; i++)
 
 Console.WriteLine(GenerateSimplePassword());
 

I get this result:

 88yuQg7H
 u5UqU36o
 u5UqU36o
 u5UqU36o
 u5UqU36o
 u5UqU36o
 u5UqU36o
 u5UqU36o
 u5UqU36o
 u5UqU36o

which is not so random.

The reason is the Random object being instantiated each time the method is called. Instead you must move the Random object outside the method as a static one time initialized field.

You could make the valid chars as a static field string:

const string chars = "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
static int DigitStart = chars.IndexOf('0');
static int UpperStart = chars.IndexOf('A');

Instead of concatenate strings throughout the process it would be better to use:

char[] password = new char[length]

it will improve both performance and readability:

password[0] = chars[random.Next(0, DigitStart)];

All in all my suggestion would be something like:

public static class PasswordGenerator


 static readonly Random random = new Random();
 const string chars = "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
 static int DigitStart = chars.IndexOf('0');
 static int UpperStart = chars.IndexOf('A');

 public static string GenerateSimplePassword(int length = 8)
 
 length = Math.Max(length, 8);

 char[] password = new char[length];
 password[0] = chars[random.Next(0, DigitStart)];
 password[1] = chars[random.Next(DigitStart, UpperStart)];
 password[2] = chars[random.Next(UpperStart, chars.Length)];

 for (int i = 3; i < length; i++)
 
 password[i] = chars[random.Next(0, chars.Length)];
 

 return new string(password.OrderBy(_ => random.Next()).ToArray());
 

Fully Refactored Code: see this Gist

Example Usage:

static void Main(string[] args) 
 var generator = new PasswordGenerator(new PasswordGeneratorOptions 
 MinimumNumberOfLowerCaseCharacters = 1,
 MinimumNumberOfNumericCharacters = 1,
 MinimumNumberOfUpperCaseCharacters = 1,
 OutputLength = 8,
 RandomNumberGenerator = new SecureRandom(),
 SpecialCharacters = new char[0] ,
 );

 Console.WriteLine(generator.Next());

Review Comments:

First off, using Random as a source of entropy probably isn't the best idea since it is possible (however unlikely) that an attacker could gain the underlying seed; with it they would trivially be able to deduce all of the outputs used to generate our passwords. Let's implement a generator that uses a static instance of the RNGCryptoServiceProvider class instead:

// makes it easier to replace the implementation I demonstrate with something better
public interface ISecureRandom

 uint Next(uint x, uint y);


// a possible implementation of a secure RNG, not exhaustively tested...
public sealed class SecureRandom : ISecureRandom

 private static RandomNumberGenerator m_randomNumberGenerator = new RNGCryptoServiceProvider();

 public SecureRandom(RandomNumberGenerator randomNumberGenerator) 
 if (null == randomNumberGenerator) 
 throw new ArgumentNullException(paramName: nameof(randomNumberGenerator));
 

 m_randomNumberGenerator = randomNumberGenerator;
 
 public SecureRandom() : this(new RNGCryptoServiceProvider()) 

 public byte[] GetBytes(byte[] buffer) 
 m_randomNumberGenerator.GetBytes(buffer);

 return buffer;
 
 public byte[] GetBytes(int count) => GetBytes(new byte[count]);
 public uint Next() => BitConverter.ToUInt32(GetBytes(sizeof(uint)), 0);
 public uint Next(uint x, uint y) 
 if (x > y) 
 var z = x;

 x = y;
 y = z;
 

 var range = (y - x);

 if (range == 0) 
 return x;
 
 else if (range == uint.MaxValue) 
 return Next();
 
 else 
 return (Next(exclusiveHigh: range) + x);
 
 

 private uint Next(uint exclusiveHigh) 
 var range = (uint.MaxValue - (((uint.MaxValue % exclusiveHigh) + 1) % exclusiveHigh));
 var result = 0U;

 do 
 result = Next();
 while (result > range);

 return (result % exclusiveHigh);
 

Regarding maintainability, we could start by creating a proper class to encapsulate all of our settings:

public sealed class PasswordGeneratorOptions

 private static readonly char[] DefaultSpecialChars = new[] '!', '@', '#', '$', '%', '^', '&', '*', '(', ')', '-', '=', '`', '~', '_', '+', ',', '.', ''', '"', ';', ':', '?', ';

 public int MinimumNumberOfNumericCharacters get; set; 
 public int MinimumNumberOfLowerCaseCharacters get; set; 
 public int MinimumNumberOfSpecialCharacters get; set; 
 public int MinimumNumberOfUpperCaseCharacters get; set; 
 public int OutputLength get; set; 
 public ISecureRandom RandomNumberGenerator get; set; 
 public IReadOnlyList<char> SpecialCharacters get; set; 

 public PasswordGeneratorOptions() 
 MinimumNumberOfLowerCaseCharacters = 0;
 MinimumNumberOfNumericCharacters = 0;
 MinimumNumberOfSpecialCharacters = 0;
 MinimumNumberOfUpperCaseCharacters = 0;
 SpecialCharacters = DefaultSpecialChars;
 

Then we can declare a PasswordGenerator class that consumes our options:

public sealed class PasswordGenerator

 private readonly PasswordGeneratorOptions m_options;

 public PasswordGenerator(PasswordGeneratorOptions options) 
 if (options.OutputLength < (options.MinimumNumberOfLowerCaseCharacters + options.MinimumNumberOfNumericCharacters + options.MinimumNumberOfSpecialCharacters + options.MinimumNumberOfUpperCaseCharacters)) 
 throw new ArgumentOutOfRangeException(message: "output length must be greater than or equal to the sum of all MinimumNumber* properties", actualValue: options.OutputLength, paramName: nameof(options.OutputLength));
 

 m_options = options;
 

Now we can start thinking about implementation details, I personally like the idea of breaking things up into various categories so that we can provide more complex configuration options later. Let's try classifying your characters into lower-case, upper-case, number, and special. The ASCII encoding has largely taken care of all of this for us "for free" since three of these four classes have contiguous regions in the specification; we can write a simple generator for each like so:

var randomNumberGenerator = new SecureRandom();

Func<char> GetAsciiLetterLowerCase = () => ((char)randomNumberGenerator.Next(97, 123));
Func<char> GetAsciiLetterUpperCase = () => ((char)randomNumberGenerator.Next(65, 91));
Func<char> GetAsciiNumber = () => ((char)randomNumberGenerator.Next(48, 58));

Special characters aren't as easy so we'll resort to using a table of values:

var specialCharacters = new char[] '!', '@', '#', ;

Func<char> GetAsciiSpecial = () => specialCharacters[randomNumberGenerator.Next(0U, ((uint)specialCharacters.Count))];

The only remaining thing left to do is implement the generator function. This can be accomplished by more or less following the same strategy you already have in place. I chose to implement Fisher–Yates shuffle instead of using LINQ for the final randomization step:

public string Next() 
 var index = 0;
 var length = m_options.OutputLength;
 var randomNumberGenerator = m_options.RandomNumberGenerator;
 var result = new char[length];
 var useSpecial = (0 < m_options.SpecialCharacters.Count);

 for (var i = 0; (i < m_options.MinimumNumberOfLowerCaseCharacters); i++) 
 result[index++] = m_getAsciiLetterLowerCase();
 

 for (var i = 0; (i < m_options.MinimumNumberOfNumericCharacters); i++) 
 result[index++] = m_getAsciiNumeric();
 

 for (var i = 0; (i < m_options.MinimumNumberOfSpecialCharacters); i++) 
 result[index++] = m_getAsciiSpecial();
 

 for (var i = 0; (i < m_options.MinimumNumberOfUpperCaseCharacters); i++) 
 result[index++] = m_getAsciiLetterUpperCase();
 

 for (var i = index; (i < length); i++) 
 char c;

 switch (randomNumberGenerator.Next(0U, (useSpecial ? 4U : 3U))) 
 case 3U:
 c = m_getAsciiSpecial();
 break;
 case 2U:
 c = m_getAsciiNumeric();
 break;
 case 1U:
 c = m_getAsciiLetterLowerCase();
 break;
 case 0U:
 c = m_getAsciiLetterUpperCase();
 break;
 default:
 throw new InvalidOperationException();
 

 result[i] = c;
 

 FisherYatesShuffle(randomNumberGenerator, result);

 return new string(result);


private static void SwapRandom<T>(ISecureRandom randomNumberGenerator, IList<T> list, uint indexLowerBound, uint indexUpperBound) 
 var randomIndex = randomNumberGenerator.Next(indexLowerBound, indexUpperBound);
 var tempValue = list[(int)randomIndex];

 list[(int)randomIndex] = list[(int)indexUpperBound];
 list[(int)indexUpperBound] = tempValue;

private static void FisherYatesShuffle<T>(ISecureRandom randomNumberGenerator, IList<T> list) 
 var length = list.Count;
 var offset = 0U;

 while (offset < length) 
 SwapRandom(randomNumberGenerator, list, 0U, offset++);