sql injection

[ngreenwood6]

I have been pretty much in a testing code stage up until this point. I have gotten pretty good at coding and figuring out my problems but now I wanted to know if someone could give me a brief explanation of what sql injection is an example of how it could be done and how I can check to see if it is possible on my site.

[premiso]

Someone inputs on an unchecked/unescaped form this:

' OR 1=1 AND

 

From there it returns a row and can essentially validate them. That is a "good" example. They can wipe out entire databases if that is allowed.

 

The best protection against SQL injection: mysql_real_escape_string on any data going into a DB, along with a good validation of inputted data.

[ngreenwood6]

how could someone do this:

 

Someone inputs on an unchecked/unescaped form this:

 

' OR 1=1 AND

 

 

If I am the one making the query

[premiso]

Let's say you do not check your data going into the DB.

 

$sql = "SELECT * FROM table_name WHERE username = '" . $_POST['username'] . "'";

 

On the post form username, I add the ' OR 1=1 OR x='x  for the username the query now becomes

 

SELECT * FROM table_name WHERE username = '' OR 1=1 OR x='x'

 

So now, if this is a login function, it will return a valid useranme and essentially "login" this random person to the site.

[ngreenwood6]

oh ok I have got you so as long as I use mysql_real_escape_string() on all the variables that are used in my query and that are going into the database it should be fine.

[premiso]

oh ok I have got you so as long as I use mysql_real_escape_string() on all the variables that are used in my query and that are going into the database it should be fine.

 

That and I would read through this post to regarding "magic_quotes" as if they are on that will fubar your data.

 

http://www.phpfreaks.com/forums/index.php/topic,230771.0.html

 

But I would not say do it to "all data". You should know what the data is, if you expect an INT, check that the variable is an int and treat it as such using the (int) cast method. If you expect a string, then do the escape on the string.

 

Also if you want to avoid XSS injection html_entities on data that should not be including html/js data.

[ngreenwood6]

thanks for the help. I am hoping you can clarify one more thing for me though. I have been told to disable register_globals can you tell me why? Also, I know that if I disable it my scripts wont run can you tell me what I am doing that requires it (I am just doing basic register and login scripts).

[premiso]

thanks for the help. I am hoping you can clarify one more thing for me though. I have been told to disable register_globals can you tell me why? Also, I know that if I disable it my scripts wont run can you tell me what I am doing that requires it (I am just doing basic register and login scripts).

 

register_globals is a security flaw. Basically it creates variables from global variables such as $_POST and $_GET, which you should define your own variables from this.

 

What this can do, say you have a script with a session of 'loggedin' set to check if someone is logged in. I could call the url like:

 

http://www.yoursite.com/index.php?loggedin=1

 

Since register_globals is on, and if you do not set $loggedin = $_SESSION['loggedin'] and just check if ($loggedin) I am now an active member on your site.

 

That is why register_globals is now off to force people to properly code and define their variables before assuming that they are what they should be.

[ngreenwood6]

So if I understood your post correctly this is incorrect:

 

if($_POST['name'] == "nick")
{
wrong
}

 

But if I do that like this:

 

$name = $_POST['name'];

if($name == "nick")
{
right
}

 

then I shouldn't have to have it on?

[premiso]

So if I understood your post correctly this is incorrect:

 

if($_POST['name'] == "nick")
{
wrong
}

 

But if I do that like this:

 

$name = $_POST['name'];

if($name == "nick")
{
right
}

 

then I shouldn't have to have it on?

 

Calling it using $_POST['name'] is just fine, it is when you would call it by $name without any $_POST definition etc is where you get in trouble.

 

<?php

if ($name == "nick") 
    echo 'Hello Nick!';
?>

 

That is wrong.

 

<?php
$name = isset($_POST['name'])?$_POST['name']:'';

if ($name == "nick") 
    echo 'Hello Nick!';
else
    echo 'Not Logged in!';
?>

 

Would be a truly proper and safe usage. Checking the isset, will help thwart notice errors (not fatal but can be inefficient).

 

Really the main security flaw, as far as I understand it, is with the session variables.

 

 

<?php
session_start();
// you have a $_SESSION['name'] that determines if the user is logged in.
if ($name) 
    echo 'Hello Nick!';
?>

 

Calling the above with http://www.yoursite.com/index.php?name=1 would evalutate to true even though they are not logged in.

That is wrong.

 

<?php
session_start();
$name = isset($_SESSION['name'])?$_SESSION['name']:false;

if ($name) 
    echo 'Hello Nick!';
else
    echo 'Not Logged in!';
?>

 

That would prevent them from using GET data to validate themselves.

[ngreenwood6]

I get what you are saying but the thing that I dont get is why I always have to enable register_globals when I code. I do it just like you said to do it using the $_POST and defining the $_SESSION because I didnt know you could just do $name lol. Is there another reason it would require me to have it turned on (using wampserver if that makes any difference).

[premiso]

What happens when you turn off register_globals? What breaks?

[ngreenwood6]

My login page will not let me login anymore.

 

The error code is this:

Notice: Undefined variable: password in C:\wamp\www\techs\login.php on line 9

Notice: Undefined variable: username in C:\wamp\www\techs\login.php on line 18

Notice: Undefined variable: username in C:\wamp\www\techs\login.php on line 32
Please enter a username! 

 

It works fine if it is turned on.

[premiso]

Post your code, it seems you are not properly setting password or username following the methods I laid out.

 

Post the first 50 lines of that code and I will correct it for you so you can see what you would need to do for the rest of the script.

 

 

[ngreenwood6]

Here it is:

 

<?php
//include the variables
include("../vars.php");

//set the variables from the form
foreach($_POST as $value);

//encrypt the password
$encrypted_password = md5($password);

//connect to the server
$conn = mysql_connect($host, $db_user, $db_pass);

//connect to the database
mysql_select_db($db);

//make the query
$query = "SELECT * FROM login WHERE username = '$username'";

//get the results
$results = mysql_query($query);

//count the number of rows
$count = mysql_num_rows($results);

//if the username exists fetch the results
if ($count = 1)
{
$row = mysql_fetch_array($results);
}

if(!$username)
{
echo "Please enter a username!";
}
elseif(!$password)
{
echo "Please enter a password!";
}
elseif($username != $row['username'])
{
echo "Please enter a valid username!";
}
elseif($encrypted_password != $row['password'])
{
echo "That password is incorrect!";
}
else
{
session_start();
$_SESSION['logged_in'] = TRUE;
$_SESSION['username'] = $username;
$_SESSION['tech_id'] = $row['id'];
$_SESSION['membership'] = $row['membership'];
header("Location:index.php");
}

?>

 

I am no longer using that foreach anymore in my recent coding because I know about the mysql_real_escape_string. Is that causing it?

[premiso]

<?php
session_start(); // put session_start at the top.

//include the variables
include("../vars.php");


//encrypt the password
$encrypted_password = (isset($_POST['password']))?md5($password):false;

//connect to the server
$conn = mysql_connect($host, $db_user, $db_pass);

//connect to the database
mysql_select_db($db);

$username = isset($_POST['username'])?mysql_real_escape_string($username):false;

//make the query
if ($username != false && $encrypted_password != false)
    $query = "SELECT * FROM login WHERE username = '$username' LIMIT 1"; // always limit your queries if you only are expecting 1 row.
else
     die('No username or password was passed in.');

//get the results
$results = mysql_query($query);

//count the number of rows
$count = mysql_num_rows($results);

//if the username exists fetch the results
if ($count > 0) {
$row = mysql_fetch_assoc($results);

if(!$username) {
	echo "Please enter a username!";
}elseif(!$password) {
	echo "Please enter a password!";
}elseif($username != $row['username']) {
	echo "Please enter a valid username!";
}elseif($encrypted_password != $row['password']) {
	echo "That password is incorrect!";
}else {

	$_SESSION['logged_in'] = TRUE;
	$_SESSION['username'] = $username;
	$_SESSION['tech_id'] = $row['id'];
	$_SESSION['membership'] = $row['membership'];
	header("Location:index.php");
}
}else {
echo 'Invalid username.';
}
?>

 

That way if no username or password was passed in we do not check the script. Your username that you are querying is now preventing sql injection, and if no rows are returned we echo out that the username was invalid.

 

Any questions on it let me know.

[ngreenwood6]

I tried your code exactly how you typed it and this is what I am getting now:

 

Notice: Undefined variable: password in C:\wamp\www\techs\login.php on line 8

Notice: Undefined variable: username in C:\wamp\www\techs\login.php on line 16
No username or password was passed in.

[premiso]

lol sorry.

 

$encrypted_password = (isset($_POST['password']))?md5($_POST['password']):false;

 

<?php
session_start(); // put session_start at the top.

//include the variables
include("../vars.php");


//encrypt the password
$encrypted_password = (isset($_POST['password']))?md5($_POST['password']):false;

//connect to the server
$conn = mysql_connect($host, $db_user, $db_pass);

//connect to the database
mysql_select_db($db);

$username = isset($_POST['username'])?mysql_real_escape_string($_POST['username']):false;

//make the query
if ($username != false && $encrypted_password != false)
    $query = "SELECT * FROM login WHERE username = '$username' LIMIT 1"; // always limit your queries if you only are expecting 1 row.
else
     die('No username or password was passed in.');

//get the results
$results = mysql_query($query);

//count the number of rows
$count = mysql_num_rows($results);

//if the username exists fetch the results
if ($count > 0) {
   $row = mysql_fetch_assoc($results);
   // removed the password/username checks due to the check above.
   if($username != $row['username']) {
      echo "Please enter a valid username!";
   }elseif($encrypted_password != $row['password']) {
      echo "That password is incorrect!";
   }else {      
      $_SESSION['logged_in'] = TRUE;
      $_SESSION['username'] = $username;
      $_SESSION['tech_id'] = $row['id'];
      $_SESSION['membership'] = $row['membership'];
      header("Location:index.php");
   }
}else {
   echo 'Invalid username.';
}
?>

 

That should work. It was a silly mistake by me.

[ngreenwood6]

So basically in order to not use register globals if I am getting this is to never call $_POST. Register the $_POST as a variable and never call it again. By the way that worked. It would be the same with sessions I assume.

[premiso]

So basically in order to not use register globals if I am getting this is to never call $_POST. Register the $_POST as a variable and never call it again. By the way that worked. It would be the same with sessions I assume.

 

I think you are missing it. You were never setting $username or $password the $_POST variable. This is why the script was not working. In the old script you just assumed that $password would be populated instead of making sure it was populated.

 

Your old line:

$encrypted_password = md5($password);

 

This was not good practice because if register_globas gets turned off, $password now does not have a value because $password was never set. However if you did it this way:

 

$password = (isset($_POST['password']))?$_POST['password']:false;
$encrypted_password = ($password != false)?md5($password):false;

 

The above would not throw an error because you are defining $password to be equal to $_POST['password'] if that value has been set. If that value has not been set we make it a default of false, so we can check if that value was passed through or not.

 

Make sense? Before you were never defining $username or $password, you were just assuming that register_globals would always be on =)

[ngreenwood6]

oh I thought that I was defining them before with my foreach loop:

 

foreach($_POST as $value);

 

I thought was outputting this:

 

$password = $_POST['password'];

 

Apparently that does not set them and it is just getting the global value. I understood what you were saying but apparently my coding was faulty lol. Thanks for the help.

[premiso]

oh I thought that I was defining them before with my foreach loop:

 

foreach($_POST as $value);

 

I thought was outputting this:

 

$password = $_POST['password'];

 

Apparently that does not set them and it is just getting the global value. I understood what you were saying but apparently my coding was faulty lol. Thanks for the help.

 

Right, and it is not good to loop through $_POST like that and set variables. You never know what someone may try to inject. That is why you should know the variables coming in and should define them as so.

 

IMO this is where PHP is flawed. It is "lazy" on variables. In most other languages you have to define a variable before you can use it. If PHP required that it would make it a bit more secure just because you have to know what values you want to use.

[ngreenwood6]

Well now that I know the whole global variable thing I can definitely prevent that. Thanks for the patience and help premiso you are definitely an asset to this community.