Resolve open_basedir paths on ini update

Closes GH-10987

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.

Zend/tests/gh10469.phpt

@@ -1,23 +1,29 @@
 --TEST--
 GH-10469: Disallow open_basedir() with parent dir components (..)
+--EXTENSIONS--
+zend_test
 --FILE--
 <?php
 ini_set('open_basedir', __DIR__);
 
+$pathSeparator = PHP_OS_FAMILY !== 'Windows' ? ':' : ';';
 $originalDir = __DIR__;
 $tmpDir = $originalDir . '/gh10469_tmp';
 @mkdir($tmpDir, 0777, true);
 chdir($tmpDir);
-ini_set('open_basedir', ini_get('open_basedir') . ':./..');
+ini_set('open_basedir', ini_get('open_basedir') . $pathSeparator . '.' . DIRECTORY_SEPARATOR . '..');
-ini_set('open_basedir', ini_get('open_basedir') . ':./../');
+ini_set('open_basedir', ini_get('open_basedir') . $pathSeparator . '.' . DIRECTORY_SEPARATOR . '..' . DIRECTORY_SEPARATOR);
-ini_set('open_basedir', ini_get('open_basedir') . ':/a/');
+ini_set('open_basedir', ini_get('open_basedir') . $pathSeparator . '.' . DIRECTORY_SEPARATOR . 'a' . DIRECTORY_SEPARATOR);
+ini_set('open_basedir', ini_get('open_basedir') . $pathSeparator . '.' . DIRECTORY_SEPARATOR . 'a');
 
 chdir($originalDir);
 var_dump(ini_get('open_basedir'));
+var_dump(get_open_basedir());
 ?>
 --CLEAN--
 <?php
 @rmdir(__DIR__ . '/gh10469_tmp');
 ?>
 --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"

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();

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

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);

ext/zend_test/test.stub.php

@@ -224,6 +224,8 @@ namespace {
 
     /** @return resource */
     function zend_test_create_throwing_resource() {}
+
+    function get_open_basedir(): ?string {}
 }
 
 namespace ZendTestNS {

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)

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 <pwd.h>
@@ -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 resolved_name[MAXPATHLEN + 1];
-		char *substr_pos = ptr;
+		if (expand_filepath(ptr, resolved_name) == NULL) {
-		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;
-				}
-			}
-		}
-no_parent_dir_component:
-		if (php_check_open_basedir_ex(ptr, 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 (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;
 }

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--

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] ***