Re: [RFC] Name issue - is_literal/is_trusted
| From: | Larry Garfield | 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