Re: [RFC] Name issue - is_literal/is_trusted

> 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 > <mailto: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 <https://en.wikipedia.org/wiki/SQL_injection>), Cloudflare (https://www.cloudflare.com/en-au/learning/security/threats/sql-injection/ <https://www.cloudflare.com/en-au/learning/security/threats/sql-injection/>), and OWASP (https://owasp.org/www-community/attacks/SQL_Injection#example-2 <https://owasp.org/www-community/attacks/SQL_Injection>) 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 <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