Merge pull request #10840 from webfoersterei/refactor/5530-urandom-check

Refactor secure randomness check

core/js/setupchecks.js

@@ -183,10 +183,10 @@
 							type: OC.SetupChecks.MESSAGE_TYPE_INFO
 						});
 					}
-					if(!data.isUrandomAvailable) {
+					if(!data.isRandomnessSecure) {
 						messages.push({
-							msg: t('core', '/dev/urandom is not readable by PHP which is highly discouraged for security reasons. Further information can be found in the <a target="_blank" rel="noreferrer noopener" href="{docLink}">documentation</a>.', {docLink: data.securityDocs}),
-							type: OC.SetupChecks.MESSAGE_TYPE_WARNING
+							msg: t('core', 'No suitable source for randomness found by PHP which is highly discouraged for security reasons. Further information can be found in the <a target="_blank" rel="noreferrer noopener" href="{docLink}">documentation</a>.', {docLink: data.securityDocs}),
+							type: OC.SetupChecks.MESSAGE_TYPE_ERROR
 						});
 					}
 					if(data.isUsedTlsLibOutdated) {

core/js/tests/specs/setupchecksSpec.js

@@ -155,7 +155,7 @@ describe('OC.SetupChecks tests', function() {
 					hasWorkingFileLocking: true,
 					hasValidTransactionIsolationLevel: true,
 					suggestedOverwriteCliURL: '',
-					isUrandomAvailable: true,
+					isRandomnessSecure: true,
 					serverHasInternetConnection: false,
 					memcacheDocs: 'https://docs.nextcloud.com/server/go.php?to=admin-performance',
 					forwardedForHeadersWorking: true,
@@ -204,7 +204,7 @@ describe('OC.SetupChecks tests', function() {
 					hasWorkingFileLocking: true,
 					hasValidTransactionIsolationLevel: true,
 					suggestedOverwriteCliURL: '',
-					isUrandomAvailable: true,
+					isRandomnessSecure: true,
 					serverHasInternetConnection: false,
 					memcacheDocs: 'https://docs.nextcloud.com/server/go.php?to=admin-performance',
 					forwardedForHeadersWorking: true,
@@ -254,7 +254,7 @@ describe('OC.SetupChecks tests', function() {
 					hasWorkingFileLocking: true,
 					hasValidTransactionIsolationLevel: true,
 					suggestedOverwriteCliURL: '',
-					isUrandomAvailable: true,
+					isRandomnessSecure: true,
 					serverHasInternetConnection: false,
 					isMemcacheConfigured: true,
 					forwardedForHeadersWorking: true,
@@ -301,7 +301,7 @@ describe('OC.SetupChecks tests', function() {
 					hasWorkingFileLocking: true,
 					hasValidTransactionIsolationLevel: true,
 					suggestedOverwriteCliURL: '',
-					isUrandomAvailable: false,
+					isRandomnessSecure: false,
 					securityDocs: 'https://docs.owncloud.org/myDocs.html',
 					serverHasInternetConnection: true,
 					isMemcacheConfigured: true,
@@ -325,8 +325,8 @@ describe('OC.SetupChecks tests', function() {
 
 			async.done(function( data, s, x ){
 				expect(data).toEqual([{
-					msg: '/dev/urandom is not readable by PHP which is highly discouraged for security reasons. Further information can be found in the <a href="https://docs.owncloud.org/myDocs.html" rel="noreferrer noopener">documentation</a>.',
-					type: OC.SetupChecks.MESSAGE_TYPE_WARNING
+					msg: 'No suitable source for randomness found by PHP which is highly discouraged for security reasons. Further information can be found in the <a href="https://docs.owncloud.org/myDocs.html" rel="noreferrer noopener">documentation</a>.',
+					type: OC.SetupChecks.MESSAGE_TYPE_ERROR
 				}]);
 				done();
 			});
@@ -347,7 +347,7 @@ describe('OC.SetupChecks tests', function() {
 					hasWorkingFileLocking: true,
 					hasValidTransactionIsolationLevel: true,
 					suggestedOverwriteCliURL: '',
-					isUrandomAvailable: true,
+					isRandomnessSecure: true,
 					securityDocs: 'https://docs.owncloud.org/myDocs.html',
 					serverHasInternetConnection: true,
 					isMemcacheConfigured: true,
@@ -393,7 +393,7 @@ describe('OC.SetupChecks tests', function() {
 					hasWorkingFileLocking: true,
 					hasValidTransactionIsolationLevel: true,
 					suggestedOverwriteCliURL: '',
-					isUrandomAvailable: true,
+					isRandomnessSecure: true,
 					securityDocs: 'https://docs.owncloud.org/myDocs.html',
 					serverHasInternetConnection: true,
 					isMemcacheConfigured: true,
@@ -441,7 +441,7 @@ describe('OC.SetupChecks tests', function() {
 					hasWorkingFileLocking: true,
 					hasValidTransactionIsolationLevel: true,
 					suggestedOverwriteCliURL: '',
-					isUrandomAvailable: true,
+					isRandomnessSecure: true,
 					serverHasInternetConnection: true,
 					isMemcacheConfigured: true,
 					forwardedForHeadersWorking: false,
@@ -487,7 +487,7 @@ describe('OC.SetupChecks tests', function() {
 					hasWorkingFileLocking: true,
 					hasValidTransactionIsolationLevel: true,
 					suggestedOverwriteCliURL: '',
-					isUrandomAvailable: true,
+					isRandomnessSecure: true,
 					serverHasInternetConnection: true,
 					isMemcacheConfigured: true,
 					forwardedForHeadersWorking: true,
@@ -533,7 +533,7 @@ describe('OC.SetupChecks tests', function() {
 					hasWorkingFileLocking: true,
 					hasValidTransactionIsolationLevel: true,
 					suggestedOverwriteCliURL: '',
-					isUrandomAvailable: true,
+					isRandomnessSecure: true,
 					serverHasInternetConnection: true,
 					isMemcacheConfigured: true,
 					forwardedForHeadersWorking: true,
@@ -599,7 +599,7 @@ describe('OC.SetupChecks tests', function() {
 					hasWorkingFileLocking: true,
 					hasValidTransactionIsolationLevel: true,
 					suggestedOverwriteCliURL: '',
-					isUrandomAvailable: true,
+					isRandomnessSecure: true,
 					securityDocs: 'https://docs.owncloud.org/myDocs.html',
 					serverHasInternetConnection: true,
 					isMemcacheConfigured: true,
@@ -646,7 +646,7 @@ describe('OC.SetupChecks tests', function() {
 					hasWorkingFileLocking: true,
 					hasValidTransactionIsolationLevel: true,
 					suggestedOverwriteCliURL: '',
-					isUrandomAvailable: true,
+					isRandomnessSecure: true,
 					securityDocs: 'https://docs.owncloud.org/myDocs.html',
 					serverHasInternetConnection: true,
 					isMemcacheConfigured: true,
@@ -693,7 +693,7 @@ describe('OC.SetupChecks tests', function() {
 					hasWorkingFileLocking: true,
 					hasValidTransactionIsolationLevel: true,
 					suggestedOverwriteCliURL: '',
-					isUrandomAvailable: true,
+					isRandomnessSecure: true,
 					securityDocs: 'https://docs.owncloud.org/myDocs.html',
 					serverHasInternetConnection: true,
 					isMemcacheConfigured: true,
@@ -740,7 +740,7 @@ describe('OC.SetupChecks tests', function() {
 					hasWorkingFileLocking: true,
 					hasValidTransactionIsolationLevel: true,
 					suggestedOverwriteCliURL: '',
-					isUrandomAvailable: true,
+					isRandomnessSecure: true,
 					securityDocs: 'https://docs.owncloud.org/myDocs.html',
 					serverHasInternetConnection: true,
 					isMemcacheConfigured: true,

settings/Controller/CheckSetupController.php

@@ -55,6 +55,7 @@ use OCP\ILogger;
 use OCP\IRequest;
 use OCP\IURLGenerator;
 use OCP\Lock\ILockingProvider;
+use OCP\Security\ISecureRandom;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 use Symfony\Component\EventDispatcher\GenericEvent;
 
@@ -86,6 +87,8 @@ class CheckSetupController extends Controller {
 	private $dateTimeFormatter;
 	/** @var MemoryInfo */
 	private $memoryInfo;
+	/** @var ISecureRandom */
+	private $secureRandom;
 
 	public function __construct($AppName,
 								IRequest $request,
@@ -100,7 +103,8 @@ class CheckSetupController extends Controller {
 								IDBConnection $db,
 								ILockingProvider $lockingProvider,
 								IDateTimeFormatter $dateTimeFormatter,
-								MemoryInfo $memoryInfo) {
+								MemoryInfo $memoryInfo,
+								ISecureRandom $secureRandom) {
 		parent::__construct($AppName, $request);
 		$this->config = $config;
 		$this->clientService = $clientService;
@@ -114,6 +118,7 @@ class CheckSetupController extends Controller {
 		$this->lockingProvider = $lockingProvider;
 		$this->dateTimeFormatter = $dateTimeFormatter;
 		$this->memoryInfo = $memoryInfo;
+		$this->secureRandom = $secureRandom;
 	}
 
 	/**
@@ -167,20 +172,17 @@ class CheckSetupController extends Controller {
 	}
 
 	/**
-	 * Whether /dev/urandom is available to the PHP controller
+	 * Whether PHP can generate "secure" pseudorandom integers
 	 *
 	 * @return bool
 	 */
-	private function isUrandomAvailable() {
-		if(@file_exists('/dev/urandom')) {
-			$file = fopen('/dev/urandom', 'rb');
-			if($file) {
-				fclose($file);
-				return true;
-			}
+	private function isRandomnessSecure() {
+		try {
+			$this->secureRandom->generate(1);
+		} catch (\Exception $ex) {
+			return false;
 		}
-
-		return false;
+		return true;
 	}
 
 	/**
@@ -601,7 +603,7 @@ Raw output
 				'serverHasInternetConnection' => $this->isInternetConnectionWorking(),
 				'isMemcacheConfigured' => $this->isMemcacheConfigured(),
 				'memcacheDocs' => $this->urlGenerator->linkToDocs('admin-performance'),
-				'isUrandomAvailable' => $this->isUrandomAvailable(),
+				'isRandomnessSecure' => $this->isRandomnessSecure(),
 				'securityDocs' => $this->urlGenerator->linkToDocs('admin-security'),
 				'isUsedTlsLibOutdated' => $this->isUsedTlsLibOutdated(),
 				'phpSupported' => $this->isPhpSupported(),

tests/Settings/Controller/CheckSetupControllerTest.php

@@ -24,6 +24,7 @@ namespace Tests\Settings\Controller;
 use OC;
 use OC\DB\Connection;
 use OC\MemoryInfo;
+use OC\Security\SecureRandom;
 use OC\Settings\Controller\CheckSetupController;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataDisplayResponse;
@@ -79,6 +80,8 @@ class CheckSetupControllerTest extends TestCase {
 	private $dateTimeFormatter;
 	/** @var MemoryInfo|MockObject */
 	private $memoryInfo;
+	/** @var SecureRandom|\PHPUnit_Framework_MockObject_MockObject */
+	private $secureRandom;
 
 	/**
 	 * Holds a list of directories created during tests.
@@ -119,6 +122,7 @@ class CheckSetupControllerTest extends TestCase {
 		$this->memoryInfo = $this->getMockBuilder(MemoryInfo::class)
 			->setMethods(['isMemoryLimitSufficient',])
 			->getMock();
+		$this->secureRandom = $this->getMockBuilder(SecureRandom::class)->getMock();
 		$this->checkSetupController = $this->getMockBuilder('\OC\Settings\Controller\CheckSetupController')
 			->setConstructorArgs([
 				'settings',
@@ -135,6 +139,7 @@ class CheckSetupControllerTest extends TestCase {
 				$this->lockingProvider,
 				$this->dateTimeFormatter,
 				$this->memoryInfo,
+				$this->secureRandom,
 				])
 			->setMethods([
 				'isReadOnlyConfig',
@@ -482,7 +487,7 @@ class CheckSetupControllerTest extends TestCase {
 				'serverHasInternetConnection' => false,
 				'isMemcacheConfigured' => true,
 				'memcacheDocs' => 'http://docs.example.org/server/go.php?to=admin-performance',
-				'isUrandomAvailable' => self::invokePrivate($this->checkSetupController, 'isUrandomAvailable'),
+				'isRandomnessSecure' => self::invokePrivate($this->checkSetupController, 'isRandomnessSecure'),
 				'securityDocs' => 'https://docs.example.org/server/8.1/admin_manual/configuration_server/hardening.html',
 				'isUsedTlsLibOutdated' => '',
 				'phpSupported' => [
@@ -528,6 +533,7 @@ class CheckSetupControllerTest extends TestCase {
 				$this->lockingProvider,
 				$this->dateTimeFormatter,
 				$this->memoryInfo,
+				$this->secureRandom,
 			])
 			->setMethods(null)->getMock();