For while loop



  • I've posted about it in other forums, and now that I've discovered the wonderful world of WTF, I must share it here as well.

    One of our customers had a requirement that their users not be allowed to use really dumb PINs like 1111, 1234, 4321, etc... can't be same number 4 times, can't be ascending or descending runs.  Here's the lovely bit of ECMAScript that our friends offshore cooked up to satisfy the requirement:

     
              var index = 0;
              for( ; index < 4 ; )
                {
                current = ToInteger(new_pin.substring(index, index));
                if(index == 0)
                  previous = current;
                else
                  {
                  if(all_the_same == 1 && previous != current)
                    all_the_same = 0;

                  if(index = 1)
                    direction = previous - current;
                  if(((direction != 1) || (previous != (current - 1)) && ((direction != -1) || (previous !== (current + 1))))
                      consecutive = 0;
                  }
                if(consecutive == 0 && all_the_same == 0)
                  {
                  PinIsCompliant = ToString(true);
                  break;
                  }
                index ++;
                }

     

    Not only is that some whacky use of a for loop, but the thing got false negatives for stuff like 1357 or 0369



  • There are only 10,000 pins anyway, and they want to limit the keyspace more...hmm...



  • Just because I'm wondering now, how would the rest of you done it?

    Personally, I would have just had a lookup table or a local hash and pulled a:

     

    [code]if (badPins.contains(newPin)) {
      //dont use this pin
    }[/code]
     



  • Can anyone explain how ToString(true) differs from "true"?

    In any event, why would one call the primitive operators?  xxxxx.toString() and parseInt() are perfectly good.

    Apparently they've also never discovered charAt(), and I don't even think substring(i,i) returns anything but an empty, since the definition says "starting at the first argument, up to, but not including, the index of the second argument."  I'll have to test that, though.



  • @Hellz99 said:

    Just because I'm wondering now, how would the rest of you done it?

    Personally, I would have just had a lookup table or a local hash and pulled a:

     

    [code]if (badPins.contains(newPin)) {
      //dont use this pin
    }[/code]
     

    I disagree! With integers, there are 10 of the same (0000...9999) and 14 "runs" (0123, 1234, 2345, ... 6789, 9876, 8765, ..., 3210) - for a total of 24 out of 10,000 items that must be disallowed.

    This requires a much more enterprisey solution, in case someone changes it from integers to doubles. A mere hash won't do. Perhaps a series of inverted logic if-else's. Perhaps some XML. Perhaps files named for each of the disallowed combinations (then we get to use FILE_NOT_FOUND in a meaningful way!) Think of the possibilities... *smirks*

     



  • You're right progguy

    current = ToInteger(new_pin.substring(index, index)); 

    is just going to return the charAt(index) and then turn it into an int. 

    I guess if the original scope was the algorithm handled pins of larger num of digits, then maybe I can understand why you would do it with the for-loop....but not if the constraint was always going to be 4 digit. 



  • that's the same PIN I have on my luggage

    I don't see any reason to complicate it any further than matching against a list of bad pins.  No, it's not difficult to define the bad pins algorithmically, but there's only 26 bad pins out of 10,000 possible (and that's if you consider 0 to be below 1 AND above 9 in sequence).

    It's relatively futureproof, too -- if the requirements get changed so that '1337' gets banned for being so 'leet, you just have to add that pin to the list, instead of changing the validation logic to be "if isAllSameNumber or isAscendingSeq or isDescendingSeq or isLeet"...


     



  • Uh huh


    Apparently they've also never discovered charAt(), and I don't even think substring(i,i) returns anything but an empty, since the definition says "starting at the first argument, up to, but not including, the index of the second argument."  I'll have to test that, though.

    Yup, substring(a,b) is (from, up-to-but-not-excluding), or in range notation: [a,b)

    charAt is more like it. Although frankly my solution for a similar problem is a bit more of a WTF.

    function reduce (old_char, in_char, o_class, c_class) {
    var delta;
    var p_off;
    switch (c_class) {
    case 'lower':
    in_char = in_char.toUpperCase();
    old_char = old_char.toUpperCase();
    case 'upper':
    if(o_class != 'upper' && o_class != 'lower') { return false; }
    delta = Math.abs(in_char.charCodeAt(0) - old_char.charCodeAt(0));
    return (delta < 2 || delta > 24);

    case 'punct':
    p_off = ")!@#$%^&*(".indexOf(in_char); //Punctuation for each keyboard number in ASCII order
    if(p_off < 0) { return false; }
    in_char = String.fromCharCode(p_off + "0".charCodeAt(0)); //Add ASCII '0' back to it
    case 'digit':
    if(o_class == 'punct') {
    p_off = ")!@#$%^&*(".indexOf(old_char);
    if(p_off < 0) { return false; }
    old_char = String.fromCharCode(p_off + "0".charCodeAt(0));
    } else if (o_class != 'digit') { return false; }
    delta = Math.abs(in_char.charCodeAt(0) - old_char.charCodeAt(0));
    return (delta < 2 || delta > 8);

    default:
    return false;
    }
    }
    It's designed to be called from a loop that iterates over the password. It returns true when consequetive letters are "next" to each other (by one or two). If I had more time, I would have also had it check keyboard order (QWERTY only) but I ran out of time to play with it.


  • What happens when the length of the pin changes?

     I'm not really familar with how regex works in your language, but in perl I would start with...

     

    perl -w -e "$i=9012; if('1234567890123456789' =~ m/$i/){ print $i;}"

    and

    perl -w -e "$i=2109; if('09876543210987654321' =~ m/$i/){ print $i;}"

     

    or something similar with Instr if that's what you have...
     



  • I'd try this ( ruby ) 

    def checkpin pin
       /^[0-9]{4}$/.match(pin) && !/^(.)(\1)$/.match(pin) && !"0123456789 9876543210".include?(pin)
    end

    I guess it's another case of unnecessary generalization. You never now, one day we'll count in hexadecimal, and you'll see who gets the last laugh !



  • I'd post my fix if somebody hadn't brought our VOB down at 4:30.....  Basically took the same approach they did though (minus the brokenness and hideous misuse of a for loop, of course).  If diff = dig[n]-dig[n+1] is the same for all digits, AND diff is in {-1, 0, 1} (the part they ignored), then PIN is invalid.

    The regular expressions would have been an excellent solution though.  Don't think ECMAScript actually supports them, but we have other frameworks setup that would allow for us to do it fairly easy.  And at that point it'd be a lot easier to enforce other pin rules in the future.

     As to the number of digits, I think there was a written statement from the customer saying they always wanted 4 digits, but of course that'd be subject to change...we actually allow them to configure the maximum, so i probably would have thought to use that when i rewrote it.  Don't remember though.
     



  • Duh. In fact there is not much differences between ECMAScript and Javascript, AFAIK. Javascript accesses different objects while in a browser, which makes sense since their purpose is to be embedded. Still, I'm pretty sure regexp is part of the core of both.

     This should work :

    function checkpin(pin){
    return pin.match(/^[0-9]{4}$/) && !pin.match(/^(.)(\1)$/) && "0123456789 9876543210".indexOf(pin) == -1;
    };



  • @Benanov said:

    There are only 10,000 pins anyway, and they want to limit the keyspace more...hmm...

    This is the first thing that struck me, and you've put it far more succinctly then I would have.

    Surely this doesn't have a positive impact on security?

    As much as a WTF that code is, the requirement itself is also a pretty big WTF. 

    Edit: pretty big huge



  • Shouldn't !pin.match(/^(.)(\1)$/) be !pin.match(/^(.)\1{3}/) ?



  • The four digit test is simply (n mod 1111) != 0 and the digit run test is ((n - 123) mod 1111) != 0.



  • @pinkduck said:

    Shouldn't !pin.match(/^(.)(\1)$/) be !pin.match(/^(.)\1{3}/) ?

    Whoops. Yep, 3 times and the parenthesis is not necessary.

    @TGV said:

    The four digit test is simply (n mod 1111) != 0 and the digit run test is ((n - 123) mod 1111) != 0.

    (0xABCD - 0x123) % 0x1111 == 0

    Great, you even solved the "one day we might wake up having three more fingers on each hand" problem :D

    function checkpin(pin){
    return pin.match(/^[0-9]{4}$/) && pin % 1111 != 0 && (pin - 123) % 1111 != 0 && (pin - 3210) % 1111 != 0 ;
    };

    Talking about newbees focusing on number representation rather than number properties, I'd better eat my own dog food :$



  • How about:

      var valid = false
      var diff = 0
      for(i=0; i<3; i++)
      {
        var newdiff = new_pin.charCodeAt(i) - new_pin.charCodeAt(i+1)
    
        if(Math.abs(newdiff) > 1)
        {
          valid = true
          break
        }
    
        if((i != 0) && diff != newdiff)
        { 
          valid = true
          break
        }
    
        diff = newdiff
      }
    


  • @Hellz99 said:

    Just because I'm wondering now, how would the rest of you done it?

    Personally, I would have just had a lookup table or a local hash and pulled a:

     

    [code]if (badPins.contains(newPin)) {
      //dont use this pin
    }[/code]
     

    Might be the easiest way to do it.

    pinIsCompliant =  ("0000x1111x2222x3333x4444x5555x6666x7777x8888x9999x0123x1234x2345x3456x4567x5678x6789x3210x4321x5432x6543x7654x8765x9876".indexOf(pin) ==-1);



  • @ammoQ said:

    Might be the easiest way to do it.

    pinIsCompliant =  ("0000x1111x2222x3333x4444x5555x6666x7777x8888x9999x0123x1234x2345x3456x4567x5678x6789x3210x4321x5432x6543x7654x8765x9876".indexOf(pin) ==-1);

    That gets my vote! 



  • @Hellz99 said:

    Just because I'm wondering now, how would the rest of you done it?

    Personally, since even 0369 is invalid because it is considered a run yet the numbers are not 1 off each other we simply need to see if we have a repeating pattern of difference. 

    For a string 1 - n characters in length

    Difference = Char(x) - char(x+1) 

    If Difference <> PastDifference

        It is a good pin

    Increment to next character.


    If you never get to the good pin code it is bad.  This covers things like 1111 and 1234 and also 2468 and 0369.

    The one thing this doesn't catch is layout runs such as things straight down the middle of the keypad 8520.
     



  • @TGV said:

    The four digit test is simply (n mod 1111) != 0 and the digit run test is ((n - 123) mod 1111) != 0.

    You win.

    To those of you talking about limiting the number of PINs, you've gotta realize they're taking out 24 from 10000.  They hang up on the guy after 3 failed attempts, so if the hacker assumes you're stupid, he's gotta make 8 phone calls to brute force you.  With this rule in place, he's got to make 3326 calls.

    // The real tragedy is that they don't ever permanently lock the guy's account, so making 3326 calls will actually work....but I'm sure they've got some other mechanisms in place to prevent/detect that sort of thing.

    Here's my solution: 

              if(new_pin.length == 4){
                for( index=0 ; index &lt; 4; index++){
                  current = parseInt(new_pin.substring(index, index + 1), 10);
                  if(index == 0){
                    previous = current;
                  }
                  else if(index==1){
                    direction = previous - current;
                    if(Math.abs(direction) &gt; 1){
                      PinIsCompliant = 'true';
                      break;
                    }
                  }
                  else if((previous - current) != direction){
                    PinIsCompliant = 'true';
                    break;
                  }
                 
                  previous = current;
                }
              }
     Why does new_pin.substring(index, index + 1) do the same thing as new_pin.substring(index,index) ?



  • You are all missing the point the OP made right at the end, he wants to also limit runs like 0369.  This will be more than 24 pins total, but not much more.

    It isn't a change by 0 or 1 he is looking for, it is a change by a consistent number whether the change is by 0, 1, 2 or 3 so even 0246 and 1357 would be invalid.

    My solution is the only one to satisfy that requirement so far.

     



  • While we're in there, let's remove the palindromic PINs.

    Thanks for the tips on the [a,b) thing, too...I always forget that notation.  It also doesn't work when I use it in programs, but I think that's an entirely different problem.  Dunno, maybe it works in Perl.... ;)



  • @KattMan said:

    You are all missing the point the OP made right at the end, he wants to also limit runs like 0369.  This will be more than 24 pins total, but not much more.

    It isn't a change by 0 or 1 he is looking for, it is a change by a consistent number whether the change is by 0, 1, 2 or 3 so even 0246 and 1357 would be invalid.

    My solution is the only one to satisfy that requirement so far.

     

    OK... 

    pinIsCompliant = 
    ("0000x1111x2222x3333x4444x5555x6666x7777x8888x9999x0123x1234x2345x3456x4567x5678x6789x3210x4321x5432x6543x7654x8765x9876x0246x1357x2468x3579x0369x6420x7531x8642x9753x9630".indexOf(pin)
    ==-1);
     



  • @KattMan said:

    You are all missing the point the OP made right at the end, he wants to also limit runs like 0369.  This will be more than 24 pins total, but not much more.

     

    You misread.  The requirement was to find changes between -1, 0, and 1. 

    I was saying the code they wrote WTFishly identified changes by 2, 3, etc, as being invalid.  0369 is a valid pin.



  • @vt_mruhlin said:

    @KattMan said:

    You are all missing the point the OP made right at the end, he wants to also limit runs like 0369.  This will be more than 24 pins total, but not much more.

     

    You misread.  The requirement was to find changes between -1, 0, and 1. 

    I was saying the code they wrote WTFishly identified changes by 2, 3, etc, as being invalid.  0369 is a valid pin.

    Gah!  Ok I stand corrected.  Essentially I just re-wrote the previous code in a much better fashion while still outputting a to broad limitation. The other posters then have this right.



  • @vt_mruhlin said:

    false negatives for stuff like 1357 or 0369

    Why are those false negatives? They're still insecure PINs, very nearly as bad as 1234 or 4321. If I wanted to avoid insecure PINs, I'd certainly want to prevent them.



  • @CDarklock said:

    @vt_mruhlin said:

    false negatives for stuff like 1357 or 0369

    Why are those false negatives? They're still insecure PINs, very nearly as bad as 1234 or 4321. If I wanted to avoid insecure PINs, I'd certainly want to prevent them.

     

    "The customer is always right".  Yeah, I don't believe that crap anymore than you do.  I generally use "1337" as my pin for plenty of stuff (not my bank though, so don't bother trying), so I generally advocate letting users use dumb pins if they feel like it.  "Oh no! somebody listened to my voicemail! Now they'll know the wife wants me to pick up some milk on the way home!"



  • Holy crap that's complicated. Here's my solution:

    // string must be a 4-digit base-10 integer in ASCII; let the UI enforce that bit
    bool is_valid(char*string)
    {
    char same,run_up,run_down;
    same = run_up = run_down = 0;

    for(i = 1;i<4;i++)
    {
    same |= string[0]^string[i]; // will be zero only if all digits are identical
    run_up |= (string[i]-string[i-1])^1; // will be 0 iff all differences are 1
    run_down |= (string[i-1]-string[i])^1; // will be 0 iff all differences are 1
    }
    return same && run_up && run_down;
    }


  • @vt_mruhlin said:

    To those of you talking about limiting the number of PINs, you've gotta realize they're taking out 24 from 10000.

    // snip 1000+ lines //

    if pin == 1380
           message = "Please do not use the Boltzmann Constant."
    if pin == 6022
           message = "Please do not use Avogadro's Number."
    if pin == 6626
           message = "Please do not use Plank's Constant."

    // snip  //



  • @marvin_rabbit said:

    @vt_mruhlin said:

    To those of you talking about limiting the number of PINs, you've gotta realize they're taking out 24 from 10000.

    // snip 1000+ lines //

    if pin == 1380
           message = "Please do not use the Boltzmann Constant."
    if pin == 6022
           message = "Please do not use Avogadro's Number."
    if pin == 6626
           message = "Please do not use Plank's Constant."

    // snip  //

    if pin == 3482
          message = "Congratulations! you have guessed on of the 5 pins we allow!"

     



  • I haven't tried it out, but I can't see that this code ever does what it's supposed to or even work.

    Even if we ignore

    • the really bad use of the for loop
    • the fact that substring(index,index) doesn't return anything according to ECMAScript specification
    • the assignment instead of comparison in "if (index = 1)"
    • the use of !== even though we know that previous and current are the same type in "(previous !== (current + 1))"
    • the "ToString(true)"
    • the use of break instead of an extra condition in the for-loop "index < 4 && !PinIsCompliant"
    • the incorrect number of parentheses in "if(((direction != 1) || (previous != (current - 1)) && ((direction != -1) || (previous !== (current + 1))))"
    • the inconsistency in naming "all_the_same" vs. "PinIsCompliant"

    the logic is still faulty and will allow pins like 1234. Let's go through it step by step:

    new_pin = "1234";
    all_the_same = 1;
    consecutive = 1;
    index = 0;

    // Entering for-loop
    current = ToInteger(new_pin.substring(index,index)) = 1;
    previous = current = 1; // since index == 0
    index = 1

    // looping again
    current = ToInteger(new_pin.substring(index,index)) = 2;
    all_the_same = 0; //since all_the_same == 1 && previous != current (so far so good)
    direction = previous - current = 1 - 2 = -1; // since index == 1
    // here's the problem
    consecutive = 0;
    //since (((direction != 1) || (previous != (current - 1))) && ((direction != -1) || (previous != (current + 1))))
    // = (((-1 != 1) || (1 != (2 - 1)) && ((-1 != -1) || (1 != (2 + 1)))
    // = ((   true    ||   false       ) && (  false    ||      true       ))
    // = true
    PinIsCompliant = ToString(true) = "true"; // since consecutive == 0 && all_the_same == 0

    //For-loop is broken out of

     I can't explain the false negatives, but then again I can't explain it running at all.



  • Wow, yeah that code's totally broken.  When posting this, I just went and found the first instance of this loop that was committed.  They had actually done some "fixing" between then and when I worked on it.  The fact that they ever commited this stuff without any manner of testing is insane though.

    Employers, I know outsourcing seems like it will save you money, but not when you're getting code like this.

    // the substring(index, index) was one of the things that changed, which makes a little more sense now.
     



  • @marvin_rabbit said:



    if pin == 1380
           message = "Please do not use the Boltzmann Constant."
    if pin == 6022
           message = "Please do not use Avogadro's Number."
    if pin == 6626
           message = "Please do not use Plank's Constant."

    Better not allow Fibonacci's sequence either.
     



  • The real WTF is people saying this idea hampers security.

    Let's say there are O(100) "objectionable" pins.  Eliminating those makes a worst case brute force about 1% faster.  But if a significant portion of the population uses these pins, eliminating them dramatically increases the average case brute force attack time.  This is the same reason you shouldn't use dictionary words as a password.  There are a lot more "random" strings than words, and yet words are used as passwords more often.

    Security is hard, but it isn't rocket science.


  • @Jojosh_the_Pi said:

    Better not allow Fibonacci's sequence either.

    Especially after The Da Vinci Code!



  • It's extremely broken. I looked at it some more and realized that even if you fix the faulty logics, it still won't work.

    correct logic: if(((direction != 1) || (previous != (current + 1)) && ((direction != -1) || (previous !== (current - 1))))

    But still previous is only set to current when index == 0 which means that the only pins it will reject is 1222, 2333, 3444, ...



  • @vt_mruhlin said:

    I generally advocate letting users use dumb pins if they feel like it.

    That's why I said "If I wanted to avoid insecure PINs". I never said that was smart; I just think that given the parameters of the algorithm (avoid insecure PINs), 1357 and 0369 (along with 2468 and 3579) are insecure. So why are they false negatives? You're still eliminating less than 50 possible PINs, and if you reduce the keyspace by less than 1/2 a percent you're honestly not making that big a difference.

     


Log in to reply