unserialize: Strictly check for :{ at object start (#10214)

* unserialize: Strictly check for `:{` at object start

* unserialize: Update CVE tests

It's unlikely that the object syntax error contributed to the actual CVE. The
CVE is rather caused by the incorrect object serialization data of the `C`
format. Add a second string without such a syntax error to ensure that path is
still executed as well to ensure the CVE is absent.

* Fix test expectation in gmp/tests/bug74670.phpt

No changes to the input required, because the test actually is intended to
verify the behavior for a missing `}`, it's just that the report position changed.

* NEWS

* UPGRADING

NEWS

@@ -38,6 +38,8 @@ PHP                                                                        NEWS
 
 - Standard:
   . Fix GH-10187 (Segfault in stripslashes() with arm64). (nielsdos)
+  . Fixed bug GH-10214 (Incomplete validation of object syntax during
+    unserialize()). (timwolla)
 
 05 Jan 2023, PHP 8.2.1
 

UPGRADING

@@ -223,6 +223,10 @@ PHP 8.2 UPGRADE NOTES
     widened to iterable from Iterator, allowing arrays to be passed.
     RFC: https://wiki.php.net/rfc/iterator_xyz_accept_array
 
+- Standard
+  . unserialize() now performs a stricter validation of the structure of serialized
+    objects.
+
 - XML
   . xml_parser_set_option() now actually returns false when attempting to set a
     negative tag start. Previously it returned true while emitting an E_WARNING.

ext/gmp/tests/bug74670.phpt

@@ -8,5 +8,5 @@ $str = 'C:3:"GMP":4:{s:6666666666:""}';
 var_dump(unserialize($str));
 ?>
 --EXPECTF--
-Notice: unserialize(): Error at offset 13 of 29 bytes in %s on line %d
+Notice: unserialize(): Error at offset 17 of 29 bytes in %s on line %d
 bool(false)

ext/spl/tests/bug73029.phpt

@@ -3,6 +3,13 @@ Bug #73029: Missing type check when unserializing SplArray
 --FILE--
 <?php
 try {
+$a = 'C:11:"ArrayObject":19:{x:i:0;r:2;;m:a:0:{}}';
+$m = unserialize($a);
+$x = $m[2];
+} catch(UnexpectedValueException $e) {
+    print $e->getMessage() . "\n";
+}
+try {
 $a = 'C:11:"ArrayObject":19:0x:i:0;r:2;;m:a:0:{}}';
 $m = unserialize($a);
 $x = $m[2];
@@ -11,6 +18,10 @@ $x = $m[2];
 }
 ?>
 DONE
---EXPECT--
+--EXPECTF--
 Error at offset 10 of 19 bytes
+
+Notice: unserialize(): Error at offset 22 of 43 bytes in %s on line %d
+
+Warning: Trying to access array offset on value of type bool in %s on line %d
 DONE

ext/standard/tests/serialize/bug73341.phpt

@@ -2,6 +2,13 @@
 Bug #73144 (Use-afte-free in ArrayObject Deserialization)
 --FILE--
 <?php
+try {
+$token = 'a:2:{i:0;O:1:"0":2:{s:1:"0";i:0;s:1:"0";a:1:{i:0;C:11:"ArrayObject":7:{x:i:0;r}';
+$obj = unserialize($token);
+} catch(Exception $e) {
+    echo $e->getMessage()."\n";
+}
+
 try {
 $token = 'a:2:{i:0;O:1:"0":2:0s:1:"0";i:0;s:1:"0";a:1:{i:0;C:11:"ArrayObject":7:{x:i:0;r}';
 $obj = unserialize($token);
@@ -20,5 +27,7 @@ unserialize($exploit);
 --EXPECTF--
 Error at offset 6 of 7 bytes
 
+Notice: unserialize(): Error at offset 19 of 79 bytes in %s on line %d
+
 Notice: ArrayObject::unserialize(): Unexpected end of serialized data in %sbug73341.php on line %d
 Error at offset 24 of 34 bytes

ext/standard/tests/serialize/bug74111.phpt

@@ -6,5 +6,5 @@ $s = 'O:8:"stdClass":00000000';
 var_dump(unserialize($s));
 ?>
 --EXPECTF--
-Notice: unserialize(): Error at offset 25 of 23 bytes in %s on line %d
+Notice: unserialize(): Error at offset 23 of 23 bytes in %s on line %d
 bool(false)

ext/standard/tests/serialize/serialization_objects_017.phpt

@@ -0,0 +1,17 @@
+--TEST--
+Object serialization / unserialization: Strict format
+--FILE--
+<?php
+class A {public $a;}
+
+var_dump(unserialize('O:1:"A":1x{s:1:"a";N;}'));
+                    //0123456789012345678901
+var_dump(unserialize('O:1:"A":1:xs:1:"a";N;}'));
+                    //0123456789012345678901
+?>
+--EXPECTF--
+Notice: unserialize(): Error at offset 9 of 22 bytes in %s on line %d
+bool(false)
+
+Notice: unserialize(): Error at offset 10 of 22 bytes in %s on line %d
+bool(false)

ext/standard/tests/serialize/serialization_objects_018.phpt

@@ -0,0 +1,33 @@
+--TEST--
+Object serialization / unserialization: Strict format (2)
+--FILE--
+<?php
+class A implements Serializable {
+	public function serialize() {}
+	public function unserialize($data) {}
+	public function __serialize() {}
+	public function __unserialize($data) {}
+}
+
+var_dump(unserialize('C:1:"A":3x{foo}'));
+                    //012345678901234
+var_dump(unserialize('C:1:"A":3:xfoo}'));
+                    //012345678901234
+var_dump(unserialize('C:1:"A":3:{foox'));
+                    //012345678901234
+var_dump(unserialize('C:1:"A":'));
+                    //01234567
+
+?>
+--EXPECTF--
+Notice: unserialize(): Error at offset 9 of 15 bytes in %s on line %d
+bool(false)
+
+Notice: unserialize(): Error at offset 10 of 15 bytes in %s on line %d
+bool(false)
+
+Notice: unserialize(): Error at offset 14 of 15 bytes in %s on line %d
+bool(false)
+
+Notice: unserialize(): Error at offset 8 of 8 bytes in %s on line %d
+bool(false)

ext/standard/var_unserializer.re

@@ -743,6 +743,19 @@ static inline int object_custom(UNSERIALIZE_PARAMETER, zend_class_entry *ce)
 
 	datalen = parse_iv2((*p) + 2, p);
 
+	if (max - (*p) < 2) {
+		return 0;
+	}
+
+	if ((*p)[0] != ':') {
+		return 0;
+	}
+
+	if ((*p)[1] != '{') {
+		(*p) += 1;
+		return 0;
+	}
+
 	(*p) += 2;
 
 	if (datalen < 0 || (max - (*p)) <= datalen) {
@@ -754,6 +767,7 @@ static inline int object_custom(UNSERIALIZE_PARAMETER, zend_class_entry *ce)
 	 * with unserialize reading past the end of the passed buffer if the string is not
 	 * appropriately terminated (usually NUL terminated, but '}' is also sufficient.) */
 	if ((*p)[datalen] != '}') {
+		(*p) += datalen;
 		return 0;
 	}
 
@@ -1293,6 +1307,16 @@ object ":" uiv ":" ["]	{
 		return 0;
 	}
 
+	YYCURSOR = *p;
+
+	if (*(YYCURSOR) != ':') {
+		return 0;
+	}
+	if (*(YYCURSOR+1) != '{') {
+		*p = YYCURSOR+1;
+		return 0;
+	}
+
 	*p += 2;
 
 	has_unserialize = !incomplete_class && ce->__unserialize;