Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87647
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.212.175 as permitted sender)
MIME-Version: 1.0
Sender: julienpauli@gmail.com
In-Reply-To: <CAEZfaM5t3MpVk=rz7JUPdao=qq96J3cowWQmKboqTgLOXLqaCA@mail.gmail.com>
References: <CAEZfaM5t3MpVk=rz7JUPdao=qq96J3cowWQmKboqTgLOXLqaCA@mail.gmail.com>
Date: Wed, 5 Aug 2015 16:40:52 +0200
Message-ID: <CAMUwpuTJKO+JCWgiVMYRxGDbj1qdhR3Dvoho004KUh_MJoZD4g@mail.gmail.com>
To: Matt Tait <matt.tait@gmail.com>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=047d7bae469617e350051c916669
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: jpauli@php.net (Julien Pauli)

--047d7bae469617e350051c916669
Content-Type: text/plain; charset=UTF-8

On Tue, Jul 28, 2015 at 7:33 PM, Matt Tait <matt.tait@gmail.com> wrote:

> Hi all,
>
> I've written an RFC (and PoC) about automatic detection and blocking of SQL
> injection vulnerabilities directly from inside PHP via automated taint
> analysis.
>
> https://wiki.php.net/rfc/sql_injection_protection
>
> In short, we make zend_strings track where their value originated. If it
> originated as a T_STRING, from a primitive (like int) promotion, or as a
> concatenation of such strings, it's query that can't have been SQL-injected
> by an attacker controlled string. If we can't prove that the query is safe,
> that means that the query is either certainly vulnerable to a SQL-injection
> vulnerability, or sufficiently complex that it should be parameterized
> just-to-be-sure.
>
> There's also a working proof of concept over here:
>
> http://phpoops.cloudapp.net/oops.php
>
> You'll notice that the page makes a large number of SQL statements, most of
> which are not vulnerable to SQL injection, but one is. The proof of concept
> is smart enough to block that one vulnerable request, and leave all of the
> others unchanged.
>
> In terms of performance, the cost here is negligible. This is just basic
> variable taint analysis under the hood, (not an up-front intraprocedurale
> static analysis or anything complex) so there's basically no slow down.
>
> PHP SQL injections are the #1 way PHP applications get hacked - and all SQL
> injections are the result of a developer either not understanding how to
> prevent SQL injection, or taking a shortcut because it's fewer keystrokes
> to do it a "feels safe" rather than "is safe" way.
>
> What do you all think? There's obviously a bit more work to do; the PoC
> currently only covers mysqli_query, but I thought this stage is an
> interesting point to throw it open to comments before working to complete
> it.
>

I haven't read all the answers to the thread, but the RFC.

What fools me, is that you want to patch an internal, highly critical used
concept of the engine : zend_string, just for one use case.

Everything touching SQL should be independant from the engine.
Have a look at ext/mysqlnd, that plays with strings and builds a parser on
top of them to analyze SQL queries. Same for PDO.

I think such a concept should not be engine global.

Also, patching zend_string will break ABI, and should then not be merged
until next major, aka PHP8.
We have now a stable code base, PHP7 is beta, no more ABI breakage and
every extension recompilation please.

PHP has always been an extension based language : embed your thoughts into
extensions, and prevent from hacking the global engine if it is not to
support a global idea used everywhere.

Also, we have ext/tainted ; and back in 2006 when PHP5.2 were released,
such ideas as SQL injection prevention into the engine, were abandonned
because too specific. We concluded by thinking about PHP as being a general
purpose language, and high level security such as SQL injection prevention
should be taken care of in userland, or self-contained in extensions.


My thoughts.


Julien.Pauli

--047d7bae469617e350051c916669--