1

I'm trying to create something that will be like a basic random password generator including uppercase, lowercase and digits - but for some reason this

        static void Main(string[] args)
    {
        bool validLength = false; 
        int userDefinedLength = 0;
        Console.WriteLine("How many characters would you like your password to be?");
        do 
        {
            try
            {
                userDefinedLength = int.Parse(Console.ReadLine());
                validLength = true;
                if (userDefinedLength < 3)
                {
                    Console.WriteLine("Please enter something larger than 3.");
                    validLength = false; 
                }
            }
            catch (Exception)
            {
                Console.WriteLine("Please input a valid integer length.");
            }
        } while (validLength == false);
        char[] passwordArray = new char[userDefinedLength]; 
        int asciiValue = 0;
        char asciiChar = ' ';
        bool validPassword = false; 
        Random ranAsciiGroup = new Random();
        Random ascValue = new Random(); 
        do
        {
            for (int i = 0; i < passwordArray.Length; i++) 
            {
                int randomAsc = 0;
                randomAsc = ranAsciiGroup.Next(1, 4);
                //Console.WriteLine(randomAsc);
                if (randomAsc == 1)
                {
                    asciiValue = ascValue.Next(65, 91);
                    //Console.WriteLine(asciiValue);
                }
                else if (randomAsc == 2) 
                {
                    asciiValue = ascValue.Next(97, 123);
                    //Console.WriteLine(asciiValue);
                }
                else if (randomAsc == 3) 
                {
                    asciiValue = ascValue.Next(48, 58);
                    //Console.WriteLine(asciiValue);
                }
                asciiChar = (char)asciiValue; 
                passwordArray[i] = asciiChar; 
                //validPassword = true;
            }


            bool isDigit = false; 
            bool isUpper = false;
            bool isLower = false;

            for (int i = 0; i < passwordArray.Length; i++) 
            {
                if (char.IsDigit(passwordArray[i]))
                {
                    isDigit = true; 
                }

                if (char.IsUpper(passwordArray[i]))
                {
                    isUpper = true; 
                }

                if (char.IsLower(passwordArray[i]))
                {
                    isLower = true; 
                }
            }

            if (isDigit == true && isUpper == true && isLower == true) 
            {
                validPassword = true; 
            }


        } while (validPassword == false); 

        Console.WriteLine("Your password is..."); 
        Console.ForegroundColor = ConsoleColor.DarkGreen;
        foreach (char c in passwordArray)
        {
            Console.Write(c);
        }
        Console.ReadLine();
    }

The password that it produces seems to not be using any numbers that are less than 6. And some of the characters that it produces are quite repeated - e.g. the lower case tend to have characters that appear much more than some others - or some that don't appear at all. I'll leave a 100 character example here.

m9nj88m8GBpF7Hk87E8p9CAE987pEj7pm7j89iHplo7DIpB87B9irlAk9Ik9C8q8i97B9o8l8GDjj88j898Dmk9A69969Ino988I
Uwe Keim
  • 39,551
  • 56
  • 175
  • 291
vK 3 1 RON
  • 113
  • 8
  • 1
    You're using `Random` incorrectly, see https://stackoverflow.com/questions/2706500/how-do-i-generate-a-random-int-number-in-c?rq=1 – Eris Nov 21 '17 at 22:57
  • I haven't looked it thoroughly but your code does seem to use numbers less than 6, see these few examples of 100 characters: `2mT1L99JZt41VKeiHu47g42zS1oovD6q934a3MjasHG523c8SU1oIM0QJ9lEyL164W3iYh0g8W5r1I2JZGy6rdtJ526d3RqvJ9cz` and `wsn5L11Bz72Lp4umpO2RmQW3WPc47P02XxcajItoItIHkXK2AoXYm2w431vAC7NUu6Z62tyuo49JN8E9zcm69Hx5PYe3d9QjHX23` – Sash Sinha Nov 21 '17 at 22:57
  • This is actually pretty interesting and not your imagination, it's due to the in-sync instances of Random. – harold Nov 21 '17 at 23:01
  • Yeah I've just removed the second instance of random and just used the first one throughout the program. That seems to work now. – vK 3 1 RON Nov 21 '17 at 23:02
  • Here's a small example that makes some histograms of this effect: https://ideone.com/0x2dv9 – harold Nov 21 '17 at 23:04
  • It will also work if you use multiple randoms with different seed times. E.g., I tried it with a random for the character type to use and each character type. A simple example of giving a seed is `Random ranAsciiGroup = new Random(DateTime.Now.Millisecond);` You'll want to change that seed value for every random if you go with multiple randoms. This was an interesting issue. – ps2goat Nov 21 '17 at 23:19

2 Answers2

2

Seed your RNG

Don't forget to seed your instance of random, e.g.

var random = new System.Random(Environment.TickCount);

Also, one instance should be enough.

Eliminating repetition

If you wish to ensure that all characters are represented, you can use a different random selection technique. For example, you could generate a very very long string that contains all the characters you want, sort it randomly, then take the first n characters. This approach would completely eliminate repeated characters and guarantee that every character gets used eventually. For example:

using System;
using System.Linq;

public class Program
{
    static readonly Random _random = new System.Random(Environment.TickCount);
    const string dictionary = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklpmnopqrstuvwxyz0123456789!@#$%^&*()_+-=";

    static string GetPassword(int length)
    {
        return new String
            (
                dictionary
                   .OrderBy( a => _random.NextDouble() )
                   .Take(length)
                   .ToArray()
            );
    }

    public static void Main()
    {
        var s = GetPassword(30);
        Console.WriteLine(s);
    }
}

In this example we treat the string as an Enumerable<char> (yes, this is allowed) and use LINQ methods to sort it randomly and take the first n characters. We then pass the results to the constructor for a new System.String containing the answer.

Sample output:

Run #1:

!#t(hfTz0rB5cvKy1d$oeVI2mnsCba

Run #2:

y7Y(MB1pWH$wO5XPD0b+%Rkn9a4F!_

Run #3:

tH92lnh*sL+WOaTYR160&xiZpC5@G3

Looks pretty random to me.

Controlled repetition

The above solution of course only allows at most one instance of each character in the dictionary. But maybe you want them to be able to appear more than once, but not too much. If you'd like a controlled, limited number of possible repeats, you can make a small change:

    static string GetPassword(int length, int maxRepeats)
    {
        return new String
            (
                Enumerable.Range(0, maxRepeats)
                   .SelectMany( i => dictionary )
                   .OrderBy( a => _random.NextDouble() )
                   .Take(length)
                   .ToArray()
            );
    }

In this example we clone the dictionary maxRepeats times and concatenate them using SelectMany. Then we sort that gigantic string randomly and take the first n characters for the password.

Working code on .NET Fiddle

John Wu
  • 50,556
  • 8
  • 44
  • 80
  • You don't need to manually seed it. Using the default constructor does exactly the same thing you have listed https://referencesource.microsoft.com/#mscorlib/system/random.cs,52 – TyCobb Nov 22 '17 at 16:39
0

The System.Random class is not designed to be a strong RNG, it is designed to be a fast RNG. If you want a quality random number generator you need to use one based on the System.Security.Cryptography.RandomNumberGenerator class.

Here is a gist I found that uses a crypto random generator as the internals of the existing random class.

Running it with the new generator gives me

mm77D5EDjO0OhOOe8kppiY0toc0HWQjpo37b4LFj56LvcQvA4jE83J8BS8xeX6zcEr2Od8A70v2xFKiY0ROY3gN105rZt6PE8F2i

which you can see appears to not have the biases you found.

Scott Chamberlain
  • 124,994
  • 33
  • 282
  • 431