Page 1 of 1
Would this be a secure script?
Posted: Mon Sep 15, 2008 4:25 pm
by 8ennett
I've been looking through various methods of preventing security leaks when applied to a user logging in to a site. Is this script below secure enough to prevent SQL injection and any other exploits? If so are there any other security hints I could implement?
Code: Select all
if (isset($_POST['[b]username[/b]']) && isset($_POST['[b]userpass[/b]'])){
$link = mysql_connect('mysql_host', 'mysql_user', 'mysql_password');
$uname = strip_tags($_POST['[b]username[/b]'];
$upass = strip_tags($_POST['[b]userpass[/b]'];
if(!is_resource($link)) {
echo "Failed to connect to the server\n";
}
else {
$query = sprintf("[b]query goes here, remembering to end it with -- and enclose php vars in { }[/b]",
mysql_real_escape_string($uname, $link),
mysql_real_escape_string($upass, $link));
mysql_query($query, $link);
if (mysql_affected_rows($link) > 0) {
echo "[b]do all the login stuff here now[/b]";
}
else {
echo "[b]the user has input incorrect details[/b]";
}
}
}
else {
echo "[b]user hasn't filled in all the inputs[/b]";
}
There may be a bug in syntax somewhere as I haven't tested this on any level yet, but you get the idea!
## EDIT: Just added an extra ) on line 13, forgot to close the sprintf function! ##
Re: Would this be a secure script?
Posted: Mon Sep 15, 2008 6:24 pm
by alex.barylski
Is $query some kind of prepared statement? Enclose PHP variables in {} why?
If it's a prepared statement you don't need to escape the PHP variables cause most systems that support prepared statements do that for you.
Because you are using: mysql_real_escape_string I find it weird you call this after constructing your query. I assume then that you are replacing the {} placeholders with the escaped PHP variables.
Probably secure but a weird practice and weird or bad usually result in security loop holes.
Re: Would this be a secure script?
Posted: Mon Sep 15, 2008 7:13 pm
by 8ennett
Is $query some kind of prepared statement?
If you notice at the beginning of the script it is checking if the username and pass have been posted, these are the variables required for the query to run so it can find a row where both the username and password are on together. Because the username and pass require client input I run the escape string before the query.
Because you are using: mysql_real_escape_string I find it weird you call this after constructing your query.
Yes, it enables mysql_real_escape_string to be executed with the proper connection info through the sprintf function before the query is run.
This script is modelled around example #3 on here
http://uk2.php.net/mysql_real_escape_string just with a few modifications and the strip_tags function included. I thought this was quite a sound construction method and gets the required result. I was just wondering if there should really be anything else in there, something I've overlooked?
Re: Would this be a secure script?
Posted: Mon Sep 15, 2008 7:27 pm
by alex.barylski
I looked at the original example (php.net)
The formatting/convention of storing each mysql_real_escape_string confused me, my bad.
What I seen was:
Code: Select all
$query = sprintf("query goes here, remembering to end it with -- and enclose php vars in { }");
mysql_real_escape_string($uname, $link);
mysql_real_escape_string($upass, $link);
mysql_query($query, $link);
I would personally still assume the default $link as opposed to specifying it manually.
Re: Would this be a secure script?
Posted: Tue Sep 16, 2008 2:47 am
by 8ennett
I would personally still assume the default $link as opposed to specifying it manually.
I suppose you're right, I would have used a custom function in a seperate functions.php script to open and close the db anyway, just need to make sure I do before creating $query.
Re: Would this be a secure script?
Posted: Tue Sep 16, 2008 3:36 am
by Stryks
I'm still not sure why ...
8ennett wrote:"query goes here, remembering to end it with -- and enclose php vars in { }"
Not saying that it's incorrect really .. just wondering why.
Re: Would this be a secure script?
Posted: Tue Sep 16, 2008 12:03 pm
by 8ennett
Well I was reading through the php manual and on the topic of SQL injection there's this statement,
Note: It is common technique to force the SQL parser to ignore the rest of the query written by the developer with -- which is the comment sign in SQL.
Which apparently helps to avoid someone inputting the following in to an input field,
0; DELETE * FROM table
or something similar with the 0; which cancels out the start of the query.
Can't really remember about {} although it does something, but it's on the same page in the manual and I've seen it used in more secure scripts, can't harm to put it in though!
Re: Would this be a secure script?
Posted: Tue Sep 16, 2008 7:10 pm
by Stryks
8ennett wrote:Well I was reading through the php manual and on the topic of SQL injection there's this statement,
Note: It is common technique to force the SQL parser to ignore the rest of the query written by the developer with -- which is the comment sign in SQL.
Which apparently helps to avoid someone inputting the following in to an input field,
0; DELETE * FROM table
or something similar with the 0; which cancels out the start of the query.
Oh ... OK. I see the logic there. But ... if someone passed that string into your query, and your query ended with --, wouldn't their input be inserted before the --, thereby negating it?
As for the {}'s ... I know that they are often used to allow array references to be passed like normal variables in double quoted strings. For example ...
Code: Select all
$test_1 = array('whose'=>'My','appendage'=>'foot');
$test_2 = "smells";
echo "{$test_1['whose']} {$test_1['appendage']} $test_2";
Taking the curly braces away from the one of the array references will throw an error I think. Replacing the double quotes with single quotes will of course print the whole thing as a string. Actually, it'll probably throw an error until you switch the single quotes around the array indexes to double quotes ... but you know what I mean.
But you're right ... adding them around normal variables doesn't seem to cause problems, though I don't think it's strictly required.
Cheers
Re: Would this be a secure script?
Posted: Wed Sep 17, 2008 4:07 am
by 8ennett
Well i feel educated lol thanks!