Re: [RFC] Name issue - is_literal/is_trusted

On Thu, Jun 24, 2021 at 2:10 AM Stephen Reay <php-lists@koalephant.com> wrote: > 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. > 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. 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. > 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 > Cheers > > Stephen >