Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:115032
Message-ID: <B308178D-526D-4E9E-A5D7-F8614FD88C76@newclarity.net>
Content-Type: multipart/alternative;
	boundary="Apple-Mail=_8FF5070A-EFAA-42A4-B020-9388B0A77BCC"
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\))
Date: Tue, 22 Jun 2021 12:14:48 -0400
In-Reply-To: <8eb429ba-6f1f-c234-69af-3f707381ae09@processus.org>
Cc: PHP internals <internals@lists.php.net>
To: Pierre <pierre-php@processus.org>
References: <4FFA0160-1A05-4DA0-9C9A-79F778443A35@newclarity.net>
 <CAGBsUre=Asp+arHBWK0VdT-YdM=_3rR+WOhQQeghFVwsR+nGUg@mail.gmail.com>
 <F861461F-D070-4808-8B2E-A9BBF753CE6C@newclarity.net>
 <83D62559-0B87-4723-93B8-801905508E1F@koalephant.com>
 <F8DAF477-25CE-4AD9-B9CB-F292C23DA276@newclarity.net>
 <8eb429ba-6f1f-c234-69af-3f707381ae09@processus.org>
Subject: Re: [PHP-DEV] Sql Object Model Parser & Sanitizer (was [RFC]
 is_literal)
From: mike@newclarity.net (Mike Schinkel)

--Apple-Mail=_8FF5070A-EFAA-42A4-B020-9388B0A77BCC
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8



> On Jun 22, 2021, at 11:56 AM, Pierre <pierre-php@processus.org> wrote:
>=20
> Le 22/06/2021 =C3=A0 17:35, Mike Schinkel a =C3=A9crit :
>> =
https://github.com/WordPress/WordPress/blob/master/wp-includes/wp-db.php#L=
2050
> Sorry for the discussion pollution here but, but ouch, plugins are =
still using this unsafe form ? Reminds when I was a student, I learnt to =
parametrize queries there, it was 20 years ago. I never understood =
people not doing that in the first place.
>> But also because of much legacy code exists in the form of plugins =
and themes that do not support parameterized queries.
> Yes I agree, legacy is legacy, you have to deal with it. But all =
legacy code cannot be fixed, and doing a highly complex SQL parsing / =
escaping / vulnerability detection code that explicitly targets legacy =
code and not modern code seems weird to me.
>> HOWEVER, whether mysqli supports parameterised queries or not is all =
a moot point because parameterised queries do not allow for =
parameterizing field names or table names.  And the point of this thread =
was to discuss how to mark SQL that has been composed at run-time to be =
"safe."  Without being able to parameterize field names and table names =
parameterised queries are not a sufficiently complete solution.
>=20
> Not being able to parametrize table or field names is not only a =
problem for mysqli, but it is for PDO and pgsql too. That's a place =
where userland query-builders and others DBALs, even the most basic ones =
do shine, and brings a real added-value.
>=20
> But having anyone, writing SQL with user-given table names or column =
names, and executing it using something like WP's _do_query() method =
seems like they *WANT* to be hacked.

Are you not familiar with PHPMyAdmin[1] and/or Adminer[2]? =20

Do they, or anyone else who has a similar use-case want to be hacked?

> I'm not sure how you will succeed in plugging the is_trusted() / =
is_literal() / is_wathever() method correctly in an SQL Model Parser & =
Sanitizer anyway, knowing that at this point, all you'll receive is a =
huge string issued by some plugin API which has already done crazy dark =
(and probably bugguy as well) magic.
>=20
> I don't see how adding magic in PHP core will avoid the need to fix =
all those legacy plugins, they probably would need themselves to use =
this new shiny API to benefit from it ? In the opposite, if something =
alters the behavior of mysqli implicitly for everyone in order to make =
it safe, it sounds like there will be a lot of BC ? In both case, it =
seems that it will not do any shiny magic to me.

It would not affect *any* of those plugins by itself.  They would be =
left to fend for their own.

What it *would* do is allow those developers writing new sites and/or =
plugins or maintaining old ones who proactively choose to sanitize their =
SQL to be able to do so before they pass their SQL to $wpdb->query(), =
especially if the RFC is_trusted() and/or is_literal() passes.

> But, I might be wrong, this thread becomes harder and harder to read, =
and I may have missed a few points.

It just started!  Note I created a new thread from the =
is_literal()/is_trusted() thread to talk about SQL.

-Mike

[1] https://www.phpmyadmin.net/
[2] https://www.adminer.org/



--Apple-Mail=_8FF5070A-EFAA-42A4-B020-9388B0A77BCC--