Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114840
Message-ID: <D1B12C84-A0FE-4D3E-811C-DF38F1FA4469@newclarity.net>
Content-Type: multipart/alternative;
	boundary="Apple-Mail=_7365E864-4CC7-44C5-89E3-E0EED5696923"
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\))
Date: Sun, 13 Jun 2021 02:46:35 -0400
In-Reply-To: <CAFv4g+F-7F7o6va5wCrojKe+Uysx2Ec3H4yV_mUZYEyNm3MkdQ@mail.gmail.com>
Cc: PHP internals <internals@lists.php.net>
To: Craig Francis <craig@craigfrancis.co.uk>
References: <CAFv4g+F-7F7o6va5wCrojKe+Uysx2Ec3H4yV_mUZYEyNm3MkdQ@mail.gmail.com>
Subject: Re: [PHP-DEV] [RFC] is_literal
From: mike@newclarity.net (Mike Schinkel)

--Apple-Mail=_7365E864-4CC7-44C5-89E3-E0EED5696923
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

Hi Craig,

> On Jun 12, 2021, at 1:00 PM, Craig Francis <craig@craigfrancis.co.uk> =
wrote:
>=20
> Hi Internals,
>=20
> I'd like to start the discussion on the is_literal() RFC:
>=20
> https://wiki.php.net/rfc/is_literal

Nice!  There is an awful lot to like here.

And few bits of concern.

What's to like?
-------------------

1. Adding a proactive method for guarding against injection =
vulnerabilities would be a huge step forward for PHP. This should not be =
underemphasized.

2. That you added the section "Previous Work" and covered what other =
languages are doing in this regard.

3. The "Thanks" section and all the external references and prior =
comments that provides insight into how this RFC was developed.

4. And I love that you publicly stated these points in an RFC:

	a. Why not use static analysis? It will never be used by most =
developers.
	b. Why not educate everyone? You can't - developer training =
simply does not scale, and mistakes still happen.


What's of concern?=20
-------------------------
(Note: the last point could address concerns #2 and #4.)

1. Bike-shedding a bit, but I find a bit of cognitive dissonance over =
the semantic incorrectness of is_literal(), similar to Jakob Givoni's =
concern.=20

I also fear it will be confusing for developers who know what a literal =
is, and thus I agree with Jakob.=20

Minimally I'd ask for a separate vote on is_literal() vs. =
is_from_literal().=20

2. Regarding the sections "Non-Parameterised Values" and "Future Scope," =
if I understand correctly, your vision is to ultimately require all SQL =
and other vulnerable literals to be written as literals in the source =
code for them to be used or otherwise disallowed by future library code =
and/or PHP internal functionality?

If I did not misunderstand I believe you would disallow an important =
class of functionality from being implemented in PHP, including several =
existing and widely used applications such as but not limited to =
PhpMyAdmin and Adminer that open and interact with arbitrary SQL =
databases.  It would also disallow using any data returned by APIs where =
the PHP application cannot know in advance what data will be acquired, =
even if that data has been properly escaped and/or sanitized.

3. I notice your RFC does not grant sprintf() the ability to return a =
string with the "literal" flag even though sprintf() can frequently =
return strings composed from known literals and safe numeric value. And =
I have frequently see it used for this purpose, e.g.:

$fields =3D ['id','name'];
$table =3D 'my_table';

$sql =3D sprintf( 'SELECT %s FROM %s WHERE ID=3D%d',=20
	implode( ',', $fields ),=09
	$my_table,
	intval($_GET['id']));

Of course your section "WHERE IN" "addresses" this, but please see #4 =
below.=20

I argue sprintf() should be able to pass thru literal flags and validate =
integer values to determine that the result is literal.  Am I missing =
something here?

4. Regarding the section "WHERE IN" you argue that we should push =
everyone to use parameters properly =E2=80=94 which is a great idea in =
theory =E2=80=94 but that would have a significant breakage in existing =
sites online.  While many here in Internals seem to look down on =
WordPress if does power over 40%[1] of the web at this point and this =
would break most if not all those sites if the site owners or their =
hosts were to upgrade to a version of PHP that was militant about =
requiring is_literal() for mysqli_query()[2]. =20

And yes, WordPress could upgrade their core code[3] to address this, but =
that would not also fix the 60k plugins and 10k themes on WordPress.org =
<http://wordpress.org/>, or likely the million+ combined custom plugins =
and themes in use on the web.=20

Is it really reasonable to change PHP to just break of all those sites =
so that we can force all the site owners to beg and plead with their =
plugin and theme developers to better protect them from injection =
errors?  Seems that medicine might be worse than the cure, at least for =
all those site owners.=20

5. Given #2 and #4, I think your section "Support Functions" is missing =
what would allow WordPress, PhpMyAdmin, Adminer and all other PHP =
applications to address the concerns in #2 and #4 and it is neither =
literal_concat() nor literal_implode().=20

The function we would need is the one Lauri Kentt=C3=A4 wrote (in =
jest?), but we'd need one that is performant, not a userland version. =
Name it as_literal() or make_literal() or even as_unsafe_literal() =E2=80=94=
 whichever chosen is unimportant IMO =E2=80=94 but applications like the =
ones I mentioned need to be able to say "I absolutely know what I am =
explicitly doing so do not throw a warning or an error telling me my =
value is `not a literal`, dammit!" =20

Certainly most people would not use such a method by accident, =
especially if called something like `as_unsafe_literal()`.  There might =
even be ways to validate that this was not used by a copy-and-paste =
artist, but I will leave that validation to other people who are smarter =
than me.

6. Regarding the section "Reflection API," shouldn't their be an =
is_literal()/is_from_literal() method added to the ReflectionParameter() =
and ReflectionProperty() classes?

So assuming you address at least #5 =E2=80=94 and thus #2 and #4 =
indirectly =E2=80=94 I would really like to see this RFC pass.

-Mike


[1] https://w3techs.com/blog/entry/40_percent_of_the_web_uses_wordpress =
<https://w3techs.com/blog/entry/40_percent_of_the_web_uses_wordpress>=20
[2] =
https://www.php.net/manual/en/mysqli.query.php#refsect1-mysqli.query-param=
eters =
<https://www.php.net/manual/en/mysqli.query.php#refsect1-mysqli.query-para=
meters>=20
[3] =
https://github.com/WordPress/WordPress/blob/master/wp-includes/wp-db.php#L=
2056 =
<https://github.com/WordPress/WordPress/blob/master/wp-includes/wp-db.php#=
L2056>






--Apple-Mail=_7365E864-4CC7-44C5-89E3-E0EED5696923--