6
\$\begingroup\$

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 => 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()));
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Are these password composition rules yours, or an external requirement? Consider that the number of possible 8-character passwords composed from "0 or more characters from each list" is actually smaller than the number of possible 8-character passwords composed from "at least 1 character from each list". \$\endgroup\$ – benj2240 Jul 12 at 15:25
  • \$\begingroup\$ How are these passwords used? Can several people share the same random password? Does a password need to expire the next day? \$\endgroup\$ – dfhwze Jul 12 at 15:30
  • \$\begingroup\$ @benj2240 external requirement. I guess it avoid password with only lowercase char for example. \$\endgroup\$ – Thomas Ayoub Jul 12 at 15:40
  • 1
    \$\begingroup\$ @pacmaninbw users are quite low tech, so yes, we avoid special char in this generator \$\endgroup\$ – Thomas Ayoub Jul 12 at 15:47
  • 3
    \$\begingroup\$ @benj2240 did you confuse "smaller" with "larger" in your above comment? It doesn't make sense to me in its current form. \$\endgroup\$ – Roland Illig Jul 13 at 5:07
8
\$\begingroup\$

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());
  }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer. The string is shuffled with generatedCode.OrderBy(x => r.Next(), how the hacker could find out anything? But it gives a hint about bad code readability. I don't see how the simpler solution would assure that I get any of the 3 different char class. Does it? \$\endgroup\$ – Thomas Ayoub Jul 12 at 16:11
  • \$\begingroup\$ @ThomasAyoub: Sorry, I missed the last line of code. But why do you want at least one of each? \$\endgroup\$ – Henrik Hansen Jul 12 at 16:14
  • \$\begingroup\$ Which version of .NET you use? According to @Eric Lippert this problem with Random is resolved in later versions. \$\endgroup\$ – dfhwze Jul 12 at 17:25
  • \$\begingroup\$ @dfhwze: I use 4.8. I've see he commented that somewhere, but can't remember where. Apparently it is not solved yet. Maybe in core? his blog: "Fortunately, I have just learned from an attentive reader that this problem has been fixed in some versions of the CLR; exactly which I am not sure..."? \$\endgroup\$ – Henrik Hansen Jul 12 at 17:30
  • \$\begingroup\$ @Eric Lippert Eric's answer here talks about it: codereview.stackexchange.com/questions/223593/… \$\endgroup\$ – dfhwze Jul 12 at 17:32
7
\$\begingroup\$

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 => 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) => 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;
        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 < (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++);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Is there really no predefined SecureRandom class in .NET? It sounds really strange that everyone would have to write this class by themselves. \$\endgroup\$ – Roland Illig Jul 12 at 21:23
  • 2
    \$\begingroup\$ This code looks really bloated, compared to the original code. Generating a password using a given simple and fixed scheme should take no more than 30 lines of code. \$\endgroup\$ – Roland Illig Jul 12 at 21:27
  • \$\begingroup\$ @RolandIllig Sadly, there really isn't anything in the standard lib; I'd have used it. One concedes that the code is bloated but figured that having parameters is more "maintainable" in the long run; for example, one might be dealing with many clients that all have different configurations (or, heaven forbid, a client that changes their requirements over time). \$\endgroup\$ – Kittoes0124 Jul 12 at 21:35
  • \$\begingroup\$ Very, very firm uptick for using RNGCryptoServiceProvider. While yes, it's very unlikely that an attacker will get the seed, it's always best to use cryptographically secure system when working with any part of authentication, especially CSPRNGs over DPRNGs. \$\endgroup\$ – Ghedipunk Jul 12 at 23:40
  • \$\begingroup\$ The default Random class uses the system time as the seed. More specifically, the Environment.TickCount. This makes it REALLY EASY to guess the seed. There are so few options, you can just check them all! So, yes, in a serious implementation, using the RNGCryptoServiceProvider is a must. For homework, of course, the default Random class will be just fine. \$\endgroup\$ – Vilx- Jul 13 at 13:54
4
\$\begingroup\$

Most of your variables should be static readonly

There is no reason to new up a brand new copy of the arrays of characters each time you run this function. Instead:

public static readonly ReadOnlyCollection<char> AllowedLower = ReadOnlyCollection<char>("abcdefghijklmnopqrstuvwxyz");
public static readonly ReadOnlyCollection<char> AllowedUpper = new ReadOnlyCollection<char>(AllowedLower.Select(x => char.ToUpper(x)));
public static readonly ReadOnlyCollection<char> AllowedDigits = new ReadOnlyCollection<char>("0123456789");

Note that for static, read only lists of things, we use ReadOnlyCollection to prevent modification at run time. There are some alternative implementations of read only collections in .NET depending on your version; any of them is also fine.

While we're at it, we can greatly improve readability by passing just one string, which implements IEnumerable<char>.

Randomness problems

You have two incredibly major problems with randomness:

  1. The first is that you create a new instance of Random each time you invoke the function. That is wrong. You must reuse the same instance of a random number generator when generating many random numbers to ensure that it is sufficiently random.
  2. You're using Random which doesn't meet the level of randomness required for security algorithms. You need this level of randomness for generating passwords. You should use RNGCryptoServiceProvider.
public static readonly RNGCryptoServiceProvider SecureRNG = new RNGCryptoServiceProvider();

Selecting random characters

Once the minimum requirements for each group of characters is met, you use a two stage random selection to choose the remaining characters. Because each group is not of the same length, this actually reduces the randomness. Any single digit has a higher probability of being selected than any single letter.

To resolve this, combine all of the characters into a single collection:

public static readonly ReadOnlyCollection<char> PasswordAlphabet = new ReadOnlyCollection<char>(AllowedLower.Concat(AllowedUpper).Concat(AllowedDigits));

And then select from that:

generatedPassword += PasswordAlphabet[randomIndex]

Use a more standard shuffle algorithm

Since we can't use Random and RNGCryptoServiceProvider doesn't provide a Next(), using OrderBy to shuffle the password becomes more difficult. We need to ensure that our shuffling doesn't have biases in the order, and we need to do so without using doubles.

Luckily, this is a solved problem. We'll use that solution.

Selecting a random character from the lists

You may have noticed that we skipped over how to generate randomIndex in the code above that selects a random character. That is because RNGCryptoServiceProvider does not readily implement a means of doing so. We'll need to implement the ability to randomly select from a range of values.

The typical way of generating a random integer in a range is to first calculate a random double and then multiply it by the range's length. We can do this with a couple extension methods that we'll implement later.

Prefer LINQ to loops

We don't need a for loop here. We can use LINQ:

generatedPassword += String.Join(
    "",
    Enumerable.Range(0, length - 3).Select(i => /* Get a random character */)
);

Don't silently replace inputs

While it's good that you restricted your method to require at least an 8 character password, it's not good to return something that the caller did not ask for. Instead of replacing the length, throw an error:

if (length < 8)
{
    throw new ArgumentException(paramName: nameof(length), message: "must be at least 8");
}

Higher levels of code may choose to replace user input, but that kind of logic belongs there in the caller, not here in the depths of the implementation.

What about dependency injection?

Should any of this be written in the dependency injection style?

Firstly, I would say that the secure RNG instance should not be injected. A random number generator is one of the few elements of code where doing so is a bad idea. The reason is the very mistake you made in your code: you need a single, permanent instance of the RNG reused over and over. The secure version is notably thread safe, so it's fine to have just a single instance. In fact, I would go so far as to say that if other classes need an RNG, then it should be move out of this class and into a global static one if possible. (It's possible performance requirements may override this advice, but in general, the fewer instances of RNGs you have and the more they're shared, the better.)

Whether the method you're writing needs to be an instance that can be injected will depend on your requirements and the code base surrounding it. I don't see any particular harm in doing so, and I don't have enough context to know whether there's any benefit. The password generator I will present can be trivially converted into a stateless instance that could be injected, though.

Combining everything

First let's put our randomizing utilities in a separate class:

public static class SecureRandom
{
    // We should only ever have one instance of this.
    // Other classes can use this freely, or methods can be added here.
    public static readonly RNGCryptoServiceProvider SecureRNG = new RNGCryptoServiceProvider();

    // NextDouble and Next based on https://stackoverflow.com/a/16907577

    public static double NextDouble(this RNGCryptoServiceProvider secureRNG)
    {
        var data = new byte[sizeof(uint)];
        secureRNG.GetBytes(data);
        var randUint = BitConverter.ToUInt32(data, 0);
        return randUint / (uint.MaxValue + 1.0);
    }

    public static int Next(this RNGCryptoServiceProvider secureRNG, int minValue, int maxValue)
    {
        if (minValue > maxValue)
        {
            throw new ArgumentOutOfRangeException();
        }

        return minValue + (int)((maxValue - minValue) * secureRNG.NextDouble());
    }

    public static int Next(this RNGCryptoServiceProvider secureRNG, int maxValue)
    {
        return secureRNG.Next(0, maxValue);
    }

    private static T SelectElement<T>(this RNGCryptoServiceProvider secureRNG, IList<T> choices)
    {
        return choices[SecureRNG.Next(choices.Count)];
    }

    // Based on https://stackoverflow.com/a/1262619
    private static void ToRandomOrder<T>(this RNGCryptoServiceProvider secureRNG, IEnumerable<T> sequence)
    {
        var workingList = new List<T>(sequence);

        int n = workingList.Count;
        while (n > 1)
        {
            byte[] box = new byte[1];
            do SecureRNG.GetBytes(box);
            while (!(box[0] < n * (Byte.MaxValue / n)));
            int k = (box[0] % n);
            n--;
            T value = workingList[k];
            workingList[k] = workingList[n];
            workingList[n] = value;
        }

        return workingList;
    }
}

With our extension methods above, now we can write the full method:

using static SecureRandom;

public static class SimplePasswordGenerator
{
    public static readonly ReadOnlyCollection<char> AllowedLower = ReadOnlyCollection<char>("abcdefghijklmnopqrstuvwxyz");
    public static readonly ReadOnlyCollection<char> AllowedUpper = new ReadOnlyCollection<char>(AllowedLower.Select(x => char.ToUpper(x)));
    public static readonly ReadOnlyCollection<char> AllowedDigits = new ReadOnlyCollection<char>("0123456789");

    public static readonly ReadOnlyCollection<char> PasswordAlphabet = new ReadOnlyCollection<char>(
        AllowedLower
        .Concat(AllowedUpper)
        .Concat(AllowedDigits)
    );

    public static string GenerateSimplePassword(int length = 8)
    {
        if (length < 8)
        {
            throw new ArgumentException(paramName: nameof(length), message: "must be at least 8");
        }

        IEnumerable<char> passwordCharacters = new [] { 
            // At least one from each list
            SecureRNG.SelectElement(AllowedLower),
            SecureRNG.SelectElement(AllowedUpper),
            SecureRNG.SelectElement(AllowedDigits)
        }

        passwordCharacters = passwordCharacters.Concat(
            Enumerable.Range(0, length - 3)
            .Select(i => SecureRNG.SelectElement(PasswordAlphabet))
        );

        return String.Join("", SecureRNG.ToRandomOrder(passwordCharacters));
    }
\$\endgroup\$
  • \$\begingroup\$ Great point about the biased entropy between digits and letters. And that's the first time I see a do statement written like that. \$\endgroup\$ – dfhwze Jul 13 at 18:18
  • 1
    \$\begingroup\$ @dfhwze Just copy/pasted that do/while from the other source. lol. When I use other people's code, I try not to change it anymore than I really need to. \$\endgroup\$ – jpmc26 Jul 13 at 18:27

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.