1
\$\begingroup\$

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):

enter image description here

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.

\$\endgroup\$
1
\$\begingroup\$

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 :)

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ It's funny, I was actually considering using an index based list before posting here, but got so hung up on the if's being a necessity, that I just couldn't grasp the concept. Very nice execution, simple and clean! One thing I should point out though that while the Math.Min() works, it will hang if the character spacing and password length are set higher than the number available characters to use. However, that shows that it's another input to be validated prior to usage. \$\endgroup\$ – Chris P. Bacon Feb 20 at 0:35
  • \$\begingroup\$ Yep characters.Length - 1 works better. But it ought to throw an exception as it wont work as predicted. I'll leave that as an exercise ;) \$\endgroup\$ – tallberg Feb 20 at 7:07
4
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I probably should of added that the Random class is being used merely as a proof of concept. In production code, I would use the RNGCryptoServiceProvider class instead of Random. Regardless, this is a very valid point. \$\endgroup\$ – Chris P. Bacon Feb 20 at 0:43
  • 2
    \$\begingroup\$ @ChrisP.Bacon: Though I get that, there are two kinds of secure code. There's "secure code" and then there is "Security Code" -- code that actually implements a security system. My attitude towards Security Code is get it right at every stage -- design, prototyping, implementation, testing, documentation and disaster recovery. It is far too easy for stuff from the prototype to accidentally make it into the product, so use best practices from the start. \$\endgroup\$ – Eric Lippert Feb 20 at 0:47
  • \$\begingroup\$ @ChrisP.Bacon: In general, you should fix any known problems (even if the fixes reduce clarity or are unneeded for correctness) with your code before posting to CR. The main reason to follow this guideline is that it ensures that reviewers are not duplicating information that you already know; they'll instead spend their time coming up with improvements you did not think of. Also, production-ready code often has better context for reviewers to use. Finally, random people on the internet are going to use the Q&A as a starting point. \$\endgroup\$ – Brian Feb 20 at 14:35

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.