diff --git a/NEWS b/NEWS index 11b5deee91d5c..e577e05e1dc25 100644 --- a/NEWS +++ b/NEWS @@ -18,6 +18,7 @@ PHP NEWS . Expose time spent collecting cycles in gc_status(). (Arnaud) . Remove WeakMap entries whose key is only reachable through the entry value. (Arnaud) + . Resolve open_basedir paths on INI update. (ilutov) - Curl: . Added Curl options and constants up to (including) version 7.87. diff --git a/Zend/tests/gh10469.phpt b/Zend/tests/gh10469.phpt index 7a47afacdca98..8176d7008d7ab 100644 --- a/Zend/tests/gh10469.phpt +++ b/Zend/tests/gh10469.phpt @@ -1,23 +1,29 @@ --TEST-- GH-10469: Disallow open_basedir() with parent dir components (..) +--EXTENSIONS-- +zend_test --FILE-- --CLEAN-- --EXPECTF-- -string(%d) "%stests" +string(%d) "%stests%c.%e..%c.%e..%e%c.%ea%e%c.%ea" +string(%d) "%stests%c%stests%c%stests%e%c%stests%egh10469_tmp%ea" diff --git a/Zend/zend.c b/Zend/zend.c index 54e9a85808cb9..48a7bef867b1c 100644 --- a/Zend/zend.c +++ b/Zend/zend.c @@ -39,6 +39,11 @@ #include "zend_max_execution_timer.h" #include "zend_hrtime.h" #include "Optimizer/zend_optimizer.h" +#include "php.h" +#include "php_globals.h" + +// FIXME: Breaks the declaration of the function below +#undef zenderror static size_t global_map_ptr_last = 0; static bool startup_done = false; @@ -1266,11 +1271,29 @@ void zend_call_destructors(void) /* {{{ */ } /* }}} */ +static void zend_release_open_basedir(void) +{ + /* Release custom open_basedir config, this needs to happen before ini shutdown */ + if (PG(open_basedir)) { + zend_ini_entry *ini_entry = zend_hash_str_find_ptr(EG(ini_directives), "open_basedir", strlen("open_basedir")); + /* ini_entry->modified is unreliable, it might also be set when on_update has failed. */ + if (ini_entry + && ini_entry->modified + && ini_entry->value != ini_entry->orig_value) { + efree(PG(open_basedir)); + PG(open_basedir) = NULL; + } + } +} + ZEND_API void zend_deactivate(void) /* {{{ */ { /* we're no longer executing anything */ EG(current_execute_data) = NULL; + /* Needs to run before zend_ini_deactivate(). */ + zend_release_open_basedir(); + zend_try { shutdown_scanner(); } zend_end_try(); diff --git a/ext/session/tests/session_save_path_variation5.phpt b/ext/session/tests/session_save_path_variation5.phpt index 1ea8afc66e3df..29b924b5ca11b 100644 --- a/ext/session/tests/session_save_path_variation5.phpt +++ b/ext/session/tests/session_save_path_variation5.phpt @@ -28,6 +28,7 @@ if (file_exists($sessions) === TRUE) { var_dump(mkdir($sessions)); var_dump(chdir($sessions)); +ini_set('open_basedir', '.'); ini_set("session.save_path", $directory); var_dump(session_save_path()); @@ -45,6 +46,6 @@ rmdir($sessions); bool(true) bool(true) -Warning: ini_set(): open_basedir restriction in effect. File(%s) is not within the allowed path(s): (.) in %s on line %d +Warning: ini_set(): open_basedir restriction in effect. File(%s) is not within the allowed path(s): (%ssession_save_path_variation5) in %s on line %d string(0) "" Done diff --git a/ext/zend_test/test.c b/ext/zend_test/test.c index a8bddaac14fa3..cf6005ea083fa 100644 --- a/ext/zend_test/test.c +++ b/ext/zend_test/test.c @@ -620,6 +620,12 @@ static ZEND_FUNCTION(zend_test_fill_packed_array) } ZEND_HASH_FILL_END(); } +static ZEND_FUNCTION(get_open_basedir) +{ + ZEND_PARSE_PARAMETERS_NONE(); + RETURN_STRING(PG(open_basedir)); +} + static zend_object *zend_test_class_new(zend_class_entry *class_type) { zend_object *obj = zend_objects_new(class_type); diff --git a/ext/zend_test/test.stub.php b/ext/zend_test/test.stub.php index 62bdcb8bd52bf..6da31580d06d5 100644 --- a/ext/zend_test/test.stub.php +++ b/ext/zend_test/test.stub.php @@ -224,6 +224,8 @@ function zend_test_fill_packed_array(array &$array): void {} /** @return resource */ function zend_test_create_throwing_resource() {} + + function get_open_basedir(): ?string {} } namespace ZendTestNS { diff --git a/ext/zend_test/test_arginfo.h b/ext/zend_test/test_arginfo.h index c47250ff27397..575044bd2b4a6 100644 --- a/ext/zend_test/test_arginfo.h +++ b/ext/zend_test/test_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: 3c1b17bbb7ef84e036a251c685bd7fd79fe9f434 */ + * Stub hash: 129cbff2439b78d38435088d97607fe9ed679b13 */ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_zend_test_array_return, 0, 0, IS_ARRAY, 0) ZEND_END_ARG_INFO() @@ -133,6 +133,9 @@ ZEND_END_ARG_INFO() ZEND_BEGIN_ARG_INFO_EX(arginfo_zend_test_create_throwing_resource, 0, 0, 0) ZEND_END_ARG_INFO() +ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_get_open_basedir, 0, 0, IS_STRING, 1) +ZEND_END_ARG_INFO() + ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_ZendTestNS2_namespaced_func, 0, 0, _IS_BOOL, 0) ZEND_END_ARG_INFO() @@ -251,6 +254,7 @@ static ZEND_FUNCTION(zend_get_map_ptr_last); static ZEND_FUNCTION(zend_test_crash); static ZEND_FUNCTION(zend_test_fill_packed_array); static ZEND_FUNCTION(zend_test_create_throwing_resource); +static ZEND_FUNCTION(get_open_basedir); static ZEND_FUNCTION(ZendTestNS2_namespaced_func); static ZEND_FUNCTION(ZendTestNS2_namespaced_deprecated_func); static ZEND_FUNCTION(ZendTestNS2_ZendSubNS_namespaced_func); @@ -319,6 +323,7 @@ static const zend_function_entry ext_functions[] = { ZEND_FE(zend_test_crash, arginfo_zend_test_crash) ZEND_FE(zend_test_fill_packed_array, arginfo_zend_test_fill_packed_array) ZEND_FE(zend_test_create_throwing_resource, arginfo_zend_test_create_throwing_resource) + ZEND_FE(get_open_basedir, arginfo_get_open_basedir) ZEND_NS_FALIAS("ZendTestNS2", namespaced_func, ZendTestNS2_namespaced_func, arginfo_ZendTestNS2_namespaced_func) ZEND_NS_DEP_FALIAS("ZendTestNS2", namespaced_deprecated_func, ZendTestNS2_namespaced_deprecated_func, arginfo_ZendTestNS2_namespaced_deprecated_func) ZEND_NS_FALIAS("ZendTestNS2", namespaced_aliased_func, zend_test_void_return, arginfo_ZendTestNS2_namespaced_aliased_func) diff --git a/main/fopen_wrappers.c b/main/fopen_wrappers.c index ead11a958b32a..bcc7f6740cc99 100644 --- a/main/fopen_wrappers.c +++ b/main/fopen_wrappers.c @@ -38,6 +38,7 @@ #include "ext/standard/php_standard.h" #include "zend_compile.h" #include "php_network.h" +#include "zend_smart_str.h" #if HAVE_PWD_H #include @@ -81,19 +82,13 @@ PHPAPI ZEND_INI_MH(OnUpdateBaseDir) return SUCCESS; } - /* Otherwise we're in runtime */ - if (!*p || !**p) { - /* open_basedir not set yet, go ahead and give it a value */ - *p = ZSTR_VAL(new_value); - return SUCCESS; - } - /* Shortcut: When we have a open_basedir and someone tries to unset, we know it'll fail */ if (!new_value || !*ZSTR_VAL(new_value)) { return FAILURE; } /* Is the proposed open_basedir at least as restrictive as the current setting? */ + smart_str buf = {0}; ptr = pathbuf = estrdup(ZSTR_VAL(new_value)); while (ptr && *ptr) { end = strchr(ptr, DEFAULT_DIR_SEPARATOR); @@ -101,40 +96,37 @@ PHPAPI ZEND_INI_MH(OnUpdateBaseDir) *end = '\0'; end++; } - /* Don't allow paths with a parent dir component (..) to be set at runtime */ - char *substr_pos = ptr; - while (*substr_pos) { - // Check if we have a .. path component - if (substr_pos[0] == '.' - && substr_pos[1] == '.' - && (substr_pos[2] == '\0' || IS_SLASH(substr_pos[2]))) { - efree(pathbuf); - return FAILURE; - } - // Skip to the next path component - while (true) { - substr_pos++; - if (*substr_pos == '\0' || *substr_pos == DEFAULT_DIR_SEPARATOR) { - goto no_parent_dir_component; - } else if (IS_SLASH(*substr_pos)) { - // Also skip the slash - substr_pos++; - break; - } - } + char resolved_name[MAXPATHLEN + 1]; + if (expand_filepath(ptr, resolved_name) == NULL) { + efree(pathbuf); + smart_str_free(&buf); + return FAILURE; } -no_parent_dir_component: - if (php_check_open_basedir_ex(ptr, 0) != 0) { + if (php_check_open_basedir_ex(resolved_name, 0) != 0) { /* At least one portion of this open_basedir is less restrictive than the prior one, FAIL */ efree(pathbuf); + smart_str_free(&buf); return FAILURE; } + if (smart_str_get_len(&buf) != 0) { + smart_str_appendc(&buf, DEFAULT_DIR_SEPARATOR); + } + smart_str_appends(&buf, resolved_name); ptr = end; } efree(pathbuf); /* Everything checks out, set it */ - *p = ZSTR_VAL(new_value); + if (*p) { + /* Unfortunately entry->modified has already been set to true so we compare entry->value + * against entry->orig_value. */ + if (entry->modified && entry->value != entry->orig_value) { + efree(*p); + } + } + zend_string *tmp = smart_str_extract(&buf); + *p = estrdup(ZSTR_VAL(tmp)); + zend_string_release(tmp); return SUCCESS; } diff --git a/tests/security/bug76359.phpt b/tests/security/bug76359.phpt index df35d67d097da..5e8b8a56e703e 100644 --- a/tests/security/bug76359.phpt +++ b/tests/security/bug76359.phpt @@ -10,7 +10,7 @@ chdir(".."); chdir(".."); ?> --EXPECTF-- -bool(false) +string(%d) "%ssecurity" Warning: chdir(): open_basedir restriction in effect. File(..) is not within the allowed path(s): (%s) in %s on line %d --CLEAN-- diff --git a/tests/security/open_basedir_readlink.phpt b/tests/security/open_basedir_readlink.phpt index 00d2e8c61f59a..34a0f18f4bf80 100644 --- a/tests/security/open_basedir_readlink.phpt +++ b/tests/security/open_basedir_readlink.phpt @@ -21,6 +21,7 @@ $symlink = ($initdir."/test/ok/symlink.txt"); var_dump(symlink($target, $symlink)); chdir($initdir."/test/ok"); +ini_set("open_basedir", "."); var_dump(readlink("symlink.txt")); var_dump(readlink("../ok/symlink.txt")); @@ -49,24 +50,24 @@ bool(true) bool(true) bool(true) -Warning: readlink(): open_basedir restriction in effect. File(symlink.txt) is not within the allowed path(s): (.) in %s on line %d +Warning: readlink(): open_basedir restriction in effect. File(symlink.txt) is not within the allowed path(s): (%sok) in %s on line %d bool(false) -Warning: readlink(): open_basedir restriction in effect. File(../ok/symlink.txt) is not within the allowed path(s): (.) in %s on line %d +Warning: readlink(): open_basedir restriction in effect. File(../ok/symlink.txt) is not within the allowed path(s): (%sok) in %s on line %d bool(false) -Warning: readlink(): open_basedir restriction in effect. File(../ok/./symlink.txt) is not within the allowed path(s): (.) in %s on line %d +Warning: readlink(): open_basedir restriction in effect. File(../ok/./symlink.txt) is not within the allowed path(s): (%sok) in %s on line %d bool(false) -Warning: readlink(): open_basedir restriction in effect. File(./symlink.txt) is not within the allowed path(s): (.) in %s on line %d +Warning: readlink(): open_basedir restriction in effect. File(./symlink.txt) is not within the allowed path(s): (%sok) in %s on line %d bool(false) -Warning: readlink(): open_basedir restriction in effect. File(%s/test/ok/symlink.txt) is not within the allowed path(s): (.) in %s on line %d +Warning: readlink(): open_basedir restriction in effect. File(%s/test/ok/symlink.txt) is not within the allowed path(s): (%sok) in %s on line %d bool(false) -Warning: symlink(): open_basedir restriction in effect. File(%s/test/bad/bad.txt) is not within the allowed path(s): (.) in %s on line %d +Warning: symlink(): open_basedir restriction in effect. File(%s/test/bad/bad.txt) is not within the allowed path(s): (%sok) in %s on line %d bool(false) -Warning: readlink(): open_basedir restriction in effect. File(%s/test/ok/symlink.txt) is not within the allowed path(s): (.) in %s on line %d +Warning: readlink(): open_basedir restriction in effect. File(%s/test/ok/symlink.txt) is not within the allowed path(s): (%sok) in %s on line %d bool(false) *** Finished testing open_basedir configuration [readlink] ***