Password checker in PHP

Question

I have written a password checker using PHP, consisting of many if else statements. Is there any possible way to shorten this code?

function passtest($pass) {
 if (!empty($pass)) { //check if string is empty
 if (ctype_alnum($pass)) { //check if string is alphanumeric
 if (7 < strlen($pass)){ //check if string meets 8 or more characters
 if (strcspn($pass, '0123456789') != strlen($pass)){ //check if string has numbers
 if (strcspn($pass, 'abcdefghijklmnopqrstuvwxyz') != strlen($pass)) { //check if string has small letters
 if (strcspn($pass, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ') != strlen($pass)) { //check if string has capital letters
 return "<br />Password passed";
 }
 else {
 return "<br />No capital letter";
 }
 }
 else {
 return "<br />No small letter";
 }
 }
 else {
 return "<br />No number";
 }
 }
 else {
 return "<br />Password is short";
 }
 }
 else {
 return "<br />Password has special character";
 }
 }
 else {
 return "<br />Password field is empty";
 }
 }

• Bobby: Related: Verifying password strength using JavaScript
• Nzall: As an aside, don't just do your password validation only on the server side. Having a client-side Javascript solution as well means your users will get faster feedback that their password is not allowed, improving their user experience.
• wchargin: Negate and un-nest. if (empty($pass)) return 'empty'; if (ctype_alnum($pass)) return 'has special character'; etc
• TRiG: You're storing hashes of passwords, aren't you? Of course you are. In that case, why do you care whether they have special characters? What's the problem with them?
• jsanc623: Relevant: codinghorror.com/blog/2006/01/flattening-arrow-code.html

Answer 1

I have an answer which also changes the logic of your code but I think there is a good reason why you should consider it:

function passtest($pass) {
 $errors = array();
 if (empty($pass)) $errors[] = 'Password field is empty';
 if (ctype_alnum($pass)) $errors[] = 'Password has special character';
 [...]
 return '<br />' . implode('<br>', $errors);
}

Since it appears like this is somehow shown to the user I would notifyabout all the errors that happened in choosing the password, so that they can all be corrected in a single try.

Another sidenote: Since your code reminds me of the time where I was just getting started and this seems to be at least somewhat related: Don't save passwords, save their hashes. And use a good hashing implementation. If you're using PHP>= 5.5, there is a really easy way.

And thanks to "Fge", I can also give you a slightly less easy way for earlier PHP versions (>= 5.3.7).

• Bobby: Was it the intention that you check for empty twice?
• Fge: And for those not using PHP5.5+ there is github.com/ircmaxell/password_compat for backward compability :)
• Elias Van Ootegem: erm... $errors.push('...') <-- I take it you mean $errors[] = '...'; and $errors.join('<br>') --> implode('<br>', $errors). If you're not using XHTML (and seeing as we're shifting towards HTML5, this is likely not to be the case), <br>` will do fine. If you're posting on code-review, and your answer contains code, please ensure that the code you post isn't in need of additional reviewing
• Gregor Weber: @EliasVanOotegem Thank you, you're 100% right. I should have checked it before. Too much JS and self-confidence on my side...
• TRiG: Shouldn't that be if (!ctype_alnum($pass))? And again, I question the necessity of that check at all. Any decent program should be able to deal with all of Unicode, especially since it's being hashed. (It is being hashed, isn't it?)

Answer 2

First thing first - if your else condition ends with a return, you don't need nest your if statements. Just reverse all of your test conditions and return if they are true. This also makes your code much more readable, because the condition you are returning is next to the test in your code, not the reciprocal of it's distance from the top of your nest:

function passtest($pass) {
 if (empty($pass)) 
 {
 return "<br />Password field is empty";
 } 
 if (!ctype_alnum($pass))
 {
 return "<br />Password has special character";
 } 
 //etc... 

The second thing that I'd mention is a usability issue (and a pet-peeve of a lot of validations I run across). If there is more than 1 thing that is being validated, and the user has to pass all of the checks, let them know everything that was wrong the first time. It's possible to have no capital, no number, and be too short -- don't make the user attempt a password 3 times to figure that out (and don't assume they'll read the instructions first).

• Bob: On your second point - one possible implementation is to append an error message to a string for everything that was wrong, and return that entire string at the end. Personally, I'd prefer flags for proper processing of error messages later, instead of directly returning text/markup to be displayed - separation of logic and presentation and all that.
• Comintern: For me, the implementation usually depends on how many conditions I'm checking. Agreed on the logic and presentation and all that - validations belong in the business logic.

Answer 3

Create clause guards:

if (empty($pass)) {
 return "<br />Password field is empty";
}
if (!ctype_alnum($pass)) {
 return "<br />Password has special character";
}

However, this all seems like a lot for an average user. If the user wants a weak password, let them have it. Just make sure you're doing all you can to protect them in your database.

I read that somewhere, let me see if I can find the source. Oh, and why would you not let them have special characters in their password?

Found something sort of like what I was talking about

• Comintern: Nice link on password complexity. I personally take a test based approach to creating passwords - pick the password I want without even reading the complexity requirements and then change it to pass the unit tests. ;-)
• Alex L: Haha after reading all the responses on security.se regarding passwords, I've got a unique passphrase (that's memorizable!!) for each website! I have yet to find a site which won't allow it. Except my bank. They don't allow passwords > 14 characters!
• Score_Under: "Oh, and why would you not let them have special characters in their password?" -- A pet peeve of mine. Not only does it mean I have to generate a less secure password, but it can be symptomatic of bad backend code (blocking special characters to prevent SQL injection, for example) which does not make me confident in the site's protection of my data.

Answer 4

Not only can this validation be made shorter, it can also be improved on...
Here's the list of remarks I have, I'll be elaborating on them as we go along:

1. Don't restrict users from using passwords containing chars other than alphanumeric chars. You accept f00barer, but not f00b@r!#.
2. In light of this, returning specific errors (such as an error message saying the password contains a special char), it won't take me long to work out what kinds of passwords users of your app may be using. I can then compile a more targeted list of possible passwords for a brute-force attack. Bottom line: your error messages are too specific, and your password rules are too strict. Combine the two and you end up with a possible security issue
3. In a function, it's very easy (and it often improves readability) to chance if (true) {/*do stuff*/} else { return $stuff; } to if (false) { return $stuff;} /*do stuff*/. this essentially gets rid of all of those pesky else's
4. Though you have to be careful not to overdo it, all but one of your if's can be replaced with a single preg_match call

I'll address the first and last issue in one go. I will admit, the regex isn't that simple, but here goes:

$pattern = '/(?<=\d).*((?<=[a-z]).*[A-Z]|(?<=[A-Z]).*[a-z])|(?<=[a-z]).*((?<=[A-Z]).*\d|(?<=\d).*[A-Z])|(?<=[A-Z]).*((?<=[a-z]).*\d|(?<=\d).*[a-z])/';
$pass = trim($pass);
if (!empty($pass) && strlen($pass) > 7 && preg_match($pattern, $pass))
{//not empty, match ANY character after trimming the string 8 or more times
 return 'Pass approved';
}
return 'Pass not approved';

I'm accepting alphanumeric chars, aswel as special chars for people to use in their password.
The pattern basically repeats the same principle three times, this is the basic idea:

(?<=\d).*((?<=[a-z]).*[A-Z]|(?<=[A-Z]).*[a-z])

• (?<=\d) If the previous char was a digit
• .* match any char zero or more times
• (?<=[a-z]).*[A-Z]: find a lower-case char, followed by zero or more chars, and an upper-case letter
• |: OR -(?<=[A-Z]).*[a-z]: After the initial digit, if the lower-case followed by an upper case wasn't found: find an upper-case char that is followed by zero or more chars of any type and a lower-case letter.

Now this matches things like "foO()!naR". After the digit, there are still upper and lower-case letters. However, the digit may be at the end of the string, or either the upper or lower-case character(s) might precede the digit. That's why I've repeated the same pattern three times, only changing the order in which we search:

(?<=\d).*((?<=[a-z]).*[A-Z]|(?<=[A-Z]).*[a-z]) | //digit followed by upper and lower OR
(?<=[a-z]).*((?<=[A-Z]).*\d|(?<=\d).*[A-Z])| //lower followed by digit and upper OR
(?<=[A-Z]).*((?<=[a-z]).*\d|(?<=\d).*[a-z]) //upper followed by lower and digit

Either way, we'll find the sucker. To ensure a length of 8 or more chars, all you do is call strlen. Make sure the preg_match is the last condition you check: PHP short-circuits evaluations. if the string isn't long enough, preg_match won't be called. given that preg_match can be slow, we don't want to call it unless we really need it.

I'm returning a generic error message here, because the pass is not accepted. That's all the user really needs to know. Have some div on your page say that a pass can contain any char, must contain upper, lower and digit characters and has to be at least 8 chars long, in case of any password rejection. If the user can type, he can read, and should be able to work out why the pass was rejected.

Now, all these if's and elses are gone, but still: if you find yourself nesting if-else blocks, consider re-writing that code to:

if (strlen($string) < 8)
{
 return 'string is too short';
}
//no else, string is long enough
if (in_array($string, $forbiddenWords))
{
 return 'illegal word';
}
//string is long enough, not forbidden word...

Also consider using a switch-case. It's easier to read, and you can exploit the fall-through "bug". (some call it a bug, I call it a feature):

switch(true)
{//true!
 case empty($string): return 'no string';
 case strlen($string) < 8: return 'too short';
 case false: return 'Impossible';
 case true:
 $var = 'passed all checks, only then this case will be true';
 //no break
 default: return $var;//returns ^^
}