Password maker in C# I need to generate semi-complex throw-able passwords (just one use) in my application. I want it to be:


at least 8 characters long;
contains at least 1 digit, 1 [a-z] char, 1 [A-Z] char;


I generate a few passwords a day, and I don't mind about speed. I'm more looking for suggestions about code being maintainable (or not) and possible huge security flaws. 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 =&gt; 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 &lt; length - 3; i++)
    {
        var array = arrays[r.Next(0, arrays.Length)];
        generatedCode += array[r.Next(0, array.Length)];
    }

    return String.Join("", generatedCode.OrderBy(x =&gt; r.Next()));
}
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,
        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 readonly RandomNumberGenerator DefaultRandomNumberGenerator = new RNGCryptoServiceProvider();

    public static SecureRandom DefaultInstance =&gt; new SecureRandom(DefaultRandomNumberGenerator);

    private readonly RandomNumberGenerator m_randomNumberGenerator;

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

        m_randomNumberGenerator = randomNumberGenerator;
    }

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

        return buffer;
    }
    public byte[] GetBytes(int count) =&gt; GetBytes(new byte[count]);
    public uint Next() =&gt; BitConverter.ToUInt32(GetBytes(sizeof(uint)), 0);
    public uint Next(uint x, uint y) {
        if (x &gt; 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 &gt; 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[] { '!', '@', '#', '$', '%', '^', '&amp;', '*', '(', ')', '-', '=', '`', '~', '_', '+', ',', '.', '\'', '"', ';', ':', '?', '|', '/', '\\', '[', ']', '{', '}', '&lt;', '&gt;' };

    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&lt;char&gt; SpecialCharacters { get; set; }

    public PasswordGeneratorOptions() {
        MinimumNumberOfLowerCaseCharacters = 0;
        MinimumNumberOfNumericCharacters = 0;
        MinimumNumberOfSpecialCharacters = 0;
        MinimumNumberOfUpperCaseCharacters = 0;
        RandomNumberGenerator = SecureRandom.DefaultInstance;
        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 &lt; (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&lt;char&gt; GetAsciiLetterLowerCase = () =&gt; ((char)randomNumberGenerator.Next(97, 123));
Func&lt;char&gt; GetAsciiLetterUpperCase = () =&gt; ((char)randomNumberGenerator.Next(65, 91));
Func&lt;char&gt; GetAsciiNumber = () =&gt; ((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&lt;char&gt; GetAsciiSpecial = () =&gt; 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 &lt; m_options.SpecialCharacters.Count);

    for (var i = 0; (i &lt; m_options.MinimumNumberOfLowerCaseCharacters); i++) {
        result[index++] = m_getAsciiLetterLowerCase();
    }

    for (var i = 0; (i &lt; m_options.MinimumNumberOfNumericCharacters); i++) {
        result[index++] = m_getAsciiNumeric();
    }

    for (var i = 0; (i &lt; m_options.MinimumNumberOfSpecialCharacters); i++) {
        result[index++] = m_getAsciiSpecial();
    }

    for (var i = 0; (i &lt; m_options.MinimumNumberOfUpperCaseCharacters); i++) {
        result[index++] = m_getAsciiLetterUpperCase();
    }

    for (var i = index; (i &lt; 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&lt;T&gt;(ISecureRandom randomNumberGenerator, IList&lt;T&gt; 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&lt;T&gt;(ISecureRandom randomNumberGenerator, IList&lt;T&gt; list) {
    var length = list.Count;
    var offset = 0U;

    while (offset &lt; length) {
        SwapRandom(randomNumberGenerator, list, 0U, offset++);
    }
}