From d9cccbbe417d305bb56911cd07a7feac6b89e9a6 Mon Sep 17 00:00:00 2001 From: jvoisin Date: Tue, 27 Apr 2021 22:22:34 +0200 Subject: Protect against XXE in php8 PHP8 disables external entities by default, but they can still be explicitly used (cf. https://blog.sonarsource.com/wordpress-xxe-security-vulnerability/), which is bad™. The right way to defend against XXE is now to set libxml_set_external_entity_loader to null. --- src/sp_disable_xxe.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) (limited to 'src/sp_disable_xxe.c') diff --git a/src/sp_disable_xxe.c b/src/sp_disable_xxe.c index 113d84b..3ef1a5d 100644 --- a/src/sp_disable_xxe.c +++ b/src/sp_disable_xxe.c @@ -5,20 +5,22 @@ PHP_FUNCTION(sp_libxml_disable_entity_loader) { RETURN_TRUE; } int hook_libxml_disable_entity_loader() { TSRMLS_FETCH(); -// External entities are disabled by default in PHP8+ -#if PHP_VERSION_ID < 80000 - /* Call the php function here instead of re-implementing it is a bit - * ugly, but we do not want to introduce compile-time dependencies against - * libxml. */ zval func_name; - zval hmac; + zval retval; zval params[1]; +#if PHP_VERSION_ID < 80000 + // This function is deprecated in PHP8, but better safe than sorry for php7. ZVAL_STRING(&func_name, "libxml_disable_entity_loader"); ZVAL_STRING(¶ms[0], "true"); - call_user_function(CG(function_table), NULL, &func_name, &hmac, 1, params); + call_user_function(CG(function_table), NULL, &func_name, &retval, 1, params); #endif + // This is now the recommended way to disable external entities + ZVAL_STRING(&func_name, "libxml_set_external_entity_loader"); + ZVAL_NULL(¶ms[0]); + call_user_function(CG(function_table), NULL, &func_name, &retval, 1, params); + HOOK_FUNCTION("libxml_disable_entity_loader", sp_internal_functions_hook, PHP_FN(sp_libxml_disable_entity_loader)); -- cgit v1.3 From fb3571de3d9dd0df9bfb38579b56dbb9746df551 Mon Sep 17 00:00:00 2001 From: jvoisin Date: Wed, 28 Apr 2021 09:34:42 +0200 Subject: Add some logging for the XXE mitigation --- src/sp_disable_xxe.c | 16 +++++++++++++--- src/tests/xxe/disable_xxe_dom_disabled.phpt | 5 +++++ 2 files changed, 18 insertions(+), 3 deletions(-) (limited to 'src/sp_disable_xxe.c') diff --git a/src/sp_disable_xxe.c b/src/sp_disable_xxe.c index 3ef1a5d..9dea33c 100644 --- a/src/sp_disable_xxe.c +++ b/src/sp_disable_xxe.c @@ -1,6 +1,14 @@ #include "php_snuffleupagus.h" -PHP_FUNCTION(sp_libxml_disable_entity_loader) { RETURN_TRUE; } +PHP_FUNCTION(sp_libxml_disable_entity_loader) { + sp_log_warn( "xxe", "A call to libxml_disable_entity_loader was tried and nopped"); + RETURN_TRUE; +} + +PHP_FUNCTION(sp_libxml_set_external_entity_loader) { + sp_log_warn("xxe", "A call to libxml_set_external_entity_loader was tried and nopped"); + RETURN_TRUE; +} int hook_libxml_disable_entity_loader() { TSRMLS_FETCH(); @@ -10,19 +18,21 @@ int hook_libxml_disable_entity_loader() { zval params[1]; #if PHP_VERSION_ID < 80000 - // This function is deprecated in PHP8, but better safe than sorry for php7. + // This function is deprecated in PHP8, but better safe than sorry for php7. ZVAL_STRING(&func_name, "libxml_disable_entity_loader"); ZVAL_STRING(¶ms[0], "true"); call_user_function(CG(function_table), NULL, &func_name, &retval, 1, params); #endif - // This is now the recommended way to disable external entities + // This is now the recommended way to disable external entities ZVAL_STRING(&func_name, "libxml_set_external_entity_loader"); ZVAL_NULL(¶ms[0]); call_user_function(CG(function_table), NULL, &func_name, &retval, 1, params); HOOK_FUNCTION("libxml_disable_entity_loader", sp_internal_functions_hook, PHP_FN(sp_libxml_disable_entity_loader)); + HOOK_FUNCTION("libxml_set_external_entity_loader", sp_internal_functions_hook, + PHP_FN(sp_libxml_set_external_entity_loader)); return SUCCESS; } diff --git a/src/tests/xxe/disable_xxe_dom_disabled.phpt b/src/tests/xxe/disable_xxe_dom_disabled.phpt index 493f5a3..a49e094 100644 --- a/src/tests/xxe/disable_xxe_dom_disabled.phpt +++ b/src/tests/xxe/disable_xxe_dom_disabled.phpt @@ -44,8 +44,13 @@ printf("without xxe: %s", $dom->getElementsByTagName('testing')->item(0)->nodeVa ?> --EXPECTF-- +Warning: [snuffleupagus][0.0.0.0][xxe][log] A call to libxml_disable_entity_loader was tried and nopped in %s/tests/xxe/disable_xxe_dom_disabled.php on line %d libxml_disable_entity to true: WARNING, external entity loaded! + +Warning: [snuffleupagus][0.0.0.0][xxe][log] A call to libxml_disable_entity_loader was tried and nopped in %s/tests/xxe/disable_xxe_dom_disabled.php on line %d libxml_disable_entity to false: WARNING, external entity loaded! + +Warning: [snuffleupagus][0.0.0.0][xxe][log] A call to libxml_disable_entity_loader was tried and nopped in %s/tests/xxe/disable_xxe_dom_disabled.php on line %d without xxe: foo --CLEAN-- is_config_valid = SP_CONFIG_NONE; snuffleupagus_globals->in_eval = 0; -#define SP_INIT_HT(F) snuffleupagus_globals->F = \ - pemalloc(sizeof(*(snuffleupagus_globals->F)), 1); \ - zend_hash_init(snuffleupagus_globals->F, 10, NULL, NULL, 1); +#define SP_INIT_HT(F) \ + snuffleupagus_globals->F = pemalloc(sizeof(*(snuffleupagus_globals->F)), 1); \ + zend_hash_init(snuffleupagus_globals->F, 10, NULL, NULL, 1); SP_INIT_HT(disabled_functions_hook); SP_INIT_HT(sp_internal_functions_hook); SP_INIT_HT(sp_eval_blacklist_functions_hook); @@ -86,8 +85,9 @@ PHP_GINIT_FUNCTION(snuffleupagus) { SP_INIT_HT(config.config_disabled_functions_ret_hooked); #undef SP_INIT_HT -#define SP_INIT(F) snuffleupagus_globals->config.F = \ - pecalloc(sizeof(*(snuffleupagus_globals->config.F)), 1, 1); +#define SP_INIT(F) \ + snuffleupagus_globals->config.F = \ + pecalloc(sizeof(*(snuffleupagus_globals->config.F)), 1, 1); SP_INIT(config_unserialize); SP_INIT(config_random); SP_INIT(config_sloppy); @@ -128,16 +128,15 @@ static void free_disabled_functions_hashtable(HashTable *ht) { } PHP_MSHUTDOWN_FUNCTION(snuffleupagus) { - #define FREE_HT(F) \ - zend_hash_destroy(SNUFFLEUPAGUS_G(F)); \ - pefree(SNUFFLEUPAGUS_G(F), 1); + zend_hash_destroy(SNUFFLEUPAGUS_G(F)); \ + pefree(SNUFFLEUPAGUS_G(F), 1); FREE_HT(disabled_functions_hook); FREE_HT(sp_eval_blacklist_functions_hook); -#define FREE_HT_LIST(F) \ - free_disabled_functions_hashtable(SNUFFLEUPAGUS_G(config).F); \ - FREE_HT(config.F); +#define FREE_HT_LIST(F) \ + free_disabled_functions_hashtable(SNUFFLEUPAGUS_G(config).F); \ + FREE_HT(config.F); FREE_HT_LIST(config_disabled_functions); FREE_HT_LIST(config_disabled_functions_hooked); FREE_HT_LIST(config_disabled_functions_ret); @@ -145,12 +144,12 @@ PHP_MSHUTDOWN_FUNCTION(snuffleupagus) { #undef FREE_HT_LIST #undef FREE_HT -#define FREE_LST_DISABLE(L) \ - do { \ - sp_list_node *_n = SNUFFLEUPAGUS_G(config).L; \ - sp_disabled_function_list_free(_n); \ - sp_list_free(_n); \ - } while (0) +#define FREE_LST_DISABLE(L) \ + do { \ + sp_list_node *_n = SNUFFLEUPAGUS_G(config).L; \ + sp_disabled_function_list_free(_n); \ + sp_list_free(_n); \ + } while (0) FREE_LST_DISABLE(config_disabled_functions_reg->disabled_functions); FREE_LST_DISABLE(config_disabled_functions_reg_ret->disabled_functions); #undef FREE_LST_DISABLE @@ -184,24 +183,26 @@ PHP_MSHUTDOWN_FUNCTION(snuffleupagus) { } PHP_RINIT_FUNCTION(snuffleupagus) { - const sp_config_wrapper* config_wrapper = + const sp_config_wrapper *config_wrapper = SNUFFLEUPAGUS_G(config).config_wrapper; #if defined(COMPILE_DL_SNUFFLEUPAGUS) && defined(ZTS) ZEND_TSRMLS_CACHE_UPDATE(); #endif if (!SNUFFLEUPAGUS_G(allow_broken_configuration)) { - if (SNUFFLEUPAGUS_G(is_config_valid) == SP_CONFIG_INVALID ) { + if (SNUFFLEUPAGUS_G(is_config_valid) == SP_CONFIG_INVALID) { sp_log_err("config", "Invalid configuration file"); } else if (SNUFFLEUPAGUS_G(is_config_valid) == SP_CONFIG_NONE) { - sp_log_warn("config", "No configuration specificed via sp.configuration_file"); + sp_log_warn("config", + "No configuration specificed via sp.configuration_file"); } } - // We need to disable wrappers loaded by extensions loaded after SNUFFLEUPAGUS. + // We need to disable wrappers loaded by extensions loaded after + // SNUFFLEUPAGUS. if (config_wrapper->enabled && zend_hash_num_elements(php_stream_get_url_stream_wrappers_hash()) != - config_wrapper->num_wrapper) { + config_wrapper->num_wrapper) { sp_disable_wrapper(); } @@ -218,7 +219,7 @@ PHP_RSHUTDOWN_FUNCTION(snuffleupagus) { return SUCCESS; } PHP_MINFO_FUNCTION(snuffleupagus) { const char *valid_config; - switch(SNUFFLEUPAGUS_G(is_config_valid)) { + switch (SNUFFLEUPAGUS_G(is_config_valid)) { case SP_CONFIG_VALID: valid_config = "yes"; break; @@ -230,10 +231,11 @@ PHP_MINFO_FUNCTION(snuffleupagus) { valid_config = "no"; } php_info_print_table_start(); - php_info_print_table_row(2, "snuffleupagus support", - SNUFFLEUPAGUS_G(is_config_valid)?"enabled":"disabled"); + php_info_print_table_row( + 2, "snuffleupagus support", + SNUFFLEUPAGUS_G(is_config_valid) ? "enabled" : "disabled"); php_info_print_table_row(2, "Version", PHP_SNUFFLEUPAGUS_VERSION); - php_info_print_table_row( 2, "Valid config", valid_config); + php_info_print_table_row(2, "Valid config", valid_config); php_info_print_table_end(); DISPLAY_INI_ENTRIES(); } @@ -315,11 +317,12 @@ static PHP_INI_MH(OnUpdateConfiguration) { // If `zend_write_default` is not NULL it is already hooked. if ((zend_hash_str_find( - SNUFFLEUPAGUS_G(config).config_disabled_functions_hooked, "echo", - sizeof("echo") - 1) || - zend_hash_str_find( - SNUFFLEUPAGUS_G(config).config_disabled_functions_ret_hooked, "echo", - sizeof("echo") - 1)) && NULL == zend_write_default) { + SNUFFLEUPAGUS_G(config).config_disabled_functions_hooked, "echo", + sizeof("echo") - 1) || + zend_hash_str_find( + SNUFFLEUPAGUS_G(config).config_disabled_functions_ret_hooked, "echo", + sizeof("echo") - 1)) && + NULL == zend_write_default) { zend_write_default = zend_write; zend_write = hook_echo; } diff --git a/src/sp_disable_xxe.c b/src/sp_disable_xxe.c index 9dea33c..f9712b5 100644 --- a/src/sp_disable_xxe.c +++ b/src/sp_disable_xxe.c @@ -1,12 +1,15 @@ #include "php_snuffleupagus.h" PHP_FUNCTION(sp_libxml_disable_entity_loader) { - sp_log_warn( "xxe", "A call to libxml_disable_entity_loader was tried and nopped"); + sp_log_warn("xxe", + "A call to libxml_disable_entity_loader was tried and nopped"); RETURN_TRUE; } PHP_FUNCTION(sp_libxml_set_external_entity_loader) { - sp_log_warn("xxe", "A call to libxml_set_external_entity_loader was tried and nopped"); + sp_log_warn( + "xxe", + "A call to libxml_set_external_entity_loader was tried and nopped"); RETURN_TRUE; } diff --git a/src/sp_utils.c b/src/sp_utils.c index a1fa400..3d2dfcc 100644 --- a/src/sp_utils.c +++ b/src/sp_utils.c @@ -19,7 +19,7 @@ const char* get_ipaddr() { return fwd_ip; } - return default_ipaddr; + return default_ipaddr; } void sp_log_msgf(char const* restrict feature, int level, int type, -- cgit v1.3