[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 appdb at winehq.com and
> > > wine-patches at winehq.com 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\">this 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: Ceci est une partie de message
	=?ISO-8859-1?Q?num=E9riquement?= =?ISO-8859-1?Q?_sign=E9e?=
Url : http://www.winehq.org/pipermail/wine-devel/attachments/20060626/21c1377a/attachment-0001.pgp