General security and sql injection
Moderator: General Moderators
General security and sql injection
Hi there
I'm building an application where the public can insert data from a form into the database and I just want to make sure I covered most reasonable bases.
First, I validate my input data, make sure the numbers are numbers, alphanumeric fields are alphanumeric, check email/url fields etc etc.
For fields that don't fit the above validation criteria, I strip all: Javascript, Style, PHP and HTML tags from them.
However there are still fields in my form that allow full punctuation, and that could give rise to sql injection, right? So I would then addslashes to those fields.
Is this enough reasonable protection for my application?
Or should I take it further and use PEAR MDB2 and prepared statements, would that then cover ALL bases?
Thanks
I'm building an application where the public can insert data from a form into the database and I just want to make sure I covered most reasonable bases.
First, I validate my input data, make sure the numbers are numbers, alphanumeric fields are alphanumeric, check email/url fields etc etc.
For fields that don't fit the above validation criteria, I strip all: Javascript, Style, PHP and HTML tags from them.
However there are still fields in my form that allow full punctuation, and that could give rise to sql injection, right? So I would then addslashes to those fields.
Is this enough reasonable protection for my application?
Or should I take it further and use PEAR MDB2 and prepared statements, would that then cover ALL bases?
Thanks
- kaisellgren
- DevNet Resident
- Posts: 1675
- Joined: Sat Jan 07, 2006 5:52 am
- Location: Lahti, Finland.
Re: General security and sql injection
Using prepared statements helps a lot in an area of security. If you can use it, I can only recommend to do so, but notice that MySQLi & PDO extensions support prepared statements, so, you don't really need to use any PEAR libraries.
I would like to see your code, telling us what you have done is not enough..
I would like to see your code, telling us what you have done is not enough..
Re: General security and sql injection
This example illustrates how I go about writing the code. Nothing fancy but I'm hoping to write it securely.
The public will be able to insert a comment into the database. An admin then will then receive it, and mark it active=1 if approved. Is this secure enough?
The public will be able to insert a comment into the database. An admin then will then receive it, and mark it active=1 if approved. Is this secure enough?
Code: Select all
if (!alphanum($_POST["Full_Name"])) exit;
if (!email($_POST["E-Mail"])) exit;
if (empty($_POST["Comment"])) exit;
$fields["name"] = $_POST["Full_Name"];
$fields["email"] = $_POST["E-Mail"];
$fields["comment"] = mysql_real_escape_string(strip_tags($_POST["Comment"]));
$keys = "".implode(", ", array_keys($fields))."";
$values = "'".implode("', '", $fields)."'";
$q = "
INSERT INTO tablename
(".$keys.")
VALUES (".$values.")
";- kaisellgren
- DevNet Resident
- Posts: 1675
- Joined: Sat Jan 07, 2006 5:52 am
- Location: Lahti, Finland.
Re: General security and sql injection
That's a weird way to write SQL statements. The SQL query is vulnerable to SQL injections, because you did not escape the full name and the email address. Everything going to the database has to be escaped.
I would also like to see the code that constructs and outputs the comments.
I would also like to see the code that constructs and outputs the comments.
Re: General security and sql injection
Ok, I assumed that the tests for alphanumeric and email would fail if they tried to use a sql injection technique on the field. If I escape all the variables should I now be safe? Is there any other precaution I could take?
To display the comments
To display the comments
Code: Select all
if (!is_int($_GET["id"])) exit;
$q = "SELECT * FROM table WHERE id = ".$_GET["id"]." ";kaisellgren wrote:That's a weird way to write SQL statements. The SQL query is vulnerable to SQL injections, because you did not escape the full name and the email address. Everything going to the database has to be escaped.
I would also like to see the code that constructs and outputs the comments.
- kaisellgren
- DevNet Resident
- Posts: 1675
- Joined: Sat Jan 07, 2006 5:52 am
- Location: Lahti, Finland.
Re: General security and sql injection
That code gets the data from the database, right? I would like to see the part where you "echo" them (the comments).spartan7 wrote:Code: Select all
if (!is_int($_GET["id"])) exit; $q = "SELECT * FROM table WHERE id = ".$_GET["id"]." ";
For the alphanumeric check, yes, it stops SQL injections. I have never seen the code for email() function, so, I don't know about that, but I would never stop escaping values, because of a single alphanumeric check. It is just far too likely that the application changes in the future and does not work the way you wanted it to. You should always escape data that goes to the database. That's the safest route to take.spartan7 wrote:Ok, I assumed that the tests for alphanumeric and email would fail if they tried to use a sql injection technique on the field.
from SQL injections, yes. The query is secure as long as you escape the data values and keep quotes around them. If you do something more complex like constructing the whole query dynamically based on user supplied data then it becomes harder to secure.spartan7 wrote:If I escape all the variables should I now be safe?
Re: General security and sql injection
ok I see now.
Will strip_tags() protect me from other sorts of attacks?
This is how I would ordinarily go about echo-ing the data in the page. Before using mysql escape string I used addslashes.
Will strip_tags() protect me from other sorts of attacks?
This is how I would ordinarily go about echo-ing the data in the page. Before using mysql escape string I used addslashes.
Code: Select all
echo stripslashes($record["comment"]);- kaisellgren
- DevNet Resident
- Posts: 1675
- Joined: Sat Jan 07, 2006 5:52 am
- Location: Lahti, Finland.
Re: General security and sql injection
The strip_tags() will strip out XML/HTML tags, which can prevent XSS attacks depending on where do you output the data, but strip_tags() does not take the encoding into account. You should use htmlspecialchars() in general to prevent XSS attacks and that should be just fine for you. For example:spartan7 wrote:ok I see now.
Will strip_tags() protect me from other sorts of attacks?
This is how I would ordinarily go about echo-ing the data in the page. Before using mysql escape string I used addslashes.
Code: Select all
echo stripslashes($record["comment"]);
Code: Select all
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<?php echo htmlspecialchars($_GET['user_supplied_data'],ENT_QUOTES,'UTF-8'); ?>
</body>
</html>Code: Select all
header('Content-Type: text/html; charset=UTF-8');I think you should read about XSS: http://en.wikipedia.org/wiki/Cross-site_scripting
It is a huge topic...
Re: General security and sql injection
Thanks
And standard practise should be that I escape ALL input going into the database, even for SELECT queries, even if I've pre-filtered them. Correct?
And standard practise should be that I escape ALL input going into the database, even for SELECT queries, even if I've pre-filtered them. Correct?
Re: General security and sql injection
One last question though, so to prevent XSS attacks, I should not rely on strip_tags when entering data into the database, I should instead use htmlspecialchars (and specificy the encoding) when I echo the records from the database and that should keep me safe.
kaisellgren wrote:The strip_tags() will strip out XML/HTML tags, which can prevent XSS attacks depending on where do you output the data, but strip_tags() does not take the encoding into account. You should use htmlspecialchars() in general to prevent XSS attacks and that should be just fine for you. For example:
The above code is safe from XSS attacks. You should specify a character set either in the document (like I did above), or output it in the headers:Code: Select all
<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body> <?php echo htmlspecialchars($_GET['user_supplied_data'],ENT_QUOTES,'UTF-8'); ?> </body> </html>
This will make sure that the web browser treats the document as UTF-8 (which is what your htmlspecialchars() function uses). If you use some other encoding, specify it for the htmlspecialchars() and for the web browser. UTF-8 is, in my opinion, the best character encoding to use in general.Code: Select all
header('Content-Type: text/html; charset=UTF-8');
I think you should read about XSS: http://en.wikipedia.org/wiki/Cross-site_scripting
It is a huge topic...
- kaisellgren
- DevNet Resident
- Posts: 1675
- Joined: Sat Jan 07, 2006 5:52 am
- Location: Lahti, Finland.
Re: General security and sql injection
That would be a very good practice, yes. Whenever you are passing dynamic data (variables), you should escape them. No matter what this data is, you should escape it:spartan7 wrote:And standard practise should be that I escape ALL input going into the database, even for SELECT queries, even if I've pre-filtered them. Correct?
Code: Select all
$something = mysql_real_escape_string($_SERVER['HTTP_HOST']);
mysql_query("SELECT ... FROM ... WHERE ... = '$something';");Yes it would prevent XSS as long as you place it inside tags. For example, the following would be insecure:spartan7 wrote:One last question though, so to prevent XSS attacks, I should not rely on strip_tags when entering data into the database, I should instead use htmlspecialchars (and specificy the encoding) when I echo the records from the database and that should keep me safe.
Code: Select all
echo "<div ". htmlspecialchars($_GET['tag_name'],ENT_QUOTES,'UTF-8') . "></div>";Re: General security and sql injection
Could I also do this:
Then insert that into the database.
Then later echo the variable as normal? (making sure the doctype of my page is also UTF-8)
The escaped code should then be sent to the database and I should just be able to echo it as normal, correct?
Code: Select all
$fields["comment"] = mysql_real_escape_string(htmlspecialchars($_POST["Comment"], ENT_QUOTES, 'UTF-8'));Then later echo the variable as normal? (making sure the doctype of my page is also UTF-8)
The escaped code should then be sent to the database and I should just be able to echo it as normal, correct?
Code: Select all
echo $record["comment"];- kaisellgren
- DevNet Resident
- Posts: 1675
- Joined: Sat Jan 07, 2006 5:52 am
- Location: Lahti, Finland.
Re: General security and sql injection
You can do that, although it would be better to encode it before outputting. See, if someone can manipulate the data, then the manipulated data would be directly outputted in your echo statement, but in general, yeah, that would work fine.
Re: General security and sql injection
How about this function? It calls htmlspecialchars first and then it calls mysql_real_escape_string after to catch any characters that weren't escaped properly. I noticed that the ' character already gets replaced by the htmlspecialchars and so the mysql escape string doesn't have to escape it after but I call it anyway in case there is a character that htmlspecialchars misses?
and then:
Should this keep me safe?
Code: Select all
function sanitize($input) {
if (is_array($input)) {
foreach($input as $var=>$val) {
$output[$var] = sanitize($val);
}
}
else {
if (get_magic_quotes_gpc()) {
$input = stripslashes($input);
}
$input = htmlspecialchars($input, ENT_QUOTES, 'iso-8859-1');
$output = mysql_real_escape_string($input);
}
return $output;
}Code: Select all
$_POST = sanitize($_POST);- kaisellgren
- DevNet Resident
- Posts: 1675
- Joined: Sat Jan 07, 2006 5:52 am
- Location: Lahti, Finland.
Re: General security and sql injection
That's basically a replacement for Magic Quotes. 
You should just run mysql_real_escape_string() for anything that goes to the database, but right now it does it for all input... that would corrupt some data and if you later modify those variables, you risk losing the protection you applied to the variables. The escaping process should be the last thing you ever do to a variable that goes to the database. Variables that do not go to the database should not be escaped (unless you have a special situation where it might make sense).
It's pretty much the same thing with htmlspecialchars(). If a variable is "echoed", then apply it on the variable - otherwise do not, because it would create unexpectional bugs and modifying the variable value later might break the protection just like with escaping.
A basic example of how to not do it:
Even though the user supplied data was escaped and the folder was hard-coded, we would be vulnerable to SQL injections, because the $path which goes to the database should be escaped - not the $file. If I just add mysql_real_escape_string() into $path, then the $file part would be double escaped - which causes data corruption, so, the right approach would be:
That's why you should not automate escaping or encoding processes. They should be the very last things to apply to variables.
Personally, I always just run a function in my code whenever I am about to output something with echo/print/etc. If you use prepared statements, then you can basically "forget" escaping totally. I recommend having a look at PDO - it's a very good extension.
You should just run mysql_real_escape_string() for anything that goes to the database, but right now it does it for all input... that would corrupt some data and if you later modify those variables, you risk losing the protection you applied to the variables. The escaping process should be the last thing you ever do to a variable that goes to the database. Variables that do not go to the database should not be escaped (unless you have a special situation where it might make sense).
It's pretty much the same thing with htmlspecialchars(). If a variable is "echoed", then apply it on the variable - otherwise do not, because it would create unexpectional bugs and modifying the variable value later might break the protection just like with escaping.
A basic example of how to not do it:
Code: Select all
$folder = 'some\\folder\\';
$file = mysql_real_escape_string($_GET['file']); // file.zip
$path = $folder.$file;
mysql_query(... $path ...); // using the path in a queryCode: Select all
$folder = 'some\\folder\\';
$file = $_GET['file']; // let's not escape any data until it really goes to the database
$path = mysql_real_escape_string($folder.$file);
mysql_query(... $path ...); // using the path in a queryPersonally, I always just run a function in my code whenever I am about to output something with echo/print/etc. If you use prepared statements, then you can basically "forget" escaping totally. I recommend having a look at PDO - it's a very good extension.