Use the new JCryptPassword in JUser. Deprecate the old way. #1527

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@realityking
Joomla! member

No description provided.

@ianmacl ianmacl commented on the diff Sep 14, 2012
libraries/joomla/crypt/crypt.php
+ /*
+ * Start with a cryptographic strength random string, then convert it to
+ * a string with the numeric base of the salt.
+ * Shift the base conversion on each character so the character
+ * distribution is even, and randomize the start shift so it's not
+ * predictable.
+ */
+ $random = self::genRandomBytes($length + 1);
+ $shift = ord($random[0]);
+ for ($i = 1; $i <= $length; ++$i)
+ {
+ $makepass .= $salt[($shift + ord($random[$i])) % $base];
+ $shift += ord($random[$i]);
+ }
+
+ return $makepass;
@ianmacl
ianmacl Sep 14, 2012

Is there a particular reason for this implementation over against the one in getSalt? Many of the hash types support 64 bit digits and this implementation only supports 62 bits.

@realityking
realityking Sep 15, 2012

Because I'm a chicken. I know that this implementation was reviewed by at least one paper - to me that makes this the default choice.

@ianmacl
ianmacl Sep 16, 2012

Either way, you're still missing two characters for some of the salts which should be there.

@ianmacl ianmacl commented on the diff Sep 14, 2012
libraries/joomla/user/user.php
@@ -551,22 +551,20 @@ public function bind(&$array)
return false;
}
+ $pwCrypt = new JCryptPasswordSimple;
@ianmacl
ianmacl Sep 14, 2012

JCryptPasswordSimple shouldn't be instantiated here. You need to instantiate it in the constructor or at least create a way to inject the dependency.

@ianmacl ianmacl and 1 other commented on an outdated diff Sep 14, 2012
libraries/joomla/user/user.php
// Set the registration timestamp
- $this->set('registerDate', JFactory::getDate()->toSql());
@ianmacl
ianmacl Sep 14, 2012

This change is unrelated to the pull request. Please keep it on topic.

@ianmacl ianmacl and 1 other commented on an outdated diff Sep 14, 2012
libraries/joomla/user/user.php
@@ -589,10 +587,7 @@ public function bind(&$array)
}
$this->password_clear = JArrayHelper::getValue($array, 'password', '', 'string');
-
- $salt = JUserHelper::genRandomPassword(32);
- $crypt = JUserHelper::getCryptedPassword($array['password'], $salt);
- $array['password'] = $crypt . ':' . $salt;
+ $array['password'] = JCryptPasswordSimple::create($array['password'], JCryptPassword::JOOMLA);
@ianmacl
ianmacl Sep 14, 2012

JCryptPasswordSimple::create isn't a static method so you shouldn't be calling it as one. Instead, you want to invoke the method on the supplied dependency. Also, you shouldn't hard code it to JCryptPassword::JOOMLA. The preferred hash type should either be adding to JCryptPassword or set as a configuration value in JUser. The default for the platform should probably be blowfish and if other applications really want to keep using an outdated hashing scheme they can choose to. The CMS should also consider switching to blowfish as well. The way the library works makes this very easy to do (i.e. it can verify passwords in the old format but create new hashes in a different format).

@realityking
realityking Sep 15, 2012

This is the most dead simple implementation to use the new API so we can deprecate the old one. It's not particular nice - I agree. But I have us rather think out carefully how we set up the password hashing than do a quick shot we later regret.

Also I'm still not sure if the current JCryptPassword interface is the ideal thing. If next year the new "supercrypt" algorithm should be used to do some flaw in blowfish a platform consumer has to inject a new object and define a new constant for it which the consumer needs configure into JUser or some other place. Also he probably needs to duplicate at least the verify and detect parts from JCryptPasswordSimple (or possibly inherit from it). That said I haven't come up with a better idea ;)

@ianmacl

This is the most dead simple implementation to use the new API so we can deprecate the old one. It's not particular nice - I agree. But I have us rather think out carefully how we set up the password hashing than do a quick shot we later regret.

Also I'm still not sure if the current JCryptPassword interface is the ideal thing. If next year the new "supercrypt" algorithm should be used to do some flaw in blowfish a platform consumer has to inject a new object and define a new constant for it which the consumer needs configure into JUser or some other place. Also he probably needs to duplicate at least the verify and detect parts from JCryptPasswordSimple (or possibly inherit from it). That said I haven't come up with a better idea ;)

If you're going to hard code the hashing mechanism in JUser to the legacy style MD5 password scheme, then this pull request is completely useless and doesn't accomplish anything. As it stands, it is unreasonable to write a serious application using JUser with any sort of user data.

Also, I don't entirely understand your comments regarding JCryptPassword. What I hear you suggesting is that we should hardcode JUser to use MD5 rather than allowing application developers the flexibility to choose a hashing scheme that meets their needs? In some areas there are laws that dictate password security standards and MD5 isn't it.

It's also worth noting that https://wiki.php.net/rfc/password_hash has been accepted and should appear in PHP 5.5. The minimum step of allowing this external dependency to be injected means that third party applications could easily implement the JCryptPassword interface and use the new core functions as the implementation. Leaving the instantiation of JCryptPasswordSimple inside the class with no means to inject an alternative means that you leave people with no choice but to provide alternative implementations of JUser, which is only going to provide another roadblock for application developers.

@LouisLandry

@realityking I agree with @ianmacl on this one. I like the idea of JUser moving to using JCryptPassword, but if we are going to do that there needs to be a way of injecting that dependency so that any object compliant with the interface can be used.

I'm closing this for now. If you want to make those adjustments please re-open it so we can review it further.

@LouisLandry LouisLandry closed this Oct 9, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment