Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87738
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.180 as permitted sender)
Content-Type: multipart/alternative; boundary="Apple-Mail=_EBCA85C5-1CE0-4CEC-B7D0-0FE883F6D425"
Mime-Version: 1.0 (Mac OS X Mail 7.3 \(1878.6\))
In-Reply-To: <CAGa2bXaOHoRk5uWei2dPBetfOQqa9bF5OL8vw6sAfN=WB42jzw@mail.gmail.com>
Date: Thu, 13 Aug 2015 12:00:41 +0100
Cc: Anthony Ferrara <ircmaxell@gmail.com>,
 Julien Pauli <jpauli@php.net>,
 Matt Tait <matt.tait@gmail.com>,
 PHP Internals <internals@lists.php.net>
Message-ID: <F6F7B55E-5BAA-4599-A57A-C4CFA77A37FB@craigfrancis.co.uk>
References: <CAEZfaM5t3MpVk=rz7JUPdao=qq96J3cowWQmKboqTgLOXLqaCA@mail.gmail.com> <CAMUwpuTJKO+JCWgiVMYRxGDbj1qdhR3Dvoho004KUh_MJoZD4g@mail.gmail.com> <CAAyV7nExkQtPd3Z0hdYp_+iF8ATsLu0itBrXCQX0zntr9y4eCA@mail.gmail.com> <CAEZfaM5kaMLv9y7T0C9E2Xi=AUuHwAFosjd8wGf6QQ120Pi46Q@mail.gmail.com> <CAAyV7nFsqNkbQwCd85p3uAxY4RrPemnqUYktcuT1Xmg50xfdWQ@mail.gmail.com> <CAEZfaM6jF=iW2Cc=kNxyFZ4B-HES9aH9ywNWzRXvXOx++06Cag@mail.gmail.com> <CAAyV7nGEy8fBYPvDqwmP+3FDSEBqjN9=0gfnbWvOSK6A1c6YJw@mail.gmail.com> <B2B729F9-DD03-4FD7-9AA1-FA35F1F764FE@gmail.com> <882D42B0-3554-4CAC-9EB9-09A0F00A35E8@craigfrancis.co.uk> <CAGa2bXaOHoRk5uWei2dPBetfOQqa9bF5OL8vw6sAfN=WB42jzw@mail.gmail.com>
To: Yasuo Ohgaki <yohgaki@ohgaki.net>
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=_EBCA85C5-1CE0-4CEC-B7D0-0FE883F6D425
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

> I don't think the proposal is useless nor ineffective.
> Taint system is nice to have, but the proposal does not seem =
preferable resolution.=20
> Don't get me wrong. I agree with your discussion overall.
>=20
> I tend to dislike all or nothing choice for security related issues =
especially, nonetheless =20
> "nothing" is my choice this time...




Hi Yasuo,

I can certainly see why you would choose not to... e.g. if magic quotes =
had not been implemented, I think things would have been better.

But I think any one of these 3 proposals (or maybe something else) would =
provide very helpful information to developers.

I don't think anyone wants to go down the "PHP fixes things =
automatically" route, as I've yet to find any kind of automatic system =
that works reliably... for example I hate tempting systems that try to =
"automatically" HTML encode values (good in theory, but personally I =
want them to complain when I get it wrong - so I can either fix it, or =
put something in place to say that this one is fine).

But this is a technique that has been proven to work in static analysis =
tools... it's probably one of their main features (even if they will =
always find it hard to check properly - as the code itself isn't really =
running), whereas PHP can be doing these checks as the code is executed.

What PHP does with this information is up for debate, I personally just =
want a simple log of things to fix... and I take Ronabop's point that =
maybe this should not be enabled by default, as so many developers are =
out there who will see it as an annoyance (at least until the feature is =
introduced to them properly).

I've also added a couple of notes below.

Craig







On 12 Aug 2015, at 00:27, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Hi all,
>=20
> On Mon, Aug 10, 2015 at 6:57 PM, Craig Francis =
<craig@craigfrancis.co.uk> wrote:
> I have been reading your conversation with great interest.
>=20
> But I would urge you to see Matts suggestion as a simple addition to =
the language (it's simpler than my suggestion), where his RFC seems to =
have already addressed your concerns (and he seems to have a working =
proof of concept).
>=20
> Personally I would like to see any one of these solutions being added =
to the core language, as there are far too many PHP programmers making =
ridiculous mistakes that the core language can be (and should be) =
identifying. That said, I am still biased to my suggestion, which also =
tries to consider other problems like XSS.
>=20
> But anyway...
>=20
> Take the code below, it is obviously wrong, but it executes without =
any complaints, and unless someone is checking every line of code that =
is written (note: PHP is doing so already), then the developer will just =
move on without thinking anything is wrong:
>=20
>=20
>         http://php.net/manual/en/pdo.exec.php
>         $dbh->exec("DELETE FROM fruit WHERE colour =3D =
'{$_GET['colour']}'");
>=20
> This is awful... It seems someone already fixed the doc.
> =20



Actually, this was me demonstrating what I've seen a couple of =
developers do before now... take the example from the documentation, =
then hack it to make it work as they want it to (without reading =
anything else on the page).




>=20
>=20
> Over the years I've heard many people say things like "use static =
analysis" or "prepared statements fix everything", but I don't think =
these are enough.
>=20
> I fully agree "prepared statements fix everything" is simply wrong.
>=20
> To be secure truly, secure API must split command and all others =
completely=20
>  _and_ command part must be programmer supplied =
constant/static/validated string.
> (i.e. Perfectly secure API must prohibit command injections at all)
> Prepared statement does not satisfy both conditions.=20
>=20
> Many APIs, that are considered secure API, are not perfectly secure=20
> because "command part must be programmer supplied =
constant/static/validated=20
> string" condition is not checked. It's left to developers.
>=20
> e.g. execl/execv splits command and arguments, only single command is =
allowed,=20
> but if "command" is user parameter, it cannot be secure.
>=20



Fair point... but I think we should still support escaping (kind of =
necessary for HTML output).

But Matt's suggestion works with your preferred setup, where the main =
SQL command is made up of constants defined in the PHP code itself (i.e. =
no variables from outside).

So if we had a way to check the source of a variables content, then =
functions like mysqli_query() or a wrapper for shell_exec() can check =
that the main command has only come from string constants, anything =
variable related would be passed in separately.




> You only have to skim read things like the second comment (with 27 up =
votes) on the PDO prepare page to see that these problems are happening =
all the time:
>=20
>=20
>         http://php.net/manual/en/pdo.prepare.php#111458
>         SELECT * FROM users WHERE $search=3D:email
>=20
>=20
> So accept that education alone is not enough, and that the basic =
building blocks like prepared statements or ORMs are not enough, and =
look at these proposals and see how they will make the language =
better... because I can assure you, having a simple tainting system that =
can be switched on to show your mistakes, is considerably better than =
what we have today (i.e. nothing... or maybe using some half baked / =
expensive static analysis tool).
>=20
> Or are you scared that this will highlight the mistakes you have made =
in your own (probably over-complicated) code? as this is why I want this =
feature, because I know I have made mistakes, and with OOP, it is very =
easy todo so (abstractions like ORMs have a tendency to allow for these =
mistakes to creep in extremely easily).
>=20
> I don't think the proposal is useless nor ineffective.
> Taint system is nice to have, but the proposal does not seem =
preferable resolution.=20
> Don't get me wrong. I agree with your discussion overall.
>=20
> I tend to dislike all or nothing choice for security related issues =
especially, nonetheless =20
> "nothing" is my choice this time...
>=20
> Regards,
>=20
> P.S. There are many outputs other than SQL. Context aware automatic =
escaping/use of=20
> secure API/validation for various outputs would be nice to have. This =
would be new API=20
> and wouldn't help users shooting their own foot, though. I don't have =
concrete idea now,=20
> but PHP may have features help it.
>=20
> --
> Yasuo Ohgaki=20
> yohgaki@ohgaki.net
>=20


--Apple-Mail=_EBCA85C5-1CE0-4CEC-B7D0-0FE883F6D425--