Re: [AppDb] [2/3] safe functions

[Chris Morgan]

Hi Jonathan.

You'll want to talk to EA about the filtering changes.  The plan is to
filter using the same syntax and flags that the php filter extension
is going to use so we can easily switch over to this extension in the
future.

Also, I've submitted a patch for review to [e-mail] and
[e-mail] that removes all of our get_magic_quotes_gpc()
use and adds a check in include/incl.php that warns and prevents appdb
from running if magic quotes is enabled.  So you shouldn't need to
have any get_magic_quotes_gpc() checks anymore.

I also noticed your quote_smart_sql() call.  This call isn't used
anywhere, we shouldn't add calls to functions that aren't called.  We
also already have a function that will make sql calls safe called
query_paramters() in include/db.php.  Also, do we want to strip tags
from sql?  Won't that remove all tags from things like app/version
descriptions, comments and notes?

Chris

On 6/25/06, Jonathan Ernst  wrote:
> Hi,
>
> The code I made is not dependent on magic_quote value and shouldn't
> allow anybody to mess with the values read or written from/to the
> database. Also it doesn't require to make copies of every single string the user
> passes to us.
>
> It is based on php manual's best practices for avoiding injection.
>
> I'd be very glad if we'll use such mechanism for the rest of the
> queries and get rid of compile_whatever, makeSafe & co.
>
> Changelog:
> - functions to avoid sql/html injection without having issues with addslashes/magic_quotes_gpc
>
> Files changed:
> - include/util.php
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.2.2 (GNU/Linux)
>
> iD8DBQBEnt7SIW5mR/h6b38RAii/AKCB26o0DAM978gQz8baVz6VuubW3ACgqBoJ
> Or4cDg49YqCUD3dughsW1oY=
> =keDV
> -----END PGP SIGNATURE-----

[AppDb] [2/3] safe functions

[Jonathan Ernst]

Le dimanche 25 juin 2006 à 20:00 -0400, Chris Morgan a écrit :
> Hi Jonathan.
>
> You'll want to talk to EA about the filtering changes.  The plan is to
> filter using the same syntax and flags that the php filter extension
> is going to use so we can easily switch over to this extension in the
> future.

I know we could use PEAR and we could also use a database abstraction
layer, I just thought my solution was better because it has proven to
work well on several projects I worked recently and is recommanded by
the php manual (and it makes queries more readable than using other
syntaxes).
> Also, I've submitted a patch for review to [e-mail] and
> [e-mail] that removes all of our get_magic_quotes_gpc()
> use and adds a check in include/incl.php that warns and prevents appdb
> from running if magic quotes is enabled.  So you shouldn't need to
> have any get_magic_quotes_gpc() checks anymore.

Isn't it better to support both configurations ? My solution works with
or without magic quotes.
> I also noticed your quote_smart_sql() call.  This call isn't used
> anywhere, we shouldn't add calls to functions that aren't called.  We

It is used in 3/3.
> also already have a function that will make sql calls safe called
> query_paramters() in include/db.php.  Also, do we want to strip tags
> from sql?  Won't that remove all tags from things like app/version
> descriptions, comments and notes?

No, there is a parameter in this function (quote_smart_sql). By default
we don't remove html, but for some fields we might want to filter out
html (comment titles, etc.)

Thanks.

Jonathan

[AppDb] [2/3] safe functions

[Chris Morgan]

On Monday 26 June 2006 1:50 am, Jonathan Ernst wrote:
> Le dimanche 25 juin 2006 à 20:00 -0400, Chris Morgan a écrit :
> > Hi Jonathan.
> >
> > You'll want to talk to EA about the filtering changes.  The plan is to
> > filter using the same syntax and flags that the php filter extension
> > is going to use so we can easily switch over to this extension in the
> > future.
>
> I know we could use PEAR and we could also use a database abstraction
> layer, I just thought my solution was better because it has proven to
> work well on several projects I worked recently and is recommanded by
> the php manual (and it makes queries more readable than using other
> syntaxes).

The most effective solution involves both filtering and sql protection.  The 
first layer of protection is to filter each input variable down to the most 
restrictive data we can accept.

The next step is to ensure that each query, independent of the data passed in, 
is safe because of the appropriate quoting.

With both of these layers in place we can be pretty sure that injection and 
other attacks on the code will be much less effective.  We could probably be 
secure with only the sql protection in place but the filtering raises the bar 
greatly for any potential exploits of sql or any other logic in the appdb.
> > Also, I've submitted a patch for review to [e-mail] and
> > [e-mail] that removes all of our get_magic_quotes_gpc()
> > use and adds a check in include/incl.php that warns and prevents appdb
> > from running if magic quotes is enabled.  So you shouldn't need to
> > have any get_magic_quotes_gpc() checks anymore.
>
> Isn't it better to support both configurations ? My solution works with
> or without magic quotes.

I don't believe so.  If you read the patch that I submitted or look in 
include/incl.php you'll see the reasoning behind not bothering with magic 
quotes.
> > I also noticed your quote_smart_sql() call.  This call isn't used
> > anywhere, we shouldn't add calls to functions that aren't called.  We
>
> It is used in 3/3.
> > also already have a function that will make sql calls safe called
> > query_paramters() in include/db.php.  Also, do we want to strip tags
> > from sql?  Won't that remove all tags from things like app/version
> > descriptions, comments and notes?
>
> No, there is a parameter in this function (quote_smart_sql). By default
> we don't remove html, but for some fields we might want to filter out
> html (comment titles, etc.)

Ahh I see.

Chris

[AppDb] [2/3] safe functions

[Jonathan Ernst]

Le lundi 26 juin 2006 à 11:54 -0400, Chris Morgan a écrit :
> [...]
> The most effective solution involves both filtering and sql protection.  The 
> first layer of protection is to filter each input variable down to the most 
> restrictive data we can accept.
>
> The next step is to ensure that each query, independent of the data passed in, 
> is safe because of the appropriate quoting.
>
> With both of these layers in place we can be pretty sure that injection and 
> other attacks on the code will be much less effective.  We could probably be 
> secure with only the sql protection in place but the filtering raises the bar 
> greatly for any potential exploits of sql or any other logic in the appdb.

IMHO SQL protection (mysql_real_escape_string()) and correct database
design (ex. if we want integer make the field integer, an sql query with
something else will fail), along with data filtering when we display
user submitted data that doesn't come from database gives the same
protection (both SQL and HTML injection) without making the code
unreadable and having to copy strings everywhere and have is_integer(),
is_empty(), whatever checks everywhere.
> > > Also, I've submitted a patch for review to [e-mail] and
> > > [e-mail] that removes all of our get_magic_quotes_gpc()
> > > use and adds a check in include/incl.php that warns and prevents appdb
> > > from running if magic quotes is enabled.  So you shouldn't need to
> > > have any get_magic_quotes_gpc() checks anymore.
> >
> > Isn't it better to support both configurations ? My solution works with
> > or without magic quotes.
>
> I don't believe so.  If you read the patch that I submitted or look in 
> include/incl.php you'll see the reasoning behind not bothering with magic 
> quotes.
> echo "No!  <b>addslashes()</b> isn't adequate.  You should use
> <b>mysql_real_escape_string()</b> or some other function";
>    echo " that will handle multi-byte characters.  See <a href="http://shiflett.org/archive/184%5C%22%3Ethis article</a>";
>    echo " for a way to exploit <b>addslash()</b>ed parameters.<br/><br/>";

My function does just that.
> echo "A second reason is that with magic quotes enabled, due to the use of <b>mysql_real_escape_string()</b> to";
>    echo " protect from sql injection attacks we'll end up with variables that have been addslash()ed and";
>    echo " <b>mysql_real_escape_string()</b>ed.  So you end up having to call stripslashes() on EVERY variable. ";

No big deal, my function stripslashes only when magic quotes are enabled so the only
drawback of having magic_quotes enabled is that the AppDB is
theoretically a bit slower because we have to make a stripslashes
before applying mysql_real_escape_string if someone wishes to let magic_quotes enabled.
> > > I also noticed your quote_smart_sql() call.  This call isn't used
> > > anywhere, we shouldn't add calls to functions that aren't called.  We
> >
> > It is used in 3/3.
> > > also already have a function that will make sql calls safe called
> > > query_paramters() in include/db.php.  Also, do we want to strip tags
> > > from sql?  Won't that remove all tags from things like app/version
> > > descriptions, comments and notes?
> >
> > No, there is a parameter in this function (quote_smart_sql). By default
> > we don't remove html, but for some fields we might want to filter out
> > html (comment titles, etc.)
>
> Ahh I see.

Some questions :

- Will you apply my coding_standards patch ?
- Will you apply my show_error_page patch ?
- Will you refuse my proposed way of securing the AppDB ? (I need to
know because I'm willing to cleanup the rest of the AppDB regarding this
issue (and others) but I don't want to send more patches if they will be
discarded)

Thanks

[AppDb] [2/3] safe functions

[Chris Morgan]

On Monday 26 June 2006 12:29 pm, Jonathan Ernst wrote:
> Le lundi 26 juin 2006 à 11:54 -0400, Chris Morgan a écrit :
> > [...]
> > The most effective solution involves both filtering and sql protection. 
> > The first layer of protection is to filter each input variable down to
> > the most restrictive data we can accept.
> >
> > The next step is to ensure that each query, independent of the data
> > passed in, is safe because of the appropriate quoting.
> >
> > With both of these layers in place we can be pretty sure that injection
> > and other attacks on the code will be much less effective.  We could
> > probably be secure with only the sql protection in place but the
> > filtering raises the bar greatly for any potential exploits of sql or any
> > other logic in the appdb.
>
> IMHO SQL protection (mysql_real_escape_string()) and correct database
> design (ex. if we want integer make the field integer, an sql query with
> something else will fail), along with data filtering when we display
> user submitted data that doesn't come from database gives the same
> protection (both SQL and HTML injection) without making the code
> unreadable and having to copy strings everywhere and have is_integer(),
> is_empty(), whatever checks everywhere.

People who write books about php security disagree:
http://shiflett.org/articles/security-corner-apr2004
> > > > Also, I've submitted a patch for review to [e-mail] and
> > > > [e-mail] that removes all of our
> > > > get_magic_quotes_gpc() use and adds a check in include/incl.php that
> > > > warns and prevents appdb from running if magic quotes is enabled.  So
> > > > you shouldn't need to have any get_magic_quotes_gpc() checks anymore.
> > >
> > > Isn't it better to support both configurations ? My solution works with
> > > or without magic quotes.
> >
> > I don't believe so.  If you read the patch that I submitted or look in
> > include/incl.php you'll see the reasoning behind not bothering with magic
> > quotes.
> >
> > echo "No!  <b>addslashes()</b> isn't adequate.  You should use
> > <b>mysql_real_escape_string()</b> or some other function";
> >    echo " that will handle multi-byte characters.  See <a
> > href="http://shiflett.org/archive/184%5C%22%3Ethis article</a>"; echo " for a
> > way to exploit <b>addslash()</b>ed parameters.<br/><br/>";
>
> My function does just that.

Ahh I see, you were addslash()ing the html stuff.
> > echo "A second reason is that with magic quotes enabled, due to the
> > use of <b>mysql_real_escape_string()</b> to"; echo " protect from sql
> > injection attacks we'll end up with variables that have been addslash()ed
> > and"; echo " <b>mysql_real_escape_string()</b>ed.  So you end up having
> > to call stripslashes() on EVERY variable. ";
>
> No big deal, my function stripslashes only when magic quotes are enabled so
> the only drawback of having magic_quotes enabled is that the AppDB is
> theoretically a bit slower because we have to make a stripslashes
> before applying mysql_real_escape_string if someone wishes to let
> magic_quotes enabled.
> > > > I also noticed your quote_smart_sql() call.  This call isn't used
> > > > anywhere, we shouldn't add calls to functions that aren't called.  We
> > >
> > > It is used in 3/3.
> > > > also already have a function that will make sql calls safe called
> > > > query_paramters() in include/db.php.  Also, do we want to strip tags
> > > > from sql?  Won't that remove all tags from things like app/version
> > > > descriptions, comments and notes?
> > >
> > > No, there is a parameter in this function (quote_smart_sql). By default
> > > we don't remove html, but for some fields we might want to filter out
> > > html (comment titles, etc.)
> >
> > Ahh I see.
>
> Some questions :
> - Will you apply my coding_standards patch ?
> - Will you apply my show_error_page patch ?
> - Will you refuse my proposed way of securing the AppDB ? (I need to
>
> know because I'm willing to cleanup the rest of the AppDB regarding this
> issue (and others) but I don't want to send more patches if they will be
> discarded)

coding standard patch has been applied.  It was in the queue to be but was 
busy with other appdb things.

show_error_page patch is behind the sql injection patches that are higher 
priority.

Securing the appdb is going to be done in two layers like current experts in 
the field such as Chris Shiflett and others recommend.

The first layer is proper filtering of data, see php.net/filter.  Filtering 
data should ensure that only appropriately typed and valued data can even get 
into use If we can't get this extension on the server, and I've emailed 
Jeremy Newman a few times about this without any response, then we should 
implement our own class that implements just what we need but uses the same 
syntax and parameter values.  This way if we upgrade to php >= 5.2 we'll be 
able to switch over without much trouble.

The second layer is the use of query_parameters() for all sql queries.  This 
encapsulates the function we will use to protect each parameter and matches 
the formatting used in pear db.  Making the user call sprintf() and 
quote_smart_sql() breaks encapsulation and is more complex than the 
query_parameters() way of doing things.  sprintf() also lacks the 
functionality of the ?, ~ and & characters that query_parameters() supports. 
query_parameters() also matches the use of prepared statements that we may 
want to use in the future to speed up sql queries.

Today the final piece of the second layer is nearly completed and in place so 
this part of your patches seems to overlap a cleaner and completed way of 
fixing sql injection issues.

Fixing the filtering layer may or may not have been started yet by EA but 
we'll want to get something to replace makeSafe() with that lines up with 
php's filter extension.

Chris