> On 24 Jun 2021, at 14:29, Scott Arciszewski <sc...@paragonie.com> wrote: > > On Thu, Jun 24, 2021 at 2:10 AM Stephen Reay <php-li...@koalephant.com> wrote: >> >> >> >> On 24 Jun 2021, at 08:30, Scott Arciszewski <sc...@paragonie.com> wrote: >> >> On Wed, Jun 23, 2021, 9:23 PM Bruce Weirdan <weir...@gmail.com> wrote: >> >> On Thu, Jun 24, 2021 at 3:41 AM Scott Arciszewski <sc...@paragonie.com> >> wrote: >> >> The failure condition of this query is >> "return all rows from the table already being queried", not "return >> arbitrary data the attacker selects from any table that the >> application can read". >> >> >> Imagine that was a DELETE rather than SELECT and the failure condition >> becomes 'the table is emptied'. >> It may have less disastrous consequences (depending on how good your >> backup / restore procedures are) compared to arbitrary reads you >> demonstrated, but it is still, quite clearly, a glaring security hole >> caused by user input in SQL query - AKA SQL injection in layman's >> terms. >> >> it differs from Injection vulnerabilities in one >> fundamental way: The attacker cannot change the structure of the SQL >> query being executed. >> >> >> I would say replacing a column name with value is changing the >> structure of SQL query, and, basically, in exactly the way you >> describe SQL injection: confusing the code (column name) with data. >> >> I wholeheartedly welcome this RFC as it was originally proposed: >> is_literal() doing exactly what it says on the tin, without any >> security claims. But it has gone far from there real quick and now >> people can't even name the thing. >> >> >> -- >> Best regards, >> Bruce Weirdan mailto: >> weir...@gmail.com >> >> >> >> >> We can agree that it is a bug. We don't agree on the definition of SQL >> injection. >> >> Changing a column name to a number (which prepared statements shouldn't >> allow in the first place) is a bug. This changes the effect of the command, >> but the *structure* of the query remains unchanged. >> >> >> Hi Scott, >> >> I wrote that example where an integer could be dangerous. >> >> So firstly - just to clarify, because some replies seemed to be confused on >> the topic, as was literally mentioned in the original comment, it is >> definitely not correct behaviour - it’s a developer mistake that might work >> some of the time, and thus go unnoticed in testing. If you can show me a >> developer who’s never inadvertently passed the wrong parameter in some >> condition, I’ll show you an imaginary developer. >> >> >> Additionally - pointing out that this is a "developer error” doesn’t help >> your case. Using non-parameterised queries should already be a “developer >> error” for anyone who can walk and breathe at the same time - and yet that >> usage is being actively encouraged if this function supports integers. I’ve >> still seen zero responses about legitimate reasons this needs to support >> integers - giving people a shitty way to build an IN() clause is not >> legitimate. Parameterisation exists, and works. >> >> >> I don’t even understand why you mentioned prepared statements (I guess you >> meant using parameterised queries?) - the column name inherently can’t be >> parameterised - hence having to use a string substitution in the query. >> >> >> That part was weird and confusing, but not as odd as your claim that >> altering the query, so that it causes the where clause to become moot, is >> not an SQL Injection? REALLY? That’s your claim? >> >> >> I did a little research, and it turns out Wikipedia >> (https://en.wikipedia.org/wiki/SQL_injection#Technical_implementations), >> Cloudflare >> (https://www.cloudflare.com/en-au/learning/security/threats/sql-injection/), >> and OWASP (https://owasp.org/www-community/attacks/SQL_Injection#example-2) >> all have examples with a `1=1` type query manipulation. Do you want to write >> and tell them that they’re all wrong, or should I ask them to call you? >> >> Also, while researching the specifics of what is considered an “SQL >> Injection” I came across an article, that talks specifically about the >> dangers of allowing user input (i.e. the thing `is_trusted` is meant to >> prevent) as a column or table identifier. It’s from this little >> organisation, you may have heard of them: “Paragon Initiative” >> (https://paragonie.com/blog/2015/05/preventing-sql-injection-in-php-applications-easy-and-definitive-guide). >> >> >> >> I would absolutely make use of a function that tells me if the string given >> is in fact from something controlled by the developer. But once that same >> string can also include input from the request or the environment or >> whatever by nature of integers, the function becomes useless for the stated >> purpose. >> >> >> Cheers >> >> Stephen >> > > One other thing. What do you think the result of this code will be? > How many rows do you think it will spit out? > > ``` > <?php > $db = new PDO('sqlite::memory:'); > > // These are the only reasonable settings to ever use: > $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false); > $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); > > // Let's create a table: > $db->exec("CREATE TABLE foo (username TEXT, pwhash TEXT, email TEXT)"); > > // Dummy rows: > $db->exec("INSERT INTO foo(username, pwhash, email) VALUES ('scott', > 'lolno', 'sc...@paragonie.com')"); > $db->exec("INSERT INTO foo(username, pwhash, email) VALUES ('robyn', > 'fake!', 'ro...@paragonie.com')"); > > // Okay, now let's do a prepared statement, with an integer field > $int = 12345; > > $stmt = $db->prepare("SELECT * FROM foo WHERE {$int} = ?"); > $stmt->execute([12345]); > var_dump($stmt->fetchAll(PDO::FETCH_ASSOC)); > ``` >
Two things: (a) try that same query, unaltered on mysql. It won’t be 0 rows. I’m pretty sure its also not 0 on Postgres. (b) try binding the parameter as an integer and running it on SQLite. It won’t be zero rows. Actually I can even show you that one: https://3v4l.org/VL7UC#focus=8.0.7 <https://3v4l.org/VL7UC#focus=8.0.7> So what do we call the function now: is_trusted_string_or_integer_but_dont_dare_use_the_int_as_an_int_or_with_mysql_at_all() ? And, this horse is practically glue based on how much I’m beating it, but still I have to keep asking: Why integers at all? Who is using “2” or some other digit-only identifier as a table or column name, but also _doesn’t_ define that name as a string?
