Secure login..

[dreamwest]

Im really crap at securing my forms, but ive been  experimenting with making login more secure. As well as using htmlspecialchars and strip tags etc to try and clean the input before a database query is performed. This works quite well even with a 50,000 row table.

 

Can anyone see any potential issues with this? I mean they're not actually querying the database directly

 

$user = $_POST['username'];

if($user == ""){

header("Location: /index.php");

}else{

$result = mysql_query( "SELECT username FROM signup " ) ;

while ( $record = mysql_fetch_assoc( $result) ){
    
   $go = $record['username'];
   
    if($user == $go){
$ok = 1;
break;
}
   
}

if($ok != 1){

header("Location: /index.php");

}else{

continue here....

}

}

[Daniel0]

Uh... why don't you do

 

if ($result = mysql_query("SELECT COUNT(*) FROM signup WHERE username = '" . mysql_real_escape_string($_POST['username']) . "'")) {
$res = mysql_fetch_array($result);
$ok = (bool) $res[0];
}
else {
$ok = false;
}

?

 

Instead of linearly searching through all rows? Of course you'll need an index on the username field.

[corbin]

Edit:  Daniel beat me to the WHERE clause on the username query.

 

 

 

 

 

Oh man...

 

 

Have you turned on error displaying and error reporting?  Something like:

 

 

error_reporting(E_ALL | E_STRICT);

ini_set('display_errors', '1');

 

(Or, if you're doing this on a dev box, just set those values in php.ini.)

 

 

 

 

 

Anyway, first off I can't even respond to security since really nothing there involves security....  You did say though that you use htmlspecialchars and strip_tags before inserting into the database....  Don't.  Two reasons:

-That should be done when displaying on the page.  (For example echo htmlentities($row['user_name'])

-Instead of blacklisting (not allowing HTML tags), you should probably white list.  For example, if you only want user names to be able to have a-zA-Z0-9 spaces _-., you can just write a simple regular expression to check that format.

 

 

 

 

 

header("Location: /index.php");

 

 

According to the HTTP specification (and don't worry, you're by no means alone on this... in fact, 99.9% of people get this wrong), Location should always be paired with a complete URI (protocol://host/resource).  I typically use $_SERVER to build the URL dynamically.

 

http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.30

 

 

 

Also, it is typicaly a good idea to put an exit; statement after location:

 

header("Location: http://google.com/");

 

 

In the case of your script it is not important because of the if blocks, but I still do it since something like this can happen:

 

(Remember that Location is just a suggestion to a web browser...  PHP keeps parsing after a header("Location: ...") call)

 

<?php

if(!user_is_admin) {
    header("Location: ...");
}

Pretend a bunch of admin only content is here

 

If the user has his client ignore the location header, suddenly he sees the admin only content.

 

 

 

 

$user = $_POST['username'];

 

Not a security implication at all, but when dealing with user input, you should always make sure the key is set in the array.  (PHP throws a warning when a key is not set in an array.)

 

 

What I usually do:

 

$var = (isset($arr['var'])) ? $arr['var'] : NULL;

 

Or, if I have a particularly long set of variables I need to check:

 

$vars = array('var1', 'var2');

$to_array = array();

foreach($vars as $k) {

    $to_array[$k] = (isset($from_array[$k])) ? $from_array[$k] : NULL;

}

 

 

 

 

$result = mysql_query( "SELECT username FROM signup " ) ;

 

 

Instead of pulling the entire table and looping through it, you can just check for a row with that specific username.  (Luckily WHERE is case insensitive so you don't have to strtolower() everything or anything like that.)

 

 

I typically do something like this:

 

$q = mysql_query("SELECT 1 FROM users WHERE username= '".mysql_real_escape_string($username)."'");

 

 

The mysql_real_escape_string brings me to my next point.

 

 

ALWAYS escape user input when doing a database query unless the data is numeric.

 

If the data is not numeric, you should always use mysql_real_escape_string* (not addslashes...  Addslashes does not escape everything it should....  Also, if magic_slashes is on, always undo the magic slashing and redo escaping with mysql_real_escape_string).

 

 

 

 

*Or prepared statements, or if you use a DB library without prepared statements (or you just don't feel like using prepared statements), make sure the DB library escapes input correctly if it handles token replacement.

 

For example:

 

$db->query("INSERT INTO users (username, password) VALUES (?, ?);", $username, $password);

 

You would want to make sure what ever object $db is an instance of handles quoting/escaping properly.

 

 

As for numeric data types, just cast the data.  If it should be an int, make sure it's an int.  If it should be a float, make sure it's a float.  If it should be a number with 2 decimal place digits (a price), make sure it is.

 

$q = "INSERT INTO scores (user_id, $score) VALUES (".$user_id.", ".((int) $score).");";

 

(In that example, assume $user_id is known to be safe.)

 

 

Obvious it's not as simple as type casting all of the time, but you get the point.  The reason for strictly validating (or in the case of casting, forcing) the data being correctly formatted is twofold:

 

-If something is sent to MySQL (assuming we're talking about MySQL) where the data doesn't match the column type, best case a warning is issues (for example, trying to insert 1.5 into an INT column typically results in a warning), and worst case the query will fail (inserting 'A' into an INT column for example).

-Quotes should technically not be used around numeric values in queries, meaning the data needs to be checked since mysql_real_escape_string is useless (m_r_e_s escapes ' inside of ' so if there is no ' it's useless).  Quotes shouldn't technically be used because anything quoted is a string and when inserting a string into a numeric type (or in a WHERE clause or what ever), the string must be casted to the correct data type.

 

For example in:

 

INSERT INTO t (v) VALUES ('1');

 

Assuming v is an INT column (or anything numeric), the string '1' must be casted to the correct type first.

[dreamwest]

Awesome! Thanks for the info.