Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87498
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain craigfrancis.co.uk designates 209.85.212.172 as permitted sender)
Content-Type: multipart/alternative; boundary="Apple-Mail=_8B314F70-17FD-4B25-A178-CE1E01F0C1BA"
Mime-Version: 1.0 (Mac OS X Mail 7.3 \(1878.6\))
In-Reply-To: <CAFWS2+GVtTp7n+7LB78PNccEmH4QU-XtWd_BOxAAFJUp7eoFnQ@mail.gmail.com>
Date: Sun, 2 Aug 2015 10:50:34 +0100
Cc: Lester Caine <lester@lsces.co.uk>,
 internals <internals@lists.php.net>
Message-ID: <96B9C8F2-7727-4423-8E9B-BAC9A6AA3C1A@craigfrancis.co.uk>
References: <CAEZfaM5t3MpVk=rz7JUPdao=qq96J3cowWQmKboqTgLOXLqaCA@mail.gmail.com> <55B896B8.4070901@lsces.co.uk> <6F8B9B35-D487-45D9-BC84-4A782951EDC7@craigfrancis.co.uk> <55B9D14B.9010902@lsces.co.uk> <44415028-F437-4914-818C-C928BA01D7FE@craigfrancis.co.uk> <CAFWS2+FG-Ed_G4KH7o_SOxVgTCuLmpB8=WBkL47z9+v9sTquEA@mail.gmail.com> <617E0E34-E407-434F-A441-3452166E89B7@craigfrancis.co.uk> <CAFWS2+GVtTp7n+7LB78PNccEmH4QU-XtWd_BOxAAFJUp7eoFnQ@mail.gmail.com>
To: Ronald Chmara <ronabop@gmail.com>
Subject: Re: [PHP-DEV] [RFC] Block requests to builtin SQL functions where PHP can prove the call is vulnerable to a potential SQL-injection attack
From: craig@craigfrancis.co.uk (Craig Francis)

--Apple-Mail=_8B314F70-17FD-4B25-A178-CE1E01F0C1BA
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On 31 Jul 2015, at 17:40, Ronald Chmara <ronabop@gmail.com> wrote:
> The RFC and bug report both make an erroneous assumption that =
unescaped GPC input is wrong.




Hi Ronabop,

Slightly continuing our discussion, but also replying to your on-list =
message...


Starting with your examples:

1) Web applications that send variables directly to the database (e.g. =
phpMyAdmin), if I remember their codebase correctly, there is about 3 =
places where that happens, and after doing an authorisation and CSRF =
check, they could put in the 1 line to say that this value is already =
SQL escaped.

2) Overhead of escaping, where you later explained that this was more =
for legacy systems such as FoxBase and legacy ODBC  data sources, where =
they pass the values over the network to have it escaped (slow)... and =
while that is a valid concern, hopefully all escaping is done on the =
client side now (if not, my previously non-parameterised queries would =
have taken far too long to load)... but that said, =
mysql_real_escape_string() isn't good enough either (it does not apply =
quote marks, as Matts example shows), hence why my examples talk about =
pg_escape_literal().

3) Generic user written escaping libraries/functions to support multiple =
destinations... if they are still using the native escaping functions, =
then they don't need to do anything different... if the are using =
something like addslashes (maybe this is acceptable for something), then =
that library/function should mark it as having been escaped.

4) =46rom a pre-cleaned source, e.g. taking HTML from a WYSIWYG editor, =
passing it though HTMLTidy, storing in the database... likewise, this =
just needs a single function call to say that this value is already HTML =
encoded - this is the "bio_html" example shown in bug 69886 :-)

5) Where a developer "is doing a file read off of a local hard drive and =
assuming their file contents will never have an SQL injection"... well, =
I'd like all string variables (e.g. from GPC, a file, database, xml, =
etc) to start with the assumption that it is unescaped... the developer =
could mark that as having been escaped, but more likely, just escape it =
(or use it with a hard coded, parameterised query, in the case of SQL).

--

But I completely agree that raising too many notices will just mean that =
this feature would be switched off.

I've been focusing too much of my own systems, and the assumption that =
most systems would/should be using parameterised queries, html encoding, =
etc (so they shouldn't get any notices, unless they have made a couple =
of mistakes).

So the suggestion of using a new ini setting is a good idea (from Matts =
or the 2008 RFC's)... or as you suggested, perhaps have it on a per file =
basis, that could also work, like the declare(strict_types=3D1).

That way we have a useful security feature that can be switched on =
(rather than everyone investing in an expensive static code analysis =
system, which uses dark magic to find "every problem")... then hopefully =
early adopters would start using it, it slowly becomes good practice, =
and sometime in the long distant future, it can be enabled by default =
(while in the mean time we try to educate as many developers as we can =
though channels like Stack Overflow).

---

As to the "rarely" comment, fair point, I don't have numbers to back =
this up.

What I'm trying to say is that, for the typical use case for websites =
(i.e. not things like phpMyAdmin), when you are using values in SQL, it =
is for them to be used as variables in the query... e.g. where an id =
equals this value, or insert a record with these values, or update this =
record with this value... i.e. how values in ORMs should be used.

Likewise, when you have variables that need to be printed in HTML, the =
"safe" normal would be to have the PHP (or other static code) provide =
the HTML, and the variables should be html encoded (e.g. if you were =
printing someones name)... there is an exception to this was explained =
above (example 4).

I realise that I'm trying to word it differently, but what I'm trying to =
say is that all values should be assumed bad, and it is up to the =
developer to either escape them, use parameterised queries, etc... or =
call a single function to say "yes, I know this one is fine"... then PHP =
can identify anything that has been missed.

Craig







On 31 Jul 2015, at 17:40, Ronald Chmara <ronabop@gmail.com> wrote:

> On Thu, Jul 30, 2015 at 8:38 AM, Craig Francis =
<craig@craigfrancis.co.uk> wrote:
> On 30 Jul 2015, at 16:26, Ronald Chmara <ronabop@gmail.com> wrote:
> > Perhaps I have missed something in this discussion
> I think you have... my email from a couple of weeks ago was ignored... =
so I replied to Matt's suggestion (which is similar, but different).
> Please, just spend a few minutes reading my suggestion, it has =
absolutely nothing todo with breaking applications:
>    http://news.php.net/php.internals/87207
>    https://bugs.php.net/bug.php?id=3D69886
>=20
> The RFC and bug report both make an erroneous assumption that =
unescaped GPC input is wrong.
>=20
> I was addressing some cases where it is the correct, intended, =
behavior.=20
>=20
> WRT "breaking":
> Application stacks which go from zero E_NOTICE warnings, to hundreds =
or thousands of them a second or day, is (admittedly) poorly phrased as =
"breaking". It is a possible side effect of the proposed solutiions. I =
have worked in very stingent environments where an unapproved E_NOTICE =
is considered a ship-stop break, but I did not explicitly explain that. =
Such environments would require re-writes of all SQL that throws an =
E_NOTICE, or a per-line exception review and approval process, or =
disabling/not enabling the checking.
> =20
> And yes, I do have a bypass_the_nerfing function (well, a function to =
say the variable has already been escaped)... but the idea is that it's =
ever so slightly harder to use than the related escaping functions, and =
rarely needed.
>=20
> "Rarely" is subjective at this point, a quanyifyable sampling of some =
kind could be more meaningful. (How rare?)
>=20
> Based on *my* purely anecdotal experience, in the last X years of =
using PHP I have have frequently encountered situations where passing =
through engine-unescaped text strings, to SQL, is desired for some =
reason, in nearly every environment. I mentioned one use case that I =
thought might be relevant to a large number of users, there are others.
>=20
> Off the top of my head, some use cases I have dealt with relevant to =
this discussion, that should be considered (even if they're discarded as =
trivial):
> 1. Administrator has a web application that is supposed to allow them =
access functionally equivalent to a direct connection to a database.
> 2. Overhead of using the engine escaping technique (setup =
connection(if not done yet), send text to escape at network speed, =
recieve escaped text at network speed) was considered too much of an =
additional performance hit.
> 3. Text needed to have a generic, user written, escaping =
library/function to handle multiple destinations (think 5 different data =
storage systems, all with different escaping rules, some without =
connection based escaping).
> 4. Text being supplied was coming from a pre-cleaned, trusted, source, =
even though the variable was GPC, (example: it was a GET string =
assembled by a batch job that was pulling from another database)
>=20
> I'm sure there are many more.
>=20
> Basing language decisions on personal perceptions of "rarely" and =
"frequently" is not a good practice, but ensuring that we are covering =
multiple use-cases is. Discussing the use cases doesn't mean I think the =
idea is without merit.=20
>=20
> -Ronabop


--Apple-Mail=_8B314F70-17FD-4B25-A178-CE1E01F0C1BA--