Merge branch 'fix-config-deserialization' into 'dev'

Fix deserialization of config values broken by 69266cd6.

See merge request hubzilla/core!2077

2 files changed, 76 insertions, 6 deletions

diff --git a/Zotlabs/Lib/Config.php b/Zotlabs/Lib/Config.php
index 3d34c8497..b1754df09 100644
--- a/Zotlabs/Lib/Config.php
+++ b/Zotlabs/Lib/Config.php
@@ -72,7 +72,7 @@ class Config {
 	 */
 	public static function Set($family, $key, $value) {
 		// manage array value
-		$dbvalue = ((is_array($value))  ? serialise($value) : $value);
+		$dbvalue = ((is_array($value))  ? 'json:' . json_encode($value) : $value);
 		$dbvalue = ((is_bool($dbvalue)) ? intval($dbvalue)  : $dbvalue);
 
 		if (self::Get($family, $key) === false || (! self::get_from_storage($family, $key))) {
@@ -136,11 +136,20 @@ class Config {
 				return $default;
 			}
 
-			return ((! is_array(App::$config[$family][$key])) && (preg_match('|^a:[0-9]+:{.*}$|s', App::$config[$family][$key]))
-				? unserialize(App::$config[$family][$key])
-				: App::$config[$family][$key]
-			);
-
+			$value = App::$config[$family][$key];
+
+			if (! is_array($value)) {
+				if (substr($value, 0, 5) == 'json:') {
+					return json_decode(substr($value, 5), true);
+				} else if (preg_match('|^a:[0-9]+:{.*}$|s', $value)) {
+					// Unserialize in inherently unsafe. Try to mitigate by not
+					// allowing unserializing objects. Only kept for backwards
+					// compatibility. JSON serialization should be prefered.
+					return unserialize($value, array('allowed_classes' => false));
+				} else {
+					return $value;
+				}
+			}
 		}
 
 		return $default;
diff --git a/tests/unit/Lib/ConfigTest.php b/tests/unit/Lib/ConfigTest.php
new file mode 100644
index 000000000..a8ae3631b
--- /dev/null
+++ b/tests/unit/Lib/ConfigTest.php
@@ -0,0 +1,61 @@
+<?php
+declare(strict_types=1);
+
+/**
+ * Tests for the Zotlabs\Lib\Config class.
+ *
+ * Until we have database testing in place, we can only test the Congig::Get
+ * method for now. This should be improved once the database test framework is
+ * merged.
+ */
+class ConfigTest extends Zotlabs\Tests\Unit\UnitTestCase {
+	/*
+	 * Hardcode a config that we can test against, and that we can
+	 * reuse in all the test cases.
+	 */
+	public function setUp(): void {
+		\App::$config = array(
+			'test' => array (
+				'plain' => 'plain value',
+				'php-array' => 'a:3:{i:0;s:3:"one";i:1;s:3:"two";i:2;s:5:"three";}',
+				'json-array' => 'json:["one","two","three"]',
+				'object-injection' => 'a:1:{i:0;O:18:"Zotlabs\Lib\Config":0:{}}',
+				'config_loaded' => true,
+			),
+		);
+	}
+
+	public function testGetPlainTextValue(): void {
+		$this->assertEquals(
+			Zotlabs\Lib\Config::Get('test', 'plain'),
+			'plain value'
+		);
+	}
+
+	public function testGetJSONSerializedArray(): void {
+		$this->assertEquals(
+			Zotlabs\Lib\Config::Get('test', 'json-array'),
+			array('one', 'two', 'three')
+		);
+	}
+
+	/*
+	 * Test that we can retreive old style serialized arrays that were
+	 * serialized with th PHP `serialize()` function.
+	 */
+	public function testGetPHPSerializedArray(): void {
+		$this->assertEquals(
+			Zotlabs\Lib\Config::Get('test', 'php-array'),
+			array('one', 'two', 'three')
+		);
+	}
+
+	/*
+	 * Make sure we're not vulnerable to PHP Object injection attacks when
+	 * using the PHP `unserialize()` function.
+	 */
+	public function testGetMaliciousPHPSerializedArray(): void {
+		$value = Zotlabs\Lib\Config::Get('test', 'object-injection');
+		$this->assertEquals($value[0]::class, '__PHP_Incomplete_Class');
+	}
+}