raviteja07 / owasp-esapi-java

Automatically exported from code.google.com/p/owasp-esapi-java
0 stars 0 forks source link

Authenticator.verifyPasswordStrength thinks "password123" is a good password #26

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
In AuthenticatorTest.java there is this test:
try {
  instance.verifyPasswordStrength("password", "password123");
  fail();
}
catch ...

The test passes, indicating that verifyPasswordStrength raised an exception
about the password quality, but only because it is too similar to the
previous password (the first param). Passing in an empty string instead of
"password" will cause the test to fail, because in the default
implementation, "password123" will have a "strength" of 22. 11 chars * 2
charsets = strength of 22, which is > than the 16 limit. Can't we come up
with a better metric? A couple suggestions:

1. Make a verifyPasswordStrength(String) method that will only check the
new password. Also, make generateStrongPassword call verifyPasswordStrength
before returning output.

2. Add into the API doc that it is a good idea that the implementation
compare the password to a dictionary list, and that the default
implementation does not do this.

3. Can we change the "strength" algorithm to something a bit better? How
about number of bits of entropy? First, check what charsets are being used.
Add up the length of the charactersets in use, yielding the length of the
whole charset of the password, N. The entropy per symbol is then log(N)
base 2, and multiplied by the length of the password will give its entropy
in bits.

This metric is less valid for human-generated passwords. An alternative is
to up the strength limit.

What version of the product are you using? On what operating system?

SVN revision 574.

Original issue reported on code.google.com by cyounk...@gmail.com on 4 Aug 2009 at 3:11

GoogleCodeExporter commented 9 years ago
This algo needs to be addressed for ESAPI 2.1 or 3.0. At this point in time, I 
would
assume that not many users will be using the reference implementation of the
Authenticator since most applications already have some Authentication 
framework in
place (be it a home-grown or pre-packaged solution.) 

At the point that we address this, a good deal of the functionality that is in 
the
FileBasedAuthenticator needs to be moved to a base class that the user can 
extend to
provide a good RI of these functions without forcing the user to either use the
FileBasedAuthenticator or Copy/Paste the RI code into their own wrapper class 
around
their existing user authentication classes.

Original comment by chrisisbeef on 29 Oct 2009 at 5:02

GoogleCodeExporter commented 9 years ago

Original comment by chrisisbeef on 29 Oct 2009 at 5:11

GoogleCodeExporter commented 9 years ago
Agree that this whole algorithm needs to be addressed. The best approach would 
be to
use something like the Crack library (libcrack.so) that is commonly used with 
PAM on
*nix systems. There is a FOSS port of this library to Java available at 
SourceForge
done back in Jan 2000 and not touched since. It is part of the "Solinger Java
Utilities Project". See
http://sourceforge.net/projects/solinger/files/Java%20CrackLib/
for details.
However, if we want to be serious about providing a decent reference model for
verifying password strength, we really need something similar to Crack.

Original comment by kevin.w.wall@gmail.com on 9 May 2010 at 1:59

GoogleCodeExporter commented 9 years ago
In FileBasedAuthenticator, I just noticed there were several additions
to createUser() and changePassword() where a check to see if the provided
password was the acct name. E.g., in changePassword(),

    verifyPasswordIsNotAccountName(accountName,newPassword);    // Added
    verifyPasswordStrength(currentPassword, newPassword);
    user.setLastPasswordChangeTime(new Date());

So, my question is "why is this check not part of the password strength
by default?". In other words, if your password is the same as your
account name, I'd pretty much say that your password strength should
be zero and an AuthenticationCredentialsException should be thrown.

I'd propose moving a check like this into the the public
verifyPasswordStrength() method so every time there is an occassion
to check the password strength, one doesn't need to remember to call
verifyPasswordIsNotAccountName(). (I mean, is there ever a *good*
reason as to why we would want the password to be set to the user
account name?)  However, to do this, we would either have to change
the signature of verifyPasswordStrength() to include a User object
or assume that it is called on the current User object. (E.g., the
reference FileBasedAuthenticator class could call this.getCurrentUser()
to check if the password matched the account name without causing
the signature to be changed.) But the downside of using the current
user rather than changing the signature is that this would not do the
right thing for Authenticator.createUser().

Alternately, we could generalize it even more by separating this out of
the Authenticator class altogether and creating a PasswordPolicy class
that could be used to check against the same thing. It could still be
called from the reference FileBasedAuthenticator methods createUser()
and changePassword(), but would just take a slightly different form.

Original comment by manico.james@gmail.com on 7 Nov 2010 at 6:41

GoogleCodeExporter commented 9 years ago

Original comment by manico.james@gmail.com on 7 Nov 2010 at 6:41

GoogleCodeExporter commented 9 years ago

Original comment by chrisisbeef on 20 Nov 2010 at 9:16