0008312: Install script shouldn't include password in the form

ID	Project	Category	View Status	Date Submitted	Last Update
0008312	mantisbt	security	public	2007-08-26 05:49	2007-10-04 01:38

Reporter	vboctor	Assigned To	jreese
Priority	normal	Severity	minor	Reproducibility	have not tried
Status	closed	Resolution	fixed
Product Version	1.1.0a4
Target Version	1.1.0rc1	Fixed in Version	1.1.0rc1

Summary	0008312: Install script shouldn't include password in the form
Description	At the moment the install script includes the current password as the default value for the password field. Hence, if an administrator doesn't delete the admin directory, and a hacker find it, then the hacker can View Source and know the passwords. The password should be defaulted to an empty string and the action page should use the current password in case the password was left empty. The other option is to use a CURRENT token.
Tags	No tags attached.
Attached Files	mantis-install-2007-08-26.patch (1,126 bytes) ? diff Index: admin/install.php =================================================================== RCS file: /cvsroot/mantisbt/mantisbt/admin/install.php,v retrieving revision 1.34 diff -u -r1.34 install.php --- admin/install.php 9 Aug 2007 02:59:55 -0000 1.34 +++ admin/install.php 26 Aug 2007 17:08:32 -0000 @@ -80,6 +80,9 @@ $f_database_name = gpc_get( 'database_name', config_get( 'database_name', 'bugtrack') ); $f_db_username = gpc_get( 'db_username', config_get( 'db_username', '' ) ); $f_db_password = gpc_get( 'db_password', config_get( 'db_password', '' ) ); + if ( "______" == $f_db_password ) { + $f_db_password = config_get( 'db_password' ); + } $f_admin_username = gpc_get( 'admin_username', '' ); $f_admin_password = gpc_get( 'admin_password', ''); $f_log_queries = gpc_get_bool( 'log_queries', false ); @@ -454,7 +457,7 @@ Password (for Database) </td> <td> - <input name="db_password" type="password" value="<?php echo $f_db_password ?>"></input> + <input name="db_password" type="password" value="<?php echo ( !is_blank($f_db_password) ? "______" : "" ) ?>"></input> </td> </tr> mantis-install-2007-08-26.patch (1,126 bytes)

jreese 2007-08-26 12:22 reporter ~0015532	Hmm, this raises a good point, but at the same time, the admin directory should not be publicly viewable or accessible like that. Mantis even complains when logging in that the directory should be removed. However, I do feel that it would be beneficial to 'default' the password to using the config-set password instead, and I don't think that would be a very difficult task either. Maybe I can look into this today in some of my downtime.

jreese 2007-08-26 13:13 reporter ~0015534	I modified install.php to use a 'configured password' token of '__' so that the page displays a password in the field when using the password set in config_inc.php, and I've tried installing from scratch and upgrading, both with and without a config_inc file present, and it seems to work as advertised without any problems. I would like to go ahead and commit the changes to CVS, but since a) this is a security issue, and b) the patch seems way too simple, I am a bit apprehensive to commit the patch until someone can verify that it is correct and indeed this simple of a fix.

vboctor 2007-08-26 16:18 manager ~0015535	The change looks fine. Following are my comments: Define a constant for the "__". is_blank($f_db_password) -> is_blank( $f_db_password ) "__" == $f_db_password - I am wondering if we should be using ===. Also I am wondering why we are using gpc_get() rather than gpc_get_string() for user name and password?! Other than that looks fine. The reason I am worried about this is that I noticed that some people rename rather than delete the admin folder. Hence, our warning will disappear but the risk is there! I am neutral as to whether this should be ported to 1.0.x or not.

jreese 2007-08-27 16:35 reporter ~0015540	The use of === is unecessary for this conditional, because it only adds type checking useful when testing against true/false 0/1 values. However, it could only be equal to "__" if it really was a string with the value "__".

jreese 2007-08-27 16:48 reporter ~0015541	Checked patch in to CVS with suggestions from vboctor. Created constant 'CONFIGURED_PASSWORD' and modified install script.

View Issue Details

Activities