[patch] Account's password hashing algorithm selection

adakraz · Aug 16, 2010

The following patch creates new $config['password_type'] which makes it possible to use password hashing based on different algorithm than currently used

Notes:
- patch is against trunk.198
- If you don't have 'password_type' in your config, you'll get some confusing error. I didn't know the proper way to solve this (suggestions or patches welcome).
- this patch adds two methods to OTS_Account POT class. If you used recent POT, it would probably be more right to push these changes upstream, but you use some very old version, so this change is effectively a fork.
- thorough the code you use two conventions for accessing config values: global $config; or require('config.php');. I like the former better, but you should definitely decide which one to use.
- spl_autoload is moved from constructor to main body, so you don't have to create POT instances to see OTS_* classes (adopted from upstream).
- as I look through queries in account_model, they all use $_POST or function arguments pasted directly into queries. If you don't sanitize the data, that is an absolutely critically fatal security hole. If you sanitize the data, it is still pretty bad, because these things should apear close to query, so I can be sure it is all secured. The correct approach would be to use POT, provided you synchronize with POT upstream.
- removed some bogus error_reporting(0) from rss_fetch (did not show up in SourceForge.net Repository - [magpierss] Log of /magpierss/rss_fetch.inc )
- how can I get real svn read access to Modern AAC?

Code:

diff -ur ./API/createAccount.php /var/www/maac/API/createAccount.php
--- ./API/createAccount.php	2010-05-11 21:48:58.000000000 +0200
+++ /var/www/maac/API/createAccount.php	2010-08-16 13:36:31.000000000 +0200
@@ -25,7 +25,7 @@
 	die(json_encode(array('status'=>ACCOUNTNAME_TAKEN)));
 }
 $name = $account->createNamed($_GET['accountName']);
-$account->setPassword(sha1($_GET['password']));
+$account->setHashedPassword($_GET['password'], $config['password_type']);
 $account->setEmail($_GET['email']);
 $account->setCustomField('premdays', PREMDAYS);
 try {
diff -ur ./config.php /var/www/maac/config.php
--- ./config.php	2010-07-16 00:02:58.000000000 +0200
+++ /var/www/maac/config.php	2010-08-16 13:08:36.000000000 +0200
@@ -13,6 +13,10 @@
 
 /*Name of server*/
 $config['server_name'] = "%SERVER_NAME%";
+
+// Type of the password the server uses (same value from config.lua)
+// legal values: plain, md5, sha1
+$config['password_type'] = 'md5';
 
 /*End of most important configs*/
 
diff -ur ./system/application/controllers/account.php /var/www/maac/system/application/controllers/account.php
--- ./system/application/controllers/account.php	2010-08-10 19:57:17.000000000 +0200
+++ /var/www/maac/system/application/controllers/account.php	2010-08-16 13:13:24.000000000 +0200
@@ -127,7 +127,8 @@
 					$ots->connect(POT::DB_MYSQL, connection());
 					$account = new OTS_Account();
 					$name = $account->createNamed($_POST['name']);
-					$account->setPassword(sha1($_POST['password']));
+					global $config;
+					$account->setHashedPassword($_POST['password'], $config['password_type']);
 					$account->setEmail($_POST['email']);
 					$account->setCustomField('nickname', $_POST['nickname']);
 					$account->setCustomField('premdays', PREMDAYS);
Tylko w ./system/application/controllers: config.php
diff -ur ./system/application/libraries/POT/OTS_Account.php /var/www/maac/system/application/libraries/POT/OTS_Account.php
--- ./system/application/libraries/POT/OTS_Account.php	2010-04-20 00:32:16.000000000 +0200
+++ /var/www/maac/system/application/libraries/POT/OTS_Account.php	2010-08-13 14:29:15.000000000 +0200
@@ -34,6 +34,19 @@
 class OTS_Account extends OTS_Row_DAO implements IteratorAggregate, Countable
 {
 /**
+ * Hashes plaintext password for storing in the database.
+ *
+ * @param string $pass plaintext password
+ * @param string $type hash type, may be md5, sha1 and plain (no hashing)
+ */
+    public static function hashPassword($pass, $type){
+        if ($type=='plain') return $pass;
+        elseif ($type=='md5') return md5($pass);
+        elseif ($type=='sha1') return sha1($pass);
+        else throw new UnexpectedValueException('Illegal password type');
+    }
+
+/**
  * Account data.
  * 
  * @var array
@@ -532,7 +545,7 @@
  * </p>
  * 
  * <p>
- * Remember that this method just sets database field's content. It doesn't apply any hashing/encryption so if OTServ uses hashing for passwords you have to apply it by yourself before passing string to this method.
+ * This method just sets password field's content and doesn't apply any hashing/encryption. To hash the password before storing it, use setHashedPassword().
  * </p>
  * 
  * @param string $password Password.
@@ -543,6 +556,16 @@
     }
 
 /**
+ * Sets account's password, hashing it before storing.
+ * 
+ * It simply applies OTS_Account::hashPassword before storing the result. See setPassword().
+ */
+    public function setHashedPassword($password, $method)
+    {
+        $this->data['password'] = OTS_Account::hashPassword((string) $password, $method);
+    }
+
+/**
  * E-mail address.
  * 
  * <p>
diff -ur ./system/application/libraries/POT/OTS.php /var/www/maac/system/application/libraries/POT/OTS.php
--- ./system/application/libraries/POT/OTS.php	2010-04-20 00:32:16.000000000 +0200
+++ /var/www/maac/system/application/libraries/POT/OTS.php	2010-08-16 12:37:25.000000000 +0200
@@ -345,31 +345,6 @@
     }
 
 /**
- * Class initialization tools.
- * 
- * <p>
- * Never create instance of this class by yourself! Use POT::getInstance()!
- * </p>
- * 
- * <p>
- * Note: Since 0.0.2 version this method registers spl_autoload_register() callback. If you use POT with PHP 5.0 you need {@link compat.php compat.php library} to prevent from FATAL errors.
- * </p>
- * 
- * <p>
- * Note: Since 0.0.3 version this method is private.
- * </p>
- * 
- * @version 0.0.3
- */
-    private function __construct()
-    {
-        // default POT directory
-        $this->path = dirname(__FILE__) . '/';
-        // registers POT autoload mechanism
-        spl_autoload_register( array($this, 'loadClass') );
-    }
-
-/**
  * Loads POT class file.
  * 
  * <p>
@@ -1679,6 +1654,10 @@
     }
 }
 
+// default POT directory
+POT::getInstance()->setPOTPath( dirname(__FILE__) . '/');
+// registers POT autoload mechanism
+spl_autoload_register( array(POT::getInstance(), 'loadClass') ); 
 /*
  * This part is for PHP 5.0 compatibility.
  */
diff -ur ./system/application/models/account_model.php /var/www/maac/system/application/models/account_model.php
--- ./system/application/models/account_model.php	2010-08-13 20:06:54.000000000 +0200
+++ /var/www/maac/system/application/models/account_model.php	2010-08-16 13:21:17.000000000 +0200
@@ -9,7 +9,8 @@
 	function check_login() {
 		require("config.php");
 		$this->load->database();
-		$sql = $this->db->query("SELECT `id`, page_access, nickname FROM `accounts` WHERE `name` = '".$_POST['name']."' AND `password` = sha1('".$_POST['pass']."')");
+		$pass=OTS_Account::hashPassword($_POST['pass'], $config['password_type']);
+		$sql = $this->db->query("SELECT `id`, page_access, nickname FROM `accounts` WHERE `name` = '".$_POST['name']."' AND `password` = '".$pass."'");
 		$row = $sql->row_array();
 			if(!empty($row)) {
 			$_SESSION['account_id'] = $row['id'];
@@ -49,13 +50,17 @@
 	}
 	
 	public function checkPassword($pass) {
+		require("config.php");
+		$pass=OTS_Account::hashPassword($pass, $config['password_type']);
 		$this->load->database();
-		return ($this->db->query("SELECT `id` FROM `accounts` WHERE `name` = '".$_SESSION['name']."' AND `password` = sha1('".$pass."')")->num_rows == 0) ? false : true; 
+		return ($this->db->query("SELECT `id` FROM `accounts` WHERE `name` = '".$_SESSION['name']."' AND `password` = '".$pass."'")->num_rows == 0) ? false : true; 
 	}
 	
 	public function changePassword($pass, $name) {
+		require("config.php");
+		$pass=OTS_Account::hashPassword($pass, $config['password_type']);
 		$this->load->database();
-		$this->db->query("UPDATE `accounts` SET `password` = sha1('".$pass."') WHERE `name` = \"".$name."\"");
+		$this->db->query("UPDATE `accounts` SET `password` = '".$pass."' WHERE `name` = \"".$name."\"");
 	}
 	
 	public function isUserPlayer($id) {
@@ -65,15 +70,15 @@
 	
 	public function getPlayerComment($id) {
 		$this->load->database();
-		return $this->db->query("SELECT `comment`, hide_char FROM `players` WHERE `id` = \"".$id."\"")->result_array();
+		return $this->db->query("SELECT `comment`, hide_char FROM `players` WHERE `name` = \"".$id."\"")->result_array();
 	}
 	
 	public function changeComment($id, $comment, $hide = false) {
 		$this->load->database();
 		if($hide == true)
-			$this->db->query("UPDATE `players` SET `comment` = '".$comment."', `hide_char` = 1 WHERE `id` = \"".$id."\"");
+			$this->db->query("UPDATE `players` SET `comment` = '".$comment."', `hide_char` = 1 WHERE `name` = \"".$id."\"");
 		else
-			$this->db->query("UPDATE `players` SET `comment` = '".$comment."', `hide_char` = 0 WHERE `id` = \"".$id."\"");
+			$this->db->query("UPDATE `players` SET `comment` = '".$comment."', `hide_char` = 0 WHERE `name` = \"".$id."\"");
 	}
 	
 	public function deletePlayer($id) {
@@ -101,9 +106,11 @@
 	}
 	
 	public function recoveryAccount($key, $email, $password) {
+		require("config.php");
+		$password=OTS_Account::hashPassword($password, $config['password_type']);
 		$this->load->database();
 		$key = sha1($key);
-		$this->db->query("UPDATE accounts SET password = sha1(\"$password\") WHERE `key` = '$key' AND email = \"$email\"");
+		$this->db->query("UPDATE accounts SET password = \"".$password."\" WHERE `key` = '$key' AND email = \"$email\"");
 	}
 	
 	public function load($id) {
diff -ur ./system/libraries/rss_fetch.inc /var/www/maac/system/libraries/rss_fetch.inc
--- ./system/libraries/rss_fetch.inc	2010-05-03 15:43:40.000000000 +0200
+++ /var/www/maac/system/libraries/rss_fetch.inc	2010-08-16 12:25:52.000000000 +0200
@@ -15,7 +15,7 @@
  * [email protected]
  *
  */
- error_reporting(0);
+
 // Setup MAGPIE_DIR for use on hosts that don't include
 // the current path in include_path.
 // with thanks to rajiv and smarty

Paxton · Aug 16, 2010

I don't really get it, but here u go WebSVN - ModernAAC - Rev 198 - /

adakraz · Aug 17, 2010

PAXTON said:
I don't really get it, but here u go WebSVN - ModernAAC - Rev 198 - /

I wanted <url> that I could svn co <url> so I could svn up to have newest revision downloaded. But this is generally quite unimportant.

What is important is whether you'll accept my changes and what can you say about my 'Notes'. I really, really wait for your reply.

(and I add another (unimportant) note I forgot to add before: You have a file in /trunk/system/application/controllers/config.php which should be deleted)

hodleo · Aug 17, 2010

Paxton · Aug 17, 2010

I will add different hasshes soon. Probably 1.1 or 2.0 if this will be big update as well.

global $config; I started using recently, i was used to require, so I didn't change it. It doesn't really matter.

The $_POST is secured by default + there is another security added, so no worries.

adakraz · Aug 20, 2010

PAXTON said:
I will add different hasshes soon. Probably 1.1 or 2.0 if this will be big update as well.

That is: is my approach wrong or it's the wrong time in development cycle for feature additions?

The $_POST is secured by default + there is another security added, so no worries.

Still $this->db->escape($data) (as in Queries : CodeIgniter User Guide ) in queries would be a nice addition to form validation.

stian · Aug 23, 2010

svn co svn://svn.tech1.org/modernaac

[patch] Account's password hashing algorithm selection

adakraz

New Member

Paxton

Banned User

adakraz

New Member

hodleo

Formerly cbrm -Crypto enthusiast, Retired scripter

Paxton

Banned User

adakraz

New Member

stian

Banned User

Similar threads