Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:109215
Content-Type: multipart/alternative;
	boundary="Apple-Mail=_40F044F6-CF78-447F-AA62-FECD9907DE00"
Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.60.0.2.5\))
Message-ID: <B6DFDAD5-87A1-4BB5-8B61-497B72162B57@craigfrancis.co.uk>
Date: Sun, 22 Mar 2020 23:14:27 +0000
Cc: PHP internals <internals@lists.php.net>
To: mike@newclarity.net
Subject: [RFC] is_literal()
From: craig@craigfrancis.co.uk (Craig Francis)

--Apple-Mail=_40F044F6-CF78-447F-AA62-FECD9907DE00
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

On Sun, 22 Mar 2020 at 19:11, Mike Schinkel <mike@newclarity.net> wrote:
> I think it would be better to specify the problem(s) you are trying to =
solve



Thanks for your thoughts Mike,

I've updated the RFC, tweaking the definition of a literal, and moving =
that to the end of the introduction, so the problems are now in the =
second paragraph (sorry this wasn't clear, this is my first RFC).

https://wiki.php.net/rfc/is_literal =
<https://wiki.php.net/rfc/is_literal>



> Looking at my code [...] I pass to $wpdb->get_results() is in =
variables, not literals.


I think this is where Jakob's suggestion might help, to call it =
`is_from_literal()`.

Because you're right, the SQL/HTML/Command is often created by one or =
more literals that get stored in a variable, and this proposal works =
with that approach.


> [...] introduce functionality that addresses the exact problem of SQL =
injection


I'm fairly confident this proposal does that, and more.

By allowing frameworks/developers to check the SQL/HTML/Command is made =
up of hard coded literals (from within the PHP script), it proves it's =
not vulnerable to any kind of injection, as the other =
variables/parameters must be provided separately.

The examples in the RFC cover the issues that usually get raised when =
discussing this.

But I do need to address the performance question (ideally with some =
help from someone who understands the PHP Internals, as I'm not =
experienced enough in that area).


> [...] hash out potential solutions on the list rather than propose a =
specific one in advance.


I must confess, I have been discussing this for a number of years, and =
I've looked at a few different approaches, Taint checking got the =
closest, and this proposal takes that idea a bit further to resolve the =
last few issues (covered in the RFC, so I won't repeat them here).

I've also been keeping this proposal in mind over the last couple of =
years, just to see how it would effect my development practices (and I =
really think it has helped).

As to your idea of a "safe" MySQL class, fortunately mysqli already =
stops multiple queries, so a SELECT cannot have an =
UPDATE/DELETE/TRUNCATE appended on to the end, but it can still do =
things like UNION another SELECT query, so the original query returns =
nothing, then the attackers query gets appended, potentially allowing =
them to extract everything from the database.

Craig







On 22 Mar 2020, at 19:10, Mike Schinkel <mike@newclarity.net> wrote:

> BTW, I did not comment on your is_literal() proposal because I try not =
to comment on things where others are addressing concerns and where I do =
not feel very strongly about adding or avoiding the feature.  I created =
this thread as a new thread to expliclty separate discussions because =
they are orthogonal and I did not want to imply that I supported =
is_literal() as currently proposed.
>=20
> But since you brought it up let me comment on is_literal().  I =
recognize the problem I think you are trying to solve =E2=80=94 to be =
able to guard against SQL injection attacks =E2=80=94 but I don't think =
is_literal() is a viable solution for that problem.
>=20
> 1. Looking at my code most of my dynamic values use to compose SQL =
that I pass to $wpdb->get_results() is in variables, not literals. In =
fact I rarely use literals.  So I don't see that it would help me (or =
many others?) much at all.
>=20
> 2. Focusing on it being literal or not seems to me to be focusing on =
wrong thing. Something could be non-literal but still be safe. And a =
code hack could introduce a tainted literal (but with a code hack all =
bets are off.) is_taint() makes more sense to me, but even then I am not =
sure it directly addresses the use-case.
>=20
> 3. I feel like we might be better to introduce functionality that =
addresses the exact problem of SQL injection and not one that dances =
around its edges. Maybe we need a MySQL class as an alternative to the =
mysqli functions that has a "safe" property and when that "safe" =
property is true you can't run DDL, TRUNCATE, or DELETE?  Not sure how =
to protect against injection for UPDATE but maybe someone else has an =
idea?
>=20
> 4. Lastly, I think it would be better to specify the problem(s) you =
are trying to solve and then hash out potential solutions on the list =
rather than propose a specific one in advance, maybe even creating an =
ad-hoc working group to come up with a solution that is likely to be =
accepted?
>=20
> Anyway, #jmtcw on is_literal().
>=20
> -Mike

--Apple-Mail=_40F044F6-CF78-447F-AA62-FECD9907DE00--