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.t...@gmail.com> wrote:
>>> This may have been true at one point in time, but my own experience
>>> and the statistics collected by Dan Kaminsky of White Hat Security
>>> indicates that Cross-Site Scripting vulnerabilities are much more
>>> prevalent in 2015 than SQL Injection, especially in business
>>> applications. If Google has information that indicates that SQLi is
>>> still more prevalent than XSS, I'd love to see this data.
>
> 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 <sc...@paragonie.com> wrote:
>
>> On Tue, Jul 28, 2015 at 1:33 PM, Matt Tait <matt.t...@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
