From 9c184ddfd0e986af7bb99a45a3c7c8f1bf616035 Mon Sep 17 00:00:00 2001
From: Harald Eilertsen <haraldei@anduin.net>
Date: Sun, 17 Dec 2023 19:30:05 +0100
Subject: Fix deserialization of config values broken by 69266cd6.

This should fix issue #1828.

This patch makes it explicit that we store arrays in the config as json
encoded arrays, while we allow both json encoded and PHP serialized
arrays to be deserialized correctly. Unless it's a brand new install,
the existing data in the database will be PHP serialized.

I've also added a hardening measure in case we fall back to PHP
unserialize, making sure we're not vulnerable to a PHP Object Injection
attack. This means that deserializing arrays containing PHP objects will
no longer work, but afaict we never do that anyways, so I don't think
that should break anything.
---
 Zotlabs/Lib/Config.php        | 21 ++++++++++-----
 tests/unit/Lib/ConfigTest.php | 61 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 6 deletions(-)
 create mode 100644 tests/unit/Lib/ConfigTest.php

diff --git a/Zotlabs/Lib/Config.php b/Zotlabs/Lib/Config.php
index 40d5cc246..fa0abc892 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))) {
@@ -130,11 +130,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');
+	}
+}
-- 
cgit v1.2.3