|
private char[] pwdCharArray = {<br />
'a','b','c','d','e','f','g','h','i','j','k','l','m',<br />
'n','o','p','q','r','s','t','u','v','w','x','y','z',<br />
'A','B','C','D','E','F','G','H','I','J','K','L','M',<br />
'N','O','P','Q','R','S','T','U','V','W','X','Y','Z',<br />
'0','1','2','3','4','5','6','7','8','9','`','~','!',<br />
'@','#','$','%','^','&','*','(',')','-','_','=','+',<br />
'[',']','{','}','\\','|',';',':','\'','"',',','<',<br />
'.','>','/','?'<br />
}
can be simplified (for ease of typing, and perhaps also reading) using the string's ToCharArray() method, to:
<br />
private char[] pwdCharArray = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789`~!@#$%^&*()-_=+[]{}\\|;:'\",<.>/?".ToCharArray();<br />
|
|
|
|
|
Good suggestion! Might make it into my next update. Thanks!
Kevin
"Semicolons in a programming language are like mother's milk."
|
|
|
|
|
Interesting article, but there is a not so small gotcha.
A rather large bias is introduced in this line in GetCryptographicRandomNumber()
int number = ( Int32.Parse(rndnum[0].ToString()) % uBound )
The problem is that the modulo operator "%" will produce an uneven distribution, especially where the operand is only a byte (0-255). The bias is variable depending on the value of upperBound and can significantly skew the distributions of characters. If you increase the random number range to 32 bits it won't eliminate the bias but it becomes trivial.
Also this section has a significant bias to return lBound whenever lBound is >0.
if ( lBound > number )
{
number = lBound;
}
-Marty
|
|
|
|
|
Here's a possible hack:
protected int GetCryptographicRandomNumber(int lBound, int uBound)
{
byte[] rndnum = new Byte[4];
rng.GetBytes(rndnum);
uint urndnum = System.BitConverter.ToUInt32(rndnum,0);
return (int)(urndnum % (uBound-lBound)) + lBound;
}
-Marty
|
|
|
|
|
You made a couple of very good points, Marty.
Alternatively, you can simply reject values outside the requested range. This removes any bias that is not in the random number generator itself, including the 0.00001% that's left in your solution.
protected int GetCryptographicRandomNumber(int lBound, int uBound)
{
byte[] rndnum = new Byte[1];
int number;
do
{
rng.GetBytes(rndnum);
number = Convert.ToInt32(rndnum[0]) & 0x7F;
} while (number < lBound || number > uBound);
return number;
} Note that the "& 0x7F " should be removed if uBound can be larger than 127.
If you're concerned about performance, you can work with a buffer of random bytes to minimize calls to GetBytes and/or reduce your random byte to the smallest range 0 to 2^n-1 that fits lBound and uBound. However, since we're not generating millions of passwords here, I don't think that's much of a concern.
Jeffrey
Everything should be as simple as possible, but not simpler. -- Albert Einstein
http://www.extremeoptimization.com/
|
|
|
|
|
I was wondering how long it would take for someone to grab that residual bias "bait" Chuckle.
It does show how subtle flaws can be in these sorts of things. I also concur about the linear congruential generator being flawed. The rand() in clib is so badly biased that Monte Carlo simulations of a craps game materially diverges (p <.001) from it's estimate after 10^10 passes. I went to a crypto level rnd gen as a result. It's amazing what can be done with "microprocessors" these days.
-Marty
|
|
|
|
|
Now I see what happens when you sleep through probability and statistics.
You both make excellent points and I will incorporate one of your proposed solutions in the next update, which will probably be today or tomorrow since it looks like the article posting got messed up a bit. Thanks for the feedback!
Kevin
"Semicolons in a programming language are like mother's milk."
|
|
|
|
|
Kevin,
Here's a hack that incorporates Jeffrey's exclusion and eliminates likely very long while() loops that can occur when lBound is close to uBound.
protected int GetCryptographicRandomNumber(int lBound, int uBound)
{
uint urndnum;
byte[] rndnum = new Byte[4];
if (lBound == uBound-1)
return lBound;
uint xcludeRndBase = (uint.MaxValue - (uint.MaxValue%(uint)(uBound-lBound)));
do {
rng.GetBytes(rndnum);
urndnum = System.BitConverter.ToUInt32(rndnum,0);
} while (urndnum >= xcludeRndBase);
return (int)(urndnum % (uBound-lBound)) + lBound;
}
-Marty
|
|
|
|
|
Nice hack. Passed my unit tests, so it's in the code now. I'll be sending out the update later this evening. Thanks!
Kevin
"Semicolons in a programming language are like mother's milk."
|
|
|
|
|
I see talk of a version that has some of the improvements discussed.
(checkboxes, crypto random, exclusion fixed for .NET 1.1, performance, ASP. NET..)
am I missing something or is that version not posted yet - (not trying to rush you - but when it is done, this is where it will be posted, right ? )
|
|
|
|
|
I sent the update in to be posted, but it looks like both the article and the downloads got a little screwed up. I'll try and get it fixed today.
Kevin
"Semicolons in a programming language are like mother's milk."
|
|
|
|
|
You may want to replace the Random class with RNGCryptoServiceProvider .
From the MSDN documentation[^]:
"To generate a cryptographically secure random number suitable for creating a random password, for example, use a class derived from System.Security.Cryptography.RandomNumberGenerator such as System.Security.Cryptography.RNGCryptoServiceProvider."
The reason is, apparently, that the System.Random class uses a linear algorithm which has some not-quite-perfect randomness properties that a password cracker might be able to exploit. The cryptographic generator uses a nonlinear algorithm which doesn't suffer from this deficiency.
Jeffrey
Everything should be as simple as possible, but not simpler. -- Albert Einstein
http://www.extremeoptimization.com/
|
|
|
|
|
OK, OK...you got me. I was being lazy. So, of course, I will change the implementation. Man, my to-do list for this code just keeps getting longer...
Kevin
"Semicolons in a programming language are like mother's milk."
|
|
|
|
|
1) Congratulations
2)On NET FRAMEWORK 1.1, the exclusions option doesn't run.
The ToString() method on a char array returns System.Char[] ...
Here the modification :
if ( ( null != this.Exclusions )&& (true == hasExclusions) )
{
foreach(char item in this.Exclusions)
{
if(item.Equals(nextCharacter))
{
nextCharacter = GetRandomCharacter(charFlag);
break;
}
}
}
Best Regards
|
|
|
|
|
Thanks! I just got Visual Studio.NET 2003 and will be making a few modifications to the code with it and test it using .NET 1.1. I should have a new posting in a few days.
Kevin
"Semicolons in a programming language are like mother's milk."
|
|
|
|
|
Another way to fix that is to construct a string from the char array:
string exclude = new String(this.Exclusions);
...
while ( -1 != exclude.IndexOf(nextCharacter) )
...
Regards.
|
|
|
|
|
1) Maybe I'm just missing something, but why are the upper and lower bounds (e.g. UBoundDigit ) public if the actual character array is not?
2) Although I realize I keep asking for more (sorry), it would be nice to be able to specify PwdCharMask for the characters in between the first and the last. For example, I usually would not want symbols. Also, since PwdCharMask can be used as a flag, i.e. more than one of its members can be applied at once, what is the use of any as opposed to upper | lower | digit | symbol ? Wouldn't it just make things harder, internally? Well, I suppose if it was one or the other, then it wouldn't, but if you want upper | lower (exluding symbols and numbers) then it makes more sense to simply use upper | lower | digit | symbol , no?
Thanks again for a great program!
-Domenic Denicola- [CPUA 0x1337]
“I was born human. But this was an accident of fate—a condition merely of time and place. I believe it's something we have the power to change…”
|
|
|
|
|
Domenic [Geekn] wrote:
1) Maybe I'm just missing something, but why are the upper and lower bounds (e.g. UBoundDigit) public if the actual character array is not?
You're not missing something...I am. It's called common sense!
Domenic [Geekn] wrote:
2) Although I realize I keep asking for more (sorry), it would be nice to be able to specify PwdCharMask for the characters in between the first and the last. For example, I usually would not want symbols. Also, since PwdCharMask can be used as a flag, i.e. more than one of its members can be applied at once, what is the use of any as opposed to upper | lower | digit | symbol? Wouldn't it just make things harder, internally? Well, I suppose if it was one or the other, then it wouldn't, but if you want upper | lower (exluding symbols and numbers) then it makes more sense to simply use upper | lower | digit | symbol, no?
Any is just intended to be a syntactic shortcut for lazy programmers like myself. Which answers your question about the first and last character restriction. Now that you've called me on it, I'll have to respond appropriately! Consider it addressed in the next update. And don't feel bad about asking for more...I like the feedback and want to produce something useful. All your feedback is welcome!
Domenic [Geekn] wrote:
Thanks again for a great program!
You're very welcome and I should have an update done by early next week...I've got a lot going on this weekend, otherwise the update would come sooner. And, I have another article in the works. Stay tuned!
"Semicolons in a programming language are like mother's milk."
|
|
|
|
|
Kevin Stewart wrote:
You're very welcome and I should have an update done by early next week...I've got a lot going on this weekend, otherwise the update would come sooner. And, I have another article in the works. Stay tuned!
Woohoo! I'm looking forward to it!
-Domenic Denicola- [CPUA 0x1337]
“I was born human. But this was an accident of fate—a condition merely of time and place. I believe it's something we have the power to change…”
|
|
|
|
|
May I suggest the use of checkboxes instead of radio buttons for the generator program itself? THis would allow one to exclude symbols but include numbers, etc... much more control, and makes a bit more intuitive sense too. Great program!
-Domenic Denicola- [CPUA 0x1337]
“I was born human. But this was an accident of fate—a condition merely of time and place. I believe it's something we have the power to change…”
|
|
|
|
|
Good suggestion! Honestly, I just whipped the interface together to demonstrate how the class is used but you're right about the checkboxes. I'll update the UI. Glad you like the code!
Kevin
"Semicolons in a programming language are like mother's milk."
|
|
|
|
|
Enough said....
|
|
|
|
|
Aaaarrggghhh!!!
I'll get them fixed. Thanks!
Kevin
"Semicolons in a programming language are like mother's milk."
|
|
|
|
|
Links are fixed.
cheers,
Chris Maunder
|
|
|
|
|
Thanks, Chris!
Kevin
"Semicolons in a programming language are like mother's milk."
|
|
|
|
|