Open WhiteWinterWolf opened 3 years ago
The memory leak should be fixed via db14b8549e7bb9c132435f845a16f8d33677e865, I'm currently looking at why it's trying to match so many times.
I also landed 194b0bc9f0a4699854ea314ffa23e59f8082ddae, which might help memory-wise.
I add your testcase in ad623f1d78151dfe2eaee85e93ed1058be1c7f91 but it doesn't reproduce in the CI. Can you confirm that it should be a faithful reproducer of your problem?
I'm investigating for other things which may be involved in this issue.
Note however that I do not think that this issue is linked to PCRE, as it happens no matters if the parameter name actually exists or not (using param("foo")
also triggers the issue), and no matter if you use match
or match_r
on it.
I got the "why", but don't ask me about the "how" ;) .
Seemingly Snuffleupagus doesn't like when its configuration file is set from the Apache configuration file using the php_admin_value
and php_value
statements:
sp.configuration_file
, this issue cannot be reproduced.php_value sp.configuration_file "/path/to/rules"
, the given file is simply ignored (abnormal behavior), the local value for the sp.configuration_file
setting just reflects the master one set in php.ini, and the current issue cannot be reproduced.php_admin_value sp.configuration_file "/path/to/rules"
, the master and local values show the value defined respectively in the php.ini and in the Apache configuration files (as expected). This is when the issue appears. Maybe Snuffleupagus relies somewhere on the assumption that the local and the master values for sp.configuration_file
are the same ?Knowing this, I have remade the layout of my php.ini files to avoid the issue and will add a drop rule preventing runtime overwrite of the sp.configuration_file
setting (even-though, as per my understanding, Snuffleupagus currently only use it only on startup). This is sufficient to solve the issue in my environment.
Nevertheless, I don't know whether there is any plan to support the php_value
and php_admin_value
?
sp.configuration_file
.For me the first choice seem both the easiest and the safest, as it should also prevent an attacker from potentially disabling Snuffleupagus through .htaccess or .user.ini files.
I didn't went deep in Snuffleupagus source code, but as per my understanding snuffleupagus.c defines a callback function OnUpdateConfiguration()
which is invoked by PHP when loading the configuration, and this callback is in charge of expanding globs, parsing each configuration file and attach hooks.
If the sp.configuration_file
is successively set first to a master then to a local value, chances are that this callback function will be called two times with two different set of configuration files. I wonder what may happen in this circumstance, in particular if Snuffleupagus may not try to re-hook already hooked functions (ie. hook its own functions), which may effectively result in infinite loops and Out-Of-Memory errors.
BTW, the way strtok_r()
is used in snuffleupagus.c seems weird to me. As per my understanding, the last argument shouldn't be the parsed string but a distinct pointer used to keep a status information (and I'm not sure either why you are using the recursive version of strtok()
).
It is also worth noting that this situation where Snuffleupagus has to handle different local and master values will also be present in #260.
I removed strtok_r
usage in d934db81cdcd64086c8867bb422bfa7e0d18bb38, and I'm looking at recursion issues, albeit this is weird, because there are guards everywhere to prevent this from happening :/
8353de0 might™ prevent the issue, can you test @WhiteWinterWolf ? Otherwise, I'll try install freebsd and apache2 to reproduce this issue this weekend :)
Sadly 8353de0 doesn't seem to have any effect on the issue :(
Reproduced on another environment, I can confirm it happens when I use the php_admin_value sp.configuration_file
directive in Apache configuration in addition to the sp.configuration_file
directive in php.ini, no matter the patch above is applied or not. As soon as I comment-out the setting in the Apache configuration, the issue disappears.
The only consequences I can see for now for Snuffleupagus in the presence of this settings:
sp.configuration_file
is associated to two different values as "local" and "master" settings.OnUpdateConfiguration()
twice, once for each value.What I've seen is that the issue doesn't come directly from the fact that the "master" and "local" value are different: as long as the globing results in the very same files to be loaded, there is no issue. The issue happens when the php_admin_value
directive causes Snuffleupagus to load an additional configuration file, no matter its content as even loading a single empty file at this step make the issue to appear.
In case it might be useful, I've added log messages when entering and quittting ZEND_FUNCTION(check_disabled_function)
:
--- sp_disabled_functions.c.orig 2021-01-02 18:22:07 UTC
+++ sp_disabled_functions.c
@@ -490,6 +490,7 @@ static void should_drop_on_ret(const zva
}
ZEND_FUNCTION(check_disabled_function) {
+ sp_log_err("debug", "Entering '%s'.", __func__);
zif_handler orig_handler;
const char* current_function_name = get_active_function_name(TSRMLS_C);
@@ -508,6 +509,7 @@ ZEND_FUNCTION(check_disabled_function) {
.config_disabled_functions_reg_ret->disabled_functions,
SNUFFLEUPAGUS_G(config).config_disabled_functions_ret_hooked,
execute_data);
+ sp_log_err("debug", "Exiting '%s'.", __func__);
}
static int hook_functions_regexp(const sp_list_node* config) {
The normal behavior, when there is no issue, produces the following logs:
May 16 21:33:21 freebsd snuffleupagus[14706]: [snuffleupagus][0.0.0.0][debug][log] Entering 'zif_check_disabled_function'. in /var/www/main/data/index.php on line 4
May 16 21:33:21 freebsd snuffleupagus[14706]: [snuffleupagus][0.0.0.0][debug][log] Exiting 'zif_check_disabled_function'. in /var/www/main/data/index.php on line 4
=> A single entry immediately followed by a single exit.
The erroneous behavior produces the following logs:
May 16 21:33:32 freebsd snuffleupagus[14747]: [snuffleupagus][0.0.0.0][debug][log] Entering 'zif_check_disabled_function'. in /var/www/main/data/index.php on line 4
May 16 21:33:45 freebsd syslogd: last message repeated 521165 times
May 16 21:33:45 freebsd php: PHP Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 4096 bytes) in /var/www/main/data/index.php on line 4
=> Infinite entry until memory exhaustion, no exit.
This gives the impression that the recursion happens here.
The last line being executed from this function being orig_handler(INTERNAL_FUNCTION_PARAM_PASSTHRU)
, then we get back on the entering message.
I also took a look on the name and address of the function being hooked in this function:
Entering 'zif_check_disabled_function'.
current_function_name = 'exec'
orig_handler = 0x803215ff0
Exiting 'zif_check_disabled_function'.
Entering 'zif_check_disabled_function'.
current_function_name = 'exec'
orig_handler = 0x8050226f0
We can see that while in both cases the name of the targeted function remains correct, exec
, its pointer value changes (I ensured that, with identical settings, the pointer value remains the same across several Apache restarts).
With a bit of further reverse engineering, my understanding is that this pointer value is defined in hook_function
. I've added a debug message in it:
--- sp_utils.c.orig 2021-01-02 18:22:07 UTC
+++ sp_utils.c
@@ -404,6 +404,7 @@ int hook_function(const char* original_n
if ((func = zend_hash_str_find_ptr(CG(function_table),
VAR_AND_LEN(original_name)))) {
+ sp_log_err("debug", "hook_function: '%s' = %p, new: %p", original_name, func->handler, new_function);
if (func->handler == new_function) {
return SUCCESS; // the function is already hooked
} else {
It shows that there is no issue during the initialization phase, as both the sane and bogus Apache settings produce the exact same output, showing two call to this function (I would expect a single call, I don't know if this is normal, but the same behavior can also be seen for all other hooked functions):
hook_function: 'exec' = 0x803215ff0, new: 0x8050225b0 in [no active file] on line 0
hook_function: 'exec' = 0x8050225b0, new: 0x805022710 in [no active file] on line 0
However, as soon as PHP handles an incoming request invoking the hooked exec
:
hook_function: 'exec' = 0x805022710, new: 0x8050225b0 in [no active file] on line 0
hook_function: 'exec' = 0x8050225b0, new: 0x805022710 in [no active file] on line 0
hook_function: 'exec' = 0x805022710, new: 0x805022710 in [no active file] on line 0
Once all functions have been re-hooked comes the loop:
Entering 'zif_check_disabled_function'. in /var/www/main/data/index.php on line 4
current_function_name = 'exec' in /var/www/main/data/index.php on line 4
orig_handler = 0x805022710 in /var/www/main/data/index.php on line 4
Entering 'zif_check_disabled_function'. in /var/www/main/data/index.php on line 4
current_function_name = 'exec' in /var/www/main/data/index.php on line 4
orig_handler = 0x805022710 in /var/www/main/data/index.php on line 4
...
Sadly, 0x805022710
is not the pointer to the original PHP function (0x803215ff0
), but the pointer to Snuffleupagus' own zif_check_disabled_function
which was set during the initial hook session, hence the recursive call and the out-of-memory crash.
Next step: why does Snuffleupagus attempt to reload its configuration / re-hook its functions?
I don't know yet, but as a reminder this issue occurs only when hooking exec
-like functions, rules about hooking mysqli
functions for instance work with no issue in every case. I can just guess that there is something special with these functions, maybe because of a fork or something like that.
Hi, I'll look into it sometime next week o/
Any updates regarding this or any help needed?
Can you try with the current -git
version? A lot of code was changed since this issue was opened.
Thanks @jvoisin, I will check this as soon as I can and keep you informed.
The default rules matching the
exec
-like functions (tested withexec()
andsystem()
) crashes PHP with an Out-Of-Memory error.Using the following minimal test file directly invoked from the web root:
The default rule:
Generates the following error:
This issue is not linked to the regex or even anything actually related to the parameter, as replacing this rule by:
or even:
Generates even worse Out-Of-Memory errors, this time uncontrolled by PHP: the HTTP process goes from around 100 MB to over 1 GB, depending what limit is imposed by the OS, and finally gets killed by the OS, producing either:
or more often:
error messages.
Commenting-out these rules in Snuffleupagus configuration removes the issues.
Other rules seem to work OK, including regex rules matching on SQL requests for instance which use the same syntax. I have therefore the impression that this issue is linked to some specific behavior or implementation of these
exec
-like functions.I'm using FreeBSD with PHP as an Apache module, latest packages version: php80-8.0.6, php80-snuffleupagus-0.7.0.