Random Password Generator in C#

Question

Security is a huge complicated subject. What I'm looking for here is three things:

1. The password generating algorithm isn't reversible, meaning that when someone looks at the source code of this application, that won't help them to break passwords made by it other than the fact that they can see the possibilities as strings there.
2. Could I somehow remove those possibilities as strings to make it more secure?
3. Am I overlooking any potential memory issues e.g. should I be zeroing/wiping memory in areas where the password is stored after?

enum Password_Options
{
    ALPHANUM,
    ALL
}
private string CreatePassword(int length, Password_Options options)
{
    const string valid = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890";
    const string valid_all = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890!@#$%^&*()_-=+{}:;\\<>?|,./`~[]'";
    if (options == Password_Options.ALPHANUM){
        StringBuilder res = new StringBuilder();
        byte[] random = new byte[1];
        RNGCryptoServiceProvider rProvider = new RNGCryptoServiceProvider();
        while (0 < length--){
            rProvider.GetBytes(random);
            res.Append(valid[random[0] % (valid.Length - 1)]);
        }
        return res.ToString();
    }
    else{
        StringBuilder res = new StringBuilder();
        byte[] random = new byte[1];
        RNGCryptoServiceProvider rProvider = new RNGCryptoServiceProvider();
        while (0 < length--)
        {
            rProvider.GetBytes(random);
            res.Append(valid_all[random[0] % (valid_all.Length - 1)]);
        }
        return res.ToString();
    }
}



private void button1_Click(object sender, EventArgs e)
{
    Password_Options po;
    if (radioButton1.Checked){
        po = Password_Options.ALL;
    }
    else{
        po = Password_Options.ALPHANUM;
    }
    textBox1.Text = CreatePassword((int)numericUpDown1.Value,po);
}

Answer 1

One way to avoid using constant strings is to use the char.IsLetterOrDigit method, just restrict the values mod 92 and add to 33 to get all printable characters to 125:

enum Password_Options
{
    ALPHANUM,
    ALL
}
RNGCryptoServiceProvider rProvider = new RNGCryptoServiceProvider();
private string CreatePassword(int length, Password_Options options)
{
    StringBuilder res = new StringBuilder();
    byte[] random = new byte[1];
    using (rProvider)
    {
        while (0 < length--)
        {
            char rndChar = '\0';
            do
            {
                rProvider.GetBytes(random);
                rndChar = (char)((random[0] % 92) + 33);
            } while (options == Password_Options.ALPHANUM && !char.IsLetterOrDigit(rndChar));
            res.Append(rndChar);
        }
    }
    return res.ToString();
}

According to MSDN you should always dispose of the rng provider after using it. One way is with a using block.

Answer 2

Lot of repeated code for only one different line

    StringBuilder res = new StringBuilder();
    byte[] random = new byte[1];
    RNGCryptoServiceProvider rProvider = new RNGCryptoServiceProvider();
    while (0 < length--){
        rProvider.GetBytes(random);
        if (options == Password_Options.ALPHANUM){
            res.Append(valid[random[0]     % (valid.Length - 1)]);
        } 
        else{
            res.Append(valid_all[random[0] % (valid_all.Length - 1)]);
        }
    }
    return res.ToString();

Not sure if it had seed issues like Random but I would create RNGCryptoServiceProvider rProvider = new RNGCryptoServiceProvider(); outside of the method.

Answer 3

Seeing your code could help me attack your passwords - it has a couple of flaws which mean certain characters are more likely to appear than others.

Here's the offending part:

valid[random[0] % (valid.Length - 1)]

Firstly, by doing % (length - 1) you're never going to be getting the last element of the array. That's going to help me a lot when I'm trying to crack your passwords.

Secondly, we know that random[0], being a byte can be 0 to 255 inclusive. Your valid array is 62 items...

255/62 = 4 remainder 7

That remainder 7 is bad news! That means the first 7 elements have 5 numbers that will produce their index and the others only have 4 numbers that will. I.e. your passwords aren't truly random.

You can add a guard to make sure you don't use the number if it lands on one of the "unfair" numbers:

var maxFairNumber = (byte.MaxValue/valid.Length) * valid.Length;
while (0 < length--)
{
    rProvider.GetBytes(random);
    if (random[0] >= maxFairNumber)
    {
        // Don't use this number.
        continue;
    }
    res.Append(valid[random[0] % valid.Length]);
}

(Edit: I had the equality the wrong way round the first time!)

Answer 4

this can be alternate..

    private string CreatePassword(int length, Password_Options options)
    {
        const string valid = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890";
        const string valid_all = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890!@#$%^&*()_-=+{}:;\\<>?|,./`~[]'";
        using (RNGCryptoServiceProvider rProvider = new RNGCryptoServiceProvider())
        {
            var source = options == Password_Options.ALL ? valid_all : valid;
            byte[] random = new byte[source.Length];
            rProvider.GetBytes(random);
            var pwd = (from k in
                           from c in source select new { c = c, o = random[source.IndexOf(c)] }
                       orderby k.o
                       select k.c).Take(length).ToArray();
            return new string(pwd);
        }
    }

Password_Options is not must

    private string CreatePassword(int length)
    {
        using (RNGCryptoServiceProvider rProvider = new RNGCryptoServiceProvider())
        {
            // ASCII printable characters
            char[] chars = Enumerable.Range(33, 94).ToArray().Select(a => (char)a).ToArray();
            string source = new string(chars);
            byte[] random = new byte[source.Length];
            rProvider.GetBytes(random);
            var pwd = (from k in
                           from c in source select new { c = c, o = random[source.IndexOf(c)] }
                       orderby k.o
                       select k.c).Take(length).ToArray();
            return new string(pwd);
        }
    }