Re: [RFC] Name issue - is_literal/is_trusted

From: Date: Thu, 24 Jun 2021 16:57:39 +0000
Subject: Re: [RFC] Name issue - is_literal/is_trusted
References: 1 2 3 4 5 6 7 8 9 10 11 12 13  Groups: php.internals 
Request: Send a blank email to internals+get-115124@lists.php.net to get a copy of this message
On Thu, Jun 24, 2021, at 11:18 AM, Scott Arciszewski wrote: > On Thu, Jun 24, 2021 at 5:22 AM Stephen Reay <php-lists@koalephant.com> wrote: > 1. I never claimed that it wasn't a bug. > 2. I never claimed it wasn't impactful. > 3. I never claimed it wasn't security-affecting. > > I've simply said that this isn't an example of SQL Injection, because > nothing is being *injected*. > > > > > > > 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. > > In my original email, I said. "Outside the chr()/pack()/sprintf()/etc. > technique demonstrated above, there's no risk of injection inherent to > concatenating a trusted string with an untrusted integer." You brought > up a high-severity logic bug that could happen if someone concatenates > variables with wild abandon, but it's still not an example of > **injection**. > > If you're going to call someone a hypocrite (which is a needless > personal attack), take care that you're actually correct when you do > so. First, I think it's best if we all stop the definitional meta-debate, it's not really helpful. Second, FFS people, use the Reply to List feature. I've gotten double copies of the last 30 messages in this thread. Third, the core point here is: 1. A literal string comes from the developer, in source. 2. Thus a literal string is safe against SQL security errors. 3. An integer cannot contain SQL syntax. 4. Thus a literal string concat with an integer cannot contain SQL syntax errors. 5. Thus a literal string concat with an integer is as safe as a literal string. 6. Thus we can still call a literal string contact with an integer a literal string. That's the logic that leads to "a string literal contact an integer should still return true from is_literal()." It's the logic that most users would go through upon seeing that, and act on that conclusion. However, it's been demonstrated that step 4 is NOT true, and a literal string contact with an int can lead to a security vulnerability (whether it's technically an SQL injection per se is not relevant). Thus, I would argue that "literal string concat int --> literal string" is incorrect and should be removed from the RFC, as it is misleading. That it may hurt adoption is irrelevant, as creating a false sense of security is vastly worse than slower adoption. --Larry Garfield

« previous php.internals (#115124) next »