Fix bug #62397 - disable_functions does not work with eval.

[beberlei]

Related to: https://bugs.php.net/bug.php?id=62397

While eval is technically not a function, from a user perspective this implementation detail should not be necessary to know.

The disable_functions INI setting is a system level setting and the overwrite of compile_string function is then done directly after module init/startup phase of PHP. Technically another extension could change this overwrite again, but the same is true for the way how disabled functions already works.

The alternative introducing a new INI setting disable_eval is also not good, because it would require users to know that the ini setting exists vs just listing eval in disable_functions. Also this would require adding a global variable which would prevent this fix from getting merged down into stable 7.2 and 7.3

[carusogabriel]

The added display_disabled_compile_string function is because we can't use this one?

[beberlei]

Yes, the signatures are different. display_disabled_function has the signature of an internal function and it can be replaced as function pointer in the function_table (see zend_disable_function just below this code in the same file).

The eval disable support makes use of the function pointer to zend_compile_string which can be replaced with different implementation using the same idea as the function table disable functionality.

[KalleZ]

After letting this float for a while, I actually quite like this approach as the userland should not have to care about whether or not this is a language construct.

If there is no feedback against this after another week or so, then it should be fine to go into PHP-7.4.

[petk]

Then, applied via 050d299 to PHP-7.4 and changelogs updated via c27b531. Thank you @beberlei

[derickr]

This is a terrible idea. It makes Xdebug not being able to change variables, or allow for watches either, if eval is turned off this way. Xdebug uses zend_eval_string, which calls zend_compile_string that you now override with something broken. Not only that, as it is not easily chained, changing this back in an extension reliably is a pain. This should be reverted.

[derickr]

This needs to be reverted.

[krakjoe]

Reverted.

The idea of trying to disable eval doesn't make any sense whatever, if I'm a malicious hacker and find myself on a system that has somehow disabled eval, I can just use include ...

This should have been reviewed by someone with Zend karma before merging.

[petk]

👍 Bug report re-opened then: https://bugs.php.net/bug.php?id=62397

[krakjoe]

I closed it with an explanation, this is not a thing that needs implementing, it would provide absolutely nothing in terms of security, except false hope :)

[petk]

Sounds good with me and true actually... Thanks @krakjoe for a quick fix and sorry @beberlei for not implementing this one then. I think disabling functions overall in many cases isn't really needed anyhow so drastically - and eval language construct included here with Joe's explanation on top of it...

[beberlei]

@petk no worries :-) Trying to get a grip with small changes, doesn't matter if they get in or not in the end.

[iansltx]

Didn't realize this already got discussed, but here goes another round: https://externals.io/message/107860

@krakjoe I wouldn't go so far as to say "absolutely nothing in terms of security"; forcing exploit authors to rewrite their toolkits in a way that is harder to obfuscate is a win, as is disallowing "download this file, base64_decode it, and immediately execute it". It's absolutely a half-measure, but if that half-measure makes a site a bit harder to compromise with existing tools, that's a win.

[paulocoghi]

In the end, ecosystems like Wordpress are so filled with thousands of plugins and themes with vulnerable code, that updating CVE's databases to find which ones are vulnerable is also "too late", because when you discover one in a customer installation, the damage is already done on most of the cases.

Really: working with Wordpress security it's like an eternal ice drying.

To help decrease such issues, as well as the number of corrective support calls they inevitably create, many companies end up going to solutions like Suhosin and Snuffleupagus, since they can't find ways to apply such security measures on native PHP itself (measures that go well beyond disabling eval).

Maybe this PR isn't a proper solution, maybe it doesn't go to the right direction, but accepting the reality of the numerous unsolved security problems is a first step towards thinking in solutions.

[petk]

@paulocoghi I totally agree here. This was opened/merged/reverted/reopened/closed very long time ago, and I think we should rethink this again. PHP needs to be improved in the future more drastically.

Update: @paulocoghi, I would suggest to continue the eval disabling feature also here: https://externals.io/message/107860 there seemed to be ongoing discussion happening. There are some quite valid reasons given there also. Disabling eval is just one of the issues. Well, security is tricky topic because there are always further ways.

[goteguru]

I don't consider myself a PHP advocate, but I have fair understanding of cyber security (being my field of research) and I find this discussion perplexing. I have a feeling that some commenters have no firm understanding of the goals, tools and controls of security.

Indeed, if we have a general purpose language, we can do "anything" the IO allows, in theory, the language is turing complete after all. But ignoring the complexity required is huge mistake. Is it possible to empty a lake with spoon? Certainly. Will you try? I don't think so.

Let's check some reasons above:
"disabling eval gives false security". I hear this (not very smart) reason a lot. First it's purely a documentation issue. Secondly it's a nonsense. A sufficiently strong average man can kill me with a hammer any time. Should we fire all policemen because Police and Juries just give us "false sense of security"? Countermeasure which is not 100% effective is still a countermeasure. If you are unable to actually stop the attacker, but you successfully slow her down or deter her instead, the countermeasure was effective. We have trusted solution to "false sense": It's called education.

"if eval is disabled, I can just use include". No. You can't. To use include you need file write (!) access. It's an enormous difference in terms of security. Moreover, your ability of injection attack is greatly reduced in case of include (as the parameter MUST exists on the filesystem), while injection in eval() leads to an immediate escalation. Not a small difference to ignore.

"disabling eval hurts XDebug": Really? Disabling eval is trivially useful on production systems. Would we really want to run a profiler/debugger on a production system? Is it that difficult to turn off/change the config file if we do?

Can a malicious attacker create her full blown interpreter and use that instead of eval()? Yes, indeed, she can. Is it considerably harder to maintain, upload, enable and run that, than just eval+base64? Certainly!

Cybersecurity isn't just about potential; it's also concerned with determination and capability. In fact, it's predominantly focused on determination and capability. If your countermeasure excludes a set of attackers not skilled enough, makes attacks significantly more difficult and slower then it's good enough to take into consideration.

[paulocoghi]

Completely agree with @goteguru .

Unfortunately, because of such security issues, users and companies are forced encouraged to look for alternatives outside the PHP ecosystem. And when they don't find them, they build them.

This was a tough (but necessary) decision that my team had to make recently.

[bukka]

From reading the above, it seems to me that this just needs an RFC. I think it actually makes sense to be able to disable eval so I re-opened the feature request. I will not re-open the PR because it is outdated and as I said this really needs an RFC first. Such RFC should mentioned impact on Xdebug and other things discussed here so voters can make a decision based on that. If anyone wants to volunteer to do the RFC, that would be great. I can add it to my long to TODO list but that will likely take very long time so some help would be appreciated.

[Raven-Singularity]

@krakjoe:

The idea of trying to disable eval doesn't make any sense whatever, if I'm a malicious hacker and find myself on a system that has somehow disabled eval, I can just use include ...

If you have a hardened web server without write access to any folders, how would someone include() anything?

I closed it with an explanation, this is not a thing that needs implementing, it would provide absolutely nothing in terms of security, except false hope :)

Just because you don't like a feature request, does not make it invalid. You have given no actual reasons why it shouldn't be an admin's choice to disable this feature on their server. When files need to be written to before executing code, a hardened system can prevent write access, can monitor the file writes, virus scanners can scan those files, etc. When eval() exists, many methods of harm detection are eliminated. It is not the same at all.

And if you will then say that there are other functions like assert() can be used to execute code without writing it to disk, those can also be disabled by an admin if they want.

In over 35 years of programming, I've never had a legitimate reason to use something like eval(). It is an extremely dangerous command that can allow full code execution with a slight typo by the programmer. For almost 100% of programmers they will never have a legitimate case to use eval(). If a project needs it, leave it on, otherwise let people turn it off.