Password generator and hashing

Question

To generate a password, I generate a key using RNGCryptoServiceProvider. The key is 9 alphanumeric characters long into which I insert a single non-alphanumeric character for a full length of 10 characters.

Having a mix of Uppercase, lowercase, numbers and symbols and being 10 characters long, as a plain text password (which you'd enter when logging in somewhere), this password should be more than adequate for most situations.

// used for more than just generating a password
public static string CreateKey(int length)
{
    string Chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdejghijklmnopqrstuvwxyz1234567890";
    StringBuilder Sb = new StringBuilder();

    using (RNGCryptoServiceProvider Rng = new RNGCryptoServiceProvider())
    {
        byte[] Buffer = new byte[sizeof(uint)];

        while (length-- > 0)
        {
            Rng.GetBytes(Buffer);
            uint N = BitConverter.ToUInt32(Buffer, 0);
            Sb.Append(Chars[(int)(N % (uint)Chars.Length)]);
        }
    }

    return Sb.ToString();
}

public static string CreatePasswordString()
{
    string Chars = @"~`!@#$%^&*()\/',.?;:|";
    var RNG = new Random();

    // Select a character to insert into the password
    string Character = new String(Enumerable.Repeat(Chars, 1).Select(s => s[RNG.Next(s.Length)]).ToArray());

    return CreateKey(9).Insert(RNG.Next(9), Character);
}

This generates a password which is then hashed in my application using bcrypt as per below:

public static string CreatePassword(string password)
{
    byte[] PasswordBytes = Encoding.ASCII.GetBytes(password);

    return Crypter.Blowfish.Crypt(PasswordBytes);
}

I can then validate the input at login more or less with this function - I haven't tested this code since I'm writing it for the first time for this question.

public static bool ValidatePassword(string emailAddress, string password)
{
    var User = db.Users.FirstOrDefault(x => x.EmailAddress == emailAddress);

    if (User == null) return false;

    return Crypter.Blowfish.CheckPassword(password, User.Password);
}

I used to use PBKDF2 but I was informed that bcrypt is better so here we are. While I understand the rule "never use your own auth", is this code likely to provide decent security?

What sort of vulnerabilities are present in implementing this system for security?

Answer 1

This is a great sample for review btw! I'm glad you posted this. Hopefully it will get some good google traffic.

I see a number of problems here however, and they're not all with the code:

1. Why are you generating a password in the first place? The user should be doing this! Are you going to hash and store this plain and destroy it? Or are you going to email it to them or show it on screen? In any case, you shouldn't be generating passwords for users if you can help it. If you need to represent state for a user account who has not yet set a password, use null, string.Empty or some other flag to model this state explicitly. Do not choose a password for them please.
2. If you absolutely must choose a random password, then make it secure. I contest this statement "this password should be more than adequate for most situations". Alphanumerics + special characters of at least 30-50 characters is more what you want. LastPass for example defaults to 30 chars and the full mix of symbols. Also, you're composing your randomly generated plain using RNGCryptoServiceProvider and Random! Do not do this! You want cryptographically secure random numbers which you will get with RNGCryptoServiceProvider but you won't with Random. So please never use Random for security related work.
3. Lastly, I don't believe Crypter.Blowfish.Crypt(PasswordBytes) is bcrypt. What is the package/reference for that call? I believe the package you want is BCrypt-Official which should look something like BCrypt.Net.BCrypt.HashPassword(passwordPlain, BcryptWorkfactor) when used. If what you have there is what I think it is, it's not an adaptive function which is possibly worse than MD5 or SHA1 (which for clarity are also not acceptable).

Here is an alternative to generating a simple plain securely:

    private static string GenerateSimpleKeySecurely(int keyLength)
    {
        // add whatever special characters in here you like
        var keyspace = @"0123456789abcdefghijklmnopqustuvwxyz()[]{}".ToCharArray();

        var data = new byte[1];
        using (var crypto = new RNGCryptoServiceProvider())
        {
            crypto.GetNonZeroBytes(data);
            data = new byte[keyLength];
            crypto.GetNonZeroBytes(data);
        }
        var result = new StringBuilder(keyLength);
        foreach (var b in data)
            result.Append(keyspace[b % (keyspace.Length)]);

        return result.ToString();
    }

Now go and read the OWASP: Password Storage Cheat Sheet.

Answer 2

You should not try to change a parameter, you should assign the value of the parameter to a variable declared inside the method.

public static string CreateKey(int length)
{
    string Chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdejghijklmnopqrstuvwxyz1234567890";
    StringBuilder Sb = new StringBuilder();

    using (RNGCryptoServiceProvider Rng = new RNGCryptoServiceProvider())
    {
        byte[] Buffer = new byte[sizeof(uint)];

        while (length-- > 0)
        {
            Rng.GetBytes(Buffer);
            uint N = BitConverter.ToUInt32(Buffer, 0);
            Sb.Append(Chars[(int)(N % (uint)Chars.Length)]);
        }
    }

    return Sb.ToString();
}

You should instead use a for loop and make the incrementation variable equal to the length parameter and then decrement the variable by one every loop until the incrementation variable is zero, like this

public static string CreateKey(int length)
{
    string Chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdejghijklmnopqrstuvwxyz1234567890";
    StringBuilder Sb = new StringBuilder();
    using (RNGCryptoServiceProvider Rng = new RNGCryptoServiceProvider())
    {
        byte[] Buffer = new byte[sizeof(uint)];

        for (var i = length; i > 0; i--)
        {
            Rng.GetBytes(Buffer);
            uint N = BitConverter.ToUInt32(Buffer, 0);
            Sb.Append(Chars[(int)(N % (uint)Chars.Length)]);
        }
    }

    return Sb.ToString();
}