[RESOLVED] Injection security check

[fanhamster]

I am wondering if what I have so far looks vulnerable to injection attacks.

I have left out some array echoes and stuff.

Here is the function in my db_configuration file:

function filter($data) {

$data = trim(htmlentities(strip_tags($data)));

if (get_magic_quotes_gpc())
	$data = stripslashes($data);

$data = mysql_real_escape_string($data);

return $data;

}

Here is the login and reg script:

include"db_configuration.php";

if($_POST['reg'] == 'submit')  {

foreach($_POST as $key => $value) {
	$somedata[$key] = filter($value);
}

$loginname = $somedata['loginname'];
$pwd = md5($somedata['pwd']);
$mail = $somedata['mail'];
$name = $somedata['name'];

$dupers = mysql_query("select count(*) as total from mytable where loginname='$loginname'") or die(mysql_error());
list($total) = mysql_fetch_row($dupers);



if ($total < 1)

 {

$insert = 'INSERT into users(loginname, mail, name, pwd) VALUES("'.$loginname.'","'.$mail.'","'.$name.'", "'.$pwd.'")';

mysql_query($insert);

session_start();

}


}


if($_POST['login'] == 'submit')  {

foreach($_POST as $key => $value) {
	$somedata[$key] = filter($value);
}

$loginname = $somedata['loginname'];
$pwd = md5($somedata['pwd']);


$querydb = mysql_query("SELECT * FROM users WHERE 
loginname = '$loginname' AND pwd = '$pwd'");
$dbsays = mysql_fetch_assoc($querydb);


if(mysql_num_rows($query)){

	session_start();

	$_SESSION['loginname'] = $dbsays['loginname'];	
	$_SESSION['pwd'] = $dbsays['pwd'];	

header("Location: page1.php");
exit;
}


}

?>

Then on the admin page I have

foreach($_POST as $key => $value) {
	$somedata[$key] = filter($value);
}

right at the top and just once. I suspect it is better like that, because then there's no chance of escaping twice, but I have seen it coded both ways so I am wondering if anyone knows for sure. Here's the admin page:

include 'db_configuration.php';

foreach($_POST as $key => $value) {
	$somedata[$key] = filter($value);
}

if($_POST['addusr'] == 'submit') {

$loginname = $somedata['loginname'];
$pwd = md5($somedata['pwd']);
$mail = $somedata['mail'];
$name = $somedata['name'];

$dupers = mysql_query("select count(*) as total from mytable where username='$username'") or die(mysql_error());
list($total) = mysql_fetch_row($dupers);

if ($total < 1) {	

$insert = 'INSERT into mytable(loginname, mail, name, pwd) VALUES("'.$loginname.'","'.$mail.'","'.$name.'", "'.$pwd.'")';

mysql_query($insert);

  }

}


if($_POST['addsomething'] == 'submit') {

$addit = $somedata['addit'];

mysql_query("UPDATE mytable SET `somefield` = '$addit'
WHERE mail='$mail'");

mysql_query("update mytable set anotherfield='1' where loginname='$_SESSION[loginname]'");

}

Then finally there's this next page that has a form for entering dates, and a form for users to change their screen names:

include 'db_configuration.php';

if($_POST['date'] == 'submit') {

foreach($_POST as $key => $value) {
	$somedata[$key] = filter($value);
}

$StartDate = $somedata['StartDate'];
$EndDate = $somedata['EndDate'];

mysql_query("update mytable set start='$StartDate' 
where loginname = '$_SESSION[loginname]'");

mysql_query("update mytable set end='$EndDate' where loginname='$_SESSION[loginname]'");



if($_POST['changemyname'] == 'change')  

{

foreach($_POST as $key => $value) {
	$somedata[$key] = filter($value);
}

$dbnm = mysql_query("select name from mytable where username='$_SESSION[username]'");
list($oldname) = mysql_fetch_row($dbnm);

if($oldname === ($somedata['oldname']))
{
$newname = ($somedata['newname']);
mysql_query("update mytable set name='$newname' where loginname='$_SESSION[loginname]'");

}

There is a datepicker that enters the dates into a regular text field in 0000-00-00 format. I am wondering if the filter function I am using is ok with numbers like this.

Thanks for looking.

[dagon]

The mysql extension should not be used for a new project, its had its day, PDO or MYSQLi (if you must) should now be used.

[Derokorian]

As a continuance to what dagon said, prepared statements are the probably the best way to prevent injection.

[Roger_Ramjet]

Apart from using prepared statements, Parametric Stored Procedures will also protect against sql injections: unless of course you do something silly like build a query string inside the stored procedure and then execute it, which defeats the point of using a stored procedure in the first place.

[johanafm]

Also, if you don't use prepared statements and thus have to sanitize input, do it properly according to the type of each item. An int should not be escaped like a string. A string should not be handled like an int etc.

$safe_int = (int) $_POST['input_is_always_of_type_string_but_this_one_is_stored_as_int_in_database'];

Also, I recommend never ever assigning values to POST and GET (and to never user _REQUEST). If you never ever assign anything to those, you will never ever assume that they are safe to use, nor will you ever by accident escape data in them twice.

Also note that escaping something for one purpose may then mean you'd first have to unescape it for the last purpose before escaping it for another purpose.

$mysqli = new mysqli('ip', 'user', 'pass', 'schema');

$d = array(
	"O'Neil",
);
echo '<h3>Original incoming data</h3>';
print_r($d); echo '<br/>';


foreach ($d as &$v)
{
	$v = $mysqli->real_escape_string($v);
}
echo '<br/><h3>Escaped</h3>';
print_r($d); echo '<br/>';


echo '<br/><h3>htmlentities</h3>';
for ($i = 0; $i < count($d); ++$i)
{
	$d[$i] = htmlentities($d[$i], ENT_QUOTES, 'utf-8');
}

print_r($d); echo '<br/>';

But if you really really still wish to do modify POST and GET, at least do it in one single place and make sure its always done, i.e. specify auto_prepend_file in php.ini. But what if I might not want it every single time? Simple, don't modify POST and GET to begin with.

[bradgrafelman]

As johanafm points out, there is no single escaping technique that you can apply to all types of data - it simply isn't that easy.

Second, I have issues with this:

$data = trim(htmlentities(strip_tags($data)));

None of those three function calls have anything to do with SQL-related sanitizing, so why are they grouped together with a function that is? Furthermore, I personally would never modify the data like that while storing it in the data - I would instead use those functions where appropriate when retrieving and displaying the data from the database.

Otherwise, if I had a secure password like:

<My Password Is Safe!>

the result of that line of PHP code above would result in a password of: nothing. All if it would be lost in the call to strip_tags(), despite the fact that my password has nothing to do with HTML markup.

[fanhamster]

Thanks everyone. I am disillusioned and sad and grateful.

johanafm, two questions: first, when you say never assign values to $_POST, you mean if I have:

foreach($_POST as $key => $value) {
	$somedata[$key] = filter($value);
}

and form:

 
<form name="fooey" method="post" action="foo.php">
username:<input type="text" name="loginname" />
$cudgel = $somedata['cudgel'];

then I should not have:

$loginname = $somedata['loginname'];

but should only have:

$loginname = $_POST['loginname'];

...and then have:

$insert = 'INSERT into mytable(loginname, cudgel) VALUES("'.$somedata[loginname].'", "'.$somedata[cudgel].'")';

Correct? (Just for now while I am working on migrating to PDO).

Second, for the date form: is it ok to use (int) as below when the db field type it is going to be stored in is date and not int?

$safe_int = (int) $_POST['input'];

I've been looking around for examples of (date) $_POST['input'] and nothing has turned up.

Many, many thanks for your help.

[fanhamster]

Whoops sorry I meant:

 
<form name="fooey" method="post" action="foo.php">
username:<input type="text" name="loginname" />
cudgel:<input type="text" name="cudgel" />

[johanafm]

fanhamster;10995260 wrote:

johanafm, two questions: first, when you say never assign values to $_POST, you mean if I have:

foreach($_POST as $key => $value) {
	$somedata[$key] = filter($value);
}

Err well, I read that as $_POST[...] = stuff($_POST[...]);
My recommendation still stands but really has nothing to do with the code you presented...

fanhamster;10995260 wrote:

then I should not have:
(...)
...and then have:

The only thing I meant was to not assign to POST as in $POST =. Wether you do as you did and store everything in another array, use one variable per _POST array element or escape data where it's used doesn't really matter.

# Escape $_POST['varname'] and assign to $varname
foreach ($_POST as $k => $v)
{
	$$k = escape($v);
}
# or assign to array before using
foreach ($_POST as $k => $v)
{
	$safe[$k] = escape($v);
}
# or simply escape stuff as you use it
$query = sprintf("SELECT some, stuff FROM tbl WHERE text='%s' AND number=%d AND birth='%s'",
	$mysqli->real_escape_string($_POST['text']),
	(int) $_POST['number'],
	$mysqli->real_escape_string($_POST['date'])
);

fanhamster;10995260 wrote:

Second, for the date form: is it ok to use (int) as below when the db field type it is going to be stored in is date and not int?

If you store a date(time) as UTC, i.e. seconds since the unix epoc, then your data type is INT and you cast it to int. Once you have cast it to int there is no need to escape it since it can do no harm.
If you use a date, time or datetime field, then you escape it as a string since it has to be treated as a string initially

WHERE birth = '1950-04-20 08:12:40'

[fanhamster]

Got it. Thank you very much for all your help. I'll use whatever feature they have here to rate you up.

[fanhamster]

Thank you bradgrafelman. I see what you mean about htmlentities.

[fanhamster]

... and if there is a vote-up or whatever button I can't find it.

[Derokorian]

There is only a thread rating option. At the top right, next to display modes.