• The Dude (unregistered)

    when I ran this in our dev environment, it returned 'bushLovesPigs'. whta the fluff?

    prist.

  • (cs)

    I'm especially fond of the clever new abbreviation of 'password' in the function name: 'Pswd'

  • user (unregistered)

    Second?! not sure what it does(n't) do ohter than a ton of iterating

  • Ryan (unregistered)

    The code looks like crap, but it seems to generate random passwords when I call it.

  • Iluvatar (unregistered)

    I must be missing it; looks like is does what it says. But then again, I'm not too familiar with php.

    Can someone 'ruin the fun' for me?

    (fifth, I guess)

  • JT (unregistered) in reply to Iluvatar

    Yeah... all looks ok to me too, but I've not done any php for a couple of years...

  • Simen (unregistered)

    The “!strpos($disallow, chr($n)))” part is a bit funny...

    CAPTCHA: doom

  • (cs)

    Yeah, it should do what it's supposed to be. Unless you set $usaalpha and $usenumeric to false. Than it does an infinite loop, I think. It does it in a funny way though.

    Addendum (2007-03-09 09:51): Oh, I should have looked more closely. No ininite loop at all.

  • trav (unregistered)

    Everybody knows that the most commonly used passwords are love, sex, secret, and god.

  • kanna (unregistered)

    I don't know any PHP really, but as per yesterday's example, with the !strpos check it will allow "I" to be in the password. (strpos($disallow, "I") = 0)

  • James (unregistered)

    Appears to generate a random password every time here too :)

  • cruise (unregistered)

    Yup, same here. Works fine for me too. Only possible issue is it's inefficiency and that allowing numbers/letters doesn't guarantee their presence, though that is of debatable desirability.

  • Ed (unregistered)

    This site would benefit by deleting every post containing "first", "second", "fifth", or "captcha:". It's gone beyond obnoxious and worst of all, people who frequent this site ought to be capable of recognizing this pattern of behavior and learning from it how NOT to behave. But there are a certain few in every single thread who do it anyway, bringing the quality of this site down to their ignorant level.

  • (cs)

    Looks rather inefficient to me (constructing a set of allowed chars and picking from that would save failed loops), but not a total WTF. In fact, I like that he excludes chars easily confused with others. Perhaps I'm missing something by not knowing PHP?

  • (cs)

    Sure, the code could be better - but it's not a WTF!!

  • (cs)

    The problem's with the check !strpos($disallow, chr($n)), which was supposed to disallow any of the illegal characters. strpos returns false (that is, 0) if the search string is not found, but it can also return 0 if the search string is at position 0. So, if the character "I" is chosen randomly, since it's the first char in the disallow string, will pass the strpos test.

    Programmer should have used the === comparison to check for boolean false. Besides that, the function seems to work as expected. Not the grandest WTF ever.

  • Unomi (unregistered)

    There are some real WTF's in here... If setting usealpha to false, there some I's are discovered in the numeric string (wich makes it not numeric anymore).

    Besides that, the programmer uses ordinances for the characters and a second later reverts them back to characters.

    It seems like a lot of hocuscadabra to me.

    Captcha: wigwam (the sound of a programmers head, when slammed to his desk)

  • JL (unregistered) in reply to Simen
    Simen:
    The “!strpos($disallow, chr($n)))” part is a bit funny...
    Is that the problem? I don't know PHP, but the strpos PHP manual page says that it can return "false" values, like zero, which it would return when the chr($n) is at the beginning of the string. So I guess the function will allow passwords containing the first character of $disallow, which is the character "I".

    I guess the function could run long if you get a very bad sequence of random numbers, too.

    But there must be something worse than these two things that I'm not seeing, as it hardly seems worthy of an article otherwise.

  • Erstarr (unregistered)

    ... so this generator is optimized for people that write their passwords down

  • Rick (unregistered)

    Ed, it's a technology site that's programming-oriented... you can't expect it to be a lot better than slashdot.

    The only problem I can see with the function is that with the right options it will take (almost) any ASCII characters between 0 (the lowest character) and z (the highest character), which includes several symbols between 9 and A as well as Z and a. The comments say that it generates alpha-numeric passwords so that.

    Besides that I would say it's better than failure.

    CAPTCH: who cares. it's not funny. you aren't special. do something useful.

  • Bob (unregistered)

    function generatePassword($length, $numeric) { $possibles="abcdefghijkmnopqrstuvwxyz"; // Edit to suit if($numeric) $possibles.="0123456789"; // Edit to suit for($i=0; $i<6; $i++) $password.=substr($possibles, rand()%length($posibles), 1); return $password; }

    ... or something...

  • (cs)

    Unless we are all missing something huge - this isn't even worthy of being a side-bar WTF.

    Poor show!!

  • (cs) in reply to Ed
    Ed:
    This site would benefit by deleting every post containing "first", "second", "fifth", or "captcha:". It's gone beyond obnoxious and worst of all, people who frequent this site ought to be capable of recognizing this pattern of behavior and learning from it how NOT to behave. But there are a certain few in every single thread who do it anyway, bringing the quality of this site down to their ignorant level.

    By those rules, this post would be deleted. Wouldn't surprise me in the least.

    Captcha= (wait mine is missing, the site hates me so I don't get one)

  • (cs) in reply to Rick
    Rick:
    The only problem I can see with the function is that with the right options it will take (almost) any ASCII characters between 0 (the lowest character) and z (the highest character), which includes several symbols between 9 and A as well as Z and a. The comments say that it generates alpha-numeric passwords so that.

    No, it has a set of checks just to prevent that!

  • (cs)

    OK, this isn't the worst WTF on the block, but there are problems. It's terribly inefficient, generating characters and throwing most away, especially when creating a numeric-only string. Secondly, it makes the same mistake as the WTF from yesterday: it uses strpos incorrectly so that the first character in the $disallow is not handled - 'I' is supposedly disallowed, but it actually may appear in any password, numeric or otherwise.

    EDIT: beaten to the punch. Oh, well.

  • (cs) in reply to trav
    trav:
    Everybody knows that the most commonly used passwords are love, sex, secret, and god.
    Damnit, you stole my post! :P
  • wilkeson (unregistered) in reply to Erstarr
    Erstarr:
    ... so this generator is optimized for people that write their passwords down
    Well, in theory you would have to give the password to the end-user somehow.

    I'm sure in some cases you would give it to them over the phone, but kudos to people who can memorize a randomly generated password the first time they hear it.

  • php-illiterate (unregistered)

    Disclaimer: my PHP knowledge is non-existent and I'm making the assumption that .= is used for String concatenation. If it's to add array elements my guess only changes slightly ;)

    I'm trying to figure out the use of the flags and the if block. The if-block combined with a "true" on a disallow flag is the WTF from how I see it (the rest is just sloppy coding).

    The method allows generation of all alphanumeric characters (upper- and lowercase), even if its flagged not to.

    If its numeric, and .= means string concatenation, then the numeric char is also added, no matter the flag settings unless the generated character contains the string "ILOQZiloqz0123456789" (don't ask and I promise I won't either).

    In short, I don't get the whole disallow function, which is flawed from my PHP-illiterate perspective. And the method, well the method is ehmmm, well, it is... Nuff said.

    Ps. I think I'll start using Pswd as an acronym for password from now on as well, I love the creativity of it. Then again, better not, I value my job too much for that.

  • (cs)

    I think this WTF was for people like me. I usually don't understand them and have to read the comments, so I feel stupid. Now everyone feels stupid!

  • Unomi (unregistered)

    Here's my version, a quickie:

    function get_random_string($size, $alpha, $numeric) {

    while ($string_length < $size) {

  $x = 1;
  $y = 3;

  $pos = rand($x,$y);

  if ($pos == 1 && $numeric === TRUE) {

  	$a = 48;
  	$b = 57;
  }
  elseif ($pos == 1) {

  	$a = 65;
  	$b = 90;
  }

  if ($pos == 2 && $alpha === TRUE) {

  	$a = 65;
  	$b = 90;
  }
  elseif ($pos == 2) {

  	$a = 48;
  	$b = 57;
  }

  if ($pos == 3 && $alpha === TRUE) {

  	$a = 97;
  	$b = 122;
  }
  elseif ($pos == 3) {

  	$a = 48;
  	$b = 57;
  }

  $part=chr(rand($a,$b));

  $string_length++;
  $string = $string.$part;
}

return $string;

    }

    • Unomi -
  • nobody (unregistered)

    The behavior will depend on the version of PHP, as the rand function changed syntax, and some versions have very poor randomness.

    I've never used PHP, but about 30 seconds on google and the php web site was enough to find this.

    And of course, there is the problem that people don't remember random passwords well, so they write them down, and that is less secure than using a password filtering tool to only allow "secure" passwords.

  • (cs)

    The whole idea of "oh, let me generate a random number, then check to see if it's valid or not" is a dangerous idea. Every so often you get an inordinate number of consecutive invalid numbers.

    Simulation and Modeling class in college, final project was to simulate the on-campus parking catastrophe. One dude thought it would be a good idea to have each car randomly decide which of the 15 lots it wanted to go to, and decide again if that lot was full. Things slowed down quite a bit once there was only one open lot....

    /EDIT: Yeah, in the real world you'd go to the full lot, realize it was full, then move on to the next. For simplicity's sake, we were assuming the guy could see all the lots as he approached campus, and would "instantly" know where he was going...

  • Aaron (unregistered)

    /* This section takes the ordinal values (I think that's the ASCII codes) of the characters to do numeral comparisons later on./ $uczchar = ord("Z"); $ucachar = ord("A"); $lczchar = ord("z"); $lcachar = ord("a"); $n0char = ord("0"); $n9char = ord("9"); // /////////////////////// / This block checks to see if the script should use letters -- that parameter is provided as a function parameter (true or false). If they WON'T use letters, then loop from the ordinal values of 'A' (UCA = upper case A) to 'Z' and append the letters to the "Disallow" list. Repeat the same for lowercase letters. / if (!$usealpha) { for ($n=$ucachar;$n<=$uczchar;$n++) { $disallow .= chr($n); } for ($n=$lcachar;$n<=$lczchar;$n++) { $disallow .= chr($n); } // /////////////////////// / The logic here is weird. If they're not-not-using letters (so if they ARE using letters) then check to see if they're NOT using numbers, and if they aren't, then iterate from 0 to 10 (what's kind of weird here is that he's using the literal numerals rather than the ordinals of the numerals that he set above. PHP doesn't really care either way, but a more strongly-typed language would complain big time about it.) / } else { if (!$usenumeric) { for ($n=0;$n<10;$n++) { $disallow .= $n; } } } // //////////////////////////////// / Get the max ordinal of 'z', 'Z', and '9', and the minimum ordinal of 'a', 'A' and '0'. What's weird here is that all six of those tested values are statically set at the beginning of the script, so he should KNOW beforehand which one will be the smallest. Using the max and min functions would really only be useful if the tested values were set dynamically based on whether or not numbers or letters were used. / $maxord = max($uczchar,$lczchar,$n9char); $minord = min($ucachar,$lcachar,$n0char); // /////////////////////////////// / Ok. Initialize the output string ($pwd) to a blank string. While that string's length is less than the maxlength (function param), process the next character. This much is good, and works, so far. / $pwd = ""; while (strlen($pwd) < $nlen) { // ///////////////////////////////////// / $n gets set to a random number between the min and max ord (set above). If $n is within the bounds of 'A'-'Z' or 'a'-'z' or '0'-'9' and the character version of that number is not in the Disallow list (set at the beginning), then append the character version of it to the password. When done, return the password. */ $n = rand($minord,$maxord); if ((($n >= $ucachar && $n <= $uczchar) || ($n >= $lcachar && $n <= $lczchar) || ($n >= $n0char && $n <= $n9char)) && !strpos($disallow, chr($n))) { $pwd .= chr($n); } } return $pwd; } // /////////////////////////////////////

    • in PHP, I'm pretty sure you can do test-operators (>=, <= etc.) with characters, so there's no need to convert to numerals. (I think you can even use the random function)

    • He probably could have used a regular expression to test for the "disallowed" values instead of a complicated strpos list.

    • the "maxord" / "minord" thing is broken because it will always provide a range between ordinals 48 ['0'] and 122 ['z'], regardless of whether or not "useAlpha" or "useNumeric" is set.

    • the "!isAlpha" and "!isNumeric" logic is confusing to read, and should be revised.

  • Unomi (unregistered)

    Oh, and to be complete, leaving out the confusing characters:

    Insert this after the $part declaration:

    if (strpos('ILOQZiloqz', $part) !== FALSE) { $part = get_random_string(1, $alpha, $numeric); }

    Than it works.

    • Unomi -
  • some guy (unregistered) in reply to Unomi
    Unomi:
    Here's my version, a quickie:

    function get_random_string($size, $alpha, $numeric) {

    while ($string_length < $size) {

  $x = 1;
  $y = 3;

  $pos = rand($x,$y);

  if ($pos == 1 && $numeric === TRUE) {

  	$a = 48;
  	$b = 57;
  }
  elseif ($pos == 1) {

  	$a = 65;
  	$b = 90;
  }

  if ($pos == 2 && $alpha === TRUE) {

  	$a = 65;
  	$b = 90;
  }
  elseif ($pos == 2) {

  	$a = 48;
  	$b = 57;
  }

  if ($pos == 3 && $alpha === TRUE) {

  	$a = 97;
  	$b = 122;
  }
  elseif ($pos == 3) {

  	$a = 48;
  	$b = 57;
  }

  $part=chr(rand($a,$b));

  $string_length++;
  $string = $string.$part;
}

return $string;

    }

    • Unomi -
    That's pretty... bad.
  • ASDF (unregistered)

    Coded by a Swede. Maxord and minord mean "max word" and "min word", Ord obviously meaning word.

    But then Dimitiy doesn't sound Swedish at all.

  • Unomi (unregistered) in reply to some guy

    Well... have you tried it?

    I did and it works. So what is bad about it? Always here to learn something about bad code.

    • Unomi -
  • ML (unregistered)

    So here we have well-documented code working mostly as advertised. Maybe a small, easy-to-make bug with strpos(), bad behavior when you give it nonsense arguments, not the most efficient code ever written, but its not that bad really. It actually has some pretty redeeming positives too, such as disallowing easily confused characters such as 1l, 0OQ, Z2, and the like.

  • (cs) in reply to ASDF
    ASDF:
    Coded by a Swede. Maxord and minord mean "max word" and "min word", Ord obviously meaning word.

    But then Dimitiy doesn't sound Swedish at all.

    Actually, I believe the Swedish translation of maxord (if I remember my Muppet Chef correctly) is "a large, grassy fiefdom", while minord means "a small, grassy fiefdom".

    What they're doing in this code I haven't the foggiest.

  • durizzio (unregistered)

    the wtf here is not the code itself, but appearance of this code here. recursive wtf i would say. post modernism.

  • (cs)

    Aww. Alex, we 1xFCwEAN9e you too. hugz

  • some guy (unregistered) in reply to Unomi
    Unomi:
    Well... have you tried it?

    I did and it works. So what is bad about it? Always here to learn something about bad code.

    • Unomi -
    You keep testing for conditions in a loop that don't change in that loop.

    Also, you could try to put the ranges for the second rand() call in an array whose values are based on those conditions, and use the first rand() call as an index. I'm not very clear, but I hope you get the gist.

    And there are magic numbers in the assignments to $a and $b. I happen to know what those numbers mean, but a maintainer other than you may have trouble seeing those numbers for what they are at a first glance.

    I realize I was a bit harsh in my first comment, so I'm sorry for that.

  • Scotty (unregistered)

    I think there is a problem here:

          if (!$usenumeric) {
          for ($n=0;$n<10;$n++) {
              $disallow .= $n;
          }
      }

    It prevents me from ever randomly getting my favorite password characters like ^A, ^B, ^C, etc.

  • BlueCollarAstronaut (unregistered) in reply to swapo
    swapo:
    Yeah, it should do what it's supposed to be. Unless you set $usaalpha and $usenumeric to false. Than it does an infinite loop, I think. It does it in a funny way though.

    Addendum (2007-03-09 09:51): Oh, I should have looked more closely. No ininite loop at all.

    I think you are on to something. The loop probably won't ever be infinite (unless the "random" values happen to be horribly un-random), but he is randomizing a range of values -- many of which will not be allowed -- until a string of certain length is constructed. At best, it's inefficient, at worst, it could approach infinite iterations if you happen to run into a streak of bad luck with the randomizer.

    Any way it's approached, there are at least some invalid values, but if the $usealpha and/or $usenumeric are not both true, then things get more "risky". How many iterations would it take to build a 5 character password? Somewhere between 5 and eleventy-billion :)

  • (cs)

    considering I've never written a lick of PHP, that code was surprisingly readable.

    I'm not sure why s/he's using the ord/chr thing. That's pretty WTFy.

    Of course the real WTF is that code isn't portable: if it were running on a EBCDIC based system, A..Z do not have consecutive ord$ values. it's A..I gap J..R gap S..Z

  • Cotillion (unregistered)

    I think that the people complaining about the first, second, captcha stuff are taking themselves a little too seriously to be reading this site.

    Captcha: Sanitarium: where some of the complainers may find help.

  • ML (unregistered) in reply to Aaron
    Aaron:
    The logic here is weird. If they're not-not-using letters (so if they ARE using letters) then check to see if they're NOT using numbers
    You didn't read the comments, did you. Its doing EXACTLY what the comments said it would:
    usenumeric - Boolean argument that specifies whether to use numeric digits in the password. True - password will contain numeric digits. False - password will only contain alpha characters. Ignored if usealpha is false, so if both are false, passwords will be numeric only.
    Nothing weird about that at all. It's defensive programming.
  • (cs) in reply to Unomi

    Unomi, your implementation assumes an ASCII character set. I ran it on my EBCDIC-based mainframe and it generated a lot of invalid passwords.

  • htg (unregistered) in reply to ASDF
    ASDF:
    Coded by a Swede. Maxord and minord mean "max word" and "min word", Ord obviously meaning word.

    But then Dimitiy doesn't sound Swedish at all.

    Look up the meaning of 'ordinal' (well, one of the meanings).

  • Scotty (unregistered) in reply to Scotty
    Scotty:
    I think there is a problem here: 
          if (!$usenumeric) {
          for ($n=0;$n<10;$n++) {
              $disallow .= $n;
          }
      }

    It prevents me from ever randomly getting my favorite password characters like ^A, ^B, ^C, etc.

    To clarify, should it not have been:

    for ($n=$n0char;$n<=$n9char;$n++) { $disallow .= chr($n); }

    Or does PHP convert the number 0 into the character '0' in the given code? If not, then when digits are not supposed to be allowed, they would still be allowed.

Leave a comment on “Generating Secure P455w0rd5”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article