Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:109194
MIME-Version: 1.0
References: <CAFv4g+EXxWsD5jvfbYVKwML7fMCp62vFPfP0VfOmNBLR-tUpHQ@mail.gmail.com>
 <DM5PR07MB30673AEA3D86375BF41F7DD6F9F20@DM5PR07MB3067.namprd07.prod.outlook.com>
In-Reply-To: <DM5PR07MB30673AEA3D86375BF41F7DD6F9F20@DM5PR07MB3067.namprd07.prod.outlook.com>
Date: Sat, 21 Mar 2020 22:55:33 +0000
Message-ID: <CAFv4g+HBk5pEz2kUu6cpiDwvTVrt-tNCwzCGLm+PJpZ3kW5cwA@mail.gmail.com>
To: tyson andre <tysonandre775@hotmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="00000000000050117f05a1654d15"
Subject: Re: [PHP-DEV] [RFC] is_literal()
From: craig@craigfrancis.co.uk (Craig Francis)

--00000000000050117f05a1654d15
Content-Type: text/plain; charset="UTF-8"

Hi Tyson,

Thanks for your email, I really appreciate it.

I'm not a C developer, but if I was to have a go at implementing
`is_literal()`, I probably would start with the existing taint extension.

But you're right, that approach can cause performance issues, and doesn't
play well with XDebug (much to my annoyance).

I'm hoping that someone with more C and PHP Internals experience might have
a better solution, as there was some mention of the existing engine knowing
about literals being semi-special, and there might be (I'm really showing
my ignorance here) a better way to work with the compiler.

As to using PECL; my hope is that we can use this to help educate
developers about the dangers of mixing variables with commands. While they
won't think to use this function themselves, it can be used by frameworks
to ensure mistakes aren't being made (and who knows, maybe one day
functions like mysqli_query() could use this as well).

As to the related projects / static analysis; while I do appreciate them,
similar tools have been available for years (admittedly some with huge
price tags), and I don't think they have really solved these problems. That
said, I have added a note in the RFC.

Thanks again,
Craig



On Sat, 21 Mar 2020 at 21:59, tyson andre <tysonandre775@hotmail.com> wrote:

> Hi Craig,
>
> https://github.com/laruence/taint#taint notes that
> "Please note that do not enable this extension in product(ion) env, since
> it will slowdown your app."
>
> - That repo already provides is_tainted()  http://docs.php.net/is_tainted
>
>   A fork of that repo would theoretically allow implementing is_literal()
> as described in the RFC - is that the implementation plan?
> - The slowdown would be a large problem if this feature was always on.
>
>   And if it can be implemented as a PECL module, that would be more
> preferable to me than a core module of php.
>   If it was in core, having to support that feature may limit
> optimizations or implementation changes that could be done in the future.
>
> If it's implemented in the same way as taint (i.e. cannot be used in
> combination with XDebug, blackfire, newrelic, etc),
> that would also be a problem for including it in core.
> If it wasn't, then it'd slow down concatenation, calls, etc. even when the
> application didn't need is_literal.
>
> I also imagine that whether or not opcache was enabled is likely to affect
> whether or not
> something ends up being a literal or not
> (e.g. opcache can evaluate functions such as str_repeat() for literals at
> compile time)
> https://github.com/laruence/taint/blob/master/taint.c seems to already
> support a whitelist (php_taint_override_func),
> so that isn't insurmountable for functions,
> but it's possible `if ($local === 'literal') { process($local); }` would
> only satisfy is_literal() with opcache enabled.
>
> Related projects (static analysis instead of runtime checks, though):
>
> It's also worth noting that `vimeo/psalm` had an in progress way to detect
> some ways in which tainted strings may be used by applications, based on a
> paper by Facebook.
> (
> https://cacm.acm.org/magazines/2019/8/238344-scaling-static-analyses-at-facebook/fulltext
> (for HHVM, though))
> https://github.com/vimeo/psalm/issues/611#issuecomment-515153305 - but it
> isn't completed or usable yet, as far as I can tell.
>
> Wikimedia also created
> https://gerrit.wikimedia.org/g/mediawiki/tools/phan/SecurityCheckPlugin/
> , but that's currently beta.
> Both would have ways they fail to catch every way an argument could be
> passed to a function (e.g. unanalyzable dynamic/framework calls)
>
> - Tyson
>

--00000000000050117f05a1654d15--