From 703d9696c611ea5d69c172cf0d705e8800ab2694 Mon Sep 17 00:00:00 2001 From: Lennart van den Dool Date: Sun, 8 Jul 2012 15:39:33 +0200 Subject: [PATCH 1/3] Implement password salts --- UserModule.php | 8 ++++---- components/UserIdentity.php | 14 ++++++++++++-- controllers/AdminController.php | 9 ++++++--- controllers/ProfileController.php | 3 ++- controllers/RegistrationController.php | 8 ++++++-- models/User.php | 11 +++++++++++ models/UserChangePassword.php | 4 +++- 7 files changed, 44 insertions(+), 13 deletions(-) diff --git a/UserModule.php b/UserModule.php index 58d9783..7ec33bb 100644 --- a/UserModule.php +++ b/UserModule.php @@ -160,14 +160,14 @@ public static function t($str='',$params=array(),$dic='user') { /** * @return hash string. */ - public static function encrypting($string="") { + public static function encrypting($string="", $salt="") { $hash = Yii::app()->getModule('user')->hash; if ($hash=="md5") - return md5($string); + return md5($string.$salt); if ($hash=="sha1") - return sha1($string); + return sha1($string.$salt); else - return hash($hash,$string); + return hash($hash,$string.$salt); } /** diff --git a/components/UserIdentity.php b/components/UserIdentity.php index 6f8986c..5cc063a 100644 --- a/components/UserIdentity.php +++ b/components/UserIdentity.php @@ -26,13 +26,23 @@ public function authenticate() } else { $user=User::model()->notsafe()->findByAttributes(array('username'=>$this->username)); } - if($user===null) + if($user===null) { if (strpos($this->username,"@")) { $this->errorCode=self::ERROR_EMAIL_INVALID; } else { $this->errorCode=self::ERROR_USERNAME_INVALID; } - else if(Yii::app()->getModule('user')->encrypting($this->password)!==$user->password) + return false; + } + + //salted password + $salt = ""; + $password = $user->password; + $passwordParts = explode(":",$user->password); + if(count($passwordParts) == 2) + list($password, $salt) = $passwordParts; + + if(Yii::app()->getModule('user')->encrypting($this->password, $salt)!==$password) $this->errorCode=self::ERROR_PASSWORD_INVALID; else if($user->status==0&&Yii::app()->getModule('user')->loginNotActiv==false) $this->errorCode=self::ERROR_STATUS_NOTACTIV; diff --git a/controllers/AdminController.php b/controllers/AdminController.php index ab28421..88e7a7d 100644 --- a/controllers/AdminController.php +++ b/controllers/AdminController.php @@ -85,7 +85,8 @@ public function actionCreate() $profile->attributes=$_POST['Profile']; $profile->user_id=0; if($model->validate()&&$profile->validate()) { - $model->password=Yii::app()->controller->module->encrypting($model->password); + $salt = User::getNewSalt(); + $model->password=Yii::app()->controller->module->encrypting($model->password.$salt).":".$salt; if($model->save()) { $profile->user_id=$model->id; $profile->save(); @@ -116,8 +117,10 @@ public function actionUpdate() if($model->validate()&&$profile->validate()) { $old_password = User::model()->notsafe()->findByPk($model->id); - if ($old_password->password!=$model->password) { - $model->password=Yii::app()->controller->module->encrypting($model->password); + list($old_password, $old_salt) = explode($old_password->password); + if ($old_password != Yii::app()->controller->module->encrypting($model->password.$old_salt)) { + $new_salt = User::getNewSalt(); + $model->password=Yii::app()->controller->module->encrypting($model->password.$new_salt) . ":" . $new_salt; $model->activkey=Yii::app()->controller->module->encrypting(microtime().$model->password); } $model->save(); diff --git a/controllers/ProfileController.php b/controllers/ProfileController.php index 05e3fa0..78b73c0 100644 --- a/controllers/ProfileController.php +++ b/controllers/ProfileController.php @@ -75,8 +75,9 @@ public function actionChangepassword() { if(isset($_POST['UserChangePassword'])) { $model->attributes=$_POST['UserChangePassword']; if($model->validate()) { + $new_salt = User::getNewSalt(); $new_password = User::model()->notsafe()->findbyPk(Yii::app()->user->id); - $new_password->password = UserModule::encrypting($model->password); + $new_password->password = UserModule::encrypting($model->password,$new_salt) . ":" . $new_salt; $new_password->activkey=UserModule::encrypting(microtime().$model->password); $new_password->save(); Yii::app()->user->setFlash('profileMessage',UserModule::t("New password is saved.")); diff --git a/controllers/RegistrationController.php b/controllers/RegistrationController.php index 45bc5f9..4aad1fe 100644 --- a/controllers/RegistrationController.php +++ b/controllers/RegistrationController.php @@ -39,10 +39,12 @@ public function actionRegistration() { $profile->attributes=((isset($_POST['Profile'])?$_POST['Profile']:array())); if($model->validate()&&$profile->validate()) { + $salt = User::getNewSalt(); + $soucePassword = $model->password; $model->activkey=UserModule::encrypting(microtime().$model->password); - $model->password=UserModule::encrypting($model->password); - $model->verifyPassword=UserModule::encrypting($model->verifyPassword); + $model->password=UserModule::encrypting($model->password, $salt) . ":" . $salt; + $model->verifyPassword=UserModule::encrypting($model->verifyPassword, $salt) . ":" . $salt; $model->superuser=0; $model->status=((Yii::app()->controller->module->activeAfterRegister)?User::STATUS_ACTIVE:User::STATUS_NOACTIVE); @@ -74,6 +76,8 @@ public function actionRegistration() { } } else $profile->validate(); } + $model->password = ''; + $model->verifyPassword = ''; $this->render('/user/registration',array('model'=>$model,'profile'=>$profile)); } } diff --git a/models/User.php b/models/User.php index 388c008..6be7084 100644 --- a/models/User.php +++ b/models/User.php @@ -196,4 +196,15 @@ public function getLastvisit() { public function setLastvisit($value) { $this->lastvisit_at=date('Y-m-d H:i:s',$value); } + + /** + * simply returns the value of PHP's uniqid(). + * That should be sufficient to make most rainbowtables useless. + * + * Modify this function to change the way a salt is generated if you disagree. + */ + public static function getNewSalt() + { + return uniqid(); + } } \ No newline at end of file diff --git a/models/UserChangePassword.php b/models/UserChangePassword.php index e1256f5..77a6403 100644 --- a/models/UserChangePassword.php +++ b/models/UserChangePassword.php @@ -39,7 +39,9 @@ public function attributeLabels() */ public function verifyOldPassword($attribute, $params) { - if (User::model()->notsafe()->findByPk(Yii::app()->user->id)->password != Yii::app()->getModule('user')->encrypting($this->$attribute)) + $old_password = User::model()->notsafe()->findByPk(Yii::app()->user->id)->password; + list($old_password, $salt) = explode(":",$old_password); + if ($old_password != Yii::app()->getModule('user')->encrypting($this->$attribute, $salt)) $this->addError($attribute, UserModule::t("Old Password is incorrect.")); } } \ No newline at end of file From ce7c5d811ddae3ade1949e96e6f1204df9174d86 Mon Sep 17 00:00:00 2001 From: Lennart van den Dool Date: Mon, 1 Oct 2012 03:20:52 +0200 Subject: [PATCH 2/3] Improvement of password salts & small bugfix * Added a salt column to the user table. * Adds a salt to users that have no salted password upon succesfull login. * Salts are now more unique/random * Bug fixed: This commit fixes a bug where a password could by stored to the database in plaintext format by the AdminController->actionUpdate() function, when an admin would supply an unencrypted version of the old password (effectively changing the password to itself, but skipping the encryption methods). --- components/UserIdentity.php | 16 +++---- controllers/AdminController.php | 18 +++++--- controllers/LoginController.php | 6 +-- controllers/ProfileController.php | 6 +-- controllers/RegistrationController.php | 10 ++-- migrations/m121001_001625_userSalt.php | 64 ++++++++++++++++++++++++++ models/User.php | 7 +-- models/UserChangePassword.php | 7 +-- views/admin/view.php | 2 - 9 files changed, 102 insertions(+), 34 deletions(-) create mode 100644 migrations/m121001_001625_userSalt.php diff --git a/components/UserIdentity.php b/components/UserIdentity.php index 5cc063a..d5c59d4 100644 --- a/components/UserIdentity.php +++ b/components/UserIdentity.php @@ -35,14 +35,7 @@ public function authenticate() return false; } - //salted password - $salt = ""; - $password = $user->password; - $passwordParts = explode(":",$user->password); - if(count($passwordParts) == 2) - list($password, $salt) = $passwordParts; - - if(Yii::app()->getModule('user')->encrypting($this->password, $salt)!==$password) + if(Yii::app()->getModule('user')->encrypting($this->password, $user->salt)!==$user->password) $this->errorCode=self::ERROR_PASSWORD_INVALID; else if($user->status==0&&Yii::app()->getModule('user')->loginNotActiv==false) $this->errorCode=self::ERROR_STATUS_NOTACTIV; @@ -52,6 +45,13 @@ public function authenticate() $this->_id=$user->id; $this->username=$user->username; $this->errorCode=self::ERROR_NONE; + + //when user has no salt, let's be generous and give him some. + if(empty($user->salt)) { + $user->salt = User::getNewSalt(); + $user->password = Yii::app()->getModule('user')->encrypting($this->password, $user->salt); + $user->save(); + } } return !$this->errorCode; } diff --git a/controllers/AdminController.php b/controllers/AdminController.php index 88e7a7d..cad496f 100644 --- a/controllers/AdminController.php +++ b/controllers/AdminController.php @@ -64,6 +64,7 @@ public function actionAdmin() public function actionView() { $model = $this->loadModel(); + $model->password = ''; $this->render('view',array( 'model'=>$model, )); @@ -85,8 +86,8 @@ public function actionCreate() $profile->attributes=$_POST['Profile']; $profile->user_id=0; if($model->validate()&&$profile->validate()) { - $salt = User::getNewSalt(); - $model->password=Yii::app()->controller->module->encrypting($model->password.$salt).":".$salt; + $model->salt = User::getNewSalt(); + $model->password=Yii::app()->controller->module->encrypting($model->password,$model->salt); if($model->save()) { $profile->user_id=$model->id; $profile->save(); @@ -117,18 +118,21 @@ public function actionUpdate() if($model->validate()&&$profile->validate()) { $old_password = User::model()->notsafe()->findByPk($model->id); - list($old_password, $old_salt) = explode($old_password->password); - if ($old_password != Yii::app()->controller->module->encrypting($model->password.$old_salt)) { - $new_salt = User::getNewSalt(); - $model->password=Yii::app()->controller->module->encrypting($model->password.$new_salt) . ":" . $new_salt; + $old_salt = $old_password->salt; + $old_password = $old_password->password; + if (isset($_POST['User']['password']) && $old_password != Yii::app()->controller->module->encrypting($model->password,$old_salt)) { + $model->salt = User::getNewSalt(); + $model->password=Yii::app()->controller->module->encrypting($model->password,$model->salt); $model->activkey=Yii::app()->controller->module->encrypting(microtime().$model->password); + } else { + unset($model->password); } $model->save(); $profile->save(); $this->redirect(array('view','id'=>$model->id)); } else $profile->validate(); } - + $model->password = ''; $this->render('update',array( 'model'=>$model, 'profile'=>$profile, diff --git a/controllers/LoginController.php b/controllers/LoginController.php index 4c19e22..4028d4e 100644 --- a/controllers/LoginController.php +++ b/controllers/LoginController.php @@ -17,8 +17,8 @@ public function actionLogin() $model->attributes=$_POST['UserLogin']; // validate user input and redirect to previous page if valid if($model->validate()) { - $this->lastViset(); - if (Yii::app()->getBaseUrl()."/index.php" === Yii::app()->user->returnUrl) + $this->lastVisit(); + if (Yii::app()->user->returnUrl=='/index.php') $this->redirect(Yii::app()->controller->module->returnUrl); else $this->redirect(Yii::app()->user->returnUrl); @@ -30,7 +30,7 @@ public function actionLogin() $this->redirect(Yii::app()->controller->module->returnUrl); } - private function lastViset() { + private function lastVisit() { $lastVisit = User::model()->notsafe()->findByPk(Yii::app()->user->id); $lastVisit->lastvisit = time(); $lastVisit->save(); diff --git a/controllers/ProfileController.php b/controllers/ProfileController.php index 78b73c0..8fe51e3 100644 --- a/controllers/ProfileController.php +++ b/controllers/ProfileController.php @@ -75,10 +75,10 @@ public function actionChangepassword() { if(isset($_POST['UserChangePassword'])) { $model->attributes=$_POST['UserChangePassword']; if($model->validate()) { - $new_salt = User::getNewSalt(); $new_password = User::model()->notsafe()->findbyPk(Yii::app()->user->id); - $new_password->password = UserModule::encrypting($model->password,$new_salt) . ":" . $new_salt; - $new_password->activkey=UserModule::encrypting(microtime().$model->password); + $new_password->salt = User::getNewSalt(); + $new_password->password = UserModule::encrypting($model->password,$new_password->salt); + $new_password->activkey = UserModule::encrypting(microtime().$model->password); $new_password->save(); Yii::app()->user->setFlash('profileMessage',UserModule::t("New password is saved.")); $this->redirect(array("profile")); diff --git a/controllers/RegistrationController.php b/controllers/RegistrationController.php index 4aad1fe..56df947 100644 --- a/controllers/RegistrationController.php +++ b/controllers/RegistrationController.php @@ -39,12 +39,12 @@ public function actionRegistration() { $profile->attributes=((isset($_POST['Profile'])?$_POST['Profile']:array())); if($model->validate()&&$profile->validate()) { - $salt = User::getNewSalt(); + $model->salt = User::getNewSalt(); - $soucePassword = $model->password; + $sourcePassword = $model->password; $model->activkey=UserModule::encrypting(microtime().$model->password); - $model->password=UserModule::encrypting($model->password, $salt) . ":" . $salt; - $model->verifyPassword=UserModule::encrypting($model->verifyPassword, $salt) . ":" . $salt; + $model->password=UserModule::encrypting($model->password, $model->salt); + $model->verifyPassword=UserModule::encrypting($model->verifyPassword, $model->salt); $model->superuser=0; $model->status=((Yii::app()->controller->module->activeAfterRegister)?User::STATUS_ACTIVE:User::STATUS_NOACTIVE); @@ -57,7 +57,7 @@ public function actionRegistration() { } if ((Yii::app()->controller->module->loginNotActiv||(Yii::app()->controller->module->activeAfterRegister&&Yii::app()->controller->module->sendActivationMail==false))&&Yii::app()->controller->module->autoLogin) { - $identity=new UserIdentity($model->username,$soucePassword); + $identity=new UserIdentity($model->username,$sourcePassword); $identity->authenticate(); Yii::app()->user->login($identity,0); $this->redirect(Yii::app()->controller->module->returnUrl); diff --git a/migrations/m121001_001625_userSalt.php b/migrations/m121001_001625_userSalt.php new file mode 100644 index 0000000..99df5a4 --- /dev/null +++ b/migrations/m121001_001625_userSalt.php @@ -0,0 +1,64 @@ +getModule('user')) { + echo "\n\nAdd to console.php :\n" + ."'modules'=>array(\n" + ."...\n" + ." 'user'=>array(\n" + ." ... # copy settings from main config\n" + ." ),\n" + ."...\n" + ."),\n" + ."\n"; + return false; + } + + switch ($this->dbType()) { + case "mysql": + $this->addColumn(Yii::app()->getModule('user')->tableUsers,'salt',"VARCHAR(40) NULL DEFAULT NULL AFTER `password`"); + break; + case "sqlite": + default: + $this->addColumn(Yii::app()->getModule('user')->tableUsers,'salt',"VARCHAR(40) DEFAULT NULL"); + break; + } + } + + public function safeDown() + { + switch ($this->dbType()) { + case "mysql": + $this->dropColumn(Yii::app()->getModule('user')->tableUsers,'salt'); + break; + case "sqlite": + + $this->execute('ALTER TABLE `'.Yii::app()->getModule('user')->tableUsers.'` RENAME TO `'.__CLASS__.'_'.Yii::app()->getModule('user')->tableUsers.'`'); + $this->createTable(Yii::app()->getModule('user')->tableUsers, array( + "id" => "pk", + "username" => "varchar(20) NOT NULL", + "password" => "varchar(128) NOT NULL", + "email" => "varchar(128) NOT NULL", + "activkey" => "varchar(128) NOT NULL", + "superuser" => "int(1) NOT NULL", + "status" => "int(1) NOT NULL", + "create_at" => "TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP", + "lastvisit_at" => "TIMESTAMP", + )); + $this->execute('INSERT INTO `'.Yii::app()->getModule('user')->tableUsers.'` SELECT "id","username","password","email","activkey","superuser","status","create_at","lastvisit_at" FROM `'.__CLASS__.'_'.Yii::app()->getModule('user')->tableUsers.'`'); + $this->dropTable(__CLASS__.'_'.Yii::app()->getModule('user')->tableUsers); + + break; + } + } + + public function dbType() + { + list($type) = explode(':',Yii::app()->db->connectionString); + echo "type db: ".$type."\n"; + return $type; + } +} \ No newline at end of file diff --git a/models/User.php b/models/User.php index 6be7084..9141f66 100644 --- a/models/User.php +++ b/models/User.php @@ -14,6 +14,7 @@ class User extends CActiveRecord * @var integer $id * @var string $username * @var string $password + * @var string $salt * @var string $email * @var string $activkey * @var integer $createtime @@ -121,7 +122,7 @@ public function scopes() 'condition'=>'superuser=1', ), 'notsafe'=>array( - 'select' => 'id, username, password, email, activkey, create_at, lastvisit_at, superuser, status', + 'select' => 'id, username, password, salt, email, activkey, create_at, lastvisit_at, superuser, status', ), ); } @@ -198,13 +199,13 @@ public function setLastvisit($value) { } /** - * simply returns the value of PHP's uniqid(). + * simply returns the md5 digested value of PHP's uniqid() * That should be sufficient to make most rainbowtables useless. * * Modify this function to change the way a salt is generated if you disagree. */ public static function getNewSalt() { - return uniqid(); + return md5(uniqid(mt_rand(), true)); } } \ No newline at end of file diff --git a/models/UserChangePassword.php b/models/UserChangePassword.php index 77a6403..0cad07d 100644 --- a/models/UserChangePassword.php +++ b/models/UserChangePassword.php @@ -39,9 +39,10 @@ public function attributeLabels() */ public function verifyOldPassword($attribute, $params) { - $old_password = User::model()->notsafe()->findByPk(Yii::app()->user->id)->password; - list($old_password, $salt) = explode(":",$old_password); - if ($old_password != Yii::app()->getModule('user')->encrypting($this->$attribute, $salt)) + $old_password = User::model()->notsafe()->findByPk(Yii::app()->user->id); + $old_salt = $old_password->salt; + $old_password = $old_password->password; + if ($old_password != Yii::app()->getModule('user')->encrypting($this->$attribute, $old_salt)) $this->addError($attribute, UserModule::t("Old Password is incorrect.")); } } \ No newline at end of file diff --git a/views/admin/view.php b/views/admin/view.php index 4f2dafe..56b48b8 100644 --- a/views/admin/view.php +++ b/views/admin/view.php @@ -36,9 +36,7 @@ } array_push($attributes, - 'password', 'email', - 'activkey', 'create_at', 'lastvisit_at', array( From f574da4278daa3b0274b950e37eb5ecad4527e65 Mon Sep 17 00:00:00 2001 From: Lennart van den Dool Date: Mon, 1 Oct 2012 10:15:53 +0200 Subject: [PATCH 3/3] Add salt column in schema files. --- data/schema.mysql.sql | 1 + data/schema.sqlite.sql | 3 ++- data/user.sqlite | Bin 7168 -> 7168 bytes 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/data/schema.mysql.sql b/data/schema.mysql.sql index 60fb859..8d969f5 100644 --- a/data/schema.mysql.sql +++ b/data/schema.mysql.sql @@ -2,6 +2,7 @@ CREATE TABLE `tbl_users` ( `id` int(11) NOT NULL AUTO_INCREMENT, `username` varchar(20) NOT NULL, `password` varchar(128) NOT NULL, + `salt` varchar(40) NULL DEFAULT NULL, `email` varchar(128) NOT NULL, `activkey` varchar(128) NOT NULL DEFAULT '', `create_at` TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, diff --git a/data/schema.sqlite.sql b/data/schema.sqlite.sql index 0762f39..9c0db0c 100644 --- a/data/schema.sqlite.sql +++ b/data/schema.sqlite.sql @@ -7,7 +7,8 @@ CREATE TABLE tbl_users ( create_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, lastvisit_at TIMESTAMP NOT NULL DEFAULT '0000-00-00 00:00:00', superuser int(1) NOT NULL DEFAULT '0', - status int(1) NOT NULL DEFAULT '0' + status int(1) NOT NULL DEFAULT '0', + salt varchar(40) DEFAULT NULL ); CREATE TABLE tbl_profiles ( diff --git a/data/user.sqlite b/data/user.sqlite index 96077326b15e9fa6d690f4f44b55525ff2306a85..902a5f591855f86402f459cc9a32248376babd6d 100644 GIT binary patch delta 345 zcmZp$Xt0RBLHCgwD^Q4$`8P;{9Z3C~*x1X+GTB=AC|7d| zGrPFFJY&1~=6bku5ON@J|PNzp*}ubnv<0U zeFcFgv9a(1NhYRe49o>g&o(yRV&ZFLXX0iM_0{z?HfCgGnEaJ3+}KFh&_LI~P{F{$ z%Fxuxz{CIubiqi$z`zQO7#JIQ8Mzt6ea)fz>eyoi(DiM8%`VBYm_>wzhnbOyfsu(Z zgxMP8kxxM91aqSLLXn^^CR+-h7GY!*WB?*IkY9k91>~EFjXx&~N}4jVZg!X4$piqC CvPyOU delta 233 zcmZp$Xt0d9LHCg?D^Q4$`8P;{o%#1>L5?0~K2{c9AeV{h z1p{*-(+i;39VXsJHYP3xQD0qOV`E0f$zRyQjSO`SEOZSG6$~t_jLoeK%nX1)7mO4P z46MM2fw7T?k&8jx*Bqj)jy+ZYUEAi@?2;T@%#6wmjLM9q%+{L)SwtBpGl>RsIy1A2 u%gZyig>JscxQlVJrTFQ^ECMWSjEsT|jDkRA?2MBIB~2OGHoHsiWC8&Fax3rv