Feature: log and optionally terminate function_exists()-calls on non-whitelisted or blacklisted functions

[sjorek]

I learned in https://github.com/stefanesser/suhosin/issues/18 the following: calls to function_exists() silently return FALSE, if the function to test for has been blacklisted or not whitelisted in one of the these configurations:

• suhosin.executor.func.blacklist
• suhosin.executor.func.whitelist
• suhosin.executor.eval.blacklist
• suhosin.executor.eval.whitelist

The related discussion shows (to my mind), that the current solution leads to misinterpretations depending on the implementation and/or hosting environment. From a security point of view the silent return is a perfect solution, but during development or initial deployment of applications this might be annoying. Therefore I implemented a new error level/class S_EXISTENCE and used it to log and if one wishes also to terminate direct and indirect function_exists()-calls on blacklisted or non-whitelisted functions-names.

---

A (hopefully) complete list of changes and additions:

1. registered new constant S_EXISTENCE as a new log level; as it has never been part of the suhosin-patch it will always get registered, regardless of PHP being patched or not
   • Reason: I wanted to distinguish disabled function-execution from checking existence of disabled functions
   • If suhosin-extension is loaded into an already suhosin-patched PHP Interpreter the S_EXISTENCE-Level is not part of S_ALL
   • If suhosin-extension is loaded into vanilla PHP Interpreter the S_EXISTENCE-Level should be part of S_ALL
     • Maybe not such a good idea (untested, and is this even possible) ?
2. Implemented two new flags:
   • suhosin.executor.func.exists_forbidden = Off
     • If enabled script-execution terminates if function_exists() has been called for a blacklisted or non-whitelisted function
   • suhosin.executor.eval.exists_forbidden = Off
     • If enabled script-execution terminates if function_exists() has been called for a blacklisted or non-whitelisted function from within an eval()-statement.
   • As both are disabled by default, there is no difference to the former implemenentation.
3. Updated all related suhosin.ini-descriptions accordingly
4. Implemented unit-tests to check eval's und func's whitelist and blacklist, with the their exists_forbidden-flag enabled and disabled

---

build and install, as well as make test have been successfully run on:

• FreeBSD 9.1
  • Ports PHP 5.3.27, PHP 5.4.19 and PHP 5.5.3 partially using these patches: https://github.com/NewEraCracker/suhosin-patches/tree/master/patches
• OS X 10.8
  • Apple's PHP 5.3.15
  • MacPorts PHP 5.3.27, PHP 5.4.19 and PHP 5.5.3 partially using these patches: https://github.com/NewEraCracker/suhosin-patches/tree/master/patches

@narfbg : this pull-request enhances https://github.com/stefanesser/suhosin/issues/18 … at least I hope so.

I hope i didn't miss something and more important, I hope you like this solution …

Gut's Nächtle, Stephan

[narfbg]

Dude ... all of my arguments in #18 were based around the fact that sudden script termination with no way to work around it is a huge pain in the ass. What's with the exists_forbidden options? Optional indeed, but it's actually worse than function_exists() not returning FALSE for blacklisted functions. Seriously, I'm not trying to bash you, your patch, or anything (thanks for linking me btw) ... this just seems absurd to me.

Otherwise, I'd consider E_WARNING a more proper log level, but since this is not part of the PHP core, I don't really care about it. :)

Cheers.

[sjorek]

@narfbg

Without this patch there is no hint, that function_exists() returns FALSE, because suhosin forbid it. Even if you parse the suhosin configuration you cannot distinguish if the function to test for, was just forbidden by suhosin or really not available, without doing more. Now you can - in two flavors - log the test and/or stop the script. You were claiming that it's hard to deal with forbidden functions because they did not return false. Now as it returns FALSE this patch gives you a greater chance to get a clue of what is going on. Is this absurd ? Well, I'd like to know if a framework uses an possibly awfully slow alternative solution just because suhosin forbids it. It gets logged, if I don't exclude this new log-level in suhosin.log.sapi. And E_WARNING (php) and S_EXISTENCE (suhosin) should not get mixed up.

And paranoid people like me can turn the new exists_forbidden-flags on to immediately stop scripts that even look for things we don't like. So, I think this is an enhancement. Of course there are still other means left to look for functions, but its a tiny step to make it more controllable what going on.

And I made everything completely optional so upgrading won't change anything in your current setup, except suhosin being a little bit more verbose about what it does.

Stephan

[narfbg]

I understand the need for logging and debugging tools for this in general, there is no argument over the logging part and that you need to know when something like this happens.

What I don't agree on is having to terminate script execution when something like that happens. Obviously, it helps you during development and I don't know ... it may help somebody else in the same manner. At least that's a different argument to what you gave in my thread, but there's no guarantee that everybody would use it for the same purpose. Yes, it would be dumb to use it in a production environment, yes, whoever does that probably deserves all the heat they would get, etc. Still, it would eventually cause problems for end users.

[sjorek]

Can I convince you by changing the description of the two exists_forbidden-flags to:

• ; When this configuration flag is turned on, the script will terminate, if ; function_exists() is executed on a function not in the whitelist (if given) ; or within the blacklist (if given), after the problem has been logged. This ; flag is meant for development, deployment or paranoid use only. Unless you ; really want to annoy users, there are (very) good reasons NOT to enable this ; flag. If it's enabled, you deserve all the heat you'll get. ;suhosin.executor.func.exists_forbidden = Off
• ; When this configuration flag is turned on, the script will terminate, if ; function_exists() is executed from within eval() on a function not in the ; whitelist (if given) or within the blacklist (if given), after the problem ; has been logged. This flag is meant for development, deployment or paranoid ; use only. Unless you really want to annoy users, there are (very) good reasons ; NOT to enable this flag. If it's enabled, you deserve all the heat you'll get. ;suhosin.executor.eval.exists_forbidden = Off

? … :innocent:

[narfbg]

Those are indeed some very improved descriptions, but otherwise - no, IMO nothing can justify the existence of the exits_forbidden options. I guess we'll have to agree to disagree on that and ultimately wait for other people's input. :)

[sjorek]

Ack … :zzz:

[sjorek]

I quickly googled, and found an exploit which would terminate earlier in a suhosin-setup having the exists_forbidden flags enabled and all kinds of socket-related functions blacklisted. This makes it harder for an attacker to hide this intrusion, see http://stackoverflow.com/questions/9421414/what-exactly-does-this-php-exploit-code-found-on-my-app :

function tryfsockopen_777 ($url)
    {
        if (function_exists('fsockopen') === false)
            return false;
        …

function trysocket_777 ($url)
    {
        if (function_exists('socket_create') === false)
            return false;
        …

But as stated above, we appreciate any feedback we get - even negative. :smile:

Stephan

[narfbg]

And that would get logged, while the functions will be unusable because they are blacklisted ... the exploit won't be halted during it's runtime, but won't do any harm either, so no win for the attacker ... why do you care if they are scratching their heads or they get the script terminated with possibly an error message? The latter would actually give them useful information.

[sjorek]

• I'll never disclose error messages to an attacker by setting display_errors to On. So the attacker should not have any clue about what is going on. Everything else is somehow arkward, if not to say a kind of insane.
• Without the patch nothing will get logged, ok. This is what we currently have.
• With this patch and exists_forbidden set to Off something will get logged, but if the attacker is successful and clever enough he/she will wipe the logged message from the log-file and later forensic analysis gets more complicated.
• With this patch and exists_forbidden set to On the attacker will have it a little bit harder to be successful and moreover to hide himself/herself, hence one has a better chance in a forensic analysis to understand when and how something happend.

… I know this is a really special (and for production purposes a absolutely paranoid) requirement, but I'd like to have the chance to stop code execution if someone tries to detect the presence of functions before using them with the objective to hide his/her intrusion.

So, I'll wait for more feedback. Stephan

[sjorek]

I never expected such a strong opinion against the stop-on-function_exists()-detection feature, but absolutely respect @narfbg 's fears, but I don't share them … it has always been hard to find a good balance between usability and security. That's why I made this feature optional, in the hope to trigger sane usage of it.

What about other aspects of this patch, like:

• Does the patch weaken security by disclosing (too much) information to an attacker ?
• Does the additional logging raise any performance bottlenecks, eg. in your ( @narfbg ) CodeIgniter setups having exec or system blacklisted via suhosin ?
  • So, do we have to enhance the patch and exclude the S_EXISTENCE-log level from S_ALL ?
• Are the log messages descriptive enough (and syntactic correct, with a proper grammar, as my english is not always the best) ?
• Does the per-directory configuration actually work ? I never tested it …

@narfbg thx, for being so kind and actually read my stuff … really. Better critical, than no feedback at all. Which reminds me of being patient, in the hope of feedback from other users … :zzz:

[narfbg]

• I'll never disclose error messages to an attacker by setting display_errors to On. So the attacker should not have any clue about what is going on. Everything else is somehow arkward, if not to say a kind of insane.

You wouldn't, but as I said in a previous comment - there's no guarantee that nobody would.

• With this patch and exists_forbidden set to On the attacker will have it a little bit harder to be successful and moreover to hide himself/herself, hence one has a better chance in a forensic analysis to understand when and how something happend.

Just like you're saying you wouldn't have display_errors turned on, you probably would be paranoid enough to forbid every potentially dangerous function anyway. :)

Otherwise, again - I have nothing against the rest of this patch, it's just the exits_forbidden part that bothers me. The logging is great, I just would've loved it if you had limited yourself to that alone. I'm usually not against optional features as long as they make some sense and are Off by default. It just seems to me that this could cause more frustration for end-users than it would be useful.

Glad you like my feedback. I tend to get really hard into similar arguments (hence why you get it as a strong opinion from me alone) and people often confuse that with simply refusing to accept their apparently correct opinions.

[Ressy66]

"are Off by default"

+1

it is very annoying when things that are anally paranoid become default on (been there, experienced that)

[stefanesser]

Not really sure if I like this idea/feature. Have to discuss internally.

[sjorek]

… I appreciate any feedback, especially which objections you have.

And thanks for suhosin !