[RESOLVED] Input validation woes.

spookztar

Hi guys,

I'm having newbietroubles with validating username and password properly as my "If/else" doesn't behave as I expect it to.

$loginform = "<form method='post' action='{$_SERVER['PHP_SELF']}'>
<fieldset>
<legend>LOG-IN</legend>
<p><label>Name:</label><input name='formusername' type='text' size='15' maxlength='22' /></p>
<p><label>Password:</label><input name='formpassword' type='password' size='15' maxlength='22' /></p>
<p><input class='submit' type='submit' value='Log-in' name='login' /></p>
</fieldset>
</form>";
if (!isset($_POST['login'])) {
echo $loginform;
die ('</div>
     </body>
     </html>');
     }
     else {
     $formusername = strip_tags(trim($_POST['formusername']));
     $formpassword = strip_tags(trim($_POST['formpassword']));
//
// checking password criteria
//
     if (!preg_match('/[a-z]+/', $formpassword) AND !preg_match('/[A-Z]+/', $formpassword) AND !preg_match('/[0-9]+/', $formpassword)) {
        echo "<span class='warning'>$formpassword Failed minimum charactercriteria for valid passwords. Please adhere to the specifications given.</span>";
        die($loginform);
        }
        elseif (!strlen($formpassword > 8) OR !strlen($formpassword < 20)) {
        echo "<span class='warning'>Password illegal length</span>";
        die($loginform);
        }
//
// checking username criteria
//
     if (!preg_match('/[a-z]+/', $formusername) AND !preg_match('/[A-Z]+/', $formusername) AND !preg_match('/[0-9]+/', $formusername)) {
        echo "<span class='warning'>$formusername Failed minimum charactercriteria for valid usernames. Please adhere to the specifications given.</span>";
        die($loginform);
        }
        elseif (!strlen($formusername > 8) OR !strlen($formusername < 20)) {
        echo "<span class='warning'>Username illegal length</span>";
        die($loginform);
        }
//
// DB stuff below and authorization if userdata data validates and matches DB values.
//
        $_SESSION['authorized'] = TRUE;
        echo "<span class='success'>HELLCOME!!</span>";
        }

I keep getting the "password illegal length" even though it isn't below 8 or above 20 characters. It also lands there when I enter illegal symbols instead of the correct message for that.

What do I need to do differently here?

alimadzi

Your password length test is not quite right. Try this...

elseif ( strlen($formpassword) < 8 || strlen($formpassword) > 20 ) {

spookztar

Thanx for your input.

That clearly helped. What is the most important characters to declare illegal in order to make this code as secure as possible while stil allowing the most possible safe combinations?

alimadzi

Are you talking about characters you should disallow in passwords?

bradgrafelman

If you're worried about breaking the SQL query... you really don't have to worry about any "important characters".

spookztar

I mean anything that represents a security risk in a formfield.

bradgrafelman

Other than using the data in a SQL query, I don't know of any other way typing something into a form field could become a "security risk"... I mean, there's no magic phrase that makes PHP format your server, so that's what I talked about the SQL queries.

spookztar

Someone just suggested using:

$formusername = mysql_real_escape_string(strip_tags(trim($POST['formusername'])));
$formpassword = mysql_real_escape_string(strip_tags(trim($POST['formpassword'])));

Instead of my own example. Having heard what it does, I suppose it was somewhat what I was looking for regarding rendering potentially harmfull input harmless.

Thanks guys,

spookztar

Well, apparently I'm not quite out of the woods yet on this one as I get the wrong error message on a given error. When typing ''aaaaaaaaaaaaa' in a formfield, I don't get the "Failed check for required symbols" message as I should, but the "wrong username" message from later down the script. But when I send an empty form then I get the "Failed check for required symbols" message from the comparison-part of the script. Shouldn't the series of preg_match()'es demand at least one of each character or halt with the appropriate die message? I could understand the situation if I used "OR" in the code, but "AND" and the "+" should demand at least one of each, right?

if (!preg_match('/[a-z]+/', $formpassword) AND
    !preg_match('/[A-Z]+/', $formpassword) AND
    !preg_match('/[0-9]+/', $formpassword)) {
    echo "<span class='warning'>$formpassword ERROR: Failed check for required symbols in password. Please adhere to the specifications given.</span>";
              die($loginform);
              }
      elseif (!preg_match('/[a-z]+/', $formusername) AND
              !preg_match('/[A-Z]+/', $formusername) AND
              !preg_match('/[0-9]+/', $formusername)) {
              echo "<span class='warning'>$formusername ERROR: Failed check for required symbols in username. Please adhere to the specifications given.</span>";
              die($loginform);
              }

spookztar

Can someone explain to me why this:

if (!preg_match('/[a-z]+/', $formusername)) {
       echo "<span class='warning'>$formusername. ERROR: Failed check for required symbols in username $formusername. Please adhere to the specifications given.</span>";
       die($loginform);
       }
    if (!preg_match('/[A-Z]+/', $formusername)) {
       echo "<span class='warning'>ERROR: Failed check for required symbols in username $formusername. Please adhere to the specifications given.</span>";
       die($loginform);
       }
    if (!preg_match('/[0-9]+/', $formusername)) {
       echo "<span class='warning'>ERROR: Failed check for required symbols in username $formusername. Please adhere to the specifications given.</span>";
       die($loginform);         

       }  

    if (!preg_match('/[a-z]+/', $formpassword)) {
       echo "<span class='warning'>$formpassword ERROR: Failed check for required symbols in password. Please adhere to the specifications given.</span>";
       die($loginform);
       }          

    if (!preg_match('/[A-Z]+/', $formpassword)) {
       echo "<span class='warning'>$formpassword ERROR: Failed check for required symbols in password. Please adhere to the specifications given.</span>";
       die($loginform);
       }
    if (!preg_match('/[0-9]+/', $formpassword)) {
       echo "<span class='warning'>$formpassword ERROR: Failed check for required symbols in password. Please adhere to the specifications given.</span>";
       die($loginform);
       }
       elseif (strlen($formusername) <5 OR strlen($formusername) >20)
                      {
                      echo "<span class='warning'>ERROR: Username $formusername is of an illegal length</span>";
                      die($loginform);
                      }                   

       elseif (strlen($formpassword) <8 OR strlen($formpassword) >20)
                      {
                      echo "<span class='warning'>ERROR: Password is of an illegal length</span>";
                      die($loginform);
                      }

- works, and the code in the above post doesn't?

alimadzi

The first code sample uses AND between all conditions. So essentially somebody would have to fail all conditions to get the error message. The second code sample tests them individually. To "fix" the first one, just replace AND with OR wherever it appears.

Also do you really want your app to die if they forget to follow just one of these conditions? Doesn't seem user friendly at all. Why not display the error along with the form and give them a chance to fix it?

spookztar

Hmmm... To me, it seems logic that "AND" demands all three criteria to be true. "OR" seems like it only asks for at least one, hence the literal meaning of the word.

"Why not display the error along with the form and give them a chance to fix it?"

I do?

laserlight

To me, it seems logic that "AND" demands all three criteria to be true.

That is true. However, because the return of each preg_match() call is negated, it means that each condition has to be false. Think De Morgan's theorem: !a AND !b AND !c = !(a OR b OR c)

Actually, you can combine them into a single preg_match(). It depends on what exactly are your requirements.

spookztar

Thank for replying, laserlight. My requirement is just one of each uppercase, lowercase and a number and a length of between 8-20 characters.

It's VERY non-newbie friendly when human words in programming suddenly don't produce a result that matches their literal "real world" meaning.

laserlight

My requirement is just one of each uppercase, lowercase and a number and a length of between 8-20 characters.

I think you mean "at least one of each". Combining to a single pattern may be a little complex (at least for my regex skills), so I might suggest something like:

function validLoginIdentifer($identifier) {
    return preg_match('/^.{8,20}$/', $identifier)
        && preg_match('/[A-Z]/', $identifier)
        && preg_match('/[a-z]/', $identifier)
        && preg_match('/\d/', $identifier);
}

if (!validLoginIdentifer($formpassword)) {
    echo '<span class="warning">' . $formpassword
        . ' ERROR: Failed check for required symbols in password. Please adhere to the specifications given.</span>';
    die($loginform);
} elseif (!validLoginIdentifer($formusername)) {
    echo '<span class="warning">' . $formusername
        . ' ERROR: Failed check for required symbols in username. Please adhere to the specifications given.</span>';
    die($loginform);
}

It's VERY non-newbie friendly when human words in programming suddenly don't produce a result that matches their literal "real world" meaning.

Blame the mathematicians who came up with the terminology for boolean logic :p

bradgrafelman

spookztar wrote:
It's VERY non-newbie friendly when human words in programming suddenly don't produce a result that matches their literal "real world" meaning.

Who says they don't? When I read your if() statement out loud, it makes perfect sense why it fails to do what you're wanting:

If the value does not contain a-z AND does not contain A-Z AND does not contain 0-9, output this error message.

Obviously, 'aaaaaaaaaaa doesn't fail all 3 conditions, thus the error message isn't outputted.

(EDIT: By the way - I "unresolved" this thread for you.)