Everything

php

Home » Project » Newsgroups » php.internals » [RFC] Block requests to builtin SQL functions where PHP can prove the call is vulnerable to a potential SQL-injection attack »

[RFC] Block requests to builtin SQL functions where PHP can prove the call is vulnerable to a potential SQL-injection attack

php.internals

Matt Tait

11 years ago

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.

Matt

Rowan Collins

11 years ago

On 28 July 2015 18:33:31 BST, 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

Have you searched the list archive and wiki for previous discussions and prototypes of variable tainting? The idea may well have some legs, but there might be some interesting points from previous discussions to note in your RFC.

Also, 7.0 is already in beta, so your RFC will need to target 7.1 at the earliest.

Regards,

--
Rowan Collins
[IMSoP]

Christoph Becker

11 years ago

Rowan Collins wrote:

> On 28 July 2015 18:33:31 BST, 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
>
> Have you searched the list archive and wiki for previous discussions and prototypes of variable tainting? The idea may well have some legs, but there might be some interesting points from previous discussions to note in your RFC.

FWIW, there is the inactive "Taint support for PHP"[1] RFC.

[1] <https://wiki.php.net/rfc/taint>

--
Christoph M. Becker

Pierre Joye

11 years ago

The
On Jul 28, 2015 11:42 PM, "Christoph Becker" <cmbecker69@gmx.de> wrote:

>
> Rowan Collins wrote:
>
> > On 28 July 2015 18:33:31 BST, 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
> >
> > Have you searched the list archive and wiki for previous discussions

and prototypes of variable tainting? The idea may well have some legs, but
there might be some interesting points from previous discussions to note in
your RFC.

>
> FWIW, there is the inactive "Taint support for PHP"[1] RFC.
>
> [1] <https://wiki.php.net/rfc/taint>

Which is what should be done (global tainted mode) and not only for SQL.

Unfiltered input can affect way more than only SQL. Environment, exec, etc
are all potentially dangerous with unfiltered data.

I fear it is an almost impossible task and may give a wrong signal,
everything is safe of tainted mode is enabled.

Cheers,
Pierre

Unnamed Person

11 years ago

Pierre Joye wrote on 28.07.2015 23:05:

> The
> On Jul 28, 2015 11:42 PM, "Christoph Becker" <cmbecker69@gmx.de> wrote:
>>
>> Rowan Collins wrote:
>>
>> > On 28 July 2015 18:33:31 BST, 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
>> >
>> > Have you searched the list archive and wiki for previous discussions
> and prototypes of variable tainting? The idea may well have some legs, but
> there might be some interesting points from previous discussions to note in
> your RFC.
>>
>> FWIW, there is the inactive "Taint support for PHP"[1] RFC.
>>
>> [1] <https://wiki.php.net/rfc/taint>
>
> Which is what should be done (global tainted mode) and not only for SQL.
>
> Unfiltered input can affect way more than only SQL. Environment, exec, etc
> are all potentially dangerous with unfiltered data.
>
> I fear it is an almost impossible task and may give a wrong signal,
> everything is safe of tainted mode is enabled.
>
> Cheers,
> Pierre
>

I think it's better to support parameter substitution and escaping directly in the extensions or the core functions:

Idea 1:

mixed mysqli_query_bind ( mysqli $link , string $query [, array $parameters [, int $resultmode = MYSQLI_STORE_RESULT ] ] )

e.g.

mysqli_query_bind($link, 'SELECT * FROM users WHERE usertype = ?', [$usertype]);

mysqli_query_bind($link, 'SELECT * FROM users WHERE id IN (?)', [[1,2,3]]);

Using mysqli_query_bind() means parameters are substituted in as (correctly) escaped strings and the result is run with mysqli_query().

and similar:

exec_bind ( string $command [, array $parameters [, array &$output [, int &$return_var ] ] ] )

echo exec_bind('ls ?', [$someDir]);

Using exec_bind() means parameters are substituted in as (correctly) escaped strings and the result is run with exec().

Those who want to secure their legacy code can use "disable_functions=mysqli_query,exec" and change the occurrences of both functions to the new bind functions.

If people still use echo exec_bind('ls '.$someDir), static code analysis can find it, similar to unsafe includes.

Regards
Thomas

Lester Caine

11 years ago

On 28/07/15 18:33, Matt Tait wrote:

> 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.

If you want a safe and stable system ... don't use mysql ...
The problem is removing all of the poor quality on-line guides and
replacing them with ones which provide a mush better working model.
Trying to get PHP too pick up a few edge cases is a poor use of time.

--
Lester Caine - G8HFL
-----------------------------
Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

Craig Francis

11 years ago

On 29 Jul 2015, at 10:02, Lester Caine <lester@lsces.co.uk> wrote:

> The problem is removing all of the poor quality on-line guides and
> replacing them with ones which provide a mush better working model.
> Trying to get PHP too pick up a few edge cases is a poor use of time.

I completely disagree... prepared statements are just as vulnerable, and so are ORM's.

You can push developers towards these solutions, and that would be good, but you are completely blind if you think an uneducated developer won't do:

if ($stmt = $mysqli->prepare("SELECT District FROM City WHERE Name=" . $_GET['name'])) {
}

And thats using a slightly edited example from:

http://php.net/manual/en/mysqli.prepare.php

It's a shame that Wietse suggested this solution in 2008, is incomplete, and does not seem to be going anywhere (I'm also tempted to say the implementation is slightly the wrong way around, but the theory is there).

Likewise the PECL extension from 2013.

http://pecl.php.net/package/taint

Matt, I realise I'm not a C programmer, and probably won't be able to help there, but if there is anything I can do, please let me know.

If you want to compare notes, my suggestion is at:

http://news.php.net/php.internals/87207

Craig

On 29 Jul 2015, at 10:02, Lester Caine <lester@lsces.co.uk> wrote:

Lester Caine

11 years ago

On 29/07/15 16:11, Craig Francis wrote:

> I completely disagree... prepared statements are just as vulnerable, and so are ORM's.
>
> You can push developers towards these solutions, and that would be good, but you are completely blind if you think an uneducated developer won't do:
>
> 	if ($stmt = $mysqli->prepare("SELECT District FROM City WHERE Name=" . $_GET['name'])) {
> 	}

But that is a perfect example of what I am talking about. You do not
educate people by publishing the very thing that is wrong. You educate
them by pointing out to them WHY the '?' was there in the first place.

Since the taint extension only covers mysql and sqlite it's of little
use if we manage to convert 'uneducated developer' to any of the more
secure databases, and that was one of the reasons why mysql was dropped
from being loaded by default. Once one starts from a base of
parametrised sql queries the lax programming methods many mysql guides
and books continue to push can be reversed. Throwing more bloat into php
to create 'WTF' errors just adds to a new users frustration and annoys
experienced users who have very good reasons for building queries using
clean variables. MANY abstraction layers use variables to add prefixes
to table names or fields.

Educate ... don't nanny ...

--
Lester Caine - G8HFL
-----------------------------
Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

Craig Francis

11 years ago

On 30 Jul 2015, at 08:24, Lester Caine <lester@lsces.co.uk> wrote:

> But that is a perfect example of what I am talking about. You do not
> educate people by publishing the very thing that is wrong. You educate
> them by pointing out to them WHY the '?' was there in the first place.

I completely agree on education, and what I'm hoping for... and this is how we can educate everyone :-)

My suggestion for taints (not quite the same as the one from Matt or Wietse) was not to change the way good programs are created/executed, but simply an education device, which can also pick up mistakes that experienced developers make.

While my first post on this mailing list gives a better overview:

http://news.php.net/php.internals/87207

The original implementation suggestion is at:

https://bugs.php.net/bug.php?id=69886

You will see that it does nothing more than create notices to say "erm, do you want to be doing this?".

This is something that only PHP can do, unless you can find a way of changing every single article / code example on the internet :-)

So, with your example... if you want to use a variable for a table/field prefix, that is perfectly fine... in fact, it won't need any changes, as the prefix will probably be hard coded as a string within a PHP script (something I called ETYPE_CONSTANT).

But if not (e.g. storing the prefix in an ini file?), then I've shown an example of how that can be handled with the proposed "string_encoding_set" function (something I should have probably called string_escaping_set)... which is simply to tell PHP that this one variable is already safe (something I can't see being needed very often).

Craig

On 30 Jul 2015, at 08:24, Lester Caine <lester@lsces.co.uk> wrote:

Joe Watkins

11 years ago

I find myself agreeing with Pierre; The wrong signal would be sent. History
should teach us there is no such thing as (a) safe mode.

Xinchen did used to work on a taint extension, I wonder why that was
stopped ?

Worth noticing that the extension is rather complex, touching many parts of
the engine, changing many things ... which I don't really like.

Cheers
Joe

On Thu, Jul 30, 2015 at 10:14 AM, Craig Francis <craig@craigfrancis.co.uk>
wrote:

Craig Francis

11 years ago

On 30 Jul 2015, at 13:14, Joe Watkins <pthreads@pthreads.org> wrote:

> I find myself agreeing with Pierre; The wrong signal would be sent. History should teach us there is no such thing as (a) safe mode.

Hi Joe,

Please can you read my proposal (see the email you just replied to, also below)... I'm replying on this thread because my first one was ignored... I'm not suggesting a "safe mode" or any kind of blocking of requests (as per the subject)... as I agree, and believe that would be worse than the old auto escaping from PHP 4.

Craig

On 30 Jul 2015, at 13:14, Joe Watkins <pthreads@pthreads.org> wrote:

Laruence

11 years ago

Hey:

On Thu, Jul 30, 2015 at 8:14 PM, Joe Watkins <pthreads@pthreads.org> wrote:

> I find myself agreeing with Pierre; The wrong signal would be sent. History
> should teach us there is no such thing as (a) safe mode.
>
> Xinchen did used to work on a taint extension, I wonder why that was
> stopped ?

yes, it is https://github.com/laruence/php-taint

Anyway, I was too busy so I didn't make it supports PHP-5.6, I was
hoping someone could help(it supports 5.5 now).

it is a complex extension, and using tricky way to keep taint infos

anyway, with PHP7's new zend_string, and string flags, the
implementation will become easier.

I have a plan to make it supports PHP7..

thanks

>
> Worth noticing that the extension is rather complex, touching many parts of
> the engine, changing many things ... which I don't really like.
>
> Cheers
> Joe
>
> On Thu, Jul 30, 2015 at 10:14 AM, Craig Francis <craig@craigfrancis.co.uk>
> wrote:
>
>> On 30 Jul 2015, at 08:24, Lester Caine <lester@lsces.co.uk> wrote:
>>
>> > But that is a perfect example of what I am talking about. You do not
>> > educate people by publishing the very thing that is wrong. You educate
>> > them by pointing out to them WHY the '?' was there in the first place.
>>
>>
>>
>>
>> I completely agree on education, and what I'm hoping for... and this is
>> how we can educate everyone :-)
>>
>> My suggestion for taints (not quite the same as the one from Matt or
>> Wietse) was not to change the way good programs are created/executed, but
>> simply an education device, which can also pick up mistakes that
>> experienced developers make.
>>
>> While my first post on this mailing list gives a better overview:
>>
>>    http://news.php.net/php.internals/87207
>>
>> The original implementation suggestion is at:
>>
>>    https://bugs.php.net/bug.php?id=69886
>>
>> You will see that it does nothing more than create notices to say "erm, do
>> you want to be doing this?".
>>
>> This is something that only PHP can do, unless you can find a way of
>> changing every single article / code example on the internet :-)
>>
>> So, with your example... if you want to use a variable for a table/field
>> prefix, that is perfectly fine... in fact, it won't need any changes, as
>> the prefix will probably be hard coded as a string within a PHP script
>> (something I called ETYPE_CONSTANT).
>>
>> But if not (e.g. storing the prefix in an ini file?), then I've shown an
>> example of how that can be handled with the proposed "string_encoding_set"
>> function (something I should have probably called string_escaping_set)...
>> which is simply to tell PHP that this one variable is already safe
>> (something I can't see being needed very often).
>>
>> Craig
>>
>>
>>
>>
>>
>> On 30 Jul 2015, at 08:24, Lester Caine <lester@lsces.co.uk> wrote:
>>
>> > On 29/07/15 16:11, Craig Francis wrote:
>> >> I completely disagree... prepared statements are just as vulnerable,
>> and so are ORM's.
>> >>
>> >> You can push developers towards these solutions, and that would be
>> good, but you are completely blind if you think an uneducated developer
>> won't do:
>> >>
>> >>      if ($stmt = $mysqli->prepare("SELECT District FROM City WHERE
>> Name=" . $_GET['name'])) {
>> >>      }
>> >
>> > But that is a perfect example of what I am talking about. You do not
>> > educate people by publishing the very thing that is wrong. You educate
>> > them by pointing out to them WHY the '?' was there in the first place.
>> >
>> > Since the taint extension only covers mysql and sqlite it's of little
>> > use if we manage to convert 'uneducated developer' to any of the more
>> > secure databases, and that was one of the reasons why mysql was dropped
>> > from being loaded by default. Once one starts from a base of
>> > parametrised sql queries the lax programming methods many mysql guides
>> > and books continue to push can be reversed. Throwing more bloat into php
>> > to create 'WTF' errors just adds to a new users frustration and annoys
>> > experienced users who have very good reasons for building queries using
>> > clean variables. MANY abstraction layers use variables to add prefixes
>> > to table names or fields.
>> >
>> > Educate ... don't nanny ...
>> >
>> > --
>> > Lester Caine - G8HFL
>> > -----------------------------
>> > Contact - http://lsces.co.uk/wiki/?page=contact
>> > L.S.Caine Electronic Services - http://lsces.co.uk
>> > EnquirySolve - http://enquirysolve.com/
>> > Model Engineers Digital Workshop - http://medw.co.uk
>> > Rainbow Digital Media - http://rainbowdigitalmedia.co.uk
>> >
>> > --
>> > PHP Internals - PHP Runtime Development Mailing List
>> > To unsubscribe, visit: http://www.php.net/unsub.php
>> >
>>
>>
>> --
>> PHP Internals - PHP Runtime Development Mailing List
>> To unsubscribe, visit: http://www.php.net/unsub.php
>>
>>

--
Xinchen Hui
@Laruence
http://www.laruence.com/

Craig Francis

11 years ago

On 30 Jul 2015, at 13:47, Xinchen Hui <laruence@php.net> wrote:

> anyway, with PHP7's new zend_string, and string flags, the
> implementation will become easier.

Hi Xinchen,

Glad to hear that you are still looking into this... please let me know if there is anything I can do to help (unfortunately I'm not a C programer).

Out of interest, if you are going to continue using "taint_marks", as the RFC suggested... can I suggest all variables start with 0 (or undefined)... then you can set flags as the variables are passed though functions like htmlentities and pg_escape_literal?

This way all variables are treated as plain (unsafe), and then developers either need to escape them (e.g. when printing to output, or using in SQL, CLI, etc), or they can mark them as having already been escaped (rare).

Likewise, I know you have examples that say "SCRIPT_FILENAME" is safe by default (I kind of disagree)... it would still be advisable to encode them, even if they are being included in the HTML... personally I would not have any variables marked as safe by default (with the single exception of strings that are defined in the PHP code itself).

Craig

On 30 Jul 2015, at 13:47, Xinchen Hui <laruence@php.net> wrote:

Ronald Chmara

11 years ago

Perhaps I have missed something in this discussion where such a change to
PHP does not break every single application that is supposed to pass raw,
user submitted, SQL *without* getting prepared/nerfed, or warned about, by
intentional application design.

If we're just limiting the nerfing for submitted GPC variables (since PHP
is used a lot for web applications).... we still have a non-trivial number
of those installed applications which require raw, user created, unescaped
SQL, passing through to function as designed.

I am thinking of the class of applications like phpMyAdmin, as well as the
the millions of other database utility scripts, application install
scripts, (etc.) out there that perform similar tasks, that need to pass raw
SQL, as crafted by users, without preparation, intentionally.

Of course, we could just add a "bypass_the_nerfing()" function, and such a
function could then possibly see widespread adoption, everywhere, rendering
the entire exercise moot.

Craig Francis

11 years ago

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=69886

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.

On 30 Jul 2015, at 16:26, Ronald Chmara <ronabop@gmail.com> wrote:

Ronald Chmara

11 years ago

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=69886


The RFC and bug report both make an erroneous assumption that unescaped GPC
input is wrong.

I was addressing some cases where it is the correct, intended, behavior.

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.

> 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.

"Rarely" is subjective at this point, a quanyifyable sampling of some kind
could be more meaningful. (How rare?)

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.

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)

I'm sure there are many more.

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.

-Ronabop

Craig Francis

11 years ago

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) From 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=1).

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:

Scott Arciszewski

11 years ago

On Tue, Jul 28, 2015 at 1: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.
>
> Matt

Hi Matt,

> 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.

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.

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")

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com>

Craig Francis

11 years ago

On 30 Jul 2015, at 14:43, Scott Arciszewski <scott@paragonie.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.

Good, because my suggestion was also addressing XSS with poor (or completely missing) HTML escaping... have a look:

http://news.php.net/php.internals/87207

https://bugs.php.net/bug.php?id=69886

Now I admit it won't fix everything with XSS (as HTML escaping is a bit harder), but it certainly will pick up quite a lot of the issues (and it wont break anything either, just help developers identify problems).

And no, SQL injection is far from a solved problem... this is why, after 15 years of me trying to tell my fellow developers to not make these mistakes, I'm still finding them making them over and over again... hence why I'm making the above suggestion.

Craig

On 30 Jul 2015, at 14:43, Scott Arciszewski <scott@paragonie.com> wrote:

Scott Arciszewski

11 years ago

On Thu, Jul 30, 2015 at 11:20 AM, Craig Francis
<craig@craigfrancis.co.uk> wrote:

> On 30 Jul 2015, at 14:43, Scott Arciszewski <scott@paragonie.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.
>
>
> Good, because my suggestion was also addressing XSS with poor (or completely missing) HTML escaping... have a look:
>
>   http://news.php.net/php.internals/87207
>
>   https://bugs.php.net/bug.php?id=69886
>
> Now I admit it won't fix everything with XSS (as HTML escaping is a bit harder), but it certainly will pick up quite a lot of the issues (and it wont break anything either, just help developers identify problems).
>
> And no, SQL injection is far from a solved problem... this is why, after 15 years of me trying to tell my fellow developers to not make these mistakes, I'm still finding them making them over and over again... hence why I'm making the above suggestion.
>
> Craig
>
>
>
>
>
>
> On 30 Jul 2015, at 14:43, Scott Arciszewski <scott@paragonie.com> wrote:
>
>> On Tue, Jul 28, 2015 at 1: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.
>>>
>>> Matt
>>
>> Hi Matt,
>>
>>> 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.
>>
>> 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.
>>
>> 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")
>>
>> Scott Arciszewski
>> Chief Development Officer
>> Paragon Initiative Enterprises <https://paragonie.com>
>>
>> --
>> PHP Internals - PHP Runtime Development Mailing List
>> To unsubscribe, visit: http://www.php.net/unsub.php
>>
>

Just because the solution is known doesn't mean it's known to
everyone. Diffusion of knowledge and good habits is the hardest
problem in application security to solve. Look, for example, at how
many college students learn to write C programs with buffer overflow
vulnerabilities in 2015. We need more effort on education, which is
part of what I've been focusing on with Paragon Initiative and Stack
Overflow.

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com>

Craig Francis

11 years ago

On 30 Jul 2015, at 16:24, Scott Arciszewski <scott@paragonie.com> wrote:

>  Just because the solution is known doesn't mean it's known to everyone.

Yes, and if you could just read what I was suggesting, rather than looking at the subject of this email (and the suggestion by Matt), then you will notice this is what I'm trying to do (so not just people asking questions on Stack Overflow).

My suggestion is to educate, it also has a nice side effect of having a simple checking process for everything else (without breaking anything).

On 30 Jul 2015, at 16:24, Scott Arciszewski <scott@paragonie.com> wrote:

Joe Watkins

11 years ago

Even if some of those people replying haven't read or don't understand what
you are suggesting, it is not a good tactic to assume that and reply with
"read the RFC".

There is a good chance the majority of the people replying have read the
RFC, and found reason to be negative, reserved, cautious, or whatever.

The best thing you can do now is read those responses again, and try to
find what they are saying, if you want the conversation to continue.

Cheers
Joe

On Thu, Jul 30, 2015 at 4:38 PM, Craig Francis <craig@craigfrancis.co.uk>
wrote:

Craig Francis

11 years ago

On 30 Jul 2015, at 17:02, Joe Watkins <pthreads@pthreads.org> wrote:

> Even if some of those people replying haven't read or don't understand what you are suggesting, it is not a good tactic to assume that and reply with "read the RFC".

Hi Joe,

Sorry about yesterday, I have done as you have said, and I have read those responses again, but unfortunately I still feel that I was right in my assumptions (notes below, maybe some interesting additions?).

In general I have been getting very frustrated that no-one seems to really care about security (and to be fair, it has been annoying me for far too many years).

Keeping in mind that I work with other developers who routinely keep introducing vulnerabilities like SQLi, XSS, CSRF... and doing annoying things like switching the CSP off, because they copy/paste some hideous/insecure JS, and can't be bothered to work out why the "eval" function isn't working.

So maybe I should start a new thread, without Matt's subject (btw Matt, I really appreciate what you are trying todo, I disagree with the blocking element, and I think we can also address more than just SQL injection vulnerabilities)... maybe something like "I've found this one weird trick that will fix every security issue"... sorry, I hate that kind of approach, but if it gets a response, maybe its worth it :-)

Craig



--------------------------------------------------
http://news.php.net/php.internals/87346
From: Matt Tait
Reply: N/A

Original suggestion.

--------------------------------------------------
http://news.php.net/php.internals/87348
From: Rowan Collins
Reply: Matt Tait

Suggestion to Matt to look for previous RFCs.

--------------------------------------------------
http://news.php.net/php.internals/87350
From: Christoph Becker
Reply: Matt Tait

Points out the existing Taint RFC from 2008, by Wietse Venema.

--------------------------------------------------
http://news.php.net/php.internals/87355
From: Pierre Joye
Reply: Matt Tait

Pointing out that there is more than SQLi (true), and it might be an impossible task (I disagree, as explained later).

--------------------------------------------------
http://news.php.net/php.internals/87361
From: Thomas Bley
Reply: Matt Tait

Suggesting the use of bound parameters / prepared statements, which I agree is how it should be coded by the website developers, but I think the PHP language itself can help identify when this has not been done (just by raising a notice, not blocking anything).

Also Thomas points out that static code analysis could identify these issues, but these are far from perfect, and PHP is in a much better position to be doing this... and has the advantage of being available to everyone.

Just to note, I've played with a couple of static code analysers which cost in the region of £19,000 per year, and they still don't find the same number of escaping issues that my suggestion can find (they do look at other issues, so don't get rid of them, but this one thing that can be done better with the programming language itself).

--------------------------------------------------
http://news.php.net/php.internals/87363
From: Lester Caine
Reply: Matt Tait

Short answer saying you should not use mysql... which I think is a bit short sighted (on the assumption that mysqli is a similar interface for most website developers).

I'm not against using tools like ORMs (e.g. Doctrine), bound parameters / prepared statements, or even stored procedures... but they all have issues, and are all vulnerable to the kind of misuse I'm trying to address (explained in the next email).

--------------------------------------------------
http://news.php.net/php.internals/87370
From: Me (Craig Francis)
Reply: Lester Caine

Just saying I disagree with Lester, and giving a very simple example of how developers can still mess up (once you start adding some abstractions, like an ORM, this becomes much harder to detect, and is why I'm so insistent that PHP needs to be checking for these issues).

This is where I also suggest an alternative to Matt's original suggestion (something I posted 12 days before, and didn't really get any response).

--------------------------------------------------
http://news.php.net/php.internals/87383
From: Lester Caine
Reply: Me (Craig Francis)

Kind of missing the point (maybe my example was too simple), and is talking about how Matts solution would cause problems.

And I agree (sorry Matt), I don't think Matt's solution would work... but if Lester had read my reply, he would have seen that my suggestion was about education (but it can also help experienced developers as well).

--------------------------------------------------
http://news.php.net/php.internals/87386
From: Me (Craig Francis)
Reply: Lester Caine

Trying to explain to Lester that I agree on education, and pointing out that my solution is different... and how (maybe my email was too long to read?).

--------------------------------------------------
http://news.php.net/php.internals/87391
From: Joe Watkins
Reply: Me (Craig Francis)

Saying you agree with Pierre... I do as well, as Pierre was talking about Matt's solution.

--------------------------------------------------
http://news.php.net/php.internals/87393
From: Me (Craig Francis)
Reply: Joe Watkins

Pointing out that my suggestion was different... I realise I'm now trying to derail Matt's original thread (this one seems to be getting some attention), but I really want to address these same issues.

--------------------------------------------------
http://news.php.net/php.internals/87396
From: Xinchen Hui
Reply: Joe Watkins

Giving a quick status update on the original 2008 implementation... which I personally think is good, but could be made better by switching the logic around (I believe this will make the implementation easier, and avoid many edge cases).

--------------------------------------------------
http://news.php.net/php.internals/87400
From: Scott Arciszewski
Reply: Matt Tait

Pointing out that Cross-Site Scripting is a bigger issue now... where I think that taints can address both issues (and more).

He also says that prepared statements is a solved problem (something that I partially disagree with, and have explained above, but I realise that Scott is replying to Matt's original email, so may not have read that far yet).

--------------------------------------------------
http://news.php.net/php.internals/87403
From: Me (Craig Francis)
Reply: Xinchen Hui

Just saying that I appreciate the work Xinchen is doing, and offering to help (assuming I can)... I do also go off on a bit of a tangent, as I think that if we invert the 2008 RFC to mark things when they are escaped (rather than tainting everything as they are created), then I think the implementation would be a lot easier.

--------------------------------------------------
http://news.php.net/php.internals/87404
From: Me (Craig Francis)
Reply: Scott Arciszewski

Pointing out that I have a different suggestion.

I also wanted to say that I don't think SQL injection is a solved problem, as I tried to explain earlier (which Scott may not have read yet)... but I certainly didn't want Scott to just sit back and think everything is solved now, so no need to discuss.

--------------------------------------------------
http://news.php.net/php.internals/87405
From: Scott Arciszewski
Reply: Me (Craig Francis)

Skipping over my suggestion, and just saying that education is needed... which is what I'm also trying to do.

But as PHP is the only thing that can be forced in front of a new developer, it is really the only common thing that can do that education (there is no way you can find and talk to every single new developer just by answering questions on Stack Overflow).

And just as an example, college students leaning to write C programs in 2015 and creating buffer overflow vulnerabilities... they still make mistakes (even highly experienced developers still make these mistakes)... and you also have to recognise that not every programmer will go to a college, and that not all colleges actually teach their students about this (unfortunately).

--------------------------------------------------
http://news.php.net/php.internals/87406
From: Ronald Chmara
Reply: ???

I'm going to assume this is a reply to Matt's suggestion (or just the subject).

And I agree, that suggestion would break every single application... hence why I'm not suggesting that approach.

--------------------------------------------------
http://news.php.net/php.internals/87407
From: Me (Craig Francis)
Reply: Ronald Chmara

I'm just explaining that I've hijacked the email thread started by Matt, and that there is more to the discussion.

--------------------------------------------------
http://news.php.net/php.internals/87408
From: Me (Craig Francis)
Reply: Scott Arciszewski

I am getting a little frustrated, and just want to move the discussion on.

Unfortunately what you don't know is that I've been having this issue for too many years, and I'm getting quite annoyed that we aren't addressing these problems in a simple way.

Instead, what I keep seeing is more complicated half solutions (e.g. ORMs and query builders, where typical website developers have no idea what the SQL/DB is doing, so vulnerabilities still creep in, and introduce other problems like n+1).

The amount of times I've been to PHP conferences where the speaker is explaining some new way that they are doing OOP (or maybe even functional programming), and I'm sure they are really proud of their highly complex solution that few people in the audience understand (and no, I'm usually able to follow along, my experience is trying to explain what the talk was about to another audience member afterwards).

But ultimately they haven't addressed anything todo with security, and it is often less performant (typical response "hey, get a faster computer"), it has instead taken them 3+ months to implement, has resulted in another 1 or 2 layers of abstraction that the typical developer does not understand (they just run it, and hope for the best).

Then in 6 months time they will have moved on to a different complex solution, and can't be bothered to support the old one any more (because they just want something hard/complex to work on? I'm assuming it's an ego thing?).

--------------------------------------------------
http://news.php.net/php.internals/87409
From: Joe Watkins
Reply: Me (Craig Francis)

Where we are today... where you are talking about the RFC (from 2008, or the one from Matt), not what I've just said about my suggestion (which, if you haven't read, is different).

--------------------------------------------------






On 30 Jul 2015, at 17:02, Joe Watkins <pthreads@pthreads.org> wrote:

Joe Watkins

11 years ago

Morning Craig,

I think Pierre and I object to the concept, regardless of the
intricacies of any particular implementation.

Even the best implementation need only have a single hole in it and
everything falls apart, one extension not doing something properly, or one
mistake in the implementation ... it's not hard to imagine these things
happening, right ?

When I made reference to safe_mode, it wasn't because I misunderstood
the concept, I know you're not introducing another safe mode, but
introducing a INI, or build, or configuration option that says "we got
this" is doomed to fail whatever.

I don't like to talk about security, it's not something I feel
qualified to talk about really, but I can observe that catch-all security
"features" have already been proven to fail ...

It's the year 3015, we're all running PHP442, our taint implementation
is flawless, every major framework (which we have a futurish word for)
relies on tainting and thinks SQLi is no longer a thing to worry about,
because "we got this". A junior sysadmin, on mars, is charged with
upgrading PHP on the server that is responsible for crop irrigation on the
terra-formed world, and forgets to configure taint properly. The upgrade
goes live, renegade ruby fans hack the system disrupting food production,
leading to the eventual death of everything.

It's Friday afternoon, and that was a bit of fun ... I hope I lifted
your mood ... push on, I can be wrong, Pierre can be wrong, we can all be
wrong ...

Cheers
Joe

On Fri, Jul 31, 2015 at 11:40 AM, Craig Francis <craig@craigfrancis.co.uk>
wrote:

Craig Francis

11 years ago

On 31 Jul 2015, at 15:00, Joe Watkins <pthreads@pthreads.org> wrote:

> Even the best implementation need only have a single hole in it and everything falls apart, one extension not doing something properly, or one mistake in the implementation ... it's not hard to imagine these things happening, right ?

Hi Joe,

I've just replied to Matt and your email came in as I sent... so a bit of a change there.

But what I was proposing was that PHP simply checks the way in which the variables (strings) are being passed around... if something from $_GET is passed into mysqli_query without being escaped (as a parameter or as a quoted escaped string), it will still continue, but a note will be written to the log (or on the page if display_errors is on).

The same would happen when you echo a $_GET variable :-)

So in 3015, if the taint checking isn't configured properly (switched on)... it wouldn't change the execution, it's just not checking things (oh well), it should still have been coded properly to begin with.

So the way in which PHP executes is not effected, it's just picking up the "oh, you probably shouldn't be doing this".

Now Matt's suggestion allowed things to be actively blocked... I'm not sure I'd use that, but it might work for some (I'd just be happy having a log to check, in the same way that I use the logs for undefined variables, script timeouts, etc).

And yes, you won't get a perfect system (there are some edge cases)... but keeping it as simple as possible (hence why I want to take a slightly different approach to the 2008 RFC), it should pick up the most common mistakes.

But please do talk about security, you may be wrong, but thats fine, I'm sure someone will be able to correct you... I personally feel that too many people try to ignore security, and that's why we keep having so many problems (that said, performance and accessibility also need some attention, but that's more for the developers creating the websites, rather than PHP internals).

Craig

On 31 Jul 2015, at 15:00, Joe Watkins <pthreads@pthreads.org> wrote:

Matt Tait

11 years ago

Thanks for the feedback Joe!

To be clear, this feature is not an alternative to coding SQL securely with
parameterized queries. Indeed, it's entire purpose is to help developers
move dangerously coded queries (i.e. ones that are not parameterized and
are dynamically constructed using parameters that attackers might control,
like $_GET[], or the output of other database queries) to ones that are
clearly and unambiguously secure, by parameterizing their queries.

This feature is not a replacement for the security guidance of
"parameterize all your queries"; but it does allow webapplication
developers who follow this guidance to enforce it at the language level to
ensure that they don't let a single SQL-injectable query "through the net".

Enabling this feature encourages developers to write code that is secure
even when this feature is disabled.

Hope that clarifies!
Matt

On 31 July 2015 at 15:00, Joe Watkins <pthreads@pthreads.org> wrote:

Matt Tait

11 years ago

>> 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.

Thanks Scott!

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).

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"].

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. From 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.

To take apart some of the other valid comments on this thread:

== SQL injection is dead ==
This is objectively not the case
https://www.exploit-db.com/search/?action=search&description=SQL+injection

== Won't this break every website? ==
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.

== We should be looking at XSS instead ==
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.

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.

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.

== This breaks applications that use OOP or other complicated ways of
sending data to a database ==
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.

As you can see from this proof-of-concept (
http://phpoops.cloudapp.net/oops.php?action=main&dbg_sql&limit=4%20uhoh),
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.

Dynamically construct SQL statements out of provably safe components are
not detected as a SQL-injectable error by this RFC.

== SQL injection is already solved! Why are you re-solving it? ==
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.

== We should log, rather than block exploit attempts ==
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.

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.

== What about applications that pass user-submitted SQL statements to the
database by design? ==
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.

Matt

On 30 July 2015 at 14:43, Scott Arciszewski <scott@paragonie.com> wrote:

Craig Francis

11 years ago

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 = 'javascript:alert(document.cookies)';
echo '<a href="' . 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('...');
}
// using escapeshellarg for the $arguments
}

Craig

On 31 Jul 2015, at 14:11, Matt Tait <matt.tait@gmail.com> wrote:

Lester Caine

11 years ago

On 28/07/15 18:33, Matt Tait wrote:

> 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.

So who addresses all the other database drivers? Which is something
other ''proposals' currently ignore as well.

--
Lester Caine - G8HFL
-----------------------------
Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

Julien Pauli

11 years ago

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

Anthony Ferrara

11 years ago

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

Matt Tait

11 years ago

Thanks for the feedback Anthony and Julien,

The case you refer to using mysqli_real_escape_string is addressed in the
RFC, and cannot be injected when this feature is enabled, as the query is
always marked as tainted and always blocked, regardless of the connection.

Here's your example running on my server:

Source code: http://phpoops.cloudapp.net/example.php?viewcode
Run: http://phpoops.cloudapp.net/example.php

To be clear: this feature does not track taint through escape functions,
regular expression filters, ctype_filters and the like by design. Security
best-practice and more than a decade of security consulting experience show
that developers who rely on filters and escaping rarely manage to do so
competently and systematically enough to prevent SQL-injection on
non-trivial websites. Rather, this feature recognizes that SQL-injection is
already a solved problem. It is uncontroversial and overwhelmingly accepted
security best-practice to ensure that every dynamic SQL query is executed
via a prepared statement rather than via string-escaping.

Consequently, the Zend part of this feature only tracks whether a string is
a string literal, or linear concatenation of string literals (which is
called "safeconstness" in the RFC). If a "safeconst" string is sent to
/any/ SQL function in any SQL extension either as the query or prepared
statement, that query is not SQL-injectable, since attackers cannot control
the string literals in your code. If the raw-query or prepared-query is
/not/ a "safeconst" string, the feature (when enabled) alerts the developer
that the query is too complicated to prove is not injectable, and is a good
candidate to be "upgraded" to a parameterized SQL statement. When the
feature is in "blocking" mode, PHP will also block the query from executing.

This feature works best for big websites that are following (or in the
process of migrating their code to follow) this security-best-practice, as
it allows them to systematically verify during development that the query
string to every SQL statement is guaranteed untainted by attackers. Any
refactoring which exposes unparameterized queries to attacker-controlled
data, that SQL query written by that intern who didn't understand
parameterized queries and just concatenated a $_GET variable into a
prepared query string, and any backend services that might be vulnerable to
blind-sql injection will now all be identified as queries that need to be
"upgraded" to use parameterized SQL.

Developers and website administrators who are confident that their entire
website only uses SQL statements safely can then enable the feature in
"blocking" mode for their production website. If an attacker finds a path
to a vulnerable SQL statement in their website (maybe hidden in some PDF
extension, or that debug page you accidentally left in the /images/
directory, or just a code-path that was never adequately tested), the
tainted SQL statement is aborted to guarantee that no tainted query string
is every issued against the database on any page across the entire website.

In terms of whether this RFC is too coupled with SQL extensions, I am
sensitive to these concerns, but I don't think they apply for three broad
reasons:

Firstly, the taint logic tracks "safeconstness" (i.e. whether a zend_string
is a string literal or linear concatenation of string literals), and
by-design does not attempt to interact with escaping functions because (as
your example clearly shows), escaping functions only /sometimes/ prevent
injections, whereas parameterized queries /always/ stop injections. For
this reason the taint feature can be trivially applied to any builltin
function where one parameter is structural and should be constant. It could
be deployed without modification to prevent injection into regular
expression queries for example to prevent denial-of-service, or to prove
that the parameter to shell_exec is constant.

Secondly, this feature applies globally to all SQL extensions, not just
mysqli. This feature encompasses protection of mysqli, PDO and Postgres. In
future, I would like to build this feature out to include some of the other
categories of injection later, but given that SQL-injections remain the
single most commonly exploited vulnerability in PHP applications, I think
starting with them is probably best. Tackling SQL-injection in PHP is a big
enough task as it is, and is a good place to start.

Thirdly, although SQL extensions are extensions, this is a technicality, in
much the same way that Zend Core is not supposed to be coupled to PHP. In
reality, /most/ non-trivial PHP websites use one SQL extension or another
to interact with a database, and despite parameterized queries existing for
over a decade, huge numbers of PHP websites remain vulnerable to them --
with a devastating impact on the company and their users when the
vulnerability is exploited.

Understanding SQL injections is hard for many junior developers to
understand, and hard for companies to apply systematically. The impact of
them getting it wrong can be catastrophic to companies and is a major
threat to users' online privacy when their data gets compromised. I think
having PHP give developers a hand to write code that is unambiguously safe
from hackers would be a good for whole PHP community.

Hope that helps clear things up!
Matt

On 5 August 2015 at 16:27, Anthony Ferrara <ircmaxell@gmail.com> wrote:

Anthony Ferrara

11 years ago

Matt,

> To be clear: this feature does not track taint through escape functions,
> regular expression filters, ctype_filters and the like by design. Security
> best-practice and more than a decade of security consulting experience show
> that developers who rely on filters and escaping rarely manage to do so
> competently and systematically enough to prevent SQL-injection on
> non-trivial websites. Rather, this feature recognizes that SQL-injection is
> already a solved problem. It is uncontroversial and overwhelmingly accepted
> security best-practice to ensure that every dynamic SQL query is executed
> via a prepared statement rather than via string-escaping.

Except that not every part of a query is preparable. And many
non-trivial complexity websites use these portions of queries
extensively.

Hence a blanket **programming language level** blocking of it is not
really going to fly.

> This feature works best for big websites that are following (or in the
> process of migrating their code to follow) this security-best-practice, as
> it allows them to systematically verify during development that the query
> string to every SQL statement is guaranteed untainted by attackers.

Actually, this allows them to verify at runtime. Not during
development (codepaths that aren't hit at devtime can still cause
problems at runtime).

If this is your goal, I'd suggest doing static analysis to prove (or
not) it. I have a working proof-of-concept of one here:
https://github.com/ircmaxell/php-security-scanner

> In terms of whether this RFC is too coupled with SQL extensions, I am
> sensitive to these concerns, but I don't think they apply for three broad
> reasons:
>
> Firstly, the taint logic tracks "safeconstness" (i.e. whether a zend_string
> is a string literal or linear concatenation of string literals), and
> by-design does not attempt to interact with escaping functions because (as
> your example clearly shows), escaping functions only /sometimes/ prevent
> injections, whereas parameterized queries /always/ stop injections.

No they don't. They only stop injections when used correctly. For
example (an edge case, but an important one):
http://stackoverflow.com/questions/134099/are-pdo-prepared-statements-sufficient-to-prevent-sql-injection/12202218#12202218

And that doesn't even touch on the DB backends that don't support PS.
And it doesn't even touch on the fact that not every dynamic part of a
query is preparable.

> reason the taint feature can be trivially applied to any builltin function
> where one parameter is structural and should be constant. It could be
> deployed without modification to prevent injection into regular expression
> queries for example to prevent denial-of-service, or to prove that the
> parameter to shell_exec is constant.
>
> Secondly, this feature applies globally to all SQL extensions, not just
> mysqli. This feature encompasses protection of mysqli, PDO and Postgres. In
> future, I would like to build this feature out to include some of the other
> categories of injection later, but given that SQL-injections remain the
> single most commonly exploited vulnerability in PHP applications, I think
> starting with them is probably best. Tackling SQL-injection in PHP is a big
> enough task as it is, and is a good place to start.
>
> Thirdly, although SQL extensions are extensions, this is a technicality, in
> much the same way that Zend Core is not supposed to be coupled to PHP. In
> reality, /most/ non-trivial PHP websites use one SQL extension or another to
> interact with a database, and despite parameterized queries existing for
> over a decade, huge numbers of PHP websites remain vulnerable to them --
> with a devastating impact on the company and their users when the
> vulnerability is exploited.

Except that there are a number of DB extensions that don't ship with
PHP. And those that aren't shipped at all (they are custom propriatary
extensions). And those that aren't really SQL, but use similar
semantics (MongoDB).

> Understanding SQL injections is hard for many junior developers to
> understand, and hard for companies to apply systematically. The impact of
> them getting it wrong can be catastrophic to companies and is a major threat
> to users' online privacy when their data gets compromised. I think having
> PHP give developers a hand to write code that is unambiguously safe from
> hackers would be a good for whole PHP community.

It's hard, because tutorials make it hard. It's hard because we show
concatenation as the right way of doing things, and then tell them
it's wrong. It's hard, because we suck at teaching.

Kent Beck had an awesome passage in his book Test-Driven-Development
By Example. I'll paraphrase here, but he said that every engineer he
worked with found testing difficult. However, his daughter found it
easy and never even thought about it. It was because she was taught
TDD from the ground up. She was taught that you don't write code and
then test, you just test & write code. So she never had time to learn
bad practices because she just learned how to do it right.

And prepared statements are precisely like that. How many other
programming languages have such major problems with SQLi? Most do not.
Don't get me wrong, it exists in other languages (most definitely).
But Parameterized Queries are a big focus in other languages.

Our beginner tutorials all teach concatenation. Then escaping. Then
PS. You want to fix the problem, fix it with education. Tainting can
be a safeguard, but it's not a solution.

My $0.02

Anthony

Matt Tait

11 years ago

Thanks for your feedback, Anthony.

I'll take a few of your points in turn.

With regards to the fact that not all SQL queries are directly
parameterizable, this is true. Structural parts of a query, such as table
names, column names and complex conditions are hard to parameterize with
"vanilla" prepared statements, and many developers like to abstract some of
these structural parts of a SQL query into config files, and append
additional conditional constraints into the query at runtime based on user
input.

This feature addresses this head on. So long as the structural parts of the
prepared statement -- that is, table names, database names and column names
-- are not themselves attacker-controlled (I can't think of a valid reason
whey they would be), this feature is happy for developers to concatenate
them into a query string. For example, the following is not detected by the
feature as dangerous, because the query (whilst dynamically constructed) is
the linear concatenation of string literals, and so is a safeconst.

$query = "SELECT * from {$config['tablename']} WHERE id=?";
if(isset($_GET["filterbycolor"]))
$query .= " AND color=?";
do_prepared_statement($query, array("id" => $_GET["id"] "color" =>
$_GET["color"]));

We can even take this to an extreme to prove how flexible this can be.
Here's a function that issues a SQL statement where everything from the
table name and column name, to the structure and form of the condition, as
well as the limit, order and direction of the output are all chosen by the
caller. So long as the structural parts (i.e. the table names, database
names and column names) are safeconsts, the resulting query is a safeconst,
and this feature correctly identifies the resulting query as untainted).

http://phpoops.cloudapp.net/dynamic_pdo.php?viewcode
http://phpoops.cloudapp.net/dynamic_pdo.php

Finally, this feature is off-by-default. This means that that the feature
only helps developers who want to (or are required to) parameterize their
queries. It detects queries where the query cannot be proven "safe", and
then informs the developer that they need to upgrade the query so that the
dynamic parts of the query are parameterized to be in compliance with the
website's PHP.INI settings for this feature.

==

With regard to static analysis, I have a lot of respect for static analysis
tools, but especially in the webapplication world, they lack adoption, and
hence have limited real-world value. In over a decade of security
consulting experience, and with the notable exception of Google and
Microsoft, I've never seen any company that routinely scanned their website
with static analysis tools more complex than "grep".

As well as poor adoption, static analysis tools quickly run into
intractible problems doing analysis over unbound-loops, indirect function
calls or eval statements, and can get confused when looking at files out of
context (e.g. scanning included files). Runtime analysis, on the other
hand, always runs, has zero startup cost, negligble runtime cost and
guarrantees to check every codepath that is actually taken; regardless of
how it was reached.

For this reason, I think static analysis is the wrong tool for this job.

==

With regard to SQL parameterization not always being safe, the case you are
citing is due to a security defect in the way PHP emulates SQL
parameterization for old versions of MySQL that don't support
parameterization. In this particular case, PHP "emulates" the
parametization by escaping the parameters, and -- since escaping is a
dangerous way to build SQL query statements -- this goes horribly wrong and
leads to a SQL injection. But to be clear: this is a defect in PHP's
/emulation/ of prepared statements when real prepared statements are not
available, not a problem of prepared statements themselves.

Where parameterized queries are available at the database layer, and PHP is
not defectively emulating them, they are (by definition) immune from SQL
injection. Parameterized queries are not flattened to a string using
escaping. They are sent to the database in parameterized form, and are
replaced at AST-construction stage in the database itself. This means they
are immune from weird bugs like character-set differences between
application and SQL-server: so long as the query is untainted, the attacker
cannot affect the structure of the AST using only parameter values, and
hence can never SQL-inject the database.

To quote Wikipedia's article on prepared statements (
https://en.wikipedia.org/wiki/Prepared_statement):

>> Prepared statements are resilient against SQL injection, because

parameter values, which are transmitted later

>> using a different protocol, need not be correctly escaped. If the

original statement template is not derived

>> from external input, SQL injection cannot occur.

To quote the W3C (http://www.w3schools.com/sql/sql_injection.asp)

>> The only proven way to protect a web site from SQL injection attacks, is

to use SQL parameters.

To quote security consortium OWASP (
https://www.owasp.org/index.php/Query_Parameterization_Cheat_Sheet)

>> SQL injection is best prevented through the use of parameterized queries.

And to quote one of Microsoft's security engineers on the SQL team,
specifically addressing PHP (
http://blogs.msdn.com/b/brian_swan/archive/2010/03/04/what_2700_s-the-right-way-to-avoid-sql-injection-in-php-scripts_3f00_.aspx
):
*>> The most common suggestion I’ve seen for preventing SQL injection
involves trying to remove or escape any possible SQL code from user input
before concatenating it with the SQL code to be executed. There are several
PHP functions (and functions in PHP extensions) that can be used to do
this, but all of them are potentially vulnerable. If you concatenate user
input with SQL code that will be executed in the database, you run the risk
of a SQL injection attack no matter how much parsing and escaping of the
input you do. How can you be 100% sure that you’ve thought of all
possibilities that a creative hacker might think of? How can you be sure
that you’ve taken the appropriate measures to mitigate an attack? How can
you be sure that the functions you are using to remove or escape dangerous
user input aren’t buggy?*
*>> The right way to prevent SQL injection is by using parameterized
queries. This means defining the SQL code that is to be executed with
placeholders for parameter values, programmatically adding the parameter
values, then executing the query. Doing this allows the server to create an
execution plan for the query, which prevents any "injected" SQL from being
executed. An example will help in explaining this. Let’s use the same
script, but I’ll define the SQL query with parameter placeholders.*

You are of course welcome to disagree with the overwhelming body of
security advice that parameterized queries are the correct, secure way to
prevent SQL injection. In that case, you only need to not enable this
feature. This feature is off-by-default, and only attempts to help secure
webapplications and webdevelopers who do (or are required, for example by
PCI compliance standards) to adopt this security-best-practice to ensure
that they do so systematically across their entire website.

==

With regard to SQL extensions that don't ship with PHP, it is true that
this doesn't add security to builtin functions that don't ship with PHP or
its various extensions. Extensions that ship entirely independently to PHP
are free to copy the two-line edit to their codebase to also take advantage
of the feature. If they choose not to, that is their prerogative. In the
meantime, securing MySQLi, Postgres and PDO seems to me to be a good place
to start.

==

With regard to training, I 100% agree that developer security training in
general is pretty bad, but especially so for PHP. For example, none of the
pages on SQL builtins (like this one
http://php.net/manual/en/function.mysql-query.php) mention SQL injection,
why it is bad, and how to prevent it.

By way of contrast, see Microsoft's Database.SqlQuery documentation (
https://msdn.microsoft.com/en-us/library/system.data.entity.database.sqlquery(v=vs.113).aspx)
which states:

>> Creates a raw SQL query that will return elements of the given type. The

type can be any type that has properties that match the names of the
columns returned from the query, or can be a simple primitive type. The
type does not have to be an entity type. The results of this query are
never tracked by the context even if the type of object returned is an
entity type. Use the SqlQuery method to return entities that are tracked by
the context. *As with any API that accepts SQL it is important to
parameterize any user input to protect against a SQL injection attack. You
can include parameter place holders in the SQL query string and then supply
parameter values as additional arguments*. Any parameter values you supply
will automatically be converted to a DbParameter.
context.Database.SqlQuery(typeof(Post), "SELECT * FROM dbo.Posts WHERE
Author = @p0", userSuppliedAuthor); Alternatively, you can also construct a
DbParameter and supply it to SqlQuery. This allows you to use named
parameters in the SQL query string. context.Database.SqlQuery(typeof(Post),
"SELECT * FROM dbo.Posts WHERE Author = @author", new
SqlParameter("@author", userSuppliedAuthor)); (emphasis added).

This feature helps address some of this technical debt. This feature gives
website owners (who may be nontechnical, or technical but with limited
oversight of their large workforce) a tool to prevent unsafe SQL-queries
being written in the first place. If one of their developers then writes
(or exposes) a query that could be tainted with attacker data -- even if
the user data isn't an injection attack in progress -- PHP blocks the query
and provides the developer with a link that can tell them about SQL
injection, why their query was blocked and how they can restructure their
query so that it's not vulnerable to SQL injection any more.  By doing this
/as the developer writes code/, rather than via some static analysis tool
or security audit weeks or months later, developers will quickly discover
that writing parameterized queries is the only way to dynamically interact
with a database when the feature is enabled.

Hope that clears some of this up,
Matt


On 5 August 2015 at 21:53, Anthony Ferrara <ircmaxell@gmail.com> wrote:

Dennis Birkholz

11 years ago

Hi Matt,

Am 06.08.2015 um 05:46 schrieb Matt Tait:

> With regards to the fact that not all SQL queries are directly
> parameterizable, this is true. Structural parts of a query, such as table
> names, column names and complex conditions are hard to parameterize with
> "vanilla" prepared statements, and many developers like to abstract some of
> these structural parts of a SQL query into config files, and append
> additional conditional constraints into the query at runtime based on user
> input.
>
> This feature addresses this head on. So long as the structural parts of the
> prepared statement -- that is, table names, database names and column names
> -- are not themselves attacker-controlled (I can't think of a valid reason
> whey they would be), this feature is happy for developers to concatenate
> them into a query string. For example, the following is not detected by the
> feature as dangerous, because the query (whilst dynamically constructed) is
> the linear concatenation of string literals, and so is a safeconst.

and how exactly do you guarantee the structural part from a
configuration is not attacker-controlled? The config may come from a
json file, from a database, from APCu, etc. and may have been
inserted/generated previously from unsafe user input. If I understand
you right, these parts will not be tainted and thus will give a wrong
feeling of safety to the unskilled/unknowing programmer. Or did I miss
that part in the RFC?

Greets
Dennis

Anthony Ferrara

11 years ago

Matt,

> You are of course welcome to disagree with the overwhelming body of security
> advice that parameterized queries are the correct, secure way to prevent SQL
> injection. In that case, you only need to not enable this feature. This
> feature is off-by-default, and only attempts to help secure webapplications
> and webdevelopers who do (or are required, for example by PCI compliance
> standards) to adopt this security-best-practice to ensure that they do so
> systematically across their entire website.

You seem to misunderstand me. I'm *not* saying that you shouldn't use
parameterized queries. Nor that they are not one of the best tools
available. I am simply pointing out that they are not a sure-fire
one-stop solution. There is a lot more that goes into it than simply
using prepare/bind. As indicated by the two cases I talked about
(structural elements not being supported and implementation quirks) as
well as others (DOS attacks from wildcards in malformed query
parameters, type validation, etc). This is not to say that we should
avoid PQ, but that we should recognize that it's not enough to just
use one thing and forget about everything else.

Anthony

PS: w3schools is NOT w3c. And their content is probably the single
largest source of security vulnerabilities for PHP, so citing them in
a security discussion is more than a little ironic.

Matt Tait

11 years ago

Thanks for the feedback Anthony,

This feature specifically addresses the points you raise; the feature allows parameterized queries constructed with structural parts of the query inserted from configuration variables, so long as the resulting query is a safe-const as defined by this RFC.

If you can come up with an example of a query that either (a) is vulnerable to a SQL injection attack and this feature wrongly does not detect it as such or (b) a query that cannot be expressed using parameterized queries where the parameter is a safe-const as defined by this RFC, I'd be genuinely interested to take a look.

Hope that clarifies,
Matt

Craig Francis

11 years ago

Hi Anthony, Julien, Yasuo,

I have been reading your conversation with great interest.

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).

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.

But anyway...

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:

http://php.net/manual/en/pdo.exec.php
$dbh->exec("DELETE FROM fruit WHERE colour = '{$_GET['colour']}'");

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.

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:

http://php.net/manual/en/pdo.prepare.php#111458
SELECT * FROM users WHERE $search=:email

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).

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).

Craig

On 6 Aug 2015, at 23:57, Matt Tait <matt.tait@gmail.com> wrote:

Yasuo Ohgaki

11 years ago

Hi all,

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.
>
> 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).
>
> 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.
>
> But anyway...
>
> 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:
>
>
>         http://php.net/manual/en/pdo.exec.php
>         $dbh->exec("DELETE FROM fruit WHERE colour = '{$_GET['colour']}'");
>

This is awful... It seems someone already fixed the doc.

>
>
> 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.
>

I fully agree "prepared statements fix everything" is simply wrong.

To be secure truly, secure API must split command and all others completely
_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.

Many APIs, that are considered secure API, are not perfectly secure
because "command part must be programmer supplied constant/static/validated
string" condition is not checked. It's left to developers.

e.g. execl/execv splits command and arguments, only single command is
allowed,
but if "command" is user parameter, it cannot be secure.


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:
>
>
>         http://php.net/manual/en/pdo.prepare.php#111458
>         SELECT * FROM users WHERE $search=:email
>
>
> 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).
>
> 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).
>

I don't think the proposal is useless nor ineffective.
Taint system is nice to have, but the proposal does not seem preferable
resolution.
Don't get me wrong. I agree with your discussion overall.

I tend to dislike all or nothing choice for security related
issues especially, nonetheless
"nothing" is my choice this time...

Regards,

P.S. There are many outputs other than SQL. Context aware automatic
escaping/use of
secure API/validation for various outputs would be nice to have. This would
be new API
and wouldn't help users shooting their own foot, though. I don't have
concrete idea now,
but PHP may have features help it.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Christoph Becker

11 years ago

On 10.08.2015 at 11:57, Craig Francis wrote:

> 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:
>
>
> 	http://php.net/manual/en/pdo.prepare.php#111458
> 	SELECT * FROM users WHERE $search=:email

"Skim reading" things might be the problem (here).  The user contributed
note states:

| In my case I allow the user to enter their username or email,
| determine which they've entered and set $search to "username" or
| "email". As this value is not entered by the user there is no
| potential for SQL injection and thus safe to use as I have done.

So to me that note looks pretty fine.

--
Christoph M. Becker

Yasuo Ohgaki

11 years ago

Hi Matt,

On Thu, Aug 6, 2015 at 12:46 PM, Matt Tait <matt.tait@gmail.com> wrote:

> I'll take a few of your points in turn.
>
> With regards to the fact that not all SQL queries are directly
> parameterizable, this is true. Structural parts of a query, such as table
> names, column names and complex conditions are hard to parameterize with
> "vanilla" prepared statements, and many developers like to abstract some of
> these structural parts of a SQL query into config files, and append
> additional conditional constraints into the query at runtime based on user
> input.
>
> This feature addresses this head on. So long as the structural parts of the
> prepared statement -- that is, table names, database names and column names
> -- are not themselves attacker-controlled (I can't think of a valid reason
> whey they would be), this feature is happy for developers to concatenate
> them into a query string. For example, the following is not detected by the
> feature as dangerous, because the query (whilst dynamically constructed) is
> the linear concatenation of string literals, and so is a safeconst.
>
> $query = "SELECT * from {$config['tablename']} WHERE id=?";
> if(isset($_GET["filterbycolor"]))
>   $query .= " AND color=?";
> do_prepared_statement($query, array("id" => $_GET["id"] "color" =>
> $_GET["color"]));
>

If you would like to prevent SQL injection via "Identifiers", how about
extend prepared query like

$query = "SELECT * from @ WHERE id=?";
if(isset($_GET["filterbycolor"]))
$query .= " AND color=?";
do_prepared_statement($query, array("id" => $_GET["id"] "color" =>
$_GET["color"]), array($config['tablename']));

where @ indicates identifier placeholder. The char could be anything that
will not violate SQL syntax. This could be implemented in user land as
there is no standard/native API for identifier placeholder.

Even if there is identifier  placeholder, SQL keyword remains.
So to be perfect, you'll need another place holder for SQL keywords.
There is no escaping for SQL keywords and it has to be validation.
e.g. ORDER BY {$_GET['order']}

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

11 years ago

On Fri, Aug 7, 2015 at 10:29 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Even if there is identifier  placeholder, SQL keyword remains.
> So to be perfect, you'll need another place holder for SQL keywords.
> There is no escaping for SQL keywords and it has to be validation.
> e.g. ORDER BY {$_GET['order']}
>

Oops the last line should be
e.g. ORDER BY col {$_GET['order']}

BTW, instead of improving PHP, users are better to request "identifier
escape API"
to DB developers like PQescapeIdentifier() in PostgreSQL's client library.
IMO.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

11 years ago

Hi Matt,

My friend made a product called sql_firewall for PostgreSQL.

https://github.com/uptimejp/sql_firewall
http://pgsnaga.blogspot.jp/2015/08/postgresql-sql-firewall.html

It's not directly relevant to your proposal, but this kind of database extension
could be alternative.

PDO has parser for params. Parser may be extended to parse basic SQL syntax
and store syntax tree hash like sql_firewall and compare it to reject
SQL injection
attacks.

Just FYI.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net


On Wed, Jul 29, 2015 at 2:33 AM, Matt Tait <matt.tait@gmail.com> wrote: