From 2b98eea1297a8024650c456fccbc33824721053e Mon Sep 17 00:00:00 2001
From: Roeland Jago Douma <roeland@famdouma.nl>
Date: Mon, 9 Sep 2019 20:39:19 +0200
Subject: [PATCH] Harden identifyproof openssl code

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
---
 .../Security/IdentityProof/Manager.php        | 35 +++++++++++++++----
 lib/private/Server.php                        |  8 -----
 .../Security/IdentityProof/ManagerTest.php    | 21 +++++++----
 3 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/lib/private/Security/IdentityProof/Manager.php b/lib/private/Security/IdentityProof/Manager.php
index fb27f04d87..6db5d4ab2e 100644
--- a/lib/private/Security/IdentityProof/Manager.php
+++ b/lib/private/Security/IdentityProof/Manager.php
@@ -29,6 +29,7 @@ namespace OC\Security\IdentityProof;
 use OC\Files\AppData\Factory;
 use OCP\Files\IAppData;
 use OCP\IConfig;
+use OCP\ILogger;
 use OCP\IUser;
 use OCP\Security\ICrypto;
 
@@ -39,19 +40,18 @@ class Manager {
 	private $crypto;
 	/** @var IConfig */
 	private $config;
+	/** @var ILogger */
+	private $logger;
 
-	/**
-	 * @param Factory $appDataFactory
-	 * @param ICrypto $crypto
-	 * @param IConfig $config
-	 */
 	public function __construct(Factory $appDataFactory,
 								ICrypto $crypto,
-								IConfig $config
+								IConfig $config,
+								ILogger $logger
 	) {
 		$this->appData = $appDataFactory->get('identityproof');
 		$this->crypto = $crypto;
 		$this->config = $config;
+		$this->logger = $logger;
 	}
 
 	/**
@@ -59,6 +59,7 @@ class Manager {
 	 * In a separate function for unit testing purposes.
 	 *
 	 * @return array [$publicKey, $privateKey]
+	 * @throws \RuntimeException
 	 */
 	protected function generateKeyPair(): array {
 		$config = [
@@ -68,7 +69,16 @@ class Manager {
 
 		// Generate new key
 		$res = openssl_pkey_new($config);
-		openssl_pkey_export($res, $privateKey);
+
+		if ($res === false) {
+			$this->logOpensslError();
+			throw new \RuntimeException('OpenSSL reported a problem');
+		}
+
+		if (openssl_pkey_export($res, $privateKey, null, $config) === false) {
+			$this->logOpensslError();
+			throw new \RuntimeException('OpenSSL reported a problem');
+		}
 
 		// Extract the public key from $res to $pubKey
 		$publicKey = openssl_pkey_get_details($res);
@@ -83,6 +93,7 @@ class Manager {
 	 *
 	 * @param string $id key id
 	 * @return Key
+	 * @throws \RuntimeException
 	 */
 	protected function generateKey(string $id): Key {
 		list($publicKey, $privateKey) = $this->generateKeyPair();
@@ -105,6 +116,7 @@ class Manager {
 	 *
 	 * @param string $id
 	 * @return Key
+	 * @throws \RuntimeException
 	 */
 	protected function retrieveKey(string $id): Key {
 		try {
@@ -124,6 +136,7 @@ class Manager {
 	 *
 	 * @param IUser $user
 	 * @return Key
+	 * @throws \RuntimeException
 	 */
 	public function getKey(IUser $user): Key {
 		$uid = $user->getUID();
@@ -144,5 +157,13 @@ class Manager {
 		return $this->retrieveKey('system-' . $instanceId);
 	}
 
+	private function logOpensslError(): void {
+		$errors = [];
+		while ($error = openssl_error_string()) {
+			$errors[] = $error;
+		}
+		$this->logger->critical('Something is wrong with your openssl setup: ' . implode(', ', $errors));
+	}
+
 
 }
diff --git a/lib/private/Server.php b/lib/private/Server.php
index 433ee044fa..b4af17ba28 100644
--- a/lib/private/Server.php
+++ b/lib/private/Server.php
@@ -1186,14 +1186,6 @@ class Server extends ServerContainer implements IServerContainer {
 		$this->registerAlias(IDashboardManager::class, DashboardManager::class);
 		$this->registerAlias(IFullTextSearchManager::class, FullTextSearchManager::class);
 
-		$this->registerService(\OC\Security\IdentityProof\Manager::class, function (Server $c) {
-			return new \OC\Security\IdentityProof\Manager(
-				$c->query(\OC\Files\AppData\Factory::class),
-				$c->getCrypto(),
-				$c->getConfig()
-			);
-		});
-
 		$this->registerAlias(ISubAdmin::class, SubAdmin::class);
 
 		$this->registerAlias(IInitialStateService::class, InitialStateService::class);
diff --git a/tests/lib/Security/IdentityProof/ManagerTest.php b/tests/lib/Security/IdentityProof/ManagerTest.php
index 9d17182e52..2d66845ba8 100644
--- a/tests/lib/Security/IdentityProof/ManagerTest.php
+++ b/tests/lib/Security/IdentityProof/ManagerTest.php
@@ -29,22 +29,26 @@ use OCP\Files\IAppData;
 use OCP\Files\SimpleFS\ISimpleFile;
 use OCP\Files\SimpleFS\ISimpleFolder;
 use OCP\IConfig;
+use OCP\ILogger;
 use OCP\IUser;
 use OCP\Security\ICrypto;
+use PHPUnit\Framework\MockObject\MockObject;
 use SebastianBergmann\Comparator\MockObjectComparator;
 use Test\TestCase;
 
 class ManagerTest extends TestCase  {
-	/** @var Factory|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var Factory|MockObject */
 	private $factory;
-	/** @var IAppData|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var IAppData|MockObject */
 	private $appData;
-	/** @var ICrypto|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var ICrypto|MockObject */
 	private $crypto;
-	/** @var Manager|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var Manager|MockObject */
 	private $manager;
-	/** @var  IConfig|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var IConfig|MockObject */
 	private $config;
+	/** @var ILogger|MockObject */
+	private $logger;
 
 	public function setUp() {
 		parent::setUp();
@@ -57,6 +61,7 @@ class ManagerTest extends TestCase  {
 			->method('get')
 			->with('identityproof')
 			->willReturn($this->appData);
+		$this->logger = $this->createMock(ILogger::class);
 
 		$this->crypto = $this->createMock(ICrypto::class);
 		$this->manager = $this->getManager(['generateKeyPair']);
@@ -73,14 +78,16 @@ class ManagerTest extends TestCase  {
 			return new Manager(
 				$this->factory,
 				$this->crypto,
-				$this->config
+				$this->config,
+				$this->logger
 			);
 		} else {
 			return $this->getMockBuilder(Manager::class)
 				->setConstructorArgs([
 					$this->factory,
 					$this->crypto,
-					$this->config
+					$this->config,
+					$this->logger
 				])->setMethods($setMethods)->getMock();
 		}
 	}