[Fusionforge-commits] FusionForge branch 6.0 updated. v6.0.3-31-gbded8e8

Sylvain Beucler beuc-inria at libremir.placard.fr.eu.org
Thu Nov 12 16:45:04 CET 2015


This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "FusionForge".

The branch, 6.0 has been updated
       via  bded8e874e35c60aa1a7a620fb7f4e31f72841e1 (commit)
       via  6e945bff5a16c5013cb08dd835f95f9c6a1a23f9 (commit)
       via  ade24c73539d7f1fa4c50cb71478474185499ba2 (commit)
      from  40d8a4ef372ee3c0fa22a50c227db41990367f3a (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://scm.fusionforge.org/anonscm/gitweb/?p=fusionforge/fusionforge.git;a=commitdiff;h=bded8e874e35c60aa1a7a620fb7f4e31f72841e1

commit bded8e874e35c60aa1a7a620fb7f4e31f72841e1
Author: Sylvain Beucler <sylvain.beucler at inria.fr>
Date:   Thu Nov 12 15:59:57 2015 +0100

    account: drop redundant (and insecure) unsalted MD5 password hashes from the database

diff --git a/src/CHANGES b/src/CHANGES
index 8f151f6..a742cef 100644
--- a/src/CHANGES
+++ b/src/CHANGES
@@ -1,5 +1,6 @@
 FusionForge 6.0.4:
 * Accounts: do not accept digits-only user and group names, to avoid confusion with UID/GID in system commands (Inria)
+* Accounts: drop redundant (and insecure) unsalted MD5 password hashes from the database (Inria)
 * MTA-Exim4: restart exim4 on install
 * Plugin SCM: fix another race condition when creating project with SCM selected (Inria)
 * Plugin SCM Git: improve user matching when computing stats, support git .mailmap (Inria)
diff --git a/src/common/include/User.class.php b/src/common/include/User.class.php
index cd203e7..ddd5376 100644
--- a/src/common/include/User.class.php
+++ b/src/common/include/User.class.php
@@ -407,10 +407,9 @@ class GFUser extends Error {
 		// if we got this far, it must be good
 		$confirm_hash = substr(md5($password1.util_randbytes().microtime()), 0, 16);
 		db_begin();
-		$result = db_query_params('INSERT INTO users (user_name,user_pw,unix_pw,realname,firstname,lastname,email,add_date,status,confirm_hash,mail_siteupdates,mail_va,language,timezone,unix_box,address,address2,phone,fax,title,ccode,theme_id,tooltips,shell)
-							VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16,$17,$18,$19,$20,$21,$22,$23,$24)',
+		$result = db_query_params('INSERT INTO users (user_name,unix_pw,realname,firstname,lastname,email,add_date,status,confirm_hash,mail_siteupdates,mail_va,language,timezone,unix_box,address,address2,phone,fax,title,ccode,theme_id,tooltips,shell)
+							VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16,$17,$18,$19,$20,$21,$22,$23)',
 			array($unix_name,
-				md5($password1),
 				account_genunixpw($password1),
 				htmlspecialchars($firstname.' '.$lastname),
 				htmlspecialchars($firstname),
@@ -850,12 +849,12 @@ Use one below, but make sure it is entered as the single line.)
 	 * @return	string	This user's MD5-crypted passwd.
 	 */
 	function getMD5Passwd() {
-		return $this->data_array['user_pw'];
+		exit(_('MD5 obsoleted'));
 	}
 
 	//Added to be compatible with codendi getUserPw function
 	function getUserPw() {
-		return $this->data_array['user_pw'];
+		exit(_('MD5 obsoleted'));
 	}
 
 	/**
@@ -1394,12 +1393,10 @@ Use one below, but make sure it is entered as the single line.)
 		}
 
 		db_begin();
-		$md5_pw = md5($passwd);
 		$unix_pw = account_genunixpw($passwd);
 
-		$res = db_query_params('UPDATE users SET user_pw=$1, unix_pw=$2 WHERE user_id=$3',
-					array($md5_pw,
-					       $unix_pw,
+		$res = db_query_params('UPDATE users SET unix_pw=$1 WHERE user_id=$2',
+					array($unix_pw,
 					       $this->getID()));
 
 		if (!$res || db_affected_rows($res) < 1) {
@@ -1433,19 +1430,8 @@ Use one below, but make sure it is entered as the single line.)
 	 * @return	boolean	success.
 	 */
 	function setMD5Passwd($md5) {
-		db_begin();
-		if ($md5) {
-			$res = db_query_params('UPDATE users SET user_pw=$1 WHERE user_id=$2',
-				array($md5, $this->getID()));
-
-			if (!$res || db_affected_rows($res) < 1) {
-				$this->setError(_('Error: Cannot Change User Password:').' '.db_error());
-				db_rollback();
-				return false;
-			}
-		}
-		db_commit();
-		return true;
+		exit(_('Error: Cannot Change User Password:').' '._('MD5 obsoleted'));
+		return false;
 	}
 
 	/**
diff --git a/src/common/include/session.php b/src/common/include/session.php
index aa3c7e9..9bd3e38 100644
--- a/src/common/include/session.php
+++ b/src/common/include/session.php
@@ -217,109 +217,66 @@ function session_check_credentials_in_database($loginname, $passwd, $allowpendin
 function session_login_valid_dbonly($loginname, $passwd, $allowpending) {
 	global $feedback, $userstatus;
 
-	// Try to get the users from the database using user_id and (MD5) user_pw
+	// Selecting by user_name/email only
 	if (forge_get_config('require_unique_email')) {
-		$res = db_query_params ('SELECT user_id,status,unix_pw FROM users WHERE (user_name=$1 OR email=$1) AND user_pw=$2',
-					array ($loginname,
-					       md5($passwd))) ;
+		$res = db_query_params ('SELECT user_id,status,unix_pw FROM users WHERE user_name=$1 OR email=$1',
+								array ($loginname)) ;
 	} else {
-		$res = db_query_params ('SELECT user_id,status,unix_pw FROM users WHERE user_name=$1 AND user_pw=$2',
-					array ($loginname,
-					       md5($passwd))) ;
+		$res = db_query_params ('SELECT user_id,status,unix_pw FROM users WHERE user_name=$1',
+								array ($loginname)) ;
 	}
 	if (!$res || db_numrows($res) < 1) {
-		// No user whose MD5 passwd matches the MD5 of the provided passwd
-		// Selecting by user_name/email only
-		if (forge_get_config('require_unique_email')) {
-			$res = db_query_params ('SELECT user_id,status,unix_pw FROM users WHERE user_name=$1 OR email=$1',
-						array ($loginname)) ;
-		} else {
-			$res = db_query_params ('SELECT user_id,status,unix_pw FROM users WHERE user_name=$1',
-						array ($loginname)) ;
-		}
-		if (!$res || db_numrows($res) < 1) {
-			// No user by that name
-			$error_msg = _('Invalid Password Or User Name');
-			return false;
-		} else {
-			// There is a user with the provided user_name/email, but the MD5 passwds do not match
-			// We'll have to try checking the (crypt) unix_pw
-			$usr = db_fetch_array($res);
-			$userstatus = $usr['status'] ;
-
-			if (crypt($passwd, $usr['unix_pw']) != $usr['unix_pw']) {
-				// Even the (crypt) unix_pw does not patch
-				// This one has clearly typed a bad passwd
-				$error_msg = _('Invalid Password Or User Name');
-				return false;
-			}
-			// User exists, (crypt) unix_pw matches
-			// Update the (MD5) user_pw and retry authentication
-			// It should work, except for status errors
-			$res = db_query_params ('UPDATE users SET user_pw=$1 WHERE user_id=$2',
-						array (md5($passwd),
-						       $usr['user_id'])) ;
-			return session_check_credentials_in_database($loginname, $passwd, $allowpending) ;
-		}
+		// No user by that name
+		$error_msg = _('Invalid Password Or User Name');
+		return false;
 	} else {
-		// If we're here, then the user has typed a password matching the (MD5) user_pw
-		// Let's check whether it also matches the (crypt) unix_pw
+		// There is a user with the provided user_name/email
+		// Check the (crypt) unix_pw
 		$usr = db_fetch_array($res);
+		$userstatus = $usr['status'] ;
 
-		if (crypt ($passwd, $usr['unix_pw']) != $usr['unix_pw']) {
-			// The (crypt) unix_pw does not match
-			if ($usr['unix_pw'] == '') {
-				// Empty unix_pw, we'll take the MD5 as authoritative
-				// Update the (crypt) unix_pw and retry authentication
-				// It should work, except for status errors
-				$res = db_query_params ('UPDATE users SET unix_pw=$1 WHERE user_id=$2',
-							array (account_genunixpw($passwd),
-									$usr['user_id'])) ;
-				return session_check_credentials_in_database($loginname, $passwd, $allowpending) ;
-			} else {
-				// Invalidate (MD5) user_pw, refuse authentication
-				$res = db_query_params ('UPDATE users SET user_pw=$1 WHERE user_id=$2',
-							array ('OUT OF DATE',
-									 $usr['user_id'])) ;
-				$warning_msg =_('Invalid Password Or User Name');
-				return false;
-			}
+		if ($usr['unix_pw'] !== crypt($passwd, $usr['unix_pw'])) {
+			// (crypt) unix_pw does not patch
+			$error_msg = _('Invalid Password Or User Name');
+			return false;
 		}
+		// User exists, (crypt) unix_pw matches
+	}
 
-		// Yay.  The provided password matches both fields in the database.
-		// Let's check the status of this user
 
-		// if allowpending (for verify.php) then allow
-		$userstatus = $usr['status'];
-		if ($allowpending && ($usr['status'] == 'P')) {
-			//1;
-		} else {
-			if ($usr['status'] == 'S') {
-				//acount suspended
-				$feedback = _('Account Suspended');
-				return false;
-			}
-			if ($usr['status'] == 'P') {
-				//account pending
-				$feedback = _('Account Pending');
-				return false;
-			}
-			if ($usr['status'] == 'D') {
-				//account deleted
-				$feedback = _('Account Deleted');
-				return false;
-			}
-			if ($usr['status'] != 'A') {
-				//unacceptable account flag
-				$feedback = _('Account Not Active');
-				return false;
-			}
-		}
-		// create a new session
-		session_set_new(db_result($res, 0, 'user_id'));
+	// Yay.  The provided password matches both fields in the database.
+	// Let's check the status of this user
 
-		return true;
+	// if allowpending (for verify.php) then allow
+	$userstatus = $usr['status'];
+	if ($allowpending && ($usr['status'] == 'P')) {
+		//1;
+	} else {
+		if ($usr['status'] == 'S') {
+			//acount suspended
+			$feedback = _('Account Suspended');
+			return false;
+		}
+		if ($usr['status'] == 'P') {
+			//account pending
+			$feedback = _('Account Pending');
+			return false;
+		}
+		if ($usr['status'] == 'D') {
+			//account deleted
+			$feedback = _('Account Deleted');
+			return false;
+		}
+		if ($usr['status'] != 'A') {
+			//unacceptable account flag
+			$feedback = _('Account Not Active');
+			return false;
+		}
 	}
+	// create a new session
+	session_set_new(db_result($res, 0, 'user_id'));
+
+	return true;
 }
 
 /**
diff --git a/src/db/20151112-drop-unsalted-md5.sql b/src/db/20151112-drop-unsalted-md5.sql
new file mode 100644
index 0000000..c616af0
--- /dev/null
+++ b/src/db/20151112-drop-unsalted-md5.sql
@@ -0,0 +1,2 @@
+-- Drop unsecure copy of unix_pw
+ALTER TABLE users DROP user_pw;
diff --git a/src/post-install.d/db/populate.sh b/src/post-install.d/db/populate.sh
index cc16c6f..7d13d64 100755
--- a/src/post-install.d/db/populate.sh
+++ b/src/post-install.d/db/populate.sh
@@ -94,9 +94,9 @@ req="SELECT COUNT(*) FROM users WHERE user_name='admin'"
 if [ "$(echo $req | su - postgres -c "psql -At $database_name")" != "1" ]; then
     psql -h $database_host -p $database_port -U $database_user $database_name <<EOF >/dev/null
 INSERT INTO users (user_name, realname, firstname, lastname, email,
-    user_pw, unix_pw, status, theme_id)
+    unix_pw, status, theme_id)
   VALUES ('admin', 'Forge Admin', 'Forge', 'Admin', 'root at localhost.localdomain',
-    'INVALID', 'INVALID', 'A', (SELECT theme_id FROM themes WHERE dirname='funky'));
+    'INVALID', 'A', (SELECT theme_id FROM themes WHERE dirname='funky'));
 EOF
     forge_make_admin admin  # set permissions
     # Note: no password defined yet
diff --git a/src/www/account/change_pw.php b/src/www/account/change_pw.php
index 003ef70..75c7504 100644
--- a/src/www/account/change_pw.php
+++ b/src/www/account/change_pw.php
@@ -46,7 +46,7 @@ if (getStringFromRequest('submit')) {
 	$passwd = getStringFromRequest('passwd');
 	$passwd2 = getStringFromRequest('passwd2');
 
-	if ($u->getMD5Passwd() != md5($old_passwd)) {
+	if ($u->getUnixPasswd() !== crypt($old_passwd, $u->getUnixPasswd())) {
 		form_release_key(getStringFromRequest('form_key'));
 		exit_error(_('Old password is incorrect'),'my');
 	}
diff --git a/src/www/account/index.php b/src/www/account/index.php
index ef85b9e..420ee40 100644
--- a/src/www/account/index.php
+++ b/src/www/account/index.php
@@ -83,16 +83,6 @@ if (getStringFromRequest('submit')) {
 	}
 
 	if ($check) {
-/*
-//needs security audit
-	if ($remember_user) {
-		// set cookie, expire in 3 months
-		setcookie("sf_user_hash",$u->getID().'_'.substr($u->getMD5Passwd(),0,16),time()+90*24*60*60,'/');
-	} else {
-		// remove cookie
-		setcookie("sf_user_hash",'',0,'/');
-	}
-*/
 		// Refresh page if language or theme changed
 		$refresh = ($language != $u->getLanguage() || $theme_id != $u->getThemeID());
 
diff --git a/tests/func/10_Site/loginTest.php b/tests/func/10_Site/loginTest.php
index 1c338f7..4489eb6 100644
--- a/tests/func/10_Site/loginTest.php
+++ b/tests/func/10_Site/loginTest.php
@@ -111,8 +111,8 @@ class LoginProcess extends FForge_SeleniumTestCase
 		$this->type("passwd", FORGE_OTHER_PASSWORD);
 		$this->type("passwd2", FORGE_OTHER_PASSWORD);
 		$this->clickAndWait("submit");
-		$this->logout();
 
+		$this->logout();
 		$this->open( ROOT );
 		$this->click("link=Log In");
 		$this->waitForPageToLoad("30000");
@@ -122,6 +122,32 @@ class LoginProcess extends FForge_SeleniumTestCase
 		$this->waitForPageToLoad("30000");
 		$this->assertTrue($this->isTextPresent("Forge Admin"));
 		$this->assertTrue($this->isTextPresent("Log Out"));
+
+		// Test changing password through user account
+		$this->clickAndWait("link=My Account");
+		$this->clickAndWait("link=Change Password");
+		$this->type("old_passwd", "awrongpassword");
+		$this->type("passwd", FORGE_ADMIN_PASSWORD);
+		$this->type("passwd2", FORGE_ADMIN_PASSWORD);
+		$this->clickAndWait("submit");
+		$this->assertTrue($this->isTextPresent("Old password is incorrect"));
+
+		$this->clickAndWait("link=My Account");
+		$this->clickAndWait("link=Change Password");
+		$this->type("old_passwd", FORGE_OTHER_PASSWORD);
+		$this->type("passwd", FORGE_ADMIN_PASSWORD);
+		$this->type("passwd2", FORGE_ADMIN_PASSWORD);
+		$this->clickAndWait("submit");
+
+		$this->logout();
+		$this->open( ROOT );
+		$this->click("link=Log In");
+		$this->waitForPageToLoad();
+		$this->type("form_loginname", FORGE_ADMIN_USERNAME);
+		$this->type("form_pw", FORGE_ADMIN_PASSWORD);
+		$this->click("login");
+		$this->waitForPageToLoad();
+		$this->assertTrue($this->isTextPresent("Forge Admin"));
 		$this->assertTrue($this->isTextPresent("Log Out"));
 	}
 }

https://scm.fusionforge.org/anonscm/gitweb/?p=fusionforge/fusionforge.git;a=commitdiff;h=6e945bff5a16c5013cb08dd835f95f9c6a1a23f9

commit 6e945bff5a16c5013cb08dd835f95f9c6a1a23f9
Author: Sylvain Beucler <sylvain.beucler at inria.fr>
Date:   Thu Nov 12 15:32:33 2015 +0100

    account: don't display printf return value

diff --git a/src/www/account/lostpw.php b/src/www/account/lostpw.php
index 72d1aaa..591769a 100644
--- a/src/www/account/lostpw.php
+++ b/src/www/account/lostpw.php
@@ -71,7 +71,8 @@ if (getStringFromRequest('submit')) {
 
 		$HTML->header(array('title'=>_('Lost Password Confirmation')));
 
-		echo '<p>'.printf(_('An email has been sent to the address you have on file. Follow the instructions in the email to change your account password.').'</p><p><a href="%s">'._("Home").'</a>', util_make_url ('/')).'</p>';
+		echo '<p>'._('An email has been sent to the address you have on file. Follow the instructions in the email to change your account password.').'</p>';
+		printf('<p><a href="%s">'._("Home").'</a></p>', util_make_url ('/'));
 
 		$HTML->footer();
 		exit();

https://scm.fusionforge.org/anonscm/gitweb/?p=fusionforge/fusionforge.git;a=commitdiff;h=ade24c73539d7f1fa4c50cb71478474185499ba2

commit ade24c73539d7f1fa4c50cb71478474185499ba2
Author: Sylvain Beucler <sylvain.beucler at inria.fr>
Date:   Thu Nov 12 15:29:53 2015 +0100

    testsuite: don't fail if apache is initially stopped - just start it

diff --git a/tests/func/fixtures.sh b/tests/func/fixtures.sh
index 82ec7d6..9b703d7 100755
--- a/tests/func/fixtures.sh
+++ b/tests/func/fixtures.sh
@@ -126,7 +126,7 @@ fi
 if [ "$reset" = 1 ]; then
     set -e
     # Reset connections
-    stop_apache --force
+    stop_apache --force || true
     service fusionforge-systasksd stop
     service postgresql restart
     su - postgres -c "dropdb $database" || true

-----------------------------------------------------------------------

Summary of changes:
 src/CHANGES                           |   1 +
 src/common/include/User.class.php     |  30 ++------
 src/common/include/session.php        | 139 ++++++++++++----------------------
 src/db/20151112-drop-unsalted-md5.sql |   2 +
 src/post-install.d/db/populate.sh     |   4 +-
 src/www/account/change_pw.php         |   2 +-
 src/www/account/index.php             |  10 ---
 src/www/account/lostpw.php            |   3 +-
 tests/func/10_Site/loginTest.php      |  28 ++++++-
 tests/func/fixtures.sh                |   2 +-
 10 files changed, 92 insertions(+), 129 deletions(-)
 create mode 100644 src/db/20151112-drop-unsalted-md5.sql


hooks/post-receive
-- 
FusionForge



More information about the Fusionforge-commits mailing list