Return to Answer

move logic from a comment into the code.

Source Link

edited Mar 21, 2017 at 10:59

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 will landlands on one of the "unfair" numbers:

while (0 < length--)
{
    rProvider.GetBytes(random);
    // 248 isvar calculatedmaxFairNumber by= Math.Floor(byte.MaxValue/valid.Length) * valid.LengthLength;
    // Don't needwhile the(0 `Floor`< thoughlength--)
{
 as it's integer arithmeticrProvider.GetBytes(random);
    if (random[0] <>= 248maxFairNumber)
    {
        // unfairDon't use this number.
        continue;
    }
    res.Append(valid[random[0] % valid.Length]);
}

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

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 will land on one of the "unfair" numbers:

while (0 < length--)
{
    rProvider.GetBytes(random);
    // 248 is calculated by Math.Floor(byte.MaxValue/valid.Length) * valid.Length
    // Don't need the `Floor` though as it's integer arithmetic.
    if (random[0] < 248)
    {
        // unfair.
        continue;
    }
    res.Append(valid[random[0] % valid.Length]);
}

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

Source Link

created Mar 17, 2017 at 15:28

RobH

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 will land on one of the "unfair" numbers:

while (0 < length--)
{
    rProvider.GetBytes(random);
    // 248 is calculated by Math.Floor(byte.MaxValue/valid.Length) * valid.Length
    // Don't need the `Floor` though as it's integer arithmetic.
    if (random[0] < 248)
    {
        // unfair.
        continue;
    }
    res.Append(valid[random[0] % valid.Length]);
}