Page 1 of 2
Protected from injection? You decide.
Posted: Fri Aug 15, 2008 4:12 pm
by <br>
I am new to PHP/SQL security... wanted to make sure I'm employing mysql_real_escape_string() properly and completely... the following select statements should be secured... right?
Code: Select all
$inlist = mysql_real_escape_string($inlist,$con);
$query = "SELECT * FROM items WHERE id IN ($inlist)";
Code: Select all
$urlnavkey=mysql_real_escape_string($urlnavkey,$con);
$query = "SELECT * FROM masters WHERE URL='$urlnavkey'";
$result = mysql_query($query);
Code: Select all
mysql_select_db('code') or die(mysql_error());
$urlnavkey=mysql_real_escape_string($urlnavkey,$con);
$query = "SELECT $select FROM $table WHERE master='$urlnavkey'";
$result = mysql_query($query);
Code: Select all
$urlnavkey=mysql_real_escape_string($urlnavkey,$con);
$urlnavkey2=mysql_real_escape_string($urlnavkey2,$con);
$urlnavkey3=mysql_real_escape_string($urlnavkey3,$con);
if ($urlnavkey3=='index.php'){
$thiscat = '/'.$urlnavkey.'/'.$urlnavkey2;
} else if ($urlnavkey2=='index.php') {
$thiscat = '/'.$urlnavkey;
}
$query = "SELECT * FROM items WHERE cat1='$thiscat' OR cat2='$thiscat' OR cat3='$thiscat'";
$result = mysql_query($query);
Re: Protected from injection? You decide.
Posted: Fri Aug 15, 2008 4:51 pm
by Christopher
Except the first one. What if $inlist is set to "1) OR (1".
Re: Protected from injection? You decide.
Posted: Fri Aug 15, 2008 5:57 pm
by <br>
how about:
Code: Select all
str_replace(')','_',$inlist);
$inlist = mysql_real_escape_string($inlist,$con);
$query = "SELECT * FROM items WHERE id IN ($inlist)";
What other characteristics of select statements lead to situations where hitting the variable with mysql_real_escape_string isn't enough (besides parentheses around a variable)?
Re: Protected from injection? You decide.
Posted: Fri Aug 15, 2008 6:01 pm
by <br>
What about setting a variable right before using it? Safe?
Example:
Code: Select all
$id=1;
$select='*';
if ($something=='somethingelse'){
$table='mytable';
} else {
$table='myothertable';
}
$query = "SELECT $select FROM $table where id='$id'";
Re: Protected from injection? You decide.
Posted: Mon Aug 18, 2008 5:44 am
by Mordred
<br> wrote:how about:
Code: Select all
str_replace(')','_',$inlist);
$inlist = mysql_real_escape_string($inlist,$con);
$query = "SELECT * FROM items WHERE id IN ($inlist)";
No. Escaping is done on
values, not lists of values. You must quote and escape while you build $inlist. Show the related code if you're not sure how to do it.
<br> wrote:What other characteristics of select statements lead to situations where hitting the variable with mysql_real_escape_string isn't enough (besides parentheses around a variable)?
http://www.logris.org/security/the-unex ... -injection
As for the other snippets:
$query = "SELECT $select FROM $table WHERE master='$urlnavkey'";
Possibly unsafe. Where do $select and $table come from?
Code: Select all
$urlnavkey=mysql_real_escape_string($urlnavkey,$con);
$urlnavkey2=mysql_real_escape_string($urlnavkey2,$con);
$urlnavkey3=mysql_real_escape_string($urlnavkey3,$con);
if ($urlnavkey3=='index.php'){
$thiscat = '/'.$urlnavkey.'/'.$urlnavkey2;
} else if ($urlnavkey2=='index.php') {
$thiscat = '/'.$urlnavkey;
}
$query = "SELECT * FROM items WHERE cat1='$thiscat' OR cat2='$thiscat' OR cat3='$thiscat'";
$result = mysql_query($query);
Incorrect. It's currently safe, but if you change the code it may not be. The correct and fireproof way is this:
Code: Select all
if ($urlnavkey3=='index.php'){
$thiscat = '/'.$urlnavkey.'/'.$urlnavkey2;
} else if ($urlnavkey2=='index.php') {
$thiscat = '/'.$urlnavkey;
}
$thiscat=mysql_real_escape_string($thiscat,$con);
$query = "SELECT * FROM items WHERE cat1='$thiscat' OR cat2='$thiscat' OR cat3='$thiscat'";
$result = mysql_query($query);
Re: Protected from injection? You decide.
Posted: Mon Aug 18, 2008 2:14 pm
by <br>
Is the following code necessary or could I escape the whole $items array with
$items=mysql_real_escape_string($items,$con) ?
anyways, this should be safe...
Code: Select all
$items = explode(',',$cart,100);
$cart = implode(",",$items);
$con = mysql_connect('XXXXXXXXX', 'XXXXXXXXX', 'XXXXXXXXX') or die(mysql_error());
$_SESSION[num_items] =count($items);
$escape_counter=1;
while ($escape_counter <= $_SESSION[num_items]){
$num=($escape_counter-1);
$items[$num]=mysql_real_escape_string($items[$num],$con);
$escape_counter++;
}
$inlist = implode(", ",$items);
Re: Protected from injection? You decide.
Posted: Mon Aug 18, 2008 4:22 pm
by Mordred
Mordred wrote:You must quote and escape while you build $inlist.
And skip the thing with the session - why on earth would you do that?
Re: Protected from injection? You decide.
Posted: Mon Aug 18, 2008 4:41 pm
by <br>
I counted the number of items in the array and escaped each one.
Like I said in my last post, didn't know if the function would escape every item in the array by itself... $_SESSION[num_items] was already stored in the session for other reasons so I used it to count how many times to loop.
Re: Protected from injection? You decide.
Posted: Tue Aug 19, 2008 2:42 am
by Mordred
$_SESSION[
'num_items
'] (quotes!)
and
Mordred wrote:You must quote and escape while you build $inlist.
(actually, to prevent further misunderstanding - first escape, then add quotes.)
and really, go read that paper I linked to in my first post, it discusses exactly that, in more gory detail.
Re: Protected from injection? You decide.
Posted: Tue Aug 19, 2008 10:40 am
by <br>
Code: Select all
$items = explode(',',$cart,100);
$cart = implode(",",$items);
$con = mysql_connect('XXXXXX', 'XXXXXX', 'XXXXXX') or die(mysql_error());
$_SESSION[num_items] =count($items);
$escape_counter=1;
while ($escape_counter <= $_SESSION[num_items]){
$num=($escape_counter-1);
$itemsy[$num]=mysql_real_escape_string($items[$num],$con);
$itemsy[$num]="'".$items[$num]."'"; // added quotes here
$escape_counter++;
}
$inlist = implode(', ',$itemsy);
$contents = array();
foreach ($items as $item) {
$contents[$item] = (isset($contents[$item])) ? $contents[$item] + 1 : 1;
}
$_SESSION[unique_items] =count($contents);
ksort($contents);
if($_SESSION[cart] != ''){
$con = mysql_connect('XXXXXX', 'XXXXXX', 'XXXXXX') or die(mysql_error());
mysql_select_db('NextLevelUnlmt') or die(mysql_error());
str_replace(')','_',$inlist);
$query = "SELECT * FROM items WHERE id IN ($inlist)";
That's what you were referring to? New select statement:
"SELECT * FROM items WHERE id IN ('2', '24', '242', '42')"
Re: Protected from injection? You decide.
Posted: Tue Aug 19, 2008 2:44 pm
by Mordred
You almost got it this time. Be more careful with your naming scheme and you won't run into this kind of errors in the future:
$
itemsy[$num]=mysql_real_escape_string($items[$num],$con);
$itemsy[$num]="'".$
items[$num]."'"; // added quotes here
On the second line should be
itemsy as well, as it is now, you have "missed" the escaping.
And several lines below there is this:
... which aims to solve (it actually doesn't) a problem, which you already don't have (since you've done correct escaping and quoting above)
Re: Protected from injection? You decide.
Posted: Tue Aug 19, 2008 4:40 pm
by <br>
Thanks for the help on this... once more, shall we?
Code: Select all
$items = explode(',',$cart,100);
$cart = implode(",",$items);
$con = mysql_connect('XXXXX', 'XXXXX', 'XXXXX') or die(mysql_error());
$_SESSION[num_items] =count($items);
$escape_counter=1;
while ($escape_counter <= $_SESSION[num_items]){
$num=($escape_counter-1);
$escaped_items[$num]=mysql_real_escape_string($items[$num],$con);
$quoted_items[$num]="'".$escaped_items[$num]."'";
$escape_counter++;
}
$inlist = implode(', ',$quoted_items[$num]);
$contents = array();
foreach ($items as $item) {
$contents[$item] = (isset($contents[$item])) ? $contents[$item] + 1 : 1;
}
$_SESSION[unique_items] =count($contents);
ksort($contents);
if($_SESSION[cart] != ''){
$con = mysql_connect('XXXXX', 'XXXXX', 'XXXXX') or die(mysql_error());
mysql_select_db('NextLevelUnlmt') or die(mysql_error());
$query = "SELECT * FROM items WHERE id IN ($inlist)";
$result = mysql_query($query) or die(mysql_error());
I'm going to keep your article handy for reference... Also, I know a bunch of my session variables don't have quotes... a la $_SESSION[num_items]... any good reason not to do this (a legit functional or security reason other than "it's not proper syntax")? It's been working fine for me so...
Re: Protected from injection? You decide.
Posted: Wed Aug 20, 2008 1:31 am
by matthijs
br, please use php code tags (
Code: Select all
). That will make your code highlighted correctly and much more readable.
Re: Protected from injection? You decide.
Posted: Wed Aug 20, 2008 2:52 am
by Mordred
"It's not proper syntax" is
the ultimate reason. You should run your scripts with full error reporting in your development environment (But
not on production servers - either turn it off or make it log to the web server error log.)
http://www.google.com/search?hl=en&q=%2 ... tnG=Search
Another point of concern is this:
Code: Select all
mysql_query($query) or die(mysql_error());
Since $inlist looks like it contains user-supplied data, a malicious attacker can manipulate the cart so it contains huge pieces of data which may hit the size limit of a SQL query. (problem 1)
So the mysql_query() function will fail, which will lead to die() printing useful (for the attacker) info about your database sctructure and FS path names (problem 2)
These two problems should be tackled separately even though they look interconnected.
Re: Protected from injection? You decide.
Posted: Wed Aug 20, 2008 1:22 pm
by <br>
How about:
1. Eliminating "die(mysql_error())" from code after it is no longer needed for testing
2. Checking $cart length after $cart is altered... if it's over 1500 characters then reset it to empty