Re: appdb security

[Christoph Frick]

On Thu, Jun 08, 2006 at 11:25:08AM -0400, Chris Morgan wrote:
> $sQuery = "Select versionId from appVersion where 
> appId='"$_REQUEST['appId']."';";
>
> Who's '' around $_REQUEST should prevent the string from being interpreted as 
> anything but a single value passed as the value of appId.

with appId="' or 1=1;'"?

-- 
cu

appdb security

[Chris Morgan]

Can you come up with a non-destructive working example for the appdb 
website(appdb.winehq.org)? ;-)

I ask because I thought we went through this some time ago but I agree that 
what you say looks like an open issue.

Chris



On Thursday 08 June 2006 11:35 am, Christoph Frick wrote:
> On Thu, Jun 08, 2006 at 11:25:08AM -0400, Chris Morgan wrote:
> > $sQuery = "Select versionId from appVersion where
> > appId='"$_REQUEST['appId']."';";
> >
> > Who's '' around $_REQUEST should prevent the string from being
> > interpreted as anything but a single value passed as the value of appId.
>
> with appId="' or 1=1;'"?

appdb security

[Christoph Frick]

On Thu, Jun 08, 2006 at 11:42:09AM -0400, Chris Morgan wrote:
> Can you come up with a non-destructive working example for the appdb 
> website(appdb.winehq.org)? ;-)

no ;P
> I ask because I thought we went through this some time ago but I agree
> that what you say looks like an open issue.

if the magic quote thingy is turned on, then my code below wont do any
harm (as the ' from the user input is turned into \'). well but there
are other ways to inject code (e.g. encode the ' in some other way). at
least a intval($_REQUEST['appId']) would be step in the right direction.
> > > appId='"$_REQUEST['appId']."';";
> > > with appId="' or 1=1;'"?

-- 
cu

appdb security

[Jonathan Ernst]

Le jeudi 08 juin 2006 à 11:42 -0400, Chris Morgan a écrit :
> Can you come up with a non-destructive working example for the appdb 
> website(appdb.winehq.org)? ;-)
>
> I ask because I thought we went through this some time ago but I agree that 
> what you say looks like an open issue.
>
> Chris

Lately I used the following snippet in all my webapps to secure them
against sql injection :

http://php.net/mysql_real_escape_string under "Best practice".

<?php
function smart_quote($value)
{
   // Stripslashes
   if (get_magic_quotes_gpc()) {
     $value = stripslashes($value);
   }
   // Protect it if it's not an integer
   if (!is_numeric($value)) {
     $value = "'" . mysql_real_escape_string($value) . "'";
   }
   return $value;
}

// Secure query
$sQuery = sprintf("SELECT *
                   FROM users
                   WHERE user=%s AND password=%s",
                   smart_quote($_POST['username']),
                   smart_quote($_POST['password']));
mysql_query($query);
?>

I think it is better than what we have now in AppDB (didn't check it
though). If nobody looks at it, I'll check the code after my master
thesis (in one month).

Jonathan

appdb security

[EA Durbin]

I always use the method of filtering user input as described at the php 
security consortium. It makes it easier to track tainted user input vs 
filtered input. If all filtered variables are put in an array it makes it 
easier to ensure you're using the non tainted variable.

http://phpsec.org/projects/guide/1.html#1.4

Then PEAR::DB to query the mysql database as PEAR::DB handles the SQL 
filtering.
> From: Jonathan Ernst <jonathan(a)ernstfamily.ch>
> To: wine-devel(a)winehq.com
> Subject: Re: appdb security
> Date: Thu, 08 Jun 2006 18:12:20 +0200
>
> Le jeudi 08 juin 2006 à 11:42 -0400, Chris Morgan a écrit :
> > Can you come up with a non-destructive working example for the appdb
> > website(appdb.winehq.org)? ;-)
> >
> > I ask because I thought we went through this some time ago but I agree 
> > that
> > what you say looks like an open issue.
> >
> > Chris
>
> Lately I used the following snippet in all my webapps to secure them
> against sql injection :
>
> http://php.net/mysql_real_escape_string under "Best practice".
>
> <?php
> function smart_quote($value)
> {
>    // Stripslashes
>    if (get_magic_quotes_gpc()) {
>      $value = stripslashes($value);
>    }
>    // Protect it if it's not an integer
>    if (!is_numeric($value)) {
>      $value = "'" . mysql_real_escape_string($value) . "'";
>    }
>    return $value;
> }
>
> // Secure query
> $sQuery = sprintf("SELECT *
>                    FROM users
>                    WHERE user=%s AND password=%s",
>                    smart_quote($_POST['username']),
>                    smart_quote($_POST['password']));
> mysql_query($query);
> ?>
>
> I think it is better than what we have now in AppDB (didn't check it
> though). If nobody looks at it, I'll check the code after my master
> thesis (in one month).
>
> Jonathan
> << signature.asc >>
>

appdb security

[Chris Morgan]

Alright.  I'm sold on having to check all user input.  We should make this 
input checking change across the board if you are up for it.

$clean = array(); //array of filtered user input
+
+$clean['catId'] = makeSafe( $_REQUEST['catId'] );
 
 function admin_menu()
 {
-    if(isset($_REQUEST['catId'])) $catId=$_REQUEST['catId'];
-    else $catId="";
+    $clean['catId'] = makeSafe( $_REQUEST['catId'] );
+    if ( empty($clean['catId']) )
+    {
+        $clean['catId']="";
+    }


Is there a reason why we don't do the if(empty()) check inside of makeSafe()?

Chris


On Thursday 08 June 2006 1:40 pm, EA Durbin wrote:
> I always use the method of filtering user input as described at the php
> security consortium. It makes it easier to track tainted user input vs
> filtered input. If all filtered variables are put in an array it makes it
> easier to ensure you're using the non tainted variable.
>
> http://phpsec.org/projects/guide/1.html#1.4
>
> Then PEAR::DB to query the mysql database as PEAR::DB handles the SQL
> filtering.
> > From: Jonathan Ernst <jonathan(a)ernstfamily.ch>
> > To: wine-devel(a)winehq.com
> > Subject: Re: appdb security
> > Date: Thu, 08 Jun 2006 18:12:20 +0200
> >
> > Le jeudi 08 juin 2006 أ  11:42 -0400, Chris Morgan a أ�crit :
> > > Can you come up with a non-destructive working example for the appdb
> > > website(appdb.winehq.org)? ;-)
> > >
> > > I ask because I thought we went through this some time ago but I agree
> >
> > that
> > > what you say looks like an open issue.
> > >
> > > Chris
> >
> > Lately I used the following snippet in all my webapps to secure them
> > against sql injection :
> >
> > http://php.net/mysql_real_escape_string under "Best practice".
> >
> > <?php
> > function smart_quote($value)
> > {
> >    // Stripslashes
> >    if (get_magic_quotes_gpc()) {
> >      $value = stripslashes($value);
> >    }
> >    // Protect it if it's not an integer
> >    if (!is_numeric($value)) {
> >      $value = "'" . mysql_real_escape_string($value) . "'";
> >    }
> >    return $value;
> > }
> >
> > // Secure query
> > $sQuery = sprintf("SELECT *
> >                    FROM users
> >                    WHERE user=%s AND password=%s",
> >                    smart_quote($_POST['username']),
> >                    smart_quote($_POST['password']));
> > mysql_query($query);
> > ?>
> >
> > I think it is better than what we have now in AppDB (didn't check it
> > though). If nobody looks at it, I'll check the code after my master
> > thesis (in one month).
> >
> > Jonathan
> >
> > << signature.asc >>

appdb security

[EA Durbin]

It will be  a large undertaking, but I'll help change this across the board. 
I'm going out of town for the next 2 days and won't be near my computer, but 
I can start on it when I get back.
> From: Chris Morgan <cmorgan(a)alum.wpi.edu>
> To: wine-devel(a)winehq.org, EA Durbin <ead1234(a)hotmail.com>
> Subject: Re: appdb security
> Date: Thu, 8 Jun 2006 16:40:55 -0400
>
> Alright.  I'm sold on having to check all user input.  We should make this
> input checking change across the board if you are up for it.
>
> $clean = array(); //array of filtered user input
> +
> +$clean['catId'] = makeSafe( $_REQUEST['catId'] );
>
> function admin_menu()
>  {
> -    if(isset($_REQUEST['catId'])) $catId=$_REQUEST['catId'];
> -    else $catId="";
> +    $clean['catId'] = makeSafe( $_REQUEST['catId'] );
> +    if ( empty($clean['catId']) )
> +    {
> +        $clean['catId']="";
> +    }
>
> Is there a reason why we don't do the if(empty()) check inside of 
> makeSafe()?
>
> Chris
>
> On Thursday 08 June 2006 1:40 pm, EA Durbin wrote:
> > I always use the method of filtering user input as described at the php
> > security consortium. It makes it easier to track tainted user input vs
> > filtered input. If all filtered variables are put in an array it makes 
> > it
> > easier to ensure you're using the non tainted variable.
> >
> > http://phpsec.org/projects/guide/1.html#1.4
> >
> > Then PEAR::DB to query the mysql database as PEAR::DB handles the SQL
> > filtering.
> > > From: Jonathan Ernst <jonathan(a)ernstfamily.ch>
> > > To: wine-devel(a)winehq.com
> > > Subject: Re: appdb security
> > > Date: Thu, 08 Jun 2006 18:12:20 +0200
> > >
> > > Le jeudi 08 juin 2006 أ  11:42 -0400, Chris Morgan a أￜcrit :
> > > > Can you come up with a non-destructive working example for the appdb
> > > > website(appdb.winehq.org)? ;-)
> > > >
> > > > I ask because I thought we went through this some time ago but I 
> > > > agree
> > >
> > > that
> > > > what you say looks like an open issue.
> > > >
> > > > Chris
> > >
> > > Lately I used the following snippet in all my webapps to secure them
> > > against sql injection :
> > >
> > > http://php.net/mysql_real_escape_string under "Best practice".
> > >
> > > <?php
> > > function smart_quote($value)
> > > {
> > >    // Stripslashes
> > >    if (get_magic_quotes_gpc()) {
> > >      $value = stripslashes($value);
> > >    }
> > >    // Protect it if it's not an integer
> > >    if (!is_numeric($value)) {
> > >      $value = "'" . mysql_real_escape_string($value) . "'";
> > >    }
> > >    return $value;
> > > }
> > >
> > > // Secure query
> > > $sQuery = sprintf("SELECT *
> > >                    FROM users
> > >                    WHERE user=%s AND password=%s",
> > >                    smart_quote($_POST['username']),
> > >                    smart_quote($_POST['password']));
> > > mysql_query($query);
> > > ?>
> > >
> > > I think it is better than what we have now in AppDB (didn't check it
> > > though). If nobody looks at it, I'll check the code after my master
> > > thesis (in one month).
> > >
> > > Jonathan
> > >
> > > << signature.asc >>

appdb security

[EA Durbin]

> > Is there a reason why we don't do the if(empty()) check inside of 
> > makeSafe()?

as in put the if(empty()) inside of the function itself, or pass if( empty 
(makeSafe( $_REQUEST['appId'] ) ) ) when we assign it?

the reason I didn't put it in the makeSafe function was because we were 
testing to see if the variable was isset or empty and determining on the 
point of the application the result was either set to "" or 0, you could do 
it inside of the makeSafe() function but returning "" may not always be the 
desired results.

you could call the empty() test while you were assigning it, I just always 
start out assigning all of the user input variables I'm going to use at the 
top of the page by passing them through makeSafe.

function makeSafe( $var )
{
    $var = trim( addslashes( $var ) );
    return $var;
}


$clean['var1'] = makeSafe( $_REQUEST['var1'] );
$clean['var2'] = makeSafe( $_REQUEST['var2'] );

then any subsequent test called upon the variables are ensured to be clean.

if your desired output of makeSafe is to be "" if its empty then you could 
put the empty() test inside of makeSafe, but further down in the app we were 
testing for empty and returning 0.

appdb security

[Christoph Frick]

On Thu, Jun 08, 2006 at 06:44:15PM -0500, EA Durbin wrote:
> function makeSafe( $var )
> {
>    $var = trim( addslashes( $var ) );
>    return $var;
> }
>
> $clean['var1'] = makeSafe( $_REQUEST['var1'] );
> $clean['var2'] = makeSafe( $_REQUEST['var2'] );

sorry for only throwing things at you guys and not providing any code -
but i am currently packed with work :/

why dont create a object, that wrapps the request and makes it "safe".
then fixing the app is not more like sed action and you can handle stuff
in future as you like:

$_REQUEST[(['"][^'"]+['"])] -> Request::get(\1)

-- 
cu

appdb security

[Jonathan Ernst]

Hi,

The recent changes you made have resulted in a regression at least in
the note edition.

\'s and ''s are too much addslashized again. I remember having fixed
this some time ago...

Thanks.

Jonathan

appdb security

[Tobias Burnus]

Hi,

Jonathan Ernst schrieb:
> Le jeudi 08 juin 2006 à 11:42 -0400, Chris Morgan a écrit :
> > Can you come up with a non-destructive working example for the appdb 
> > website(appdb.winehq.org)? ;-)
> >
> > I ask because I thought we went through this some time ago but I agree that 
> > what you say looks like an open issue.
> >
> > Chris
>
> Lately I used the following snippet in all my webapps to secure them
> against sql injection :
> [...]

Why don't you use mysql_escape_string(...)?
http://de.php.net/manual/en/function.mysql-escape-string.php

Tobias

appdb security

[Joris Huizer]

Tobias Burnus wrote:
> Why don't you use mysql_escape_string(...)?
> http://de.php.net/manual/en/function.mysql-escape-string.php
>
> Tobias

The page says it's deprecated and mentions using 
mysql_real_escape_string instead 
(http://nl2.php.net/mysql_real_escape_string)

HTH,

Joris

appdb security

[EA Durbin]

> Tobias Burnus wrote:
> > Why don't you use mysql_escape_string(...)?
> > http://de.php.net/manual/en/function.mysql-escape-string.php

Why not just use PEAR::DB as recommended in the book "Essential PHP 
Security", as it handles multiple SQL interfaces and escapes the data 
automatically for you, appropriately for the type of database you're using.

http://www.devshed.com/c/a/PHP/Accessing-Databases-with-DB/2/