Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87650
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.215.52 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAMUwpuTJKO+JCWgiVMYRxGDbj1qdhR3Dvoho004KUh_MJoZD4g@mail.gmail.com>
References: <CAEZfaM5t3MpVk=rz7JUPdao=qq96J3cowWQmKboqTgLOXLqaCA@mail.gmail.com>
	<CAMUwpuTJKO+JCWgiVMYRxGDbj1qdhR3Dvoho004KUh_MJoZD4g@mail.gmail.com>
Date: Wed, 5 Aug 2015 11:27:01 -0400
Message-ID: <CAAyV7nExkQtPd3Z0hdYp_+iF8ATsLu0itBrXCQX0zntr9y4eCA@mail.gmail.com>
To: Julien Pauli <jpauli@php.net>
Cc: Matt Tait <matt.tait@gmail.com>, PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
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: ircmaxell@gmail.com (Anthony Ferrara)

All,

On Wed, Aug 5, 2015 at 10:40 AM, Julien Pauli <jpauli@php.net> wrote:
> 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.

I agree with Julien here. There are simply too many permutations of
possible "tainted" destinations. And too many ways of verifying if
something is actually secure or not.

For example, would taint checking detect ctype_digit() guards? Or
filter_* guards?

But even deeper than that is the fact that there is not just one thing
that a variable could be tainted for. A non-exhaustive list:

- MySQLi
- Postgres
- PDO
- shell_exec
- HTML context output (XSS)
- XML
- URL encoding
- Shell output (STDOUT)
- JavaScript (using MongoDB, etc)
- etc

And there's an *even deeper* problem. Consider the following code:

$name = $_GET['name'];
$escapedName = mysqli_real_escape_string($conn1, $name);
mysqli_query($conn2, "SELECT * FROM users WHERE name='$escapedName' LIMIT 1");

Unless you're tieing the taint to specific connections, that is
*possible* to inject under certain circumstances.

Given the complexities, unless a solution can address at least *some*
of these (beyond the trivial cases) I don't know if it belongs in
core.

> 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.

Now, if you can create a framework for tainting variables that
extensions can hook into, that's something interesting. Because now
extensions can declare their own taint criteria and requirements, and
have the engine track them. Doing this without sacrificing performance
is going to be tricky, but that would be interesting.

Anthony