[RFC] Name issue - is_literal/is_trusted View as Markdown

On Thu, Jun 24, 2021 at 9:14 AM Scott Arciszewski scott@paragonie.com
wrote:

On Thu, Jun 24, 2021 at 2:10 AM Stephen Reay php-lists@koalephant.com
wrote:

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.

Why not two functions then?

• is_noble_string() -- more restrictive
• is_noble() -- YOLO

I was going to ask basically the same [with different names] a few days ago
("why can't we have both?"), but then remembered
https://externals.io/message/114835#114951 , esp. the end:

"""
And to support having 2 functions, we would need 2 flags on strings. These
flags are limited, and managing 2 flags would affect performance.
"""

Regards,

--
Guilliam Xavier

Hi Scott,

I wrote that example where an integer could be dangerous.

I don't disagree that it's an example where an integer could be dangerous.

Danger is too broad to have a meaningful discussion about. You can, of
course, always do dangerous things if you're determined or creative
enough.

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.

Agreed.

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 have a strong opinion on this.

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.

They're effectively synonyms.

I mean, they’re not - you can prepare a statement with zero parameters in it.
But my point was why you even mentioned parameterised queries OR prepared statements at all. You cannot parameterise the column name, hence the entire reason this RFC exists. The entire point is to make sure that the bits that you need to insert variables, are something the developer wrote, not some input from the user.

If that isn’t the purpose of the RFC, then it describes its actual goal really poorly.

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?

Yes, let me try to explain this more clearly.

If you can inject code, it's a code injection vulnerability. SQL
injection is a specific type of code injection vulnerability. If you
can trick the SQL into doing something invalid without injecting
code, and you call it "SQL injection", that's a category error.

Supplying an integer in the place of a column name is a bug. The bug
can even be dangerous. The bug can EVEN be a security problem for the
system.

But calling it SQL injection is categorically incorrect. Changing the
behavior of a SQL command without injecting additional code is not
SQL injection.

If we're trying to stop all forms of misuse and insecurity that can
result from string concatenation, cool. But be very clear about that.

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?

If you inject a 1=1 clause where one didn't exist before, that's
injection. Notice the introduction of an OR operator in the examples
you cited.

Please, explain to us all, how where foo=‘bar’ OR 1=1 is functionally different than where 123=123 because the developer screwed the pooch in some specific condition and passed the ID twice, rather than the column name and the id, respectively.

My classification of the original example as "Not Injection" has
nothing to do with the fact that numbers are being compared with
numbers. Rather, there was no code injection.

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).
Snark is unnecessary.

You call it snark, I call it ironic hypocrisy.

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.

Why not two functions then?

• is_noble_string() -- more restrictive
• is_noble() -- YOLO

I mean sure, if someone wants to vote on the two choices or whatever, I’ll go get the popcorn, but I still haven’t seen any answers that make sense to this question: Why integers at all, though?

Cheers

Stephen

--

To unsubscribe, visit: https://www.php.net/unsub.php

On Thu, Jun 24, 2021 at 3:41 AM Scott Arciszewski scott@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:
weirdan@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', 'scott@paragonie.com')");
$db->exec("INSERT INTO foo(username, pwhash, email) VALUES ('robyn',
'fake!', 'robyn@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?