From af93172be680bb75ebdf23ff6533c53f587da18c Mon Sep 17 00:00:00 2001 From: jvoisin Date: Tue, 3 May 2022 21:22:22 +0200 Subject: Small code formatting fix --- src/snuffleupagus.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'src/snuffleupagus.c') diff --git a/src/snuffleupagus.c b/src/snuffleupagus.c index ebb7f9c..06b93e1 100644 --- a/src/snuffleupagus.c +++ b/src/snuffleupagus.c @@ -426,7 +426,7 @@ static void dump_config() { array_init(&arr_cookies); sp_cookie *cookie; - sp_list_node *p = SPCFG(cookie).cookies; + const sp_list_node *p = SPCFG(cookie).cookies; for (; p; p = p->next) { zval arr_cookie; array_init(&arr_cookie); @@ -610,7 +610,9 @@ static PHP_INI_MH(OnUpdateConfiguration) { (PHP_VERSION_ID < 70400 && ts >= (time_t)1638745200L) || (PHP_VERSION_ID < 80000 && ts >= (time_t)1669590000L) || (PHP_VERSION_ID < 80100 && ts >= (time_t)1700953200L)) { - sp_log_warn("End-of-Life Check", "Your PHP version '" PHP_VERSION "' is not officially mainained anymore. Please upgrade as soon as possible. - Note: This message can be switched off by setting 'sp.global.show_old_php_warning.disable();' in your rules file or by setting the environment variable SP_SKIP_OLD_PHP_CHECK=1."); + sp_log_warn("End-of-Life Check", "Your PHP version '" PHP_VERSION "' is not officially maintained anymore. " \ + "Please upgrade as soon as possible. - Note: This message can be switched off by setting " \ + "'sp.global.show_old_php_warning.disable();' in your rules file or by setting the environment variable SP_SKIP_OLD_PHP_CHECK=1."); } } return SUCCESS; -- cgit v1.3 From 83014d7df165f8b8a9bf6dd4fde93fd1d42e4b7e Mon Sep 17 00:00:00 2001 From: jvoisin Date: Tue, 12 Jul 2022 21:24:44 +0200 Subject: Minor refactorisation --- src/php_snuffleupagus.h | 11 +++++++++-- src/snuffleupagus.c | 8 -------- 2 files changed, 9 insertions(+), 10 deletions(-) (limited to 'src/snuffleupagus.c') diff --git a/src/php_snuffleupagus.h b/src/php_snuffleupagus.h index a4a0ed4..95caa65 100644 --- a/src/php_snuffleupagus.h +++ b/src/php_snuffleupagus.h @@ -11,6 +11,12 @@ #include "config.h" #endif +#ifdef PHP_WIN32 +#include "win32/glob.h" +#else +#include +#endif + #include #include #include @@ -41,6 +47,7 @@ #include "zend_extensions.h" #include "zend_hash.h" #include "zend_string.h" +#include "zend_smart_str.h" #include "zend_types.h" #include "zend_vm.h" @@ -150,9 +157,9 @@ HashTable *sp_internal_functions_hook; HashTable *sp_eval_blacklist_functions_hook; #if PHP_VERSION_ID >= 80000 -zend_string* eval_source_string; +const zend_string* eval_source_string; #else -zval* eval_source_string; +const zval* eval_source_string; #endif ZEND_END_MODULE_GLOBALS(snuffleupagus) diff --git a/src/snuffleupagus.c b/src/snuffleupagus.c index 06b93e1..4c9e904 100644 --- a/src/snuffleupagus.c +++ b/src/snuffleupagus.c @@ -1,11 +1,3 @@ -#ifdef PHP_WIN32 -#include "win32/glob.h" -#else -#include -#endif - -#include "zend_smart_str.h" - #include "php_snuffleupagus.h" #ifndef ZEND_EXT_API -- cgit v1.3 From 8795ce1a92d226e31668db2f6cfbd33b40632109 Mon Sep 17 00:00:00 2001 From: jvoisin Date: Tue, 12 Jul 2022 21:54:19 +0200 Subject: Constify some variables --- src/snuffleupagus.c | 13 ++++++++----- src/sp_ini.c | 11 +++++------ 2 files changed, 13 insertions(+), 11 deletions(-) (limited to 'src/snuffleupagus.c') diff --git a/src/snuffleupagus.c b/src/snuffleupagus.c index 4c9e904..5bcd57c 100644 --- a/src/snuffleupagus.c +++ b/src/snuffleupagus.c @@ -254,7 +254,7 @@ PHP_MINFO_FUNCTION(snuffleupagus) { #define ADD_ASSOC_ZSTR(arr, key, zstr) if (zstr) { add_assoc_str(arr, key, zstr); } else { add_assoc_null(arr, key); } #define ADD_ASSOC_REGEXP(arr, key, regexp) if (regexp && regexp->pattern) { add_assoc_str(arr, key, regexp->pattern); } else { add_assoc_null(arr, key); } -static void add_df_to_arr(zval *arr, sp_disabled_function *df) { +static void add_df_to_arr(zval *arr, sp_disabled_function const *const df) { zval arr_df; array_init(&arr_df); @@ -365,7 +365,7 @@ static void dump_config() { if (splist) { \ zval arr_sp; \ array_init(&arr_sp); \ - for (sp_list_node *p = splist; p; p = p->next) { add_next_index_str(&arr_sp, p->data); } \ + for (const sp_list_node *p = splist; p; p = p->next) { add_next_index_str(&arr_sp, p->data); } \ add_assoc_zval(arr, key, &arr_sp); \ } else { add_assoc_null(arr, key); } @@ -381,6 +381,8 @@ static void dump_config() { ADD_ASSOC_SPLIST(&arr, SP_TOKEN_ALLOW_WRAPPERS, SPCFG(wrapper).whitelist); +#undef ADD_ASSOC_SPLIST + add_assoc_bool(&arr, SP_TOKEN_INI_PROTECTION "." SP_TOKEN_ENABLE, SPCFG(ini).enable); add_assoc_bool(&arr, SP_TOKEN_INI_PROTECTION "." SP_TOKEN_SIM, SPCFG(ini).simulation); add_assoc_bool(&arr, SP_TOKEN_INI_PROTECTION "." "policy_ro", SPCFG(ini).policy_readonly); @@ -392,7 +394,7 @@ static void dump_config() { zval arr_ini; array_init(&arr_ini); - sp_ini_entry *sp_entry; + const sp_ini_entry *sp_entry; ZEND_HASH_FOREACH_PTR(SPCFG(ini).entries, sp_entry) zval arr_ini_entry; array_init(&arr_ini_entry); @@ -442,7 +444,8 @@ static void dump_config() { zval arr_dfs; array_init(&arr_dfs); size_t num_df = 0; - sp_list_node *dflist, *dfp; + const sp_list_node *dflist; + const sp_list_node *dfp; ZEND_HASH_FOREACH_PTR(SPCFG(disabled_functions), dflist) for (dfp = dflist; dfp; dfp = dfp->next) { add_df_to_arr(&arr_dfs, dfp->data); @@ -597,7 +600,7 @@ static PHP_INI_MH(OnUpdateConfiguration) { (SPCFG(disabled_functions_ret) && zend_hash_num_elements(SPCFG(disabled_functions_ret))); if (SPCFG(show_old_php_warning) && getenv("SP_SKIP_OLD_PHP_CHECK") == NULL) { - time_t ts = time(NULL); + const time_t ts = time(NULL); if ((PHP_VERSION_ID < 70300) || (PHP_VERSION_ID < 70400 && ts >= (time_t)1638745200L) || (PHP_VERSION_ID < 80000 && ts >= (time_t)1669590000L) || diff --git a/src/sp_ini.c b/src/sp_ini.c index 7fec297..ed23fb7 100644 --- a/src/sp_ini.c +++ b/src/sp_ini.c @@ -12,12 +12,12 @@ } -static bool /* success */ sp_ini_check(zend_string *varname, zend_string *new_value, sp_ini_entry **sp_entry_p) { +static bool /* success */ sp_ini_check(zend_string *const restrict varname, zend_string const *const restrict new_value, sp_ini_entry **sp_entry_p) { if (!varname || ZSTR_LEN(varname) == 0) { return false; } - sp_config_ini *cfg = &(SPCFG(ini)); + sp_config_ini const* const cfg = &(SPCFG(ini)); sp_ini_entry *entry = zend_hash_find_ptr(cfg->entries, varname); if (sp_entry_p) { *sp_entry_p = entry; @@ -76,10 +76,9 @@ static bool /* success */ sp_ini_check(zend_string *varname, zend_string *new_va } static PHP_INI_MH(sp_ini_onmodify) { - zend_ini_entry *ini_entry = entry; sp_ini_entry *sp_entry = NULL; - if (!sp_ini_check(ini_entry->name, new_value, &sp_entry)) { + if (!sp_ini_check(entry->name, new_value, &sp_entry)) { return FAILURE; } @@ -91,7 +90,7 @@ static PHP_INI_MH(sp_ini_onmodify) { } void sp_hook_ini() { - sp_config_ini *cfg = &(SPCFG(ini)); + sp_config_ini const* const cfg = &(SPCFG(ini)); sp_ini_entry *sp_entry; zend_ini_entry *ini_entry; ZEND_HASH_FOREACH_PTR(cfg->entries, sp_entry) @@ -126,8 +125,8 @@ void sp_hook_ini() { void sp_unhook_ini() { sp_ini_entry *sp_entry; - zend_ini_entry *ini_entry; ZEND_HASH_FOREACH_PTR(SPCFG(ini).entries, sp_entry) + zend_ini_entry *ini_entry; if (!sp_entry->orig_onmodify) { // not hooked or no original onmodify continue; -- cgit v1.3 From cd9031935ef2306f7ba2097856b20bf116f341ee Mon Sep 17 00:00:00 2001 From: Ben Fuhrmannek Date: Tue, 19 Apr 2022 19:01:52 +0200 Subject: extended checks for readonly_exec, enabled by default introduced config options: readonly_exec.extended_checks() or xchecks() readonly_exec.no_extended_checks() or noxchecks() --- src/snuffleupagus.c | 2 + src/sp_config.h | 1 + src/sp_config_keywords.c | 7 +- src/sp_execute.c | 77 ++++++++++++++++++---- .../deny_writable_execution_simulation.phpt | 10 ++- 5 files changed, 80 insertions(+), 17 deletions(-) (limited to 'src/snuffleupagus.c') diff --git a/src/snuffleupagus.c b/src/snuffleupagus.c index 5bcd57c..448fd76 100644 --- a/src/snuffleupagus.c +++ b/src/snuffleupagus.c @@ -340,6 +340,7 @@ static void dump_config() { add_assoc_bool(&arr, "readonly_exec.enable", SPCFG(readonly_exec).enable); add_assoc_bool(&arr, "readonly_exec.sim", SPCFG(readonly_exec).simulation); ADD_ASSOC_ZSTR(&arr, SP_TOKEN_READONLY_EXEC "." SP_TOKEN_DUMP, SPCFG(readonly_exec).dump); + add_assoc_bool(&arr, "readonly_exec.extended_checks", SPCFG(readonly_exec).extended_checks); add_assoc_bool(&arr, "global_strict.enable", SPCFG(global_strict).enable); @@ -494,6 +495,7 @@ static PHP_INI_MH(OnUpdateConfiguration) { // set some defaults SPCFG(show_old_php_warning) = true; + SPCFG(readonly_exec).extended_checks = true; char *str = new_value->val; diff --git a/src/sp_config.h b/src/sp_config.h index 3d92f2f..ec73310 100644 --- a/src/sp_config.h +++ b/src/sp_config.h @@ -35,6 +35,7 @@ typedef struct { typedef struct { bool enable; bool simulation; + bool extended_checks; zend_string *dump; zend_string *textual_representation; } sp_config_readonly_exec; diff --git a/src/sp_config_keywords.c b/src/sp_config_keywords.c index f7be731..e0e5166 100644 --- a/src/sp_config_keywords.c +++ b/src/sp_config_keywords.c @@ -96,7 +96,7 @@ SP_PARSE_FN(parse_unserialize) { } SP_PARSE_FN(parse_readonly_exec) { - bool enable = false, disable = false; + bool enable = false, disable = false, xchecks = false, no_xchecks = false; sp_config_readonly_exec *cfg = (sp_config_readonly_exec*)retval; sp_config_keyword config_keywords[] = { @@ -105,6 +105,10 @@ SP_PARSE_FN(parse_readonly_exec) { {parse_empty, SP_TOKEN_SIMULATION, &(cfg->simulation)}, {parse_empty, SP_TOKEN_SIM, &(cfg->simulation)}, {parse_str, SP_TOKEN_DUMP, &(cfg->dump)}, + {parse_empty, "extended_checks", &(xchecks)}, + {parse_empty, "xchecks", &(xchecks)}, + {parse_empty, "no_extended_checks", &(no_xchecks)}, + {parse_empty, "noxchecks", &(no_xchecks)}, {0, 0, 0}}; SP_PROCESS_CONFIG_KEYWORDS_ERR(); @@ -112,6 +116,7 @@ SP_PARSE_FN(parse_readonly_exec) { cfg->textual_representation = sp_get_textual_representation(parsed_rule); SP_SET_ENABLE_DISABLE(enable, disable, cfg->enable); + if (xchecks) { cfg->extended_checks = true; } else if (no_xchecks) { cfg->extended_checks = false; } return SP_PARSER_STOP; } diff --git a/src/sp_execute.c b/src/sp_execute.c index cc401aa..56d25c5 100644 --- a/src/sp_execute.c +++ b/src/sp_execute.c @@ -1,4 +1,5 @@ #include "php_snuffleupagus.h" +#include "ext/standard/php_string.h" static void (*orig_execute_ex)(zend_execute_data *execute_data) = NULL; static void (*orig_zend_execute_internal)(zend_execute_data *execute_data, @@ -12,22 +13,72 @@ static zend_result (*orig_zend_stream_open)(zend_file_handle *handle) = NULL; // FIXME handle symlink ZEND_COLD static inline void terminate_if_writable(const char *filename) { const sp_config_readonly_exec *config_ro_exec = &(SPCFG(readonly_exec)); + char *errmsg = "unknown access problem"; + + // check write access if (0 == access(filename, W_OK)) { - if (config_ro_exec->dump) { - sp_log_request(config_ro_exec->dump, config_ro_exec->textual_representation); - } - if (true == config_ro_exec->simulation) { - sp_log_simulation("readonly_exec", "Attempted execution of a writable file (%s)", filename); - } else { - sp_log_drop("readonly_exec", "Attempted execution of a writable file (%s)", filename); - } + errmsg = "Attempted execution of a writable file"; + goto violation; + } + if (errno != EACCES) { + goto err; + } + + // other checks are 'extended checks' that can be enabled/disabled via config + if (!config_ro_exec->extended_checks) { + return; + } + + // check effective uid + struct stat buf; + if (0 != stat(filename, &buf)) { + goto err; + } + if (buf.st_uid == geteuid()) { + errmsg = "Attempted execution of file owned by process"; + goto violation; + } + + // check write access on directory + char *dirname = estrndup(filename, strlen(filename)); + php_dirname(dirname, strlen(dirname)); + if (0 == access(dirname, W_OK)) { + errmsg = "Attempted execution of file in writable directory"; + efree(dirname); + goto violation; + } + if (errno != EACCES) { + efree(dirname); + goto err; + } + + // check effecite uid of directory + if (0 != stat(dirname, &buf)) { + efree(dirname); + goto err; + } + efree(dirname); + if (buf.st_uid == geteuid()) { + errmsg = "Attempted execution of file in directory owned by process"; + goto violation; + } + + // we would actually need to check all parent directories as well, but that task is left for other tools + return; + +violation: + if (config_ro_exec->dump) { + sp_log_request(config_ro_exec->dump, config_ro_exec->textual_representation); + } + if (config_ro_exec->simulation) { + sp_log_simulation("readonly_exec", "%s (%s)", errmsg, filename); } else { - if (EACCES != errno) { - // LCOV_EXCL_START - sp_log_err("Writable execution", "Error while accessing %s: %s", filename, strerror(errno)); - // LCOV_EXCL_STOP - } + sp_log_drop("readonly_exec", "%s (%s)", errmsg, filename); } + return; + +err: + sp_log_err("readonly_exec", "Error while accessing %s: %s", filename, strerror(errno)); } inline static void is_builtin_matching( diff --git a/src/tests/deny_writable/deny_writable_execution_simulation.phpt b/src/tests/deny_writable/deny_writable_execution_simulation.phpt index e3460e4..abc276f 100644 --- a/src/tests/deny_writable/deny_writable_execution_simulation.phpt +++ b/src/tests/deny_writable/deny_writable_execution_simulation.phpt @@ -41,10 +41,14 @@ unlink("$dir/non_writable_file.txt"); unlink("$dir/writable_file.txt"); ?> --EXPECTF-- -Warning: [snuffleupagus][0.0.0.0][readonly_exec][simulation] Attempted execution of a writable file (%a/deny_writable_execution_simulation.php). in %a/deny_writable_execution_simulation.php on line 2 +Warning: [snuffleupagus][0.0.0.0][readonly_exec][simulation] Attempted execution of a writable file (%a/deny_writable_execution_simulation.php) in %a/deny_writable_execution_simulation.php on line 2 -Warning: [snuffleupagus][0.0.0.0][readonly_exec][simulation] Attempted execution of a writable file (%a/writable_file.txt). in %a/deny_writable_execution_simulation.php on line 12 +Warning: [snuffleupagus][0.0.0.0][readonly_exec][simulation] Attempted execution of a writable file (%a/writable_file.txt) in %a/deny_writable_execution_simulation.php on line 12 -Warning: [snuffleupagus][0.0.0.0][readonly_exec][simulation] Attempted execution of a writable file (%a/writable_file.txt). in %a/writable_file.txt on line 1 +Warning: [snuffleupagus][0.0.0.0][readonly_exec][simulation] Attempted execution of a writable file (%a/writable_file.txt) in %a/writable_file.txt on line 1 Code execution within a writable file. + +Warning: [snuffleupagus][0.0.0.0][readonly_exec][simulation] Attempted execution of file owned by process (%s/tests/deny_writable/non_writable_file.txt) in %s/tests/deny_writable/deny_writable_execution_simulation.php on line 13 + +Warning: [snuffleupagus][0.0.0.0][readonly_exec][simulation] Attempted execution of file owned by process (%s/tests/deny_writable/non_writable_file.txt) in %src/tests/deny_writable/non_writable_file.txt on line 1 Code execution within a non-writable file. -- cgit v1.3 From 08e87202676a4676e66a27625522374faa70704c Mon Sep 17 00:00:00 2001 From: jvoisin Date: Tue, 12 Jul 2022 22:52:13 +0200 Subject: Disable extended checks for readonly_exec by default --- src/snuffleupagus.c | 1 - src/tests/deny_writable/config/config_disable_writable_simulation.ini | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) (limited to 'src/snuffleupagus.c') diff --git a/src/snuffleupagus.c b/src/snuffleupagus.c index 448fd76..1f5b660 100644 --- a/src/snuffleupagus.c +++ b/src/snuffleupagus.c @@ -495,7 +495,6 @@ static PHP_INI_MH(OnUpdateConfiguration) { // set some defaults SPCFG(show_old_php_warning) = true; - SPCFG(readonly_exec).extended_checks = true; char *str = new_value->val; diff --git a/src/tests/deny_writable/config/config_disable_writable_simulation.ini b/src/tests/deny_writable/config/config_disable_writable_simulation.ini index 52a43ba..3aafe3f 100644 --- a/src/tests/deny_writable/config/config_disable_writable_simulation.ini +++ b/src/tests/deny_writable/config/config_disable_writable_simulation.ini @@ -1 +1 @@ - sp.readonly_exec.enable().simulation(); + sp.readonly_exec.enable().extended_checks().simulation(); -- cgit v1.3