C# Random Password Generation

Question

So I have a method that, after validation, generates a random password based on the length and characters provided. The UI uses check boxes to select using between lowercase and/or uppercase letters, numbers and special characters to populate the characters parameter. as well as the length of the password and the spacing between each character used (I.E. if character spacing is set to '10', the letter 'A', for example, can only be used again after another 10 unique characters are used first):

The Generator Method:

    private string PasswordGeneration(string characters, int length)
    {
        var random = Factory.Random(); //Generates new Random() from Factory class.
        var availableCharacters = characters; //Preps the available characters with all characters passed.
        var output = string.Empty;
        var rule = ValidateRule(characters.Length); //Sets character rule to length of parameter "characters" if the unique character spacing is set higher than the number of available characters to use. 
        for (var position = 0; position < length; position++) //loop until we've met the password's desired length.
        {
            if (rule != 0 && (characters.Length - availableCharacters.Length == rule)) //if a character rule is set & if we've met the criteria of the number of unique characters asked for...
            {
                availableCharacters = characters.Where(c => !output.Substring((position - rule) + 1, rule - 1).Contains(c)).Aggregate(availableCharacters, (current, c) => current + (current.Contains(c) ? string.Empty : c.ToString()));
                //...we add back all characters for usage other than the last n characters used dictated by the unique character rule.
            }
            var character = availableCharacters[random.Next(availableCharacters.Length)]; //Grabs a random character from availableCharacters.
            availableCharacters = rule == 0 ? availableCharacters : availableCharacters.Replace(character.ToString(), string.Empty); //If a character rule is set, we remove the currently used character from availableCharacters.
            output += character; //Append "character" to output.
        }
        return output;
    }

The 'ValidateRule()' Method:

        private int ValidateRule(int length) => 
        length - _settings.IdenticalSpacing > 0 
            ? _settings.IdenticalSpacing
            : length;

And for readability with what's happening with the LINQ expression within the if statement:

foreach(var c in characters)//...we iterate through the original set of characters...
{
    if(output.Substring((position - rule) + 1, rule -1).Contains(c) == false)//...and check to see if the a specific range contains each character. If the range doesn't contain the current character...
    {
        foreach(var availableCharacter in availableCharacters)//...we iterate through our available characters to see if it doesn't already contains the current character...
        {
            if(!Equals(availableCharacter, c))
            {
                availableCharacters += c; //...if not, we make the character available for use again.
            }
        }
    }
}

As stated before, the characters and password length parameters are all validated before being passed to PasswordGeneration(), so I didn't add any null checking or checks for bad inputs other than for the character spacing rule. While everything works perfectly fine with no errors, I feel as if the code looks messy and of course a bit hard to read. I can't help but think there's a much more simple approach without a bunch of loops and conditions or long LINQ expressions. However, I would love to hear some feedback on what you all think and of course, how I can improve this.

Answer 1

Remove unnecessary comments:

var random = Factory.Random(); //Generates new Random() from Factory class.

It's obvious if you read the code.

It's ok to break those lines

availableCharacters = characters
    .Where(c => !output.Substring((position - rule) + 1, rule - 1).Contains(c))
    .Aggregate(availableCharacters, 
        (current, c) => (current.Contains(c) 
            ? current 
            : current + c.ToString()));

Makes it a bit more readable. But still all these +1 -1 does not look very bug safe.

Here's my approach:

private string PasswordGeneration(string characters, int length, int characterSpacing)
{
    var random = new Random();
    var randomList = new List<int>();
    var spacing = Math.Min(characterSpacing, characters.Length);
    // Generate indexes
    while(randomList.Count < length) {
        var num = random.Next(0, characters.Length);
        var numNotInUse = randomList.LastIndexOf(num) == -1;
        var spacingOk = randomList.LastIndexOf(num) < (randomList.Count - spacing);
        if (numNotInUse || spacingOk ) {
            randomList.Add(num);
        }
    }
    // Set password from indexes
    var password = string.Empty;
    randomList.ForEach(idx => password += characters.ElementAt(idx));
    return password;
}

Simple is usually better. At least more readable :)

Answer 2

There are lots of ways to improve this hard-to-read code but all of them are unimportant compared to the fact that this is massively insecure against common attacks. You are implementing a component of a security system; you need to build a threat model and understand what you are doing. Implementing a security system is not a beginner task!

An important rule to follow is never use System.Random for a security purpose.

A random number generator needs to be genuinely unpredictable for it to be used in a security system that depends on unpredictability. System.Random is not truly random, and nor is it crypto strength pseudo random.

Why does this matter? Because an attacker who knows anything about the system used to generate the passwords can make good guesses about what passwords it will generate, much better than chance. How does the attacker get information about the system? A generated password leaks information about the state of the system.

That is, given a collection of passwords generated by this algorithm allows an attacker to determine the internal state of System.Random and then from that they can make good guesses as to all past and future passwords that will be generated. That's bad! The whole point of a randomly-generated password is to make it hard to guess.

Use a crypto-strength PRNG instead.