secure login script verify

[dsp77]

Hello,

I made an login script, it works but i want to be sure if its secure to use in everyday use, here is the script:

 

<?php
session_start();

require_once('include/config.inc.php');
require_once('include/functions.php');

function clean($str, $encode_ent = false) {
$str  = @trim($str);
if ($encode_ent) {
	$str = htmlentities($str);
}
if (version_compare(phpversion(),'4.3.0') >= 0) {
	if (get_magic_quotes_gpc()) {
		$str = stripslashes($str);
	}
	if (@mysql_ping()) {
		$str = mysql_real_escape_string($str);
	}
	else {
		$str = addslashes($str);
	}
}
else {
	if (!get_magic_quotes_gpc()) {
		$str = addslashes($str);
	}
}
return $str;
}

if (isset($_POST['submit'])) {
if ($_POST['code'] == $_SESSION['rand_code']) {	
	//Sanitize the POST values
	$username = clean($_POST['username']);
	$password = clean($_POST['password']);
	$ip       = clean($_SERVER['REMOTE_ADDR']);

	$query="SELECT * FROM user WHERE username='$username' AND password='".md5($_POST['password'])."'";
	$result=mysql_query($query);

	//Check whether the query was successful or not
	if ($result) {
		if (mysql_num_rows($result) == 1) {
			//Login Successful
			session_regenerate_id();
			$user = mysql_fetch_assoc($result);
			$_SESSION['SESS_MEMBER_ID'] = $user['username'];
			session_write_close();

			$query_login_ok = "INSERT INTO logs (`username`, `password`, `result`, `ip`) VALUES ('$username', '$password', 'SUCCESS', '$ip');";
			$result_query_login_ok = mysql_query($query_login_ok) or die('MYSQL ERROR');

			header("location: pmt.php");

			exit();
		} else {
			//Login failed
			$query_login_fail = "INSERT INTO logs (`username`, `password`, `result`, `ip`) VALUES ('$username', '$password', 'FAILED', '$ip');";
			$result_query_login_fail = mysql_query($query_login_fail) or die('MYSQL ERROR');

			header("location: index.php");

			exit();
		}
	} else {
		die("ERROR");
	}
}
}
?>
<form id="login" name="login" method="post" action="">
  <table width="300" border="0" align="center" cellpadding="2" cellspacing="0">
    <tr>
      <td width="112"><b>Username</b></td>
      <td width="188"><input name="username" type="text" class="textfield" id="username" value="admin" /></td>
    </tr>
    <tr>
      <td><b>Password</b></td>
      <td><input name="password" type="password" class="textfield" id="password" value="qazwsx" /></td>
    </tr>
    <img src="include/captcha.php"/>
    <tr>
      <td><b>Code</b></td>
      <td><input type="text" name="code" /></td>
    </tr>
    <tr>
      <td> </td>
      <td><input type="submit" name="submit" value="Login" /></td>
    </tr>
  </table>
</form>

 

[revraz]

I wouldn't "clean" the password, since you are hashing it.

[the182guy]

You're storing passwords in the log table as plain text that's not secure.

 

Other points:

 

1. bad: if (isset($_POST['submit'])) {

 

2. Calling mysql_ping() everytime you want to clean a var is pointless, just add a check in the db connect script to test for a successful connection.

[dsp77]

Thank you for the answers.

I forgot to remove the $password from succes login.

Removed the clean from password you're right there is no need 2 clean.

why is bad using if (isset($_POST['submit'])) {}

 

i replaced the function clean with:

	function clean($str) {
	$str = @trim($str);
	if(get_magic_quotes_gpc()) {
		$str = stripslashes($str);
	}
	return mysql_real_escape_string($str);
}

[the182guy]

See http://www.phpfreaks.com/forums/php-coding-help/this-code-is-not-ie-compatibe-(shocker-i-know)!/msg1507508/#msg1507508

 

Also, if you must store the password as plaintext in the log table, you do need to clean it because it isn't being hashed.

[Pikachu2000]

If you're storing the password in plaintext, why are you attempting to SELECT it's md5 hashed value?

[the182guy]

If you're storing the password in plaintext, why are you attempting to SELECT it's md5 hashed value?

 

He is storing the password hashed in the users table, but is also inserting each login attempt into the logs table where he stores the password as plaintext. No need to be storing the password in the logs table.

[Pikachu2000]

Didn't notice that it was the log table. Yeah, there's no reason to store the password in plaintext anywhere, IMO. And certainly no need to store it in more places than absolutely necessary.

[dsp77]

the password stored in logs was not intended i just copied the line and forgot 2 remove it, now i have it like this

$query_login_ok = "INSERT INTO logs (`username`, `password`, `result`, `ip`) VALUES ('$username', '*******', 'SUCCESS', '$ip');";

i changed the if (isset($_POST['submit'])) {} to if($_SERVER['REQUEST_METHOD'] == 'POST')

 

Thank you for the help guys, i'm happy now because my script is secure

[dsp77]

in fact i need to clean the password because i insert it into the log file and the script can be vulnerable.