Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a function which verifies the Password field and suggests the user to enter a strong password. I also have a label named Password Strength, referring to the strength of a password (very weak, weak, medium, etc).

Code:

<tr>
<%= label "Password",:mandatory=>true %>
<td>

<input id="user_password" type="password" size="30" name="user[password]"  onkeyup="chkPasswordStrength(this.value,document.getElementById('strendth'),document.getElementById('error'))">

</td>

<td id="strendth" class="strength5"><b>Password Strength</td>

</tr>
<tr><td></td>

My JavaScript:

function chkPasswordStrength(txtpass,strenghtMsg,errorMsg)
   {
     var desc = new Array();
     desc[0] = "Very Weak";
     desc[1] = "Weak";
     desc[2] = "Better";
     desc[3] = "Medium";
     desc[4] = "Strong";
     desc[5] = "Strongest";

     errorMsg.innerHTML = ''
     var score   = 0;

     //if txtpass bigger than 6 give 1 point
     if (txtpass.length > 6) score++;

     //if txtpass has both lower and uppercase characters give 1 point
     if ( ( txtpass.match(/[a-z]/) ) && ( txtpass.match(/[A-Z]/) ) ) score++;

     //if txtpass has at least one number give 1 point
     if (txtpass.match(/\d+/)) score++;

     //if txtpass has at least one special caracther give 1 point
     if ( txtpass.match(/.[!,@,#,$,%,^,&,*,?,_,~,-,(,)]/) ) score++;

     //if txtpass bigger than 12 give another 1 point
     if (txtpass.length > 12) score++;

     strenghtMsg.innerHTML = desc[score];
     strenghtMsg.className = "strength" + score;

     if (txtpass.length < 6)
     {
     errorMsg.innerHTML = "Password Should be Minimum 6 Characters"
     errorMsg.className = "errorclass"
     }


   }

And I have a CSS as well to change the style of the Password Strength label according to user input.

My CSS:

<style type="text/css">
             .strength0
            {
                width:200px;
                background:#B20E37;
                text-align: center;
                font-weight: bold;
            }

            .strength1
            {
                width:200px;
                background:#D32847;
                text-align: center;
                font-weight: bold;
            }

            .strength2
            {
                width:200px;
                background:#ff5f5f;
                text-align: center;
                font-weight: bold;
            }

            .strength3
            {
                width:200px;
                background:#83D680;
                 text-align: center;
                font-weight: bold;
            }

            .strength4
            {
                background:#4dcd00;
                width:200px;
                 text-align: center;
                font-weight: bold;
            }

            .strength5
            {
                background:#399800;
                width:200px;
                text-align: center;
                font-weight: bold;
            }

          .errorclass
          {
            font-weight:bold;
            font-size: 10px;
            color: #4F080B;
            font-family: Arial, 
          }
</style>

I'm just wondering is there a better way to re-write this code.

share|improve this question
    
binding using onkeyup= the way you are is the same as using eval. –  Tim Seguine Feb 5 at 11:48
6  
Your logic is a little bit flawed. The strength of a password is not necessarily defined by the character set it uses. So the password "abcdE1$" would yield a score of "Strong", while the password "thisisaverylongpasswordbecause" would only get a score of "Better", despite being much harder to bruteforce (and maybe also harder to acquire by "looking over someones shoulder"). –  Bobby Feb 5 at 11:50
    
@Bobby Is there a way to combine all those characters(alphabets,special characters,numbers) into Regular Expression –  Pavan Feb 5 at 12:02
2  
Obligatory: xkcd.com/936 –  gregmac Feb 6 at 5:27
    
Hi, I have removed the code-challenge tag. That one is reserved for specific posts (feel free to participate!). –  Mat's Mug Feb 6 at 19:47

5 Answers 5

up vote 32 down vote accepted

Dislcaimer: I'm not a security researcher and the following answer is compiled from my own, humble knowledge. The math is very basic and there are many things to consider, if in doubt, pay Security a visit. Also there are many factors that can kill password security completely, for example the user themselves or social engineering.

In this case I'm not reviewing the code, but I'm reviewing your program logic.

A short talk about password security

Let's first define the rules you use (the term "special chars" here for further on refers to the set !@#$%^&*?_~-()):

  • Password length > 6: 1 Point
  • Password length > 12: 1 Point
  • Password contains at least one lower and one upper case letter: 1 Point
  • Password contains at least one digit: 1 Point
  • Password contains at least one of the special chars: 1 Point

Your scale goes from 0 (Very Weak) to 5 (Strongest). That means a password based on length can never go beyond 2 (Better), but a short password can be 4 (Strong). This is bad if we keep in mind that strong passwords do not necessarily use a broad character set.

The following passwords are considered Strong by your algorithm:

  • abcdE$1
  • qwert!1
  • 1111Aa@

Now these do not look like strong ones to me, let's have a look at how many possible combinations such a password has. The total character set for these passwords is 26 (lower) + 26 (upper) + 10 (digits) + 14 (special) = 76.

76^7 = ~1.4 * 10^13 = ~14 trillion

So an attacker which knows the character set and the length of the password, has to search roughly 14 trillion combinations until they find the password. Though, that is the worst case scenario, could be that they find it on the third try, but could be that it is the next to the last. Todays computers can do more then 500 million guesses per second, that's roughly 8 hours...this can not be considered secure in any way.

Let's have a look at the other side of the spectrum, the following password is only considered Better:

thisisaverylongpasswordbecause (This is a very long password because)

It only has a character set of 26 and a length of 30 (the maximum for your system if I've seen this correctly).

26^30 = ~ 2.8 * 10^42 = ~2.8 tredecillion

I have no idea what that number is supposed to mean, so let's compare it. Okay, that doesn't help either...maybe if we put it into a timeframe, 500 million guesses per second again:

~1.8 * 10^26 years

That's actually good news! That's well before the heat death of the universe.

So, less nonsense, more talk: What's going on here?

The strength of a password can not reliable be defined. But we know two things:

  1. The longer it is, the more combinations you have to go through.
  2. The more different characters it uses, the more combinations you have to go through.

The difference is that one adds to the base, and the other to the exponent. A higher exponent weighs in heavier than a higher base and yields more total combinations. So whenever possible use a passphrase instead of a password.

Despite being, seemingly, vulnerable to dictionary attacks, the Oxford Dicitonary holds 300 thousand main entries, so if we have a passphrase of 4 words, that's:

300000^4 = 8.1 * 10^21

And that does not take into account possible misspellings, if the words start uppercase, lowercase or mixed and different languages.

Make the users favor passphrases and long, easier to remember passwords then complicated short ones.

Sell it as a feature!

This will make your marketing department happy, you just implemented support for the more secure passphrases into your software! Let your users know about it by simply adding a short information to the password box:

We support passphrases up to a length of XX characters!

Passphrases are more secure and easier to remember than ordinary passwords.

And some short information about how to "create" one, and drop any indicator if the password is secure or not.

Who you can and can't help

There are quite many user groups out there, the question is who you want to reach with this help. Let's define three groups:

  • The "average user", which does not understand why they need to press a button labeled "Delete" to delete something.
  • The normal user, always ready to learn and looks beyond their own nose.
  • The technical user, already uses a password manager and/or passphrases.

No matter how hard you try, you will not be able to help the "average user". If you tell them to use a password of minimum length 6, they will use "123456" or "asdfgh". If you tell them to use a password of minimum length 12, they will use "asdfghjkzxcvbn" or "000000000001". They are beyond hope and will actively workaround security measures you implement. Just let 'em be.

The "normal user" on the other hand is ready to learn something new, telling them about passphrases and that you support them will make them want to use passphrases. If your small help text and the rest of the system in that moment is helpful, they will use a passphrase which is not something absolutely stupid.

The "technical user" will be pleased that you support a XX length password and passphrases. They most likely already use a strong and random password and a password manager or passphrases, but that you don't limit them in any way will make them happy.

Secure passwords stored insecure are insecure

It doesn't matter how good the password or passphrase is, if you fail to store in a secure manner, they are insecure.

We will completely leave out social engineering or "harmful" users (there are people after all who have their creditcard PIN on a note in the same wallet), we can't help them.

But what is important that if somebody manages to break into your server and manage to copy the user database, they must not have access to all accounts. This is achieved by simply hashing the passwords with an appropriate method. Even if the attacker now has all the usernames and e-mail addresses, the passwords are still safe.

It will also make the marketing department happy that, despite you've been hacked, they can tell the users that their accounts are still safe (password change advised, obviously).

The great news is, if you hash the passwords there's absolutely no reason to limit password length at all! They will all end up as the same length hash in your database, so you can let me use that sentence from that book as a passphrase.

Ask the right people the right questions

Last but not least, defining what is a strong password depends on many things. If in doubt, pay the people over at Security a visit, that's what they're there for.

share|improve this answer
3  
It should be noted that the special character check is poor. There are many many more types of special characters out there than what's available on a typical western keyboard. It would be better to check that there are alphanumeric characters, rather than check for specific non-alphanumeric characters. –  cimmanon Feb 5 at 15:13
2  
shouldn't length be mostly what matters for password security? i don't see how special characters/case makes passwords more robust vs brute force/etc... (queue reference to the relevant XKCD everyone has seen). –  HC_ Feb 5 at 21:04
    
@Bobby Thank you for your great logical answer.I'd love to see your logic in to a code! –  Pavan Feb 6 at 4:42
    
@HC_: I tried to get exactly that across, did I fail at that? –  Bobby Feb 6 at 8:11
1  
@Pavan: Getting this into code is hard. There are many question on Securiy about this very topic. –  Bobby Feb 6 at 9:20

As mentioned by Tim Seguine do not use onkeyup, instead use addEventListener and attachEvent if you need to support older IE versions.

Furthermore : chkPasswordStrength(txtpass,strenghtMsg,errorMsg) is unfortunately named, you disemvoweled check for no good reason, txtpass could simply be password and your 2 Msg parameters are not messages as one might think, but DOM elements instead..

You are initially clearing errorMsg.innerHTML, but you are not resetting the className, I would:

 if (txtpass.length < 6)
 {
   errorMsg.innerHTML = "Password Should be Minimum 6 Characters";
   errorMsg.className = "errorclass";
 }
 else
 {
   errorMsg.innerHTML = "";
   errorMsg.className = "";
 }

Also, I would write

 var desc = new Array();
 desc[0] = "Very Weak";
 desc[1] = "Weak";
 desc[2] = "Better";
 desc[3] = "Medium";
 desc[4] = "Strong";
 desc[5] = "Strongest";

as

var strengths = ['Very weak','Weak','Better','Medium','Strong','Strongest'];
share|improve this answer
    
I disagree with the last, though, why not simply var desc = new Array("Very Weak", "Weak", ...);. That is easier to read as you do not need to read to the end of the line to know that desc is now an array...which is an unfortunate name, too. –  Bobby Feb 5 at 15:10
3  
So true on the name!@! Though to even go further, I guess with only 6 entries I could go for ['',''] –  konijn Feb 5 at 15:13
    
@konijn Thank you for correcting it! –  Pavan Feb 6 at 4:47

You're repeating a lot of CSS. Let's fix that by using your #strength ID.

<style type="text/css">
#strength {
    width: 200px;
    text-align: center;
    font-weight: bold;
}

.strength0 {
    background-color: #B20E37;
}

.strength1 {
    background-color: #D32847;
}

.strength2 {
    background-color: #ff5f5f;
}

.strength3 {
    background-color: #83D680;
}

.strength4 {
    background-color: #4dcd00;
}

.strength5 {
    background-color: #399800;
}

.errorclass {
    font-size: 10px;
    font-family: Arial;
    font-weight: bold;
    color: #4F080B;
}
</style>

Notes:

  • Please check your rule for .errorclass again. Your font-family declaration ends with a , instead of a ;.
  • In my opinion, one should always leave a space between : value and property declarations. It simply improves readability.
  • Be aware when using the shorthand of a property. An example of what could happen: If you write background: red;, this doesn't only set the background-color to red. It also applys the default values for the other properties available in the shorthand syntax (background-image, background-repeat, ...)
share|improve this answer
    
"you would also overwrite any background-image declarations you made earlier and so on" Ignoring the fact that the OP is not using images, what if that's the point? Using shorthand, even if it is for a single value, is shorter than using the longhand. "It's a good practise to always leave a space between : and value und property declarations" this is nonsense, it is valid either way. –  cimmanon Feb 5 at 17:45
    
@cimmanon I edited my answer to word these things better. Better? –  kleinfreund Feb 5 at 19:30
1  
+1 for addressing CSS –  konijn Feb 6 at 3:29
    
@cimmanon Prudent whitespace makes for a happy space. –  David Harkness Feb 6 at 3:42
    
@kleinfreund Thank you for correcting the CSS.And your Notes are also useful! –  Pavan Feb 6 at 5:08

Bobby delivered an excellent answer about password strength, so I will focus on the code.

Reg ex

You have the following regular expression:

/.[!,@,#,$,%,^,&,*,?,_,~,-,(,)]/

It matches one non-line-break character followed by any of the following characters !,@,#,$,%,^,&,*,?,_,~, or a character in the range comma to comma, or any of the following (,). You repeat comma as if it were a separator. A character class need no separators, so putting a comma in a character class simply means that it will match comma. A dash on the other hand has a special meaning in a character class, so you will need to escape it if you want a character class to contain a dash.

In general I would recommend escaping all special characters in regular expressions if you want to use their literal value, it helps you to avoid mistakes like this dash, and makes the intention clearer.

In the context it seems strange that you include so few characters in a special character class. For this feature it seems logical to simply define special characters as anything not in the other character classes:

/[^A-Za-z0-9]/

Alternately if you for some reason want only the basic printable ASCII special characters you can easily include them all using ranges (use a character map to find such ranges):

/[\ -\/\:-\@\[-\`\{-\~]/

Here's a short explanation on how this works:

/[         // start of the expression/range
  \ -\/    // All characters from " " to "/"
  \:-\@    // All characters from ":" to "@"
  \[-\`    // All characters from "[" to "`"
  \{-\~    // All characters from "{" to "~"
]/         // end the range/expression

Function call

You have the following in your HTML:

<input id="user_password" type="password" size="30" name="user[password]" onkeyup="chkPasswordStrength(this.value,document.getElementById('strendth'),document.getElementById('error'))">

Some people would like to avoid such inline code altogether, personally I don't think it is a big deal whether or not to inline a simple function call, but including all those parameters is a bit of an eyesore, and there is really no reason to have them, your function could easily fetch the same info itself.

Another thing to consider is that there is no guarantee that the keyup event will fire just because something was entered into the field. That would for instance not be the case if something was mouse-pasted into the field, and I'm not sure that you can rely on keyup to fire on all devices that use screen keyboards. In any case I'd double-guard a live field validation function by also having it fire on the change event.

Clean tables

I'm not sure exactly what that Rails label does, but if anything is rendered in its place the result is most likely illegal HTML, and therefore possibly inconsistent browser behaviour. You can enclose a complete table in another element, or you can put an element inside a single td or th element. Anything in-between is generally not allowed.

share|improve this answer
    
Thanks for the explanation! It would really help! –  Pavan Feb 6 at 12:30

You have 1 check (less than 6 characters) which gives an error, but you execute it AFTER all the non-error checks. This means that if I make a password 12345, it's going to give me 1 point even though it's completely invalid for this purpose.

You should move that check before all the rest and return if it fails, because right now you're just wasting valuable processor speed if the password is less than 6 characters.

apart from that, you give 1 point if it's above 6 characters, but you give an error if it's below that. so either they have a point or they get an error. so you can't have 0 points. why then have a category for 0?

Finally, you're embedding DOM element retrievals into your function call in the HTML, which should be avoided. I would just pass the name of the elements and do the DOM retrieval in the function itself.

share|improve this answer
    
No, it's not "valuable processor speed" until you have proven than it is. It simply makes more sense to make the check early: it's one mental check that you don't have to do when reading the rest of the code. –  Quentin Pradet Feb 6 at 13:10
    
It might not be valuable, but you're still wasting processor time for doing those checks if they're not needed. You should always have your checks that break or prevent functionality first so the code can continue right on if the checks return false. If you're first processing it all and then checking "oh, I need to fail if X", you're spending processing time on obsolete processes. If you got a bunch of those, the program appears to respond marginally slower, which detracts from user experience. –  Nate Kerkhofs Feb 6 at 13:28
    
I also agree that you should do those checks first, but for readability reasons. Micro optimizations are often worthless, measure the gain before saying "it's faster". There's no way for a user to notice any difference between those two versions of chkPasswordStrength. –  Quentin Pradet Feb 6 at 13:49
1  
Yes, the performance gain is unnoticable. maybe I shouldn't have mentioned that. but still, even if you wouldn't do it for the performance gain, you should still do it for non-readability reasons, even if that reason is just getting familiar with best practices. While there are no severely delaying checks in this code, other code might have those (like database or service calls). Being familiar with the concept of optimal path in trivial cases means you're used to it in non-trivial cases as well, and are more likely to use it. –  Nate Kerkhofs Feb 6 at 14:19

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.