fixed issue with session hijack prevention, token is now bound to an ip.

#1521
luyadev · Oct 9, 2017 · 2702dcb · 2702dcb
1 parent 6ae5121
commit 2702dcb
Show file tree

Hide file tree

Showing 10 changed files with 80 additions and 58 deletions.
diff --git a/modules/admin/src/components/AdminUser.php b/modules/admin/src/components/AdminUser.php
@@ -7,6 +7,7 @@
 use luya\admin\models\UserOnline;
 use yii\web\UserEvent;
 use luya\web\Application;
+use luya\admin\models\UserLogin;
 
 /**
  * AdminUser Component.
@@ -69,25 +70,6 @@ public function onAfterLogin(UserEvent $event)
  {
  Yii::$app->language = $this->getInterfaceLanguage();
  }
-
- public function getSecureSessionId()
- {
- if (Yii::$app->request->isAdmin) {
- return Yii::$app->session->get($this->uniqueHostVariable('secureSessionId'));
- }
- }
-
- public function destroySecureSessionId()
- {
- Yii::$app->session->remove($this->uniqueHostVariable('secureSessionId'));
- }
-
- public function setSecureSessionId($id)
- {
- if (Yii::$app->request->isAdmin) {
- Yii::$app->session->set($this->uniqueHostVariable('secureSessionId'), $id);
- }
- }
 
  /**
  * Return the interface language for the given logged in user.
@@ -104,13 +86,14 @@ public function getInterfaceLanguage()
  */
  public function onBeforeLogout()
  {
- UserOnline::removeUser($this->getId());
+ UserOnline::removeUser($this->id);
 
  $this->identity->updateAttributes([
  'auth_token' => Yii::$app->security->hashData(Yii::$app->security->generateRandomString(), $this->identity->password_salt),
  ]);
 
- $this->destroySecureSessionId();
+ // kill all user logins for the given user
+ UserLogin::updateAll(['is_destroyed' => true], ['user_id' => $this->id]);
  }
 
  /**

diff --git a/modules/admin/src/controllers/DefaultController.php b/modules/admin/src/controllers/DefaultController.php
@@ -11,6 +11,7 @@
 use luya\web\View;
 use luya\helpers\ArrayHelper;
 use yii\helpers\Markdown;
+use luya\admin\models\UserLogin;
 
 /**
  * Administration Controller provides, dashboard, logout and index.
@@ -51,14 +52,13 @@ public function actionIndex()
  ];
  }
 
- // register auth token
- //$this->view->registerJs("var authToken='".Yii::$app->adminuser->identity->authToken ."';", View::POS_HEAD);
- //$this->view->registerJs("var homeUrl='".Url::home(true)."';", View::POS_HEAD);
+ // register i18n
  $this->view->registerJs('var i18n=' . Json::encode($this->module->jsTranslations), View::POS_HEAD);
- //$this->view->registerJs('var helptags=' . Json::encode($tags), View::POS_HEAD);
+
+ $authToken = UserLogin::find()->select(['auth_token'])->where(['user_id' => Yii::$app->adminuser->id, 'ip' => Yii::$app->request->userIP, 'is_destroyed' => false])->scalar();
 
  $this->view->registerJs('zaa.run(function($rootScope) { $rootScope.luyacfg = ' . Json::encode([
-  'authToken' => Yii::$app->adminuser->identity->authToken,
+  'authToken' => $authToken,
  'homeUrl' => Url::home(true),
  'i18n' => $this->module->jsTranslations,
  'helptags' => $tags,

diff --git a/modules/admin/src/controllers/LoginController.php b/modules/admin/src/controllers/LoginController.php
@@ -8,6 +8,7 @@
 use luya\admin\models\LoginForm;
 use luya\admin\Module;
 use luya\admin\base\Controller;
+use luya\admin\models\UserOnline;
 
 /**
  * Login Controller contains async actions, async token send action and login mechanism.
@@ -48,11 +49,13 @@ public function actionIndex()
  $this->registerAsset('\luya\admin\assets\Login');
 
  $this->view->registerJs("
-  $('#email').focus(); 
-  checkInputLabels();
-  observeLogin('#loginForm', '".Url::toAjax('admin/login/async')."', '".Url::toAjax('admin/login/async-token')."');
+ $('#email').focus(); 
+ checkInputLabels();
+ observeLogin('#loginForm', '".Url::toAjax('admin/login/async')."', '".Url::toAjax('admin/login/async-token')."');
  ");
 
+ UserOnline::clearList();
+
  return $this->render('index');
  }
 

diff --git a/modules/admin/src/migrations/m170926_144137_add_admin_user_session_id_column.php b/modules/admin/src/migrations/m170926_144137_add_admin_user_session_id_column.php
@@ -2,11 +2,16 @@
 
 use yii\db\Migration;
 
+/**
+ * 
+ * @todo Remove in 1.0.1 release!
+ * @author Basil Suter <basil@nadar.io>
+ */
 class m170926_144137_add_admin_user_session_id_column extends Migration
 {
  public function safeUp()
  {
-  $this->addColumn('admin_user_login', 'session_id', $this->string()->notNull()->defaultValue(null));
+  $this->addColumn('admin_user_login', 'session_id', $this->string()->notNull()->defaultValue(null)); // default value supports upgrading and sync from previous system.s
  }
 
  public function safeDown()

diff --git a/modules/admin/src/migrations/m171009_083835_add_admin_user_login_destroy_info.php b/modules/admin/src/migrations/m171009_083835_add_admin_user_login_destroy_info.php
@@ -0,0 +1,18 @@
+<?php
+
+use yii\db\Migration;
+
+class m171009_083835_add_admin_user_login_destroy_info extends Migration
+{
+ public function safeUp()
+ {
+ $this->dropColumn('admin_user_login', 'session_id'); // Remove in 1.0.1 release!
+ $this->addColumn('admin_user_login', 'is_destroyed', $this->boolean()->defaultValue(true));
+ }
+
+ public function safeDown()
+ {
+ $this->addColumn('admin_user_login', 'session_id', $this->string()); // Remove in 1.0.1 release!
+ $this->dropColumn('admin_user_login', 'is_destroyed');
+ }
+}
diff --git a/modules/admin/src/models/LoginForm.php b/modules/admin/src/models/LoginForm.php
@@ -109,21 +109,25 @@ public function login()
  if ($this->validate()) {
  $user = $this->getUser();
  $user->detachBehavior('LogBehavior');
- $user->scenario = 'login';
- $user->force_reload = false;
- $user->auth_token = Yii::$app->security->hashData(Yii::$app->security->generateRandomString(), $user->password_salt);
- $user->save();
 
- $sessionId = Yii::$app->security->generateRandomString(64);
-
- Yii::$app->adminuser->setSecureSessionId($sessionId);
+ // update user model
+ $user->updateAttributes([
+ 'force_reload' => false,
+ 'auth_token' => Yii::$app->security->hashData(Yii::$app->security->generateRandomString(), $user->password_salt),
+ ]);
 
+ // kill prev user logins
+ UserLogin::updateAll(['is_destroyed' => true], ['user_id' => $user->id]);
+
+ // create new user login
  $login = new UserLogin([
  'auth_token' => $user->auth_token,
  'user_id' => $user->id,
-  'session_id' => $sessionId,
+  'is_destroyed' => false,
  ]);
  $login->save();
+
+ // refresh user online list
  UserOnline::refreshUser($user->id, 'login');
  return $user;
  } else {

diff --git a/modules/admin/src/models/User.php b/modules/admin/src/models/User.php
@@ -382,7 +382,7 @@ public function getUserLogins()
  */
  public static function findIdentity($id)
  {
- return static::find()->joinWith(['userLogins ul'])->andWhere(['admin_user.id' => $id, 'session_id' => Yii::$app->adminuser->getSecureSessionId(), 'ip' => Yii::$app->request->userIP])->one();
+ return static::find()->joinWith(['userLogins ul'])->andWhere(['admin_user.id' => $id, 'is_destroyed' => false, 'ip' => Yii::$app->request->userIP])->one();
  }
 
  /**

diff --git a/modules/admin/src/models/UserLogin.php b/modules/admin/src/models/UserLogin.php
@@ -13,7 +13,7 @@
  * @property integer $timestamp_create
  * @property string $auth_token
  * @property string $ip
- * @property string $session_id
+ * @property integer $is_destroyed
  */
 final class UserLogin extends ActiveRecord
 {
@@ -51,9 +51,10 @@ public function getUser()
  public function rules()
  {
  return [
- [['user_id', 'timestamp_create', 'auth_token', 'ip', 'session_id'], 'required'],
+ [['user_id', 'timestamp_create', 'auth_token', 'ip'], 'required'],
  [['user_id', 'timestamp_create'], 'integer'],
- [['auth_token', 'session_id'], 'string', 'max' => 120],
+ [['is_destroyed'], 'boolean'],
+ [['auth_token'], 'string', 'max' => 120],
  [['ip'], 'string', 'max' => 15],
  ];
  }
@@ -69,6 +70,7 @@ public function attributeLabels()
  'timestamp_create' => 'Timestamp Create',
  'auth_token' => 'Auth Token',
  'ip' => 'Ip',
+ 'is_destroyed' => 'Is Destroyed',
  ];
  }
 }
diff --git a/modules/admin/src/models/UserOnline.php b/modules/admin/src/models/UserOnline.php
@@ -69,6 +69,15 @@ public function getUser()
 
  // static methods
 
+ /**
+ * Lock the user for an action.
+ * 
+ * @param unknown $userId
+ * @param unknown $table
+ * @param unknown $pk
+ * @param unknown $translation
+ * @param array $translationArgs
+ */
  public static function lock($userId, $table, $pk, $translation, array $translationArgs = [])
  {
  $model = self::findOne(['user_id' => $userId]);
@@ -84,6 +93,11 @@ public static function lock($userId, $table, $pk, $translation, array $translati
  }
  }
 
+ /**
+ * Unlock the user from an action.
+ * 
+ * @param unknown $userId
+ */
  public static function unlock($userId)
  {
  $model = self::findOne(['user_id' => $userId]);
@@ -118,30 +132,23 @@ public static function refreshUser($userId, $route)
  }
 
  /**
- * Remove the given user id if exists.
+ * Remove all rows for a given User Id.
+ * 
  * @param int $userId
  */
  public static function removeUser($userId)
  {
- $model = self::findOne(['user_id' => $userId]);
- if ($model) {
- $model->delete();
- }
+ self::deleteAll(['user_id' => $userId]);
  }
 
  /**
- * @param int $maxIdleTime Default value in seconds is a half hour (30 * 60) = 1800
+ * Clear all users which are not logged in anymore.
+ * 
+ * Default value in seconds is a half hour (30 * 60) = 1800
  */
  public static function clearList()
- {
- $time = time();
-
- $maxIdleTime = YII_ENV_PROD ? 2000 : 4000;
-
- $items = self::find()->where(['<=', 'last_timestamp', $time - $maxIdleTime])->all();
- foreach ($items as $model) {
- $model->delete();
- }
+ { 
+ self::deleteAll(['<=', 'last_timestamp', (time() - YII_ENV_PROD ? 2000 : 4000)]);
  }
 
  /**

diff --git a/modules/cms/src/admin/migrations/m171003_065811_add_class_column_to_block_group_table.php b/modules/cms/src/admin/migrations/m171003_065811_add_class_column_to_block_group_table.php
@@ -12,7 +12,7 @@ class m171003_065811_add_class_column_to_block_group_table extends Migration
  */
  public function safeUp()
  {
- $this->addColumn('cms_block_group', 'class', $this->string()->notNull());
+ $this->addColumn('cms_block_group', 'class', $this->string()->notNull()->defaultValue(null)); // default value supports upgrading and sync from previous system.s
  }
 
  /**