[SOLVED] Login Script, need help

Xargo · March 16, 2007

Hello,

I've got a question according login scripts. This is the first time I ever made a working login script, based on a tutorial I've read. It works, but I'm not certain if it's safe. Could you guys give your professional opinion? :-) I'll give you some explanation, although I think this will not be needed because my methods are quite common.

1. Challenge and response

I worked with a challenge and response. The server creates a challenge (random combination) every time the user loads the page, sends it to the client and stores it in a session variable as well (so both the client and server have a copy of it). When the user fills in his username and password the php scripts creates a response using the challenge and some other data, which is a SHA1 creation.

The response is composed like this:

SHA1( username + SHA1( password ) + challenge )

This is send to the server by the client. When the server receives all the information it first checks whether the username exists, and if the password is correct. Then it composes a response itself with the username, password and challenge it stored in the session variable (thus the challenge is in no way sent from the client to the server, the challenge is something personal for both the client and server), if this newly created response is similar to the response the client send, he is logged in.

In short you could say there's a random composition which is never send from the client to the server or vice versa, so that a hacker interrupting and reading a data pack between the two computers doesn't have the random number which he will also need to ever access any private page.

2. IP-address checking

This should work against session hijacking. It simply stores the IP-address of the person who is about to login. When the user continues to another page, by submitting his username and password or simply going to a private page when is already logged in, the server always checks the remote IP-address and compares it to the first IP-address.

3. Use of htmlentities()

This is very common, just using htmlentities() for all user input, so that a malicious person won't be able to use any of the SQL symbols and mess up my tables.

I've also added a few question within the code itself using comments.

Ok, so here is my code:

<?php
session_start();
?>

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
<title>Login</title>
</head>

<body>

<?php

// Database information
$db_loc = "...";
$db_user = "...";
$db_pass = "...";
$db_name = "...";

// Avoid session hijacking, checks if the session variable exists, and compares the IP-address stored in that variable with the IP-address of the current requester.
if( isset( $_SESSION["current_ip"] ) AND $_SESSION["current_ip"] != $_SERVER["REMOTE_ADDR"] )
{
session_unset();
session_destroy();
die("\n</body>\n</html>"); // Is this a decent method to die?
}

$link = mysql_connect( $db_loc, $db_user, $db_pass );
if (!$link)
{
trigger_error(mysql_error()); // Is this a decent method to report an error? =/
}
@mysql_select_db( $db_name ) or trigger_error(mysql_error() ); // In the tutorial the guy added a @ in front of it, which I think stops error reporting, but why?

// This is the PHP function which generates a random combination
function random( $length )
{
  srand( ( (double) microtime() ) * 1000000 );
  $string = "";

  $symbols = "abcdefghijklmnopqrstuvwxyz";
  $symbols .= "ABCDEFGHIJKLMNOPQRSTUWXYZ";
  $symbols .= "01234567890123456789";

  for( $i = 0; $i < $length; $i++ )
  {
    $string .= $symbols{ rand( 0, ( strlen( $symbols ) - 1 ) ) };
  }

  return $string;
}

// Checks if the user submitted, and checks if he still has his own composed challenge in his session variable
if( isset( $_POST["username"], $_POST["response"], $_SESSION["challenge"] ) )
{
$tempUsername = htmlentities($_POST["username"]); // Checks for any malicious symbols in the username
if ($tempUsername != $_POST["username"])
{
	mysql_close($link);
	session_unset();
	session_destroy();
	die("Incorrect username or password.\n</body>\n</html>");
}

// I don't use htmlentities() for the password, because it will never be send to the MySQL database.

$query = "SELECT * FROM users WHERE username = '" . $_POST['username'] . "' LIMIT 0,1"; // Checks whether the username exists.
$result = mysql_query( $query ) or trigger_error(mysql_error());
if( mysql_num_rows( $result ) == 0 )
{
	mysql_close($link);
	session_unset();
	session_destroy();
	die("Incorrect username or password.\n</body>\n</html>");
}
else
{
	$row = mysql_fetch_assoc( $result );

	// Now he creates a response using the username and password from the user,
	// and the challenge which it stored itself. He compares it with the response generated by the client.
	if( sha1( $row['username'] . ":" . $row['password'] . ':' . $_SESSION['challenge'] ) != $_POST['response'] )
	{
		mysql_close($link);
		session_unset();
		session_destroy();
		die("Incorrect username or password.\n</body>\n</html>");
	}
	else
	{
		// Successfully logged in. The username is stored in the session.
		echo "Successfully logged in.";

		$_SESSION["username"] = $row["username"];
	}
}
}

// Checks whether the current IP-address is already stored, if not, store it.
if( isset( $_SESSION['current_ip'] ) == FALSE )
{
$_SESSION['current_ip'] = $_SERVER['REMOTE_ADDR'];
}

// He creates a random string here, which he stores in the session variable.
$challenge = random( 255 );
$_SESSION['challenge'] = $challenge;

// Closing MySQL connection
mysql_close($link);
?>

<script type="text/javascript" src="sha1.js"></script> <!-- Javascript of SHA1. Is it a problem that anyone can read it? -->

<script type="text/javascript">
<!--
function createResponse() // The function for the client-side to create a response.
{
  document.getElementById( 'response' ).value = hex_sha1( document.getElementById( 'username' ).value + ":" + hex_sha1( document.getElementById( 'password' ).value ) + ":" + document.getElementById( 'challenge' ).value );
  document.getElementById( 'password' ).value = "";
  document.getElementById( 'challenge' ).value = "";
  
  return true;
}
//-->
</script>

<!-- The form itself, it calls the createResponse() function when submitted, and echoes the challenge the server generated for the client. -->
<form action="login.php" method="post" onsubmit="return createResponse();"> 
<div>
	Username: <input type="text" name="username" id="username">
	Wachtwoord: <input type="password" id="password">
	<input type="hidden" id="challenge" value="<?php echo $challenge; ?>">
	<input type="hidden" name="response" id="response" value="">
	<input type="submit" value="Log in !">
</div>
</form>

</body>
</html>

Thank you very much for reading, and it would be great if you can give your opinion. :-)

Xargo · March 17, 2007

Oh yeh, forgot this. This is my script on other private pages to check if the user is logged in.

<?php
session_start();

if ( !isset($_SESSION["current_ip"]) or !isset($_SESSION["username"]) or $_SESSION['current_ip'] != $_SERVER['REMOTE_ADDR'] )
{
session_unset();
session_destroy();
die("You are not authorized to access this page.</body></html>");
}
?>

And this one is to logout.

<?php

session_start();
session_unset();
session_destroy();

echo "You are now logged out.";

?>

Glyde · March 17, 2007

Answers to your questions:

1. It's not that it's a "bad" way to use die(), it's just that you should work around using die() at all in your functions. The die() function, in my opinion, shouldn't be used on a page that a user actually views. You should use a stable combination of if and else statements. Plus, then you don't have to worry about having an ugly ending to your page if the die() function is called.

2. trigger_error(mysql_error()) is only not a decent way to trigger an error because it's PHP's default. It's an ugly error output. You should make a nicely formatted textarea or a table on your site that gives error details, and possibly even uses PHP's mail function to send them to the webmaster.

3. The guy did put a @ in front of it to stop error reporting. However, seeing that he has the "or" statement on the line, it's not needed. PHP will run the mysql_select_db function. If it sees the @ in front, it won't output the error. However, you can leave that out, and PHP will also search for the "or" keyword, which indicates what PHP should do if an error is encountered.

It's not so much that script isn't safe, but this script gives me the impression that it probably wasn't that great of a tutorial, or it's just very outdated. You really shouldn't have HTML and PHP running in the same page. It's not a bad thing, it just makes your code really ugly. Ever since I started using forums, I realized templates are the way to go. On the current side I'm building, I use xml templates. For example, all of the pages that relate to the account page are in an account.xml file. Then, in that file, it's formatted something like:

<template:templatename>
// HTML here
// Something like {lang.name} would load a string from the language array (predefined)
// Something like {input.username} would be parsed and replaced with $_POST['username']
</template>

The template system also supports <if> statements. I find it to be a lot stronger, then, since the templates support if statements, I really don't have to do ANYTHING relating to HTML in my PHP page.

Xargo · March 17, 2007

Ok, thank you very much for all the remarks.

Frankly I've never worked with XML or templates yet. But thanks for the information, I'll keep it in mind and take a look at the uses of it.

Sign In

[SOLVED] Login Script, need help

Recommended Posts

Xargo

Link to comment

Share on other sites

Xargo

Link to comment

Share on other sites

Glyde

Link to comment

Share on other sites

Xargo

Link to comment

Share on other sites

Archived

Browse

Activity

Important Information