Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87435
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.170 as permitted sender)
Content-Type: text/plain; charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 7.3 \(1878.6\))
In-Reply-To: <CAEZfaM4zWuZ5JBoMtKx0_RBUs=KeUMyTyRekPxs80Hj+P8=50Q@mail.gmail.com>
Date: Fri, 31 Jul 2015 15:05:51 +0100
Cc: Scott Arciszewski <scott@paragonie.com>,
 PHP Internals <internals@lists.php.net>
Content-Transfer-Encoding: quoted-printable
Message-ID: <3F0E6EA0-12AD-473F-859D-88C98C6E95EE@craigfrancis.co.uk>
References: <CAEZfaM5t3MpVk=rz7JUPdao=qq96J3cowWQmKboqTgLOXLqaCA@mail.gmail.com> <CAKws9z0k0VDFODWJbsbRGq1WhZy3Bdo3T4pv-MKy+fphkdF=PQ@mail.gmail.com> <CAEZfaM4zWuZ5JBoMtKx0_RBUs=KeUMyTyRekPxs80Hj+P8=50Q@mail.gmail.com>
To: Matt Tait <matt.tait@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)

Hi Matt,

I take it all back... I've been so sure of my own suggestion, I've not =
really given your RFC a proper read... and I think this could work.

The main difference I had was the ability to support to support the =
escaping functions (see pg_escape_literal for PostgreSQL, or =
htmlentities for html output)... and your solution of not supporting any =
of them keeps the whole thing very simple.

Personally I would still like to check html encoding, which your =
solution won't do... but I realise that is quite tricky, and won't be a =
perfect solution:

    $url =3D 'javascript:alert(document.cookies)';
    echo '<a href=3D"' . htmlentities($url) . '">';

But may I request 2 changes:

1) Rather than calling the zend_string property "sql-safe constant", =
could it simply be "string literal", as in, the variable has only been =
created by T_STRING's... as this could be used for other things, e.g. =
executing commands via exec_bind:

    http://news.php.net/php.internals/87361

2) Include a function that allows the PHP developers to check if the =
variable being passed is a "string literal", as frameworks can check the =
state before performing an action, e.g. they could provide the exec_bind =
function:

    function exec_bind($cmd, $arguments) {
        if (!is_string_literal($cmd)) {
            throw new Exception('...');=20
        }
        // using escapeshellarg for the $arguments
    }

Craig




On 31 Jul 2015, at 14:11, Matt Tait <matt.tait@gmail.com> wrote:

>>> This may have been true at one point in time, but my own experience
>>> and the statistics collected by Dan Kaminsky of White Hat Security
>>> indicates that Cross-Site Scripting vulnerabilities are much more
>>> prevalent in 2015 than SQL Injection, especially in business
>>> applications. If Google has information that indicates that SQLi is
>>> still more prevalent than XSS, I'd love to see this data.
>=20
> Thanks Scott!
>=20
> You are absolutely correct. SQL injection is a solved problem using
> parameterized SQL. In fact, parameterizing queries to avoid SQL =
injection
> is so uncontroversially accepted security advice, that this RFC =
*enforces* the
> advice at the language level (unless the website administrator =
explicitly
> disables the protection across the website via PHP.INI).
>=20
> Examples of SQL queries that *would* and *would not *be blocked by =
this
> tool are given in the RFC. In your case, "ORDER BY {$column} ASC" =
would not
> be blocked by this tool if $column came from within the PHP =
application,
> but would be blocked if it came from, say, $_GET["column"].
>=20
> Sadly, although SQL injection is a "solved problem" by using prepared
> statements, not everyone follows the advice. Prior to working at =
Google, I
> worked with a company called iSEC Partners who do application review =
for
> various companies. =46rom my experience, between a third and a half of =
all
> the often-huge PHP websites I security-reviewed were vulnerable to SQL
> injection somewhere. It wasn't uncommon for the company to have ~5000 =
or so
> SQL queries in their PHP code, only 2-3 of which were vulnerable to
> injection. But attackers only need one to ruin your company and steal =
your
> users' data.
>=20
> To take apart some of the other valid comments on this thread:
>=20
> =3D=3D SQL injection is dead =3D=3D
> This is objectively not the case
> =
https://www.exploit-db.com/search/?action=3Dsearch&description=3DSQL+injec=
tion
>=20
> =3D=3D Won't this break every website? =3D=3D
> No. In its initial release, this feature will be released in =
"disabled"
> mode. Website owners with heightened security requirements will be =
able to
> upgrade this to "logging" or "blocking" mode. This means this feature =
will
> have zero impact on websites that do not explicitly enable it.
>=20
> =3D=3D We should be looking at XSS instead =3D=3D
> Solving SQL injection with parameterized SQL queries is =
uncontroversial and
> universally accepted security advice. Lots of different (some working, =
some
> not) advice for solving XSS exist, but this makes it harder to =
standardize
> one of those ideas into the language without significant controversy.
>=20
> For example, one website may choose to store HTML content in their
> database. This should be printed out to HTML verbatim. Other websites =
may
> accidentally store HTML content in their database, leading to =
stored-XSS.
> The language will not be able to distinguish these two use-cases.
>=20
> In contrast, taking content out of a database and gluing it into a SQL
> statement is /always/ an error, as it can lead to second-order and
> blind-SQL injection vulnerabilities. For this reason, this RFC will
> initially only be looking at securing websites against SQL-injections,
> although perhaps there is scope to also look at XSS-injection
> vulnerabilities in a different RFC using similar tools at a later =
date.
>=20
> =3D=3D This breaks applications that use OOP or other complicated ways =
of
> sending data to a database =3D=3D
> This is not the case. This RFC doesn't use taint analysis to find SQL
> injections; it uses reverse-taint analysis to avoid blocking provably =
safe
> non-parameterized queries (so we don't block queries that are safely =
built
> dynamically out of static strings and variables). Many examples of =
this are
> given in the RFC.
>=20
> As you can see from this proof-of-concept (
> =
http://phpoops.cloudapp.net/oops.php?action=3Dmain&dbg_sql&limit=3D4%20uho=
h),
> all of the SQL statements are unparameterized, but only one =
unparameterized
> SQL statement that includes attacker-controllable data is blocked, =
which,
> as it turns out, is the only query vulnerable to SQL injection.
>=20
> Dynamically construct SQL statements out of provably safe components =
are
> not detected as a SQL-injectable error by this RFC.
>=20
> =3D=3D SQL injection is already solved! Why are you re-solving it? =3D=3D=

> This RFC doesn't solve SQL injection in a new way. It blocks dynamic =
SQL
> queries that are not parameterized by default, but uses reverse-taint
> analysis to reduce false positives where developers are building SQL =
query
> string out of other constant strings (such as config variables, table
> names, column names etc). This feature only takes action when tainted =
data
> is concatenated into a SQL query string and that query string is =
issued
> against the database, and even then, only if PHP.INI has been =
configured to
> do so.
>=20
> =3D=3D We should log, rather than block exploit attempts =3D=3D
> The feature has three modes, which is configurable via PHP.INI:
> "Safe" mode causes the query to be aborted, and a SecurityError =
exception
> thrown.
> "Logging" mode causes the query to be run, and an E_WARNING raised
> "Ignore" more causes the query to be run and no other action to be =
taken.
>=20
> The default mode will be "ignore", i.e. PHP will simply continue as in
> older versions. Developers who want to harden their code can run their
> website in "logging" mode to find and improve dangerous code in their
> codebase. Website administrators with heightened security awareness, =
or
> developers writing new code from scratch will be able to run the =
feature in
> "blocking" mode to prevent any possibility of SQL injection against =
their
> website.
>=20
> =3D=3D What about applications that pass user-submitted SQL statements =
to the
> database by design? =3D=3D
> This is accounted for in the RFC. Developers will be able to =
explicitly
> mark SQL queries as disabling the SQL-injection feature for the =
queries
> that explicitly warrant this (PHPMyAdmin being a good example). Again, =
this
> is only relevant if the website has been explicitly configured to use =
this
> feature.
>=20
> Matt
>=20
> On 30 July 2015 at 14:43, Scott Arciszewski <scott@paragonie.com> =
wrote:
>=20
>> On Tue, Jul 28, 2015 at 1:33 PM, Matt Tait <matt.tait@gmail.com> =
wrote:
>>> Hi all,
>>>=20
>>> I've written an RFC (and PoC) about automatic detection and blocking =
of
>> SQL
>>> injection vulnerabilities directly from inside PHP via automated =
taint
>>> analysis.
>>>=20
>>> https://wiki.php.net/rfc/sql_injection_protection
>>>=20
>>> 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.
>>>=20
>>> There's also a working proof of concept over here:
>>>=20
>>> http://phpoops.cloudapp.net/oops.php
>>>=20
>>> 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.
>>>=20
>>> 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.
>>>=20
>>> 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.
>>>=20
>>> 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.
>>>=20
>>> Matt
>>=20
>> Hi Matt,
>>=20
>>> 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.
>>=20
>> This may have been true at one point in time, but my own experience
>> and the statistics collected by Dan Kaminsky of White Hat Security
>> indicates that Cross-Site Scripting vulnerabilities are much more
>> prevalent in 2015 than SQL Injection, especially in business
>> applications. If Google has information that indicates that SQLi is
>> still more prevalent than XSS, I'd love to see this data.
>>=20
>> In my opinion, SQL injection is almost a solved problem. Use prepared
>> statements where you can, and strictly whitelist where you cannot
>> (i.e. "ORDER BY {$column} ASC")
>>=20
>> Scott Arciszewski
>> Chief Development Officer
>> Paragon Initiative Enterprises <https://paragonie.com>
>>=20