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));
```
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
