Re: [RFC] Name issue - is_literal/is_trusted

On Thu, Jun 24, 2021 at 2:10 AM Stephen Reay <php-lists@koalephant.com> wrote: > > > > On 24 Jun 2021, at 08:30, Scott Arciszewski <scott@paragonie.com> wrote: > > On Wed, Jun 23, 2021, 9:23 PM Bruce Weirdan <weirdan@gmail.com> wrote: > > 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)); ```