Use AES-256-CTR as default

CTR is recommended over CFB mode.

2 files changed, 68 insertions, 26 deletions

diff --git a/apps/encryption/lib/crypto/crypt.php b/apps/encryption/lib/crypto/crypt.php
index e387380cd95..4bed565d027 100644
--- a/apps/encryption/lib/crypto/crypt.php
+++ b/apps/encryption/lib/crypto/crypt.php
@@ -36,9 +36,21 @@ use OCP\IConfig;
 use OCP\ILogger;
 use OCP\IUserSession;
 
+/**
+ * Class Crypt provides the encryption implementation of the default ownCloud
+ * encryption module. As default AES-256-CTR is used, it does however offer support
+ * for the following modes:
+ *
+ * - AES-256-CTR
+ * - AES-128-CTR
+ * - AES-256-CFB
+ * - AES-128-CFB
+ *
+ * @package OCA\Encryption\Crypto
+ */
 class Crypt {
 
-	const DEFAULT_CIPHER = 'AES-256-CFB';
+	const DEFAULT_CIPHER = 'AES-256-CTR';
 	// default cipher from old ownCloud versions
 	const LEGACY_CIPHER = 'AES-128-CFB';
 
@@ -48,23 +60,21 @@ class Crypt {
 
 	const HEADER_START = 'HBEGIN';
 	const HEADER_END = 'HEND';
-	/**
-	 * @var ILogger
-	 */
+	/** @var ILogger */
 	private $logger;
-	/**
-	 * @var string
-	 */
+	/** @var string */
 	private $user;
-	/**
-	 * @var IConfig
-	 */
+	/** @var IConfig */
 	private $config;
-
-	/**
-	 * @var array
-	 */
+	/** @var array */
 	private $supportedKeyFormats;
+	/** @var array */
+	private $supportedCiphersAndKeySize = [
+		'AES-256-CTR' => 32,
+		'AES-128-CTR' => 16,
+		'AES-256-CFB' => 32,
+		'AES-128-CFB' => 16,
+	];
 
 	/**
 	 * @param ILogger $logger
@@ -225,8 +235,13 @@ class Crypt {
 	 */
 	public function getCipher() {
 		$cipher = $this->config->getSystemValue('cipher', self::DEFAULT_CIPHER);
-		if ($cipher !== 'AES-256-CFB' && $cipher !== 'AES-128-CFB') {
-			$this->logger->warning('Wrong cipher defined in config.php only AES-128-CFB and AES-256-CFB are supported. Fall back' . self::DEFAULT_CIPHER,
+		if (!isset($this->supportedCiphersAndKeySize[$cipher])) {
+			$this->logger->warning(
+					sprintf(
+							'Unsupported cipher (%s) defined in config.php supported. Falling back to %s',
+							$cipher,
+							self::DEFAULT_CIPHER
+					),
 				['app' => 'encryption']);
 			$cipher = self::DEFAULT_CIPHER;
 		}
@@ -237,19 +252,20 @@ class Crypt {
 	/**
 	 * get key size depending on the cipher
 	 *
-	 * @param string $cipher supported ('AES-256-CFB' and 'AES-128-CFB')
+	 * @param string $cipher
 	 * @return int
 	 * @throws \InvalidArgumentException
 	 */
 	protected function getKeySize($cipher) {
-		if ($cipher === 'AES-256-CFB') {
-			return 32;
-		} else if ($cipher === 'AES-128-CFB') {
-			return 16;
+		if(isset($this->supportedCiphersAndKeySize[$cipher])) {
+			return $this->supportedCiphersAndKeySize[$cipher];
 		}
 
 		throw new \InvalidArgumentException(
-			'Wrong cipher defined only AES-128-CFB and AES-256-CFB are supported.'
+			sprintf(
+					'Unsupported cipher (%s) defined.',
+					$cipher
+			)
 		);
 	}
 
diff --git a/apps/encryption/tests/lib/crypto/cryptTest.php b/apps/encryption/tests/lib/crypto/cryptTest.php
index e599cc28963..ce4dfb68cf1 100644
--- a/apps/encryption/tests/lib/crypto/cryptTest.php
+++ b/apps/encryption/tests/lib/crypto/cryptTest.php
@@ -105,7 +105,7 @@ class cryptTest extends TestCase {
 
 		$this->config->expects($this->once())
 			->method('getSystemValue')
-			->with($this->equalTo('cipher'), $this->equalTo('AES-256-CFB'))
+			->with($this->equalTo('cipher'), $this->equalTo('AES-256-CTR'))
 			->willReturn('AES-128-CFB');
 
 		if ($keyFormat) {
@@ -126,6 +126,9 @@ class cryptTest extends TestCase {
 		$this->crypt->generateHeader('unknown');
 	}
 
+	/**
+	 * @return array
+	 */
 	public function dataTestGenerateHeader() {
 		return [
 			[null, 'HBEGIN:cipher:AES-128-CFB:keyFormat:hash:HEND'],
@@ -134,16 +137,28 @@ class cryptTest extends TestCase {
 		];
 	}
 
+	public function testGetCipherWithInvalidCipher() {
+		$this->config->expects($this->once())
+				->method('getSystemValue')
+				->with($this->equalTo('cipher'), $this->equalTo('AES-256-CTR'))
+				->willReturn('Not-Existing-Cipher');
+		$this->logger
+			->expects($this->once())
+			->method('warning')
+			->with('Unsupported cipher (Not-Existing-Cipher) defined in config.php supported. Falling back to AES-256-CTR');
+
+		$this->assertSame('AES-256-CTR',  $this->crypt->getCipher());
+	}
+
 	/**
 	 * @dataProvider dataProviderGetCipher
 	 * @param string $configValue
 	 * @param string $expected
 	 */
 	public function testGetCipher($configValue, $expected) {
-
 		$this->config->expects($this->once())
 			->method('getSystemValue')
-			->with($this->equalTo('cipher'), $this->equalTo('AES-256-CFB'))
+			->with($this->equalTo('cipher'), $this->equalTo('AES-256-CTR'))
 			->willReturn($configValue);
 
 		$this->assertSame($expected,
@@ -161,7 +176,10 @@ class cryptTest extends TestCase {
 		return array(
 			array('AES-128-CFB', 'AES-128-CFB'),
 			array('AES-256-CFB', 'AES-256-CFB'),
-			array('unknown', 'AES-256-CFB')
+			array('AES-128-CTR', 'AES-128-CTR'),
+			array('AES-256-CTR', 'AES-256-CTR'),
+
+			array('unknown', 'AES-256-CTR')
 		);
 	}
 
@@ -303,10 +321,15 @@ class cryptTest extends TestCase {
 		$this->invokePrivate($this->crypt, 'getKeySize', ['foo']);
 	}
 
+	/**
+	 * @return array
+	 */
 	public function dataTestGetKeySize() {
 		return [
 			['AES-256-CFB', 32],
 			['AES-128-CFB', 16],
+			['AES-256-CTR', 32],
+			['AES-128-CTR', 16],
 		];
 	}
 
@@ -351,6 +374,9 @@ class cryptTest extends TestCase {
 		$this->assertSame($expected, $result);
 	}
 
+	/**
+	 * @return array
+	 */
 	public function dataTestDecryptPrivateKey() {
 		return [
 			[['cipher' => 'AES-128-CFB', 'keyFormat' => 'password'], 'HBEGIN:HENDprivateKey', 'AES-128-CFB', true, 'key'],