Page 1 of 1

Comment on my semi complete login php script.

Posted: Tue Jan 09, 2007 5:13 pm
by badgers
here is the result of the help I received here with some adds of my own.

I welcome any comments on this

Code: Select all

<?php
	if (!isset($_SESSION)) { session_start(); }
/// Quote variable to make safe-from mysql site 
function quote_smart($value)
{
   // Stripslashes
   if (get_magic_quotes_gpc()) {
       $value = stripslashes($value);
   }
   // Quote if not a number or a numeric string
   if (!is_numeric($value)) {
       $value = "'" . mysql_real_escape_string($value) . "'";
   }
   return $value;
}

//Connect database.
$host="localhost"; // Host name.
$db_user="mysqltv"; // MySQL username.
$db_password="pass"; // MySQL password.
$database="users"; // Database name.
$link = mysql_connect($host,$db_user,$db_password);
if (!$link) {
   die('Could not connect: ' . mysql_error());
}
mysql_select_db($database);//should there be some sort of error checking here?

// Make a safe query
$query = sprintf("SELECT * FROM member WHERE username=%s AND password=md5(%s)", 
	quote_smart($_POST['username']), 
	quote_smart($_POST['password']));

$result = mysql_query($query);
if(!mysql_num_rows($result)){
	//wrong username or password
	header("location:register.html");//send them to the register page.
	die;
}

//I want to put the sessid in the database for the user
session_start();
session_register($_POST['username']); // Craete session username.
$sessionID = session_id(  );
$query = "UPDATE member set session = '$sessionID' where username = '$_POST[username]' ";
$result = mysql_query($query);

//get the ip address
$ip=$_SERVER['REMOTE_ADDR'];
$query = "UPDATE member set ip = '$ip' where username = '$_POST[username]' ";
$result = mysql_query($query);


header("location:lgwk.html");//send them to the good page.

//from the mysql site it seemed like this was good practice.
mysql_close($link);
?>
the database has is capturing the data from what phpmyAdmin is telling me.

thanks[/syntax]

Posted: Tue Jan 09, 2007 9:08 pm
by aaronhall
A few things:

1. session_register() is deprecated; use the $_SESSION superglobal instead.
2. You forgot to escape $_POST['username'] in your update queries.
3. Array indices always need to be quoted, e.g. $_POST['username'] instead of $_POST[username]
4. mysql_close() isn't necessary as of PHP4
[s]5. There is no need is check isset($_SESSION) -- this will always return true, unless this file is being included into another context that may have already called session_start()[/s]

Posted: Tue Jan 09, 2007 9:31 pm
by Luke

Code: Select all

header("location:register.html");//send them to the register page.
It is considered bad practice to use relative urls when redirecting with headers. Also it is against standards. Also, I would use a function like:

Code: Select all

function redirect($url)
{
    if (!headers_sent())
    {
        header('Location: ' . $url);
    }
    exit;
}
Always a good idea to check if headers have already been sent before trying to send them

Posted: Wed Jan 10, 2007 6:33 am
by badgers
thanks

Posted: Wed Jan 10, 2007 7:36 am
by CoderGoblin
I would change

Code: Select all

$sessionID = session_id(  );
$query = "UPDATE member set session = '$sessionID' where username = '$_POST[username]' ";
$result = mysql_query($query);

//get the ip address
$ip=$_SERVER['REMOTE_ADDR'];
$query = "UPDATE member set ip = '$ip' where username = '$_POST[username]' ";
$result = mysql_query($query);
to

Code: Select all

// Process form
  $username=quote_smart($_POST['username']); // use this value anywhere you interact with db
  ...
  ...
  ...  
  // No point in two updates when you can do it with one.
  $sessionID = session_id();
  $ip=$_SERVER['REMOTE_ADDR'];
  $query = "UPDATE member set session = '$sessionID', ip='$ip' where username = '$username'";
  $result = mysql_query($query);
Personally I would hate to mistype the login and be redirected to a register page. I would much rather be told "unable to log in. If you do not have an accout go to <registration>".

Also I would hope your user table has a primary key, Preferably an auto incrementing integer. I would suggest just getting this integer from the select and storing it in the session (unless you use other information) as this should be the value you need to interact with the database rather than 'username'. "SELECT *" can be slow if you have lots of additional columns and primary keys should be integers if possible for speed. Setting the session variable allows you to check for the existance of this value whenever you need to check if someone is logged in.

Example: At the start of this script. If user has already logged in redirect them to the good page.

Code: Select all

<?php
session_start();
if (!empty($_SESSION['user_id'])) {
  header('location:www.mysite.com/good_page.php');
  exit;
}
...
?>
Hope that hasn't confused you too much

Posted: Wed Jan 10, 2007 8:21 am
by matthijs

Code: Select all

$sessionID = session_id();
  $ip=$_SERVER['REMOTE_ADDR'];
  $query = "UPDATE member set session = '$sessionID', ip='$ip' where username = '$username'";
  $result = mysql_query($query);
I was wondering, isn't $sessionID tainted in this case?

If a hacker doesn't sent a cookie but instead yoursite.com/?PHPSESSID=1234 (+ sql injection stuff ..)
So shouldn't we also escape the $sessionID? (and $ip for that matter)

Posted: Wed Jan 10, 2007 10:28 am
by Oren
aaronhall wrote:5. There is no need is check isset($_SESSION) -- this will always return true, unless this file is being included into another context that may have already called session_start()
No - it will return false unless session_start() was called before the check.

Posted: Wed Jan 10, 2007 4:21 pm
by Mordred
There are lots of wrong things with this code, I'm afraid.

0. session_start() is called twice, and since that is supposed at least to give you a warning (I can't recall the exact error level), this means that you're coding with error reporting turned off, which is not a good idea. This is not a security problem, just exremely bad practice.

1. You must use session_regenerate_id before logging to prevent session fixation.

2. There are several vectors for SQL injection in the code, and in the adding of unescaped session_id later there is another, as matthijs pointed out. In fact, every single query you wrote is vulnerable. (Btw $ip should be escaped only on the basis that everything should be escaped, it is otherwise safe, to my knowledge, as it is filled by the server, i.e. it doesn't directly come from the user.)

The thing is (and it is quite often missed by newbies) that mysql_real_escape_string() is not enough to protect against SQL injection (hi, Oren :P)
We have SQL injection when syntactic elements are inserted in the SQL command. Using an unescaped quote to prematurely finish one quoted value is one way to do it. Just inserting your elements when there are no quotes is another.

3. Keeping md5-s of passwords is not secure enough. Combined with an SQL injection, this would easily disclose login data. You must salt the hash at least once to properly secure it. I personally recommend two salt sources, one in the database, one in the PHP source.

There are also some cryptorgaphic problems with md5(), but I don't think they apply here. Still, there are other, more secure hash functions, including sha256() which feyd has posted implemented in pure PHP somewhere in the code snippets forum.

4. This is not seen in the posted code, but I hope you're checking that the user is properly logged in the 'good guy' page, or otherwise your entire login sytem can be bypassed by just going to the URL...

5. There are additional things to be considered, like bruteforce protection, session timeouts, challenge-request protocols etc, which are not entirely on-topic here, but you might as well give them a reading.

Posted: Wed Jan 10, 2007 7:11 pm
by aaronhall
Oren wrote:No - it will return false unless session_start() was called before the check.
You're right -- looking back on it, the entire sentence doesn't make sense. Scratch that
Mordred wrote:The thing is (and it is quite often missed by newbies) that mysql_real_escape_string() is not enough to protect against SQL injection (hi, Oren :P)
We have SQL injection when syntactic elements are inserted in the SQL command. Using an unescaped quote to prematurely finish one quoted value is one way to do it. Just inserting your elements when there are no quotes is another.
Let's make this clear, though -- mysql_real_escape_string() DOES adequately protect against injection attacks, as long as your query's syntax is correct. Leaving string values unquoted is improper syntax, and will usually result in an error. What newbies don't realize is that ANY $_GET or $_POST values should be considered strings, even if an integer/float is expected, and even if the column type is integer/float. In those cases that you don't quote $_GET or $_POST values in a query (even if the column type is an integer/float), mysql_real_escape_string won't save you.

Posted: Thu Jan 11, 2007 2:35 am
by matthijs
For anyone interested here's a small test:

Code: Select all

<?php
// first connect to db here ..

// then the tests
$id = "2; DELETE FROM mytest";
// $id is: 2; DELETE FROM mytest
// not what we want ...

$id2 = mysql_real_escape_string($id);
// $id2 is: 2; DELETE FROM mytest
// see, doesn't make a difference ...

// cast to an integer
$id3 = (int)"2; DELETE FROM mytest"; 
// $id3 is: 2
// that's better ...

// without quotes:
$sql = "SELECT * FROM mytest WHERE id={$id}";
// SELECT * FROM mytest WHERE id=2; DELETE FROM mytest
// when run:
// You have an error in your SQL syntax; check the manual that corresponds to your MySQL server 
// version for the right syntax to use near '; DELETE FROM mytest' at line 1

// with quotes:
$sql2 = "SELECT * FROM mytest WHERE id='{$id}'";
// SELECT * FROM mytest WHERE id='2; DELETE FROM mytest'
// when run:
// returns normal result, one single line in this case. mysql doesn't run second query, however it's not a valid query

// "escaped":
$sql3 = "SELECT * FROM mytest WHERE id='{$id2}'";
// SELECT * FROM mytest WHERE id='2; DELETE FROM mytest'
// when run:
// returns normal result, one single line in this case. mysql doesn't run second query, however it's not a valid query

// cast to integer:
$sql4 = "SELECT * FROM mytest WHERE id='{$id3}'";
// SELECT * FROM mytest WHERE id='2'
// when run:
// returns one row
?>

Posted: Thu Jan 11, 2007 3:12 am
by Mordred
mysql_real_escape_string() DOES adequately protect against injection attacks, as long as your query's syntax is correct
Generally yes, in almost all practical cases this is true. This is not theoretically true though. In some non-mysql database systems this belief may even be catastrophical :)
The true statement is that "mysql_real_escape_string() protects against injection attacks on field values in SQL statements". There are other syntactic elements which will not be protected by mysql_real_escape_string(), some of which are often controlled by the user request, namely the LIMIT parameters (but possibly others too!).

Injections in LIMIT parameters can (to my knowledge) be exploited only in limited circumstances, I will try to write a POC about this later.

Even without injecting correct SQL, an attacker can benefit much from displayed error messages, which, depending on how carelessly the system was installed, can reveal anything from "nothing", to call stack dumps with filenames and full SQL statements.

Another possible injection problem is injecting LIKE or REGEXP special characters which can help mount a DoS attack on the server.

Posted: Thu Jan 11, 2007 10:27 am
by Mordred
Here it is, a quick and dirty example, don't use it as a sample for good coding, hehe ;)

In the example I will use a database called test, which has a table login, with a column named username (and arbitrary other columns of course). The two defined functions, Dump1 and Dump2 are two silly usage cases, the important thing is that they use a parameter (normally coming from the request) which will be escaped properly. The queries used are also syntactically valid and well-defined (I think).

You will see that the second query is immune to the injection, because of the ORDER BY clause.

Code: Select all

<?php

mysql_connect();
mysql_select_db('test');

function Dump1($nCount) {
	$nCount = mysql_real_escape_string($nCount);
	$sQuery = "SELECT * FROM `login` WHERE 1 LIMIT 0, $nCount";
	echo "<br>$sQuery<br>";
	$result = mysql_query($sQuery);
	echo mysql_error();
	while ($row = mysql_fetch_array($result, MYSQL_ASSOC)) {
	   var_dump($row);
	}
}

function Dump2($nCount) {
	$nCount = mysql_real_escape_string($nCount);
	$sQuery = "SELECT * FROM `login` WHERE 1 ORDER BY `username` LIMIT 0, $nCount";
	echo "<br>$sQuery<br>";
	$result = mysql_query($sQuery);
	echo mysql_error();
	while ($row = mysql_fetch_array($result, MYSQL_ASSOC)) {
	   var_dump($row);
	}
}


Dump1('1');
echo '<hr>';
Dump1('1 UNION ALL SELECT * FROM `login`');
echo '<hr>';
Dump2('1');
echo '<hr>';
Dump2('1 UNION ALL SELECT * FROM `login`');
?>
Output:

Code: Select all

SELECT * FROM `login` WHERE 1 LIMIT 0, 1
-- (1 user)
----
SELECT * FROM `login` WHERE 1 LIMIT 0, 1 UNION ALL SELECT * FROM `login`
-- (all users)
----
SELECT * FROM `login` WHERE 1 ORDER BY `username` LIMIT 0, 1
-- (1 user)
----
SELECT * FROM `login` WHERE 1 ORDER BY `username` LIMIT 0, 1 UNION ALL SELECT * FROM `login`
-- SQL error: Incorrect usage of UNION and ORDER BY

Posted: Fri Jan 12, 2007 12:26 am
by aaronhall
Mordred wrote:There are other syntactic elements which will not be protected by mysql_real_escape_string(), some of which are often controlled by the user request, namely the LIMIT parameters (but possibly others too!).
You're absolutely right - I did overlook those values that are syntactically correct to be unquoted. In those instances, mysql_real_escape_string will not help you.