ext/session: Fix GH-17541 (ext/session NULL pointer dereferencement during ID reset)

Closes GH-17541
Closes GH-17546

Author: Girgias

NEWS

@@ -50,6 +50,8 @@ PHP                                                                        NEWS
 
 - Session:
   . Fix type confusion with session SID constant. (nielsdos)
+  . Fixed bug GH-17541 (ext/session NULL pointer dereferencement during
+    ID reset). (Girgias)
 
 - SimpleXML:
   . Fixed bug GH-17409 (Assertion failure Zend/zend_hash.c:1730). (nielsdos)

ext/session/session.c

@@ -94,6 +94,7 @@ zend_class_entry *php_session_update_timestamp_iface_entry;
 	}
 
 #define SESSION_FORBIDDEN_CHARS "=,;.[ \t\r\n\013\014"
+#define SESSION_FORBIDDEN_CHARS_FOR_ERROR_MSG "=,;.[ \\t\\r\\n\\013\\014"
 
 #define APPLY_TRANS_SID (PS(use_trans_sid) && !PS(use_only_cookies))
 
@@ -683,7 +684,12 @@ static PHP_INI_MH(OnUpdateName) /* {{{ */
 	SESSION_CHECK_OUTPUT_STATE;
 
 	/* Numeric session.name won't work at all */
-	if ((!ZSTR_LEN(new_value) || is_numeric_string(ZSTR_VAL(new_value), ZSTR_LEN(new_value), NULL, NULL, 0))) {
+	if (
+		ZSTR_LEN(new_value) == 0
+		|| zend_str_has_nul_byte(new_value)
+		|| is_numeric_str_function(new_value, NULL, NULL)
+		|| strpbrk(ZSTR_VAL(new_value), SESSION_FORBIDDEN_CHARS) != NULL
+	) {
 		int err_type;
 
 		if (stage == ZEND_INI_STAGE_RUNTIME || stage == ZEND_INI_STAGE_ACTIVATE || stage == ZEND_INI_STAGE_STARTUP) {
@@ -694,7 +700,7 @@ static PHP_INI_MH(OnUpdateName) /* {{{ */
 
 		/* Do not output error when restoring ini options. */
 		if (stage != ZEND_INI_STAGE_DEACTIVATE) {
-			php_error_docref(NULL, err_type, "session.name \"%s\" cannot be numeric or empty", ZSTR_VAL(new_value));
+			php_error_docref(NULL, err_type, "session.name \"%s\" must not be numeric, empty, contain null bytes or any of the following characters \"" SESSION_FORBIDDEN_CHARS_FOR_ERROR_MSG "\"", ZSTR_VAL(new_value));
 		}
 		return FAILURE;
 	}
@@ -1338,11 +1344,7 @@ static zend_result php_session_send_cookie(void) /* {{{ */
 		return FAILURE;
 	}
 
-	/* Prevent broken Set-Cookie header, because the session_name might be user supplied */
-	if (strpbrk(PS(session_name), SESSION_FORBIDDEN_CHARS) != NULL) {   /* man isspace for \013 and \014 */
-		php_error_docref(NULL, E_WARNING, "session.name cannot contain any of the following '=,;.[ \\t\\r\\n\\013\\014'");
-		return FAILURE;
-	}
+	ZEND_ASSERT(strpbrk(PS(session_name), SESSION_FORBIDDEN_CHARS) == NULL);
 
 	/* URL encode id because it might be user supplied */
 	e_id = php_url_encode(ZSTR_VAL(PS(id)), ZSTR_LEN(PS(id)));
@@ -1462,7 +1464,10 @@ PHPAPI zend_result php_session_reset_id(void) /* {{{ */
 	}
 
 	if (PS(use_cookies) && PS(send_cookie)) {
-		php_session_send_cookie();
+		zend_result cookies_sent = php_session_send_cookie();
+		if (UNEXPECTED(cookies_sent == FAILURE)) {
+			return FAILURE;
+		}
 		PS(send_cookie) = 0;
 	}
 

ext/session/tests/bug66481.phpt

@@ -15,6 +15,6 @@ var_dump(session_name("foo"));
 var_dump(session_name("bar"));
 ?>
 --EXPECT--
-Warning: PHP Startup: session.name "" cannot be numeric or empty in Unknown on line 0
+Warning: PHP Startup: session.name "" must not be numeric, empty, contain null bytes or any of the following characters "=,;.[ \t\r\n\013\014" in Unknown on line 0
 string(9) "PHPSESSID"
 string(3) "foo"

ext/session/tests/gh17541.phpt

@@ -0,0 +1,24 @@
+--TEST--
+GH-17541 (ext/session NULL pointer dereferencement during ID reset)
+--EXTENSIONS--
+session
+--SKIPIF--
+<?php include('skipif.inc'); ?>
+--FILE--
+<?php
+function errorHandler($errorNumber, $errorMessage, $fileName, $lineNumber) {
+    // Destroy session while emitting warning from the bogus session name in session_start
+    session_destroy();
+}
+
+set_error_handler('errorHandler');
+
+ob_start();
+var_dump(session_name("\t"));
+var_dump(session_start());
+
+?>
+--EXPECTF--
+Warning: session_destroy(): Trying to destroy uninitialized session in %s on line %d
+string(9) "PHPSESSID"
+bool(true)

ext/session/tests/session_name_variation1.phpt

@@ -38,25 +38,25 @@ ob_end_flush();
 ?>
 --EXPECTF--
 *** Testing session_name() : variation ***
+
+Warning: session_name(): session.name "" must not be numeric, empty, contain null bytes or any of the following characters "=,;.[ \t\r\n\013\014" in %s on line %d
 string(9) "PHPSESSID"
 bool(true)
 string(9) "PHPSESSID"
 bool(true)
 string(9) "PHPSESSID"
-string(9) "PHPSESSID"
 
-Warning: session_start(): session.name cannot contain any of the following '=,;.[ \t\r\n\013\014' in %s on line %d
+Warning: session_name(): session.name "	" must not be numeric, empty, contain null bytes or any of the following characters "=,;.[ \t\r\n\013\014" in %s on line %d
+string(9) "PHPSESSID"
 bool(true)
-string(1) "	"
+string(9) "PHPSESSID"
 bool(true)
-string(1) "	"
-
-Warning: session_name(): session.name "" cannot be numeric or empty in %s on line %d
-string(1) "	"
+string(9) "PHPSESSID"
 
-Warning: session_start(): session.name cannot contain any of the following '=,;.[ \t\r\n\013\014' in %s on line %d
+Warning: session_name(): session.name "" must not be numeric, empty, contain null bytes or any of the following characters "=,;.[ \t\r\n\013\014" in %s on line %d
+string(9) "PHPSESSID"
 bool(true)
-string(1) "	"
+string(9) "PHPSESSID"
 bool(true)
-string(1) "	"
+string(9) "PHPSESSID"
 Done