Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115096
MIME-Version: 1.0
References: <CAFv4g+Gv1qQ2Gtx2Qwo0Dx2Cb_gOXJ=rm7rVy+Jo89PmD-3jnA@mail.gmail.com>
 <CAEMvS4GRaqMBPn7ocBX-XU3jzrtpe+9uGfYpG10qc_iKZY80UA@mail.gmail.com>
 <03f7955c-69a8-4841-9245-449d7851e207@www.fastmail.com> <CAFv4g+EaT309KDWz63j3S7n8E58czEWTKUEw9hAvgt7s6R9eKQ@mail.gmail.com>
 <CAKws9z02yCZTb2_mw-5xUO5-QLc4jbOUcBRfJ8nFSNPP1rgO3g@mail.gmail.com>
 <CA+hqhpfUiOMLDZyXTqP+mm9jjG6v+K94Ug7K_zvuuZus_XhOaQ@mail.gmail.com>
 <CAKws9z34nTT0_CXoo4Yq7zO2CZX622has23xmfHbWjfaDrgLqA@mail.gmail.com>
 <CA+hqhpdR=W8nbWy8UBgLtKdvb_KCss5mpzturS67HhqRj4q6BQ@mail.gmail.com>
 <CAKws9z1=6fmCxJWYVfN+xKT+u=_ZTq0m7C0nZPT6LB5scbfpXQ@mail.gmail.com> <95D16F2E-E9DD-4964-A0E2-62E1FB0D976B@koalephant.com>
In-Reply-To: <95D16F2E-E9DD-4964-A0E2-62E1FB0D976B@koalephant.com>
Date: Thu, 24 Jun 2021 03:14:23 -0400
Message-ID: <CAKws9z0CbyjeKas=DNKi0ScdUxt8CnvNOLymXpWvQTaJtB0xkw@mail.gmail.com>
To: Stephen Reay <php-lists@koalephant.com>
Cc: Bruce Weirdan <weirdan@gmail.com>, Craig Francis <craig@craigfrancis.co.uk>, 
	Larry Garfield <larry@garfieldtech.com>, php internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] [RFC] Name issue - is_literal/is_trusted
From: scott@paragonie.com (Scott Arciszewski)

On Thu, Jun 24, 2021 at 2:10 AM Stephen Reay <php-lists@koalephant.com> wro=
te:
> 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 def=
initely not correct behaviour - it=E2=80=99s a developer mistake that might=
 work some of the time, and thus go unnoticed in testing. If you can show m=
e a developer who=E2=80=99s never inadvertently passed the wrong parameter =
in some condition, I=E2=80=99ll show you an imaginary developer.

Agreed.

> Additionally - pointing out that this is a "developer error=E2=80=9D does=
n=E2=80=99t help your case. Using non-parameterised queries should already =
be a =E2=80=9Cdeveloper error=E2=80=9D 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=E2=80=99ve still seen zero responses about le=
gitimate reasons this needs to support integers - giving people a shitty wa=
y to build an IN() clause is not legitimate. Parameterisation exists, and w=
orks.

I don't have a strong opinion on this.

> I don=E2=80=99t even understand why you mentioned prepared statements (I =
guess you meant using parameterised queries?) - the column name inherently =
can=E2=80=99t 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 alte=
ring the query, so that it causes the where clause to become moot, is not a=
n SQL Injection? REALLY? That=E2=80=99s 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=3D1` type query manipulation. Do you want to write and t=
ell them that they=E2=80=99re all wrong, or should I ask them to call you?

If you **inject a `1=3D1` 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 =E2=80=9CS=
QL Injection=E2=80=9D I came across an article, that talks specifically abo=
ut the dangers of allowing user input (i.e. the thing `is_trusted` is meant=
 to prevent) as a column or table identifier. It=E2=80=99s from this little=
 organisation, you may have heard of them: =E2=80=9CParagon Initiative=E2=
=80=9D (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 giv=
en is in fact from something controlled by the developer. But once that sam=
e string can also include input from the request or the environment or what=
ever by nature of integers, the function becomes useless for the stated pur=
pose.

Why not two functions then?

- is_noble_string() -- more restrictive
- is_noble() -- YOLO

> Cheers
>
> Stephen
>