Changeset 29111

Timestamp:

Jul 28, 2014, 9:27:50 PM (10 years ago)

Author:

plg

Message:

bug 3050: increase security on reset password algorithm.

• reset key has a 1-hour life
• reset key is automatically deleted once used
• reset key is stored as a hash

Thank you effigies for code suggestions

Location:

trunk

Files:

• include/functions_user.inc.php (modified) (1 diff)
• install/db/144-database.php (added)
• password.php (modified) (9 diffs)

trunk/include/functions_user.inc.php

@@ -1467 +1467 @@
     .','.pwg_db_get_recent_period_expression(1,$user['last_photo_date']).')';
 }
-
-/**
- * Returns a unique activation key.
- *
- * @return string
- */
-function get_user_activation_key()
-{
-  while (true)
-  {
-    $key = generate_key(20);
-    $query = '
-SELECT COUNT(*)
-  FROM '.USER_INFOS_TABLE.'
-  WHERE activation_key = \''.$key.'\'
-;';
-    list($count) = pwg_db_fetch_row(pwg_query($query));
-    if (0 == $count)
-    {
-      return $key;
-    }
-  }
-}
-
 ?>

trunk/password.php

@@ -90 +90 @@
   }
 
-  if (empty($userdata['activation_key']))
-  {
-    $activation_key = get_user_activation_key();
-
-    single_update(
-      USER_INFOS_TABLE,
-      array('activation_key' => $activation_key),
-      array('user_id' => $user_id)
-      );
-
-    $userdata['activation_key'] = $activation_key;
-  }
+  $activation_key = generate_key(20);
+
+  list($expire) = pwg_db_fetch_row(pwg_query('SELECT ADDDATE(NOW(), INTERVAL 1 HOUR)'));
+
+  single_update(
+    USER_INFOS_TABLE,
+    array(
+      'activation_key' => pwg_password_hash($activation_key),
+      'activation_key_expire' => $expire,
+      ),
+    array('user_id' => $user_id)
+    );
+  
+  $userdata['activation_key'] = $activation_key;
 
   set_make_full_url();
@@ -113 +115 @@
   $message.= "\r\n\r\n";
   $message.= l10n('To reset your password, visit the following address:') . "\r\n";
-  $message.= get_gallery_home_url().'/password.php?key='.$userdata['activation_key']."\r\n\r\n";
+  $message.= get_gallery_home_url().'/password.php?key='.$activation_key.'-'.urlencode($userdata['email']);
+  $message.= "\r\n\r\n";
   $message.= l10n('If this was a mistake, just ignore this email and nothing will happen.')."\r\n";
 
@@ -144 +147 @@
  * @return mixed (user_id if OK, false otherwise)
  */
-function check_password_reset_key($key)
-{
-  global $page;
-  
+function check_password_reset_key($reset_key)
+{
+  global $page, $conf;
+
+  list($key, $email) = explode('-', $reset_key, 2);
+
   if (!preg_match('/^[a-z0-9]{20}$/i', $key))
   {
@@ -154 +159 @@
   }
 
+  $user_ids = array();
+  
+  $query = '
+SELECT
+  '.$conf['user_fields']['id'].' AS id
+  FROM '.USERS_TABLE.'
+  WHERE '.$conf['user_fields']['email'].' = \''.pwg_db_real_escape_string($email).'\'
+;';
+  $user_ids = query2array($query, null, 'id');
+
+  if (count($user_ids) == 0)
+  {
+    $page['errors'][] = l10n('Invalid username or email');
+    return false;
+  }
+
+  $user_id = null;
+  
   $query = '
 SELECT
     user_id,
-    status
+    status,
+    activation_key,
+    activation_key_expire,
+    NOW() AS dbnow
   FROM '.USER_INFOS_TABLE.'
-  WHERE activation_key = \''.$key.'\'
+  WHERE user_id IN ('.implode(',', $user_ids).')
 ;';
   $result = pwg_query($query);
-
-  if (pwg_db_num_rows($result) == 0)
+  while ($row = pwg_db_fetch_assoc($result))
+  {
+    if (pwg_password_verify($key, $row['activation_key']))
+    {
+      if (strtotime($row['dbnow']) > strtotime($row['activation_key_expire']))
+      {
+        // key has expired
+        $page['errors'][] = l10n('Invalid key');
+        return false;
+      }
+
+      if (is_a_guest($row['status']) or is_generic($row['status']))
+      {
+        $page['errors'][] = l10n('Password reset is not allowed for this user');
+        return false;
+      }
+
+      $user_id = $row['user_id'];
+    }
+  }
+
+  if (empty($user_id))
   {
     $page['errors'][] = l10n('Invalid key');
@@ -169 +215 @@
   }
   
-  $userdata = pwg_db_fetch_assoc($result);
-
-  if (is_a_guest($userdata['status']) or is_generic($userdata['status']))
-  {
-    $page['errors'][] = l10n('Password reset is not allowed for this user');
-    return false;
-  }
-
-  return $userdata['user_id'];
+  return $user_id;
 }
 
@@ -188 +226 @@
 function reset_password()
 {
-  global $page, $user, $conf;
+  global $page, $conf;
 
   if ($_POST['use_new_pwd'] != $_POST['passwordConf'])
@@ -196 +234 @@
   }
 
-  if (isset($_GET['key']))
-  {
-    $user_id = check_password_reset_key($_GET['key']);
-    if (!is_numeric($user_id))
-    {
-      $page['errors'][] = l10n('Invalid key');
-      return false;
-    }
-  }
-  else
-  {
-    // we check the currently logged in user
-    if (is_a_guest() or is_generic())
-    {
-      $page['errors'][] = l10n('Password reset is not allowed for this user');
-      return false;
-    }
-
-    $user_id = $user['id'];
+  if (!isset($_GET['key']))
+  {
+    $page['errors'][] = l10n('Invalid key');
+  }
+  
+  $user_id = check_password_reset_key($_GET['key']);
+  
+  if (!is_numeric($user_id))
+  {
+    return false;
   }
     
@@ -223 +252 @@
     );
 
+  single_update(
+    USER_INFOS_TABLE,
+    array(
+      'activation_key' => null,
+      'activation_key_expire' => null,
+      ),
+    array('user_id' => $user_id)
+    );
+
   $page['infos'][] = l10n('Your password has been reset');
-
-  if (isset($_GET['key']))
-  {
-    $page['infos'][] = '<a href="'.get_root_url().'identification.php">'.l10n('Login').'</a>';
-  }
-  else
-  {
-    $page['infos'][] = '<a href="'.get_gallery_home_url().'">'.l10n('Return to home page').'</a>';
-  }
+  $page['infos'][] = '<a href="'.get_root_url().'identification.php">'.l10n('Login').'</a>';
 
   return true;
@@ -271 +301 @@
 }
 
-if (isset($_GET['key']))
+if (isset($_GET['key']) and !isset($_POST['submit']))
 {
   $user_id = check_password_reset_key($_GET['key']);