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 <ircmax...@gmail.com> wrote: > All, > > On Wed, Aug 5, 2015 at 10:40 AM, Julien Pauli <jpa...@php.net> wrote: > > On Tue, Jul 28, 2015 at 7: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. > >> > > > > 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 >
