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

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

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

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

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

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

---

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

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 Thu, Jul 30, 2015 at 11:20 AM, Craig Francis
craig@craigfrancis.co.uk 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

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

--

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

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

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:

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

---

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:

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

--

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

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

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

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.