Protected from injection? You decide.

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

<br>
Forum Commoner
Posts: 35
Joined: Thu May 01, 2008 2:42 pm

Protected from injection? You decide.

Post 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);
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Protected from injection? You decide.

Post by Christopher »

Except the first one. What if $inlist is set to "1) OR (1".
(#10850)
<br>
Forum Commoner
Posts: 35
Joined: Thu May 01, 2008 2:42 pm

Re: Protected from injection? You decide.

Post 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)?
Last edited by <br> on Fri Aug 15, 2008 6:16 pm, edited 1 time in total.
<br>
Forum Commoner
Posts: 35
Joined: Thu May 01, 2008 2:42 pm

Re: Protected from injection? You decide.

Post 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'";
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Protected from injection? You decide.

Post 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);
<br>
Forum Commoner
Posts: 35
Joined: Thu May 01, 2008 2:42 pm

Re: Protected from injection? You decide.

Post 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);
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Protected from injection? You decide.

Post 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?
<br>
Forum Commoner
Posts: 35
Joined: Thu May 01, 2008 2:42 pm

Re: Protected from injection? You decide.

Post 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.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Protected from injection? You decide.

Post 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.
<br>
Forum Commoner
Posts: 35
Joined: Thu May 01, 2008 2:42 pm

Re: Protected from injection? You decide.

Post 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')"
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Protected from injection? You decide.

Post 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:

Code: Select all

str_replace(')','_',$inlist);
... 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)
<br>
Forum Commoner
Posts: 35
Joined: Thu May 01, 2008 2:42 pm

Re: Protected from injection? You decide.

Post 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...
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Re: Protected from injection? You decide.

Post by matthijs »

br, please use php code tags (

Code: Select all

). That will make your code highlighted correctly and much more readable.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Protected from injection? You decide.

Post 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.
<br>
Forum Commoner
Posts: 35
Joined: Thu May 01, 2008 2:42 pm

Re: Protected from injection? You decide.

Post 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
Post Reply