Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115097
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:29:08 -0400
Message-ID: <CAKws9z1M6aLMJ+T7KEAYbEu7N3ufFi8Lsqpr4GP6G06+8aG3SQ@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:
>
>
>
> 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> 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 comman=
d,
> 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 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.
>
>
> 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=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.
>
>
> 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?
>
>
> 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?
>
> 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).
>
>
>
> 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.
>
>
> Cheers
>
> Stephen
>

One other thing. What do you think the result of this code will be?
How many rows do you think it will spit out?

```
<?php
$db =3D new PDO('sqlite::memory:');

// These are the only reasonable settings to ever use:
$db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

// Let's create a table:
$db->exec("CREATE TABLE foo (username TEXT, pwhash TEXT, email TEXT)");

// Dummy rows:
$db->exec("INSERT INTO foo(username, pwhash, email) VALUES ('scott',
'lolno', 'scott@paragonie.com')");
$db->exec("INSERT INTO foo(username, pwhash, email) VALUES ('robyn',
'fake!', 'robyn@paragonie.com')");

// Okay, now let's do a prepared statement, with an integer field
$int =3D 12345;

$stmt =3D $db->prepare("SELECT * FROM foo WHERE {$int} =3D ?");
$stmt->execute([12345]);
var_dump($stmt->fetchAll(PDO::FETCH_ASSOC));
```