Preventing SQL Injections

Question

Possible Duplicates:
Best way to stop SQL Injection in PHP
In PHP when submitting strings to the database should I take care of illegal characters using htmlspecialchars() or use a regular expression?

Yesterday I asked a question with regards to a script not working, whilst I in the end solved the issue myself. There was talk of SQL Injections risks.

So what I'm asking today is, with the code I have inserted below, how would one prevent SQL Injections?

So any advice of guidence. I know I can read the internet about SQL injections but there is so many conflicting articles on it, I don't know which is correct or not.

Here is the code, this is all put in a page of it's own lets say 'form-process.php' which the form then submits the data to e.g

<?
session_start(); 

$_SESSION['Title'] = stripslashes($_REQUEST['Title']); 
$_SESSION['ShortTitle'] = stripslashes($_REQUEST['Title']); 
$_SESSION['Category'] = stripslashes($_REQUEST['Category']); 
$_SESSION['Story'] = stripslashes($_REQUEST['Story']);
$_SESSION['FrontPage'] = stripslashes($_REQUEST['FrontPage']);
$_SESSION['imagefilename'] = ($_FILES['image']['name']); 

if (empty($_REQUEST['Title'])) { 
header("Location: ". $_SERVER['HTTP_REFERER'] ."?message=0"); 
exit; 
} elseif (empty($_REQUEST['ShortTitle'])) { 
header("Location: ". $_SERVER['HTTP_REFERER'] ."?message=1"); 
exit; 
} elseif (strlen($_REQUEST['Category']) < 1) {
header("Location: ". $_SERVER['HTTP_REFERER'] ."?message=2"); 
exit;
} elseif (empty($_REQUEST['Story'])) { 
header("Location: ". $_SERVER['HTTP_REFERER'] ."?message=3"); 
exit;  
} else { 

include("settings.php"); 
include("dbconnect.php"); 

if($_POST['btnSubmit'] == 'Publish'){
    $target = "../../../images/matchreports/uploaded/";
    $target = $target . time() . '-' . basename( $_FILES['image']['name']);
    if(move_uploaded_file($_FILES['image']['tmp_name'], $target)){
    $image=time() . '-' . basename( $_FILES['image']['name']);      
    $newdate = $_POST['date_y'].''.$_POST['date_m'].''.$_POST['date_d'];
$SQL = "INSERT INTO " . $match_reports_table . " (Title,ShortTitle,Story,FrontPage,active,image,date,user_ip) VALUES('" . addslashes($_REQUEST['Title']) . "','" . addslashes($_REQUEST['ShortTitle']) . "','" . addslashes($_REQUEST['Story']) . "','" . addslashes($_REQUEST['FrontPage']) . "','" . addslashes(y) . "','$image','$newdate','" . addslashes($_SERVER['REMOTE_ADDR']) . "')";
    $result = @mysql_query($SQL) or die("Error Publishing 1");

    header("Location: /cms/matchreports/index.php?message=4");  
    exit;

} else {

$newdate = $_POST['date_y'].''.$_POST['date_m'].''.$_POST['date_d'];    
$SQL = "INSERT INTO " . $match_reports_table . " (Title,ShortTitle,Story,FrontPage,active,date,user_ip) VALUES('" . addslashes($_REQUEST['Title']) . "','" . addslashes($_REQUEST['ShortTitle']) . "','" . addslashes($_REQUEST['Story']) . "','" . addslashes($_REQUEST['FrontPage']) . "','" . addslashes(n) . "','$newdate','" . addslashes($_SERVER['REMOTE_ADDR']) . "')";
    $result = @mysql_query($SQL) or die("Error Publishing 2");

    header("Location: /cms/matchreports/index.php?message=5");  
    exit;}}

if($_POST['btnSubmit'] == 'Save draft'){
    $target = "../../../images/matchreports/uploaded/";
    $target = $target . time() . '-' . basename( $_FILES['image']['name']);
    if(move_uploaded_file($_FILES['image']['tmp_name'], $target)){
    $image=time() . '-' . basename( $_FILES['image']['name']);      
    $newdate = $_POST['date_y'].''.$_POST['date_m'].''.$_POST['date_d'];
$SQL = "INSERT INTO " . $match_reports_table . " (Title,ShortTitle,Story,FrontPage,active,image,date,user_ip) VALUES('" . addslashes($_REQUEST['Title']) . "','" . addslashes($_REQUEST['ShortTitle']) . "','" . addslashes($_REQUEST['Story']) . "','" . addslashes($_REQUEST['FrontPage']) . "','" . addslashes(n) . "','$image','$newdate','" . addslashes($_SERVER['REMOTE_ADDR']) . "')";
    $result = @mysql_query($SQL) or die("Error Saving Draft 1");

    header("Location: /cms/matchreports/index.php?message=6");  
    exit;

} else {

$newdate = $_POST['date_y'].''.$_POST['date_m'].''.$_POST['date_d'];    
$SQL = "INSERT INTO " . $match_reports_table . " (Title,ShortTitle,Story,FrontPage,active,date,user_ip) VALUES('" . addslashes($_REQUEST['Title']) . "','" . addslashes($_REQUEST['ShortTitle']) . "','" . addslashes($_REQUEST['Story']) . "','" . addslashes($_REQUEST['FrontPage']) . "','" . addslashes(n) . "','$newdate','" . addslashes($_SERVER['REMOTE_ADDR']) . "')";
    $result = @mysql_query($SQL) or die("Error Saving Draft 2");

    header("Location: /cms/matchreports/index.php?message=7");  
    exit;}}

if($_POST['btnSubmit'] == 'Publish changes'){
//This gets all the other information from the form
$newdate = $_POST['date_y'].''.$_POST['date_m'].''.$_POST['date_d'];
$SQL = "UPDATE " . $match_reports_table . " SET Title='" . addslashes($_REQUEST['Title']) . "',ShortTitle='" . addslashes($_REQUEST['ShortTitle']) . "',Story='" . addslashes($_REQUEST['Story']) . "',Category='" . addslashes($_REQUEST['Category']) . "',FrontPage='" . addslashes($_REQUEST['FrontPage']) . "',active = '" . y . "',date='$newdate' WHERE ID=" . $_REQUEST['ID'] . "";
$result = @mysql_query($SQL) or die("Error Updating News"); 

header("Location: /cms/matchreports/index.php?message=8");
exit;}

if($_POST['btnSubmit'] == 'Publish draft to website'){
//This gets all the other information from the form
$newdate = $_POST['date_y'].''.$_POST['date_m'].''.$_POST['date_d'];
$SQL = "UPDATE " . $match_reports_table . " SET Title='" . addslashes($_REQUEST['Title']) . "',ShortTitle='" . addslashes($_REQUEST['ShortTitle']) . "',Story='" . addslashes($_REQUEST['Story']) . "',Category='" . addslashes($_REQUEST['Category']) . "',FrontPage='" . addslashes($_REQUEST['FrontPage']) . "',active = '" . y . "',date='$newdate' WHERE ID=" . $_REQUEST['ID'] . "";
$result = @mysql_query($SQL) or die("Error Updating News"); 

header("Location: /cms/matchreports/index.php?message=9");
exit;}

if($_POST['btnSubmit'] == 'Save changes to draft'){
//This gets all the other information from the form
$newdate = $_POST['date_y'].''.$_POST['date_m'].''.$_POST['date_d'];
$SQL = "UPDATE " . $match_reports_table . " SET Title='" . addslashes($_REQUEST            ['Title']) . "',ShortTitle='" . addslashes($_REQUEST['ShortTitle']) . "',Story='" . addslashes($_REQUEST['Story']) . "',Category='" . addslashes($_REQUEST['Category']) . "',FrontPage='" . addslashes($_REQUEST['FrontPage']) . "',active = '" . n . "',date='$newdate' WHERE ID=" . $_REQUEST['ID'] . "";
$result = @mysql_query($SQL) or die("Error Updating News"); 

header("Location: /cms/matchreports/index.php?message=10");
exit;}

}?>

one thing you really need is to get rid of TONS of repeated code. — Your Common Sense, Aug 31 '11 at 11:01
Any time you create SQL code via string concatenation of user supplied input, you are at risk of SQL injection attacks. The best way to avoid this risk is by using prepared/parameterized queries. I'm marking this as a dupe, because other questions exist that address this very same issue. — spender, Aug 31 '11 at 11:04
I can't believe some developers code like this.... Incredible. — Flukey, Aug 31 '11 at 11:04
@Jamie: We all have to learn somehow. At least OP has registered that there might be a risk and asked a community about how to improve. Denigration is not a kind approach. — spender, Aug 31 '11 at 11:06
@spender however, sometimes we can't avoid concatenation. So, we have to learn how to live with it — Your Common Sense, Aug 31 '11 at 11:07
@Col. Shrapnel: could you expand on this? The whole point of parameterized queries is to mitigate the risk. — spender, Aug 31 '11 at 11:08
Indeed, my apologies. I should have said, I can't believe people who do development as their day job for a company code like this. Sorry, OP. — Flukey, Aug 31 '11 at 11:09
@spender prepared statements have no support for the identifiers for example. So, you have to concatenate if there is a field name to be added into query dynamically — Your Common Sense, Aug 31 '11 at 11:12
`$_POST['btnSubmit'] == 'Publish'` Don't check for post-button-values if you plan to have a translation of that site someday… — feeela, Sep 15 '11 at 09:09

score 6 · Answer 1 · answered Aug 31 '11 at 11:01

6

Use PDO and prepared statements.

answered Aug 31 '11 at 11:01

Rijk

11,032
3
30
45

+1 for suggesting prepared statements – Victor Welling Aug 31 '11 at 11:04
prepared statements is not sufficient to prevent certain SQL injections and PDO is quite ugly. However, for the plain inserts it's okay – Your Common Sense Aug 31 '11 at 11:05
Do you have a better answer before downvoting mine? – Rijk Aug 31 '11 at 11:07
pdo does not prevent injections, but prepeared statements you can use also with mysqli. Also prepeared statement are slower. Also you cannot create prepeared statements with varible amount of data like WHERE id IN (1,5,9,12,45,67) where ids are from array. – codez Aug 31 '11 at 11:07
@Col. Shrapnel - how PDO is ugly? – Aug 31 '11 at 11:11
@cthulhu it makes you write tons of useless code like binding values, preparing queries and stuff. – Your Common Sense Aug 31 '11 at 11:15
@Shrapnel you could build an interface class for it that makes common stuff a little bit easier and less lines of code. I think the methods behind it are really good. – Rijk Aug 31 '11 at 11:17
@Col. Sharpnel - why you consider preparing queries as "useless"? I don't really know how do **you** write your code with PDO, but I personally never got "tons" of code. I'd say it depends on the coder itself rather than on the tool. – Aug 31 '11 at 11:19
@RijkvanWel just ignore Shrappers as he very rarely contributes [anything of value](http://en.wikipedia.org/wiki/Troll_(Internet)) to comments on answers. – Treffynnon Aug 31 '11 at 11:19
@cthulhu we were talking of *the* tool, aren't we? – Your Common Sense Aug 31 '11 at 11:56
2

@Col. Shrapnel - yes, and the "tool" called PDO is great. If you get "tons" of code while using PDO, you should probably think about what's wrong with you, rather than with the "tool" itself. Bad coder, can write an ugly code using with any "tool". But even if you're that kind of a coder, I guess it's better to write an ugly but a safe code with PDO rather than an ugly AND insecure code using OLD mysql library which hasn't been updated since the days of PHP 4.1 because all the work was concentrated around the PDO/mysqli extensions. – Aug 31 '11 at 12:10
@cthulhu What if I am writing secure code without PDO and without preparing statements? :) And even more secure, as I have identifiers support while PDO haven't :-P However, you are right about using tools (but PDO makes me do more job while make it usable anyway ). – Your Common Sense Aug 31 '11 at 12:27
@Col. Shrapnel - What do you mean when you say "secure code"? A code without prepared statements? You are still vulnerable to a various attacks even when you use `mysql_real_escape_string`. I don't even talk about the fact that your beloved standard mysql library doesn't support stored procedures and multiple statements. And as I already mentioned, it's old, slower, and have less functionality than PDO/mysqli. What about "do more job" - complete bullshit. Have you ever heard about wrappers? Have you ever heard about object mapping? Likely, the problem is YOU in this case. – Aug 31 '11 at 15:18
@cthulhu secure means no possibility for the injection. `You are still vulnerable to a various attacks` - no, I am not. I am pefectly secure. Just because my security based on knowledge and experience, not some religious gospels like "use PDO". `mysql library doesn't support stored procedures` - mysql does, that's enough for me; `it's slower` - I see no difference; `and have less functionality than PDO` - I can write anything myself. So to say `Have you ever heard about object mapping?` - I can map it with whatever I use. Good coder can write good code using any tool ;) – Your Common Sense Aug 31 '11 at 15:36
@Col. Shrapnel - yes you are. There are plenty of bugs related to this. Moreover, because `mysql_real_escape_string` depends on character encoding, which can be changed to a non utf-8 encoding, it can lead to serious problems. If you aren't familiar with PDO, that doesn't mean it is useless. Yeah, You can write anything by yourself (including the OM) and thereby reinvent the wheel. And you tell me PDO is complex? Ahh. Rewriting it with PHP will make it much slower and inefficient, so you can just kill yourself insted. So you'd better learn how to use PDO before you troll here. – Aug 31 '11 at 17:06
@cthulhu please do me a favor. Would you please run this snippet of code exactly as it is in your usual background and paste it's output here? `$dbh->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); try { $sth = $dbh->prepare("SELECT * FROM users GROUP BY WHERE 1 = ?"); $sth->execute(array("1")); } catch (PDOException $e) { echo $e->getMessage(); }`. where $dbh is a PDO object. Based on your answer I will explain you my point. – Your Common Sense Aug 31 '11 at 17:19

score 4 · Answer 2 · edited Aug 31 '11 at 11:10

4

A simple, universal rule I like to apply is this:

Always store data raw, and escape it for the appropriate application when needed.

This means, get rid of nebulous stripslashes(), and:

for string values in SQL statements, use the database's appropriate escape function, e.g. mysqli_real_escape_string(),
for system()-type command names, use escapeshellcmd(), for arguments use escapeshellarg(),
for manually assembling GET request URLs, use urlencode(), and finally
for printing content in an HTML structure, use htmlentities().

There's no point in blindly using some sort of mangling and hoping it'll filter out bad things. Be conscious of what you're doing, and do the appropriate thing at every step.

Example: To print a link with a user-provided GET parameter, you'd do

print("<a href='" . htmlentities($BASEURL . "?data=" . urlencode($untrusted)) . "'>click</a>");

Important note: For SQL queries, it is generally preferable to use prepared statements rather than building queries by hand. This is a different technology from what you're used to, so it's not the straight "how do I fix this" answer, but it is by far the better solution.

edited Aug 31 '11 at 11:10

Your Common Sense

156,878
40
214
345

answered Aug 31 '11 at 11:08

Kerrek SB

464,522
92
875
1,084

there are some other kinds of values to insert into query beside strings – Your Common Sense Aug 31 '11 at 11:11
I like the "always store data raw" rule. Is there an article or book that elaborates on this? – Rijk Aug 31 '11 at 11:12
@Rijk: It comes from seeing too many questions on "how do I use regular expressions to parse my HTML" :-) Not a book that I know of, but it's just common sense I suppose... Store what you *mean*, not some arbitrary transformation of it. – Kerrek SB Aug 31 '11 at 11:14
Ok. I live by this rule myself too (after having fallen on my face a couple of times) - was just wondering if someone smart wrote something smart about it :) – Rijk Aug 31 '11 at 11:16
I don't do PHP, but is `mysqli_real_escape_string` for real? Is there also a `mysqli_escape_string`? Madness. – spender Aug 31 '11 at 11:37
@spender: it is - the "real" version takes the connection's character set into account and only works with an open connection, while the "unreal" version works sort of in the dark. – Kerrek SB Aug 31 '11 at 11:49
@spender it's not actually PHP but Mysql – Your Common Sense Aug 31 '11 at 12:07

Preventing SQL Injections

2 Answers2