Resolve open_basedir paths on ini update

iluuu1994 · iluuu1994 · commit 9bcdf219ec6e · 2023-07-18T14:43:40.000+02:00
Closes GH-10987
diff --git 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
@@ -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') . ':./../');
-ini_set('open_basedir', ini_get('open_basedir') . ':/a/');
+ini_set('open_basedir', ini_get('open_basedir') . $pathSeparator . '.' . DIRECTORY_SEPARATOR . '..');
+ini_set('open_basedir', ini_get('open_basedir') . $pathSeparator . '.' . DIRECTORY_SEPARATOR . '..' . DIRECTORY_SEPARATOR);
+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"
diff --git 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
@@ -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
@@ -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
@@ -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
diff --git 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 <pwd.h>
@@ -81,60 +82,51 @@ 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);
 		if (end != NULL) {
 			*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
@@ -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
@@ -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] ***

Original file line number	Diff line number	Diff line change
`@@ -620,6 +620,12 @@ static ZEND_FUNCTION(zend_test_fill_packed_array)`
`620`	`620`	`} ZEND_HASH_FILL_END();`
`621`	`621`	`}`
`622`	`622`
	`623`	`+static ZEND_FUNCTION(get_open_basedir)`
	`624`	`+{`
	`625`	`+ ZEND_PARSE_PARAMETERS_NONE();`
	`626`	`+ RETURN_STRING(PG(open_basedir));`
	`627`	`+}`
	`628`	`+`
`623`	`629`	`static zend_object zend_test_class_new(zend_class_entry class_type)`
`624`	`630`	`{`
`625`	`631`	`zend_object *obj = zend_objects_new(class_type);`
Original file line number	Diff line number	Diff line change
`@@ -224,6 +224,8 @@ function zend_test_fill_packed_array(array &$array): void {}`
`224`	`224`
`225`	`225`	`/** @return resource */`
`226`	`226`	`function zend_test_create_throwing_resource() {}`
	`227`	`+`
	`228`	`+ function get_open_basedir(): ?string {}`
`227`	`229`	`}`
`228`	`230`
`229`	`231`	`namespace ZendTestNS {`