WTF my loginscript
-
I hope I'm not committing some kind of social faux paus by asking this on my first post, but anyway, I need to know if my loginscript that I've made for my kind-of-forum-type-thing is structured okay.
Basically, after entering their username and password in login.php, the variables are POST'ed to verify.php, which first filters them for any occurances of slashes, quotes, or mathematical operators (will this stop injection attacks or do I need to do more?), then assigns them to $username and $password.
Anyway, after this starts the actual username/password to database comparison, which is where I need a bit of advice.
For my login checks, I have the code structured like so://If the password doesn't equal the password in the database, die and give an error,
//otherwise start to log the user in
if($password != $userdetails['password'])
{
sysmsg(
"Incorrect login details",
"The username you entered does not exist, or you entered an incorrect password.",
"Click here to try again",
"login.php");
}//If everything so far is correct, begin the login process
In this code, sysmsg is one of the functions I coded that gives a HTML styled error message and then calls die(); the function is require()'d so if it isn't found on the webhost it won't run the script and break it. $userdetails is a mysql array fetched from the query $query_userdetails which is looking for ID, username, password, and power (privilege level) from users where the username is equal to the filtered $_POST username, with limit 1.
Now, my question is: Do I need to encase my second and third login checks/actions instead ELSEIFs from the first one?
IE, in semi-pseudocode:if(username does not exist)
{give error
}
elseif(username does exist)
{if(password is incorrect)
}
{give error
}
elseif(password is correct)
{log user in
}
Or is using conditional die() statements followed by the next step outside of the IF logic test okay?
-
Inside, not Instead. Online dyslexia claims another victim.
-
Cleaning the user input is totally the wrong way to go about avoiding SQL injection. I believe PHP is particularly crap in this regard but you need to look at prepared statements, parameterised queries or something like that. "Cleaning" your input is a wasted effort.
-
In that case, how's this alternative way of going about it sound?
Instead of cleaning the input, I've also been thinking of a function that completely cancels any script action if it detects any of the injection-related characters in either $_POST variable. Basically...if(verifyinput($_POST['username']) == false or verifyinput($_POST['password']) == false)
{
//error message and stuff if this was real code
}
I guess from a coding/usability standpoint, cleaning the input doesn't really make much sense - it's not like it's easy to hit the quote keys when you're typing your username and password in. Once I've actually done coding this thing (this' the first time I've actually really 'done' PHP), maybe I should post the source here for everyone to comment on / ridicule unmercifully?
-
the correct way to 'clean user input' in PHP is;
if(get_magic_quotes_gpc()) stripslashes($string);
mysql_real_escape_string($string);note that this is for a mysql database, other database have their own specific escape functions.
also, just addslashes() or removing characters will NOT save you, period.
-
My experience w/ SQL injection is limited to mostly a few proof-of-concepts I ran before so to try to understand it a bit better, so could you explain how the removal of quotes and operators won't stop a SQL injection attack? Perhaps a script example or something?
Also, for the actual viewing sections of the site I'm using the standard ?ID=X format to fetch from a database, but since those are strictly numeric i'm using changetype($_GET['ID'], 'integer'); on them. Does this stop any sort of SQL injection possible since it removes everything but numbers?
-
[quote user="Dagon"]My experience w/ SQL injection is limited to mostly a few proof-of-concepts I ran before so to try to understand it a bit better, so could you explain how the removal of quotes and operators won't stop a SQL injection attack? Perhaps a script example or something?
Also, for the actual viewing sections of the site I'm using the standard ?ID=X format to fetch from a database, but since those are strictly numeric i'm using changetype($_GET['ID'], 'integer'); on them. Does this stop any sort of SQL injection possible since it removes everything but numbers?
[/quote]could also use:
http://www.php.net/manual/en/function.mysql-real-escape-string.php which calls into mysql's own escape funtion to make the input safe
There was a long thread on here talking about dynamic string queries with user input versus stored procedures and parameters and it was mentioned again here in this thread..
Using the above library function makes the query safe as long as it's bug free that is, and without going into too much religion on the latter topic, I think it's fine to use dynamic built strings with userinput into a query, as long as you do the escaping.. No matter if you get injected, rooted, or accidently drop your DB, make sure to always make backup of your data, shortening the interval proportionate to how important the data is..
The security you choose is always a function of how much risk you are willing to take weighted against eg. maintainability and budget.. For personal use, I mix stored procedures and dynamic built querystrings.. I'm willing to accept the risk of using above escaping method and keeping a regular backup networked. It's not 100 safe, nothing is, but it is the model that fits my needs for this particular use. I know the risks, and have taken the step to insure a quick restore if it breaks. There is no holy grails or golden paths in IT security, it's all about reducing risks and having a restore plan for various scenarios. If you believe in the absolute statements, you are blinding yourself from evaluating the real risks and thus making a security policy based *not* on how the real world works, but on how you would *want* it to work. Nothing is absolute, evaluate the risk, make policy, implement!
