diff --git a/3rdparty b/3rdparty index 23b76aa5251..5935defbd4b 160000 --- a/3rdparty +++ b/3rdparty @@ -1 +1 @@ -Subproject commit 23b76aa5251ada94e04b47ef2513b9f1a7c8e8ce +Subproject commit 5935defbd4b614d76f84ef357f28810957d1ee11 diff --git a/apps/encryption/lib/Crypto/Crypt.php b/apps/encryption/lib/Crypto/Crypt.php index 806b6b954b7..238db34b379 100644 --- a/apps/encryption/lib/Crypto/Crypt.php +++ b/apps/encryption/lib/Crypto/Crypt.php @@ -17,7 +17,7 @@ use OCP\Encryption\Exceptions\GenericEncryptionException; use OCP\IConfig; use OCP\IL10N; use OCP\IUserSession; -use phpseclib\Crypt\RC4; +use phpseclib3\Crypt\RC4; use Psr\Log\LoggerInterface; /** @@ -722,7 +722,6 @@ class Crypt { */ private function rc4Decrypt(string $data, string $secret): string { $rc4 = new RC4(); - /** @psalm-suppress InternalMethod */ $rc4->setKey($secret); return $rc4->decrypt($data); @@ -733,7 +732,6 @@ class Crypt { */ private function rc4Encrypt(string $data, string $secret): string { $rc4 = new RC4(); - /** @psalm-suppress InternalMethod */ $rc4->setKey($secret); return $rc4->encrypt($data); diff --git a/apps/files_external/lib/Controller/AjaxController.php b/apps/files_external/lib/Controller/AjaxController.php index 5e3ceb91842..64a9fe141d0 100644 --- a/apps/files_external/lib/Controller/AjaxController.php +++ b/apps/files_external/lib/Controller/AjaxController.php @@ -78,10 +78,15 @@ class AjaxController extends Controller { */ private function generateSshKeys($keyLength) { $key = $this->rsaMechanism->createKey($keyLength); - // Replace the placeholder label with a more meaningful one - $key['publickey'] = str_replace('phpseclib-generated-key', gethostname(), $key['publickey']); - - return $key; + return [ + 'private_key' => $key->toString('PKCS1'), + // Replace the placeholder label with a more meaningful one + 'public_key' => str_replace( + 'phpseclib-generated-key', + gethostname(), + $key->getPublicKey()->toString('OpenSSH'), + ), + ]; } /** @@ -93,10 +98,7 @@ class AjaxController extends Controller { public function getSshKeys($keyLength = 1024) { $key = $this->generateSshKeys($keyLength); return new JSONResponse([ - 'data' => [ - 'private_key' => $key['privatekey'], - 'public_key' => $key['publickey'] - ], + 'data' => $key, 'status' => 'success', ]); } diff --git a/apps/files_external/lib/Lib/Auth/PublicKey/RSA.php b/apps/files_external/lib/Lib/Auth/PublicKey/RSA.php index 807445f0e27..c55bb5c4ec3 100644 --- a/apps/files_external/lib/Lib/Auth/PublicKey/RSA.php +++ b/apps/files_external/lib/Lib/Auth/PublicKey/RSA.php @@ -14,7 +14,7 @@ use OCA\Files_External\Lib\StorageConfig; use OCP\IConfig; use OCP\IL10N; use OCP\IUser; -use phpseclib\Crypt\RSA as RSACrypt; +use phpseclib3\Crypt\RSA as RSACrypt; /** * RSA public key authentication @@ -45,15 +45,16 @@ class RSA extends AuthMechanism { */ #[\Override] public function manipulateStorageConfig(StorageConfig &$storage, ?IUser $user = null) { - $auth = new RSACrypt(); - $auth->setPassword($this->config->getSystemValue('secret', '')); - if (!$auth->loadKey($storage->getBackendOption('private_key'))) { + try { + $auth = RSACrypt::loadPrivateKey( + $storage->getBackendOption('private_key'), + $this->config->getSystemValue('secret', '') + ); + } catch (\Throwable) { // Add fallback routine for a time where secret was not enforced to be exists - $auth->setPassword(''); - if (!$auth->loadKey($storage->getBackendOption('private_key'))) { - throw new \RuntimeException('unable to load private key'); - } + $auth = RSACrypt::loadPrivateKey($storage->getBackendOption('private_key')); } + $storage->setBackendOption('public_key_auth', $auth); } @@ -61,17 +62,14 @@ class RSA extends AuthMechanism { * Generate a keypair * * @param int $keyLenth - * @return array ['privatekey' => $privateKey, 'publickey' => $publicKey] */ - public function createKey($keyLength) { - $rsa = new RSACrypt(); - $rsa->setPublicKeyFormat(RSACrypt::PUBLIC_FORMAT_OPENSSH); - $rsa->setPassword($this->config->getSystemValue('secret', '')); - + public function createKey($keyLength): RSACrypt\PrivateKey { if ($keyLength !== 1024 && $keyLength !== 2048 && $keyLength !== 4096) { $keyLength = 1024; } - return $rsa->createKey($keyLength); + $secret = $this->config->getSystemValue('secret', ''); + $rsa = RSACrypt::createKey($keyLength); + return $rsa->withPassword($secret); } } diff --git a/apps/files_external/lib/Lib/Auth/PublicKey/RSAPrivateKey.php b/apps/files_external/lib/Lib/Auth/PublicKey/RSAPrivateKey.php index a954467ebe9..16260c0d074 100644 --- a/apps/files_external/lib/Lib/Auth/PublicKey/RSAPrivateKey.php +++ b/apps/files_external/lib/Lib/Auth/PublicKey/RSAPrivateKey.php @@ -13,7 +13,8 @@ use OCA\Files_External\Lib\StorageConfig; use OCP\IConfig; use OCP\IL10N; use OCP\IUser; -use phpseclib\Crypt\RSA as RSACrypt; +use phpseclib3\Crypt\RSA; +use phpseclib3\Exception\NoKeyLoadedException; /** * RSA public key authentication @@ -42,14 +43,18 @@ class RSAPrivateKey extends AuthMechanism { */ #[\Override] public function manipulateStorageConfig(StorageConfig &$storage, ?IUser $user = null) { - $auth = new RSACrypt(); - $auth->setPassword($this->config->getSystemValue('secret', '')); - if (!$auth->loadKey($storage->getBackendOption('private_key'))) { + + try { + $auth = RSA\PrivateKey::loadPrivateKey( + $storage->getBackendOption('private_key'), + $this->config->getSystemValue('secret', ''), + ); + } catch (NoKeyLoadedException) { // Add fallback routine for a time where secret was not enforced to be exists - $auth->setPassword(''); - if (!$auth->loadKey($storage->getBackendOption('private_key'))) { - throw new \RuntimeException('unable to load private key'); - } + $auth = RSA\PrivateKey::loadPrivateKey( + $storage->getBackendOption('private_key'), + '', + ); } $storage->setBackendOption('public_key_auth', $auth); } diff --git a/apps/files_external/lib/Lib/Storage/SFTP.php b/apps/files_external/lib/Lib/Storage/SFTP.php index f2b8924673a..67a38f7019f 100644 --- a/apps/files_external/lib/Lib/Storage/SFTP.php +++ b/apps/files_external/lib/Lib/Storage/SFTP.php @@ -19,7 +19,7 @@ use OCP\Constants; use OCP\Files\FileInfo; use OCP\Files\IMimeTypeDetector; use OCP\Server; -use phpseclib\Net\SFTP\Stream; +use phpseclib3\Net\SFTP\Stream; /** * Uses phpseclib's Net\SFTP class and the Net\SFTP\Stream stream wrapper to @@ -34,7 +34,7 @@ class SFTP extends Common { private $auth = []; /** - * @var \phpseclib\Net\SFTP + * @var \phpseclib3\Net\SFTP */ protected $client; private CappedMemoryCache $knownMTimes; @@ -106,16 +106,16 @@ class SFTP extends Common { /** * Returns the connection. * - * @return \phpseclib\Net\SFTP connected client instance + * @return \phpseclib3\Net\SFTP connected client instance * @throws \Exception when the connection failed */ - public function getConnection(): \phpseclib\Net\SFTP { + public function getConnection(): \phpseclib3\Net\SFTP { if (!is_null($this->client)) { return $this->client; } $hostKeys = $this->readHostKeys(); - $this->client = new \phpseclib\Net\SFTP($this->host, $this->port); + $this->client = new \phpseclib3\Net\SFTP($this->host, $this->port); // The SSH Host Key MUST be verified before login(). $currentHostKey = $this->client->getServerPublicHostKey(); @@ -130,7 +130,6 @@ class SFTP extends Common { $login = false; foreach ($this->auth as $auth) { - /** @psalm-suppress TooManyArguments */ $login = $this->client->login($this->user, $auth); if ($login === true) { break; @@ -338,7 +337,7 @@ class SFTP extends Common { case 'wb': SFTPWriteStream::register(); // the SFTPWriteStream doesn't go through the "normal" methods so it doesn't clear the stat cache. - $connection->_remove_from_stat_cache($absPath); + $connection->clearStatCache(); $context = stream_context_create(['sftp' => ['session' => $connection]]); $fh = fopen('sftpwrite://' . trim($absPath, '/'), 'w', false, $context); if ($fh) { @@ -487,7 +486,7 @@ class SFTP extends Common { $absTarget = $this->absPath($target); $connection = $this->getConnection(); - $size = $connection->size($absSource); + $size = $connection->filesize($absSource); if ($size === false) { return false; } @@ -498,7 +497,7 @@ class SFTP extends Common { return false; } /** @psalm-suppress InternalMethod */ - if (!$connection->put($absTarget, $chunk, \phpseclib\Net\SFTP::SOURCE_STRING, $i)) { + if (!$connection->put($absTarget, $chunk, \phpseclib3\Net\SFTP::SOURCE_STRING, $i)) { return false; } } diff --git a/apps/files_external/lib/Lib/Storage/SFTPReadStream.php b/apps/files_external/lib/Lib/Storage/SFTPReadStream.php index 5007c251a47..78457a7a48a 100644 --- a/apps/files_external/lib/Lib/Storage/SFTPReadStream.php +++ b/apps/files_external/lib/Lib/Storage/SFTPReadStream.php @@ -10,13 +10,13 @@ declare(strict_types=1); namespace OCA\Files_External\Lib\Storage; use Icewind\Streams\File; -use phpseclib\Net\SSH2; +use phpseclib3\Net\SSH2; class SFTPReadStream implements File { /** @var resource */ public $context; - /** @var \phpseclib\Net\SFTP */ + /** @var \phpseclib3\Net\SFTP */ private $sftp; /** @var string */ @@ -54,7 +54,7 @@ class SFTPReadStream implements File { } else { throw new \BadMethodCallException('Invalid context, "' . $name . '" options not set'); } - if (isset($context['session']) && $context['session'] instanceof \phpseclib\Net\SFTP) { + if (isset($context['session']) && $context['session'] instanceof \phpseclib3\Net\SFTP) { $this->sftp = $context['session']; } else { throw new \BadMethodCallException('Invalid context, session not set'); diff --git a/apps/files_external/lib/Lib/Storage/SFTPWriteStream.php b/apps/files_external/lib/Lib/Storage/SFTPWriteStream.php index 46735735890..5dfdead4e73 100644 --- a/apps/files_external/lib/Lib/Storage/SFTPWriteStream.php +++ b/apps/files_external/lib/Lib/Storage/SFTPWriteStream.php @@ -10,13 +10,13 @@ declare(strict_types=1); namespace OCA\Files_External\Lib\Storage; use Icewind\Streams\File; -use phpseclib\Net\SSH2; +use phpseclib3\Net\SSH2; class SFTPWriteStream implements File { /** @var resource */ public $context; - /** @var \phpseclib\Net\SFTP */ + /** @var \phpseclib3\Net\SFTP */ private $sftp; /** @var string */ @@ -54,7 +54,7 @@ class SFTPWriteStream implements File { } else { throw new \BadMethodCallException('Invalid context, "' . $name . '" options not set'); } - if (isset($context['session']) && $context['session'] instanceof \phpseclib\Net\SFTP) { + if (isset($context['session']) && $context['session'] instanceof \phpseclib3\Net\SFTP) { $this->sftp = $context['session']; } else { throw new \BadMethodCallException('Invalid context, session not set'); @@ -74,7 +74,7 @@ class SFTPWriteStream implements File { return false; } - $remote_file = $this->sftp->_realpath($path); + $remote_file = $this->sftp->realpath($path); $this->path = $remote_file; if ($remote_file === false) { diff --git a/apps/files_external/lib/Service/EncryptionService.php b/apps/files_external/lib/Service/EncryptionService.php index c36f6d06132..6a36cdad4e2 100644 --- a/apps/files_external/lib/Service/EncryptionService.php +++ b/apps/files_external/lib/Service/EncryptionService.php @@ -12,7 +12,7 @@ namespace OCA\Files_External\Service; use OCP\IConfig; use OCP\Security\ISecureRandom; -use phpseclib\Crypt\AES; +use phpseclib3\Crypt\AES; class EncryptionService { public function __construct( @@ -78,8 +78,28 @@ class EncryptionService { * Returns the encryption cipher */ private function getCipher(): AES { - $cipher = new AES(AES::MODE_CBC); - $cipher->setKey($this->config->getSystemValue('passwordsalt', null)); + $cipher = new AES('cbc'); + $cipher->setKey($this->normalizeKey((string)$this->config->getSystemValue('passwordsalt', ''))); return $cipher; } + + /** + * Normalize the configured `passwordsalt` into a valid AES key. + * + * Note: phpseclib v2 accepted keys of any length and silently normalized them: + * the key length was rounded up to the next valid AES size (16, 24 or 32 + * bytes), the key was read in whole 4-byte words (trailing bytes that did + * not form a full word were dropped) and any missing high words were + * treated as zero. phpseclib v3 rejects keys that are not exactly 16, 24 or + * 32 bytes, so we reproduce the v2 behaviour here to keep previously stored + * passwords decryptable. + */ + private function normalizeKey(string $key): string { + $length = strlen($key); + $keyLength = $length <= 16 ? 16 : ($length <= 24 ? 24 : 32); + // Drop trailing bytes that do not form a full 4-byte word (phpseclib v2 used unpack('N*')) + $key = substr($key, 0, intdiv($length, 4) * 4); + // Zero-pad missing high words and truncate to the target key length + return substr(str_pad($key, $keyLength, "\0"), 0, $keyLength); + } } diff --git a/apps/files_external/tests/Controller/AjaxControllerTest.php b/apps/files_external/tests/Controller/AjaxControllerTest.php index b186c48b29c..510fd886c75 100644 --- a/apps/files_external/tests/Controller/AjaxControllerTest.php +++ b/apps/files_external/tests/Controller/AjaxControllerTest.php @@ -21,6 +21,7 @@ use OCP\IRequest; use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; +use phpseclib3\Crypt\RSA as CryptRSA; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -37,8 +38,12 @@ class AjaxControllerTest extends TestCase { protected function setUp(): void { $this->request = $this->createMock(IRequest::class); - $this->rsa = $this->createMock(RSA::class); - $this->globalAuth = $this->createMock(GlobalAuth::class); + $this->rsa = $this->getMockBuilder(RSA::class) + ->disableOriginalConstructor() + ->getMock(); + $this->globalAuth = $this->getMockBuilder(GlobalAuth::class) + ->disableOriginalConstructor() + ->getMock(); $this->userSession = $this->createMock(IUserSession::class); $this->groupManager = $this->createMock(IGroupManager::class); $this->userManager = $this->createMock(IUserManager::class); @@ -114,24 +119,32 @@ class AjaxControllerTest extends TestCase { } public function testGetSshKeys(): void { + // phpseclib3's PrivateKey/PublicKey are final and cannot be mocked, so + // generate a real key pair and assert the controller serialises it. + $privateKey = CryptRSA::createKey(1024); + $this->rsa ->expects($this->once()) ->method('createKey') - ->willReturn([ - 'privatekey' => 'MyPrivateKey', - 'publickey' => 'MyPublicKey', - ]); - - $expected = new JSONResponse( - [ - 'data' => [ - 'private_key' => 'MyPrivateKey', - 'public_key' => 'MyPublicKey', - ], - 'status' => 'success', - ] + ->willReturn($privateKey); + + $response = $this->ajaxController->getSshKeys(); + + $this->assertInstanceOf(JSONResponse::class, $response); + $data = $response->getData(); + $this->assertSame('success', $data['status']); + $this->assertSame( + $privateKey->toString('PKCS1'), + $data['data']['private_key'], + ); + $this->assertSame( + str_replace( + 'phpseclib-generated-key', + gethostname(), + $privateKey->getPublicKey()->toString('OpenSSH'), + ), + $data['data']['public_key'], ); - $this->assertEquals($expected, $this->ajaxController->getSshKeys()); } public function testSaveGlobalCredentialsAsAdminForAnotherUser(): void { diff --git a/apps/files_external/tests/Service/EncryptionServiceTest.php b/apps/files_external/tests/Service/EncryptionServiceTest.php new file mode 100644 index 00000000000..a9bffb539c3 --- /dev/null +++ b/apps/files_external/tests/Service/EncryptionServiceTest.php @@ -0,0 +1,82 @@ +config = $this->createMock(IConfig::class); + $this->secureRandom = $this->createMock(ISecureRandom::class); + } + + private function service(string $passwordSalt): EncryptionService { + $this->config->method('getSystemValue') + ->with('passwordsalt', '') + ->willReturn($passwordSalt); + return new EncryptionService($this->config, $this->secureRandom); + } + + public static function saltProvider(): array { + return [ + 'typical 30-char salt' => [str_repeat('a', 30)], + '16-char salt' => [str_repeat('b', 16)], + '24-char salt' => [str_repeat('c', 24)], + '32-char salt' => [str_repeat('d', 32)], + 'long 40-char salt' => [str_repeat('e', 40)], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('saltProvider')] + public function testEncryptDecryptRoundTrip(string $passwordSalt): void { + $this->secureRandom->method('generate')->willReturn(str_repeat("\x11", 16)); + $service = $this->service($passwordSalt); + + $options = $service->encryptPasswords(['password' => 'my-secret-password']); + $this->assertArrayHasKey('password_encrypted', $options); + $this->assertSame('', $options['password']); + + $decrypted = $service->decryptPasswords(['password_encrypted' => $options['password_encrypted']]); + $this->assertSame('my-secret-password', $decrypted['password']); + $this->assertArrayNotHasKey('password_encrypted', $decrypted); + } + + /** + * Ciphertexts written with phpseclib v2 must remain decryptable after the + * upgrade to phpseclib v3. v2 accepted arbitrary-length keys and normalized + * them (round up to 16/24/32 bytes, read whole 4-byte words only, zero-pad + * missing high words). Here we recreate such a ciphertext with OpenSSL — an + * independent AES implementation — using that exact effective key. + */ + #[\PHPUnit\Framework\Attributes\DataProvider('saltProvider')] + public function testDecryptsLegacyPhpseclibV2Ciphertext(string $passwordSalt): void { + $plaintext = 'legacy-external-storage-password'; + + // Reproduce the phpseclib v2 effective AES key + $length = strlen($passwordSalt); + $keyLength = $length <= 16 ? 16 : ($length <= 24 ? 24 : 32); + $effectiveKey = substr(str_pad(substr($passwordSalt, 0, intdiv($length, 4) * 4), $keyLength, "\0"), 0, $keyLength); + + $iv = str_repeat("\x22", 16); + $rawCiphertext = openssl_encrypt($plaintext, 'aes-' . ($keyLength * 8) . '-cbc', $effectiveKey, OPENSSL_RAW_DATA, $iv); + $stored = base64_encode($iv . $rawCiphertext); + + $service = $this->service($passwordSalt); + $decrypted = $service->decryptPasswords(['password_encrypted' => $stored]); + $this->assertSame($plaintext, $decrypted['password']); + } +} diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index dbd7cb5ac8b..7e59e0d212c 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -1603,12 +1603,6 @@ - - - - - - @@ -3939,11 +3933,6 @@ - - getDN(X509::DN_OPENSSL)['CN']]]> - getDN(X509::DN_OPENSSL)['CN']]]> - getDN(true)['CN']]]> - @@ -4087,16 +4076,6 @@ getDefaultCertificatesBundlePath())]]> - - - - - - - - - - diff --git a/core/Command/Integrity/SignApp.php b/core/Command/Integrity/SignApp.php index e4d61e8cc5b..1c0aa18a41a 100644 --- a/core/Command/Integrity/SignApp.php +++ b/core/Command/Integrity/SignApp.php @@ -11,8 +11,8 @@ namespace OC\Core\Command\Integrity; use OC\IntegrityCheck\Checker; use OC\IntegrityCheck\Helpers\FileAccessHelper; use OCP\IURLGenerator; -use phpseclib\Crypt\RSA; -use phpseclib\File\X509; +use phpseclib3\Crypt\RSA; +use phpseclib3\File\X509; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; @@ -71,8 +71,7 @@ class SignApp extends Command { return 1; } - $rsa = new RSA(); - $rsa->loadKey($privateKey); + $rsa = RSA::loadPrivateKey($privateKey); $x509 = new X509(); $x509->loadX509($keyBundle); $x509->setPrivateKey($rsa); diff --git a/core/Command/Integrity/SignCore.php b/core/Command/Integrity/SignCore.php index 73b7e76b4ba..b718b6c3a6f 100644 --- a/core/Command/Integrity/SignCore.php +++ b/core/Command/Integrity/SignCore.php @@ -10,8 +10,8 @@ namespace OC\Core\Command\Integrity; use OC\IntegrityCheck\Checker; use OC\IntegrityCheck\Helpers\FileAccessHelper; -use phpseclib\Crypt\RSA; -use phpseclib\File\X509; +use phpseclib3\Crypt\RSA; +use phpseclib3\File\X509; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; @@ -66,8 +66,7 @@ class SignCore extends Command { return 1; } - $rsa = new RSA(); - $rsa->loadKey($privateKey); + $rsa = RSA::loadPrivateKey($privateKey); $x509 = new X509(); $x509->loadX509($keyBundle); $x509->setPrivateKey($rsa); diff --git a/lib/private/Installer.php b/lib/private/Installer.php index 5990e93277d..b1e444f6b98 100644 --- a/lib/private/Installer.php +++ b/lib/private/Installer.php @@ -31,7 +31,7 @@ use OCP\L10N\IFactory; use OCP\Migration\IOutput; use OCP\Server; use OCP\Util; -use phpseclib\File\X509; +use phpseclib3\File\X509; use Psr\Log\LoggerInterface; /** diff --git a/lib/private/IntegrityCheck/Checker.php b/lib/private/IntegrityCheck/Checker.php index 8425a96bff3..7c03274a3a9 100644 --- a/lib/private/IntegrityCheck/Checker.php +++ b/lib/private/IntegrityCheck/Checker.php @@ -22,8 +22,8 @@ use OCP\ICache; use OCP\ICacheFactory; use OCP\IConfig; use OCP\ServerVersion; -use phpseclib\Crypt\RSA; -use phpseclib\File\X509; +use phpseclib3\Crypt\RSA; +use phpseclib3\File\X509; /** * Class Checker handles the code signing using X.509 and RSA. ownCloud ships with @@ -166,24 +166,26 @@ class Checker { * * @param array $hashes * @param X509 $certificate - * @param RSA $privateKey + * @param RSA\PrivateKey $privateKey * @return array */ - private function createSignatureData(array $hashes, + private function createSignatureData( + array $hashes, X509 $certificate, - RSA $privateKey): array { + RSA\PrivateKey $privateKey, + ): array { ksort($hashes); - $privateKey->setSignatureMode(RSA::SIGNATURE_PSS); - $privateKey->setMGFHash('sha512'); - // See https://tools.ietf.org/html/rfc3447#page-38 - $privateKey->setSaltLength(0); - $signature = $privateKey->sign(json_encode($hashes)); + $signature = $privateKey + ->withPadding(RSA::SIGNATURE_PSS) + ->withMGFHash('sha512') + ->withSaltLength(0) + ->sign(json_encode($hashes)); return [ 'hashes' => $hashes, 'signature' => base64_encode($signature), - 'certificate' => $certificate->saveX509($certificate->currentCert), + 'certificate' => $certificate->saveX509($certificate->getCurrentCert()), ]; } @@ -192,12 +194,12 @@ class Checker { * * @param string $path * @param X509 $certificate - * @param RSA $privateKey + * @param RSA\PrivateKey $privateKey * @throws \Exception */ public function writeAppSignature($path, X509 $certificate, - RSA $privateKey) { + RSA\PrivateKey $privateKey) { $appInfoDir = $path . '/appinfo'; try { $this->fileAccessHelper->assertDirectoryExists($appInfoDir); @@ -210,7 +212,7 @@ class Checker { json_encode($signature, JSON_PRETTY_PRINT) ); } catch (\Exception $e) { - if (!$this->fileAccessHelper->is_writable($appInfoDir)) { + if ($this->fileAccessHelper->is_writable($appInfoDir) === false) { throw new \Exception($appInfoDir . ' is not writable'); } throw $e; @@ -221,12 +223,12 @@ class Checker { * Write the signature of core * * @param X509 $certificate - * @param RSA $rsa + * @param RSA\PrivateKey $rsa * @param string $path * @throws \Exception */ public function writeCoreSignature(X509 $certificate, - RSA $rsa, + RSA\PrivateKey $rsa, $path) { $coreDir = $path . '/core'; try { @@ -290,15 +292,14 @@ class Checker { $certificate = $signatureData['certificate']; // Check if certificate is signed by Nextcloud Root Authority - $x509 = new \phpseclib\File\X509(); + $x509 = new X509(); $rootCertificatePublicKey = $this->fileAccessHelper->file_get_contents($this->environmentHelper->getServerRoot() . '/resources/codesigning/root.crt'); $rootCerts = $this->splitCerts($rootCertificatePublicKey); foreach ($rootCerts as $rootCert) { $x509->loadCA($rootCert); } - $x509->loadX509($certificate); - if (!$x509->validateSignature()) { + if ($x509->loadX509($certificate) === false || !$x509->validateSignature()) { throw new InvalidSignatureException('Certificate is not valid.'); } // Verify if certificate has proper CN. "core" CN is always trusted. @@ -309,13 +310,18 @@ class Checker { } // Check if the signature of the files is valid - $rsa = new \phpseclib\Crypt\RSA(); - $rsa->loadKey($x509->currentCert['tbsCertificate']['subjectPublicKeyInfo']['subjectPublicKey']); - $rsa->setSignatureMode(RSA::SIGNATURE_PSS); - $rsa->setMGFHash('sha512'); - // See https://tools.ietf.org/html/rfc3447#page-38 - $rsa->setSaltLength(0); - if (!$rsa->verify(json_encode($expectedHashes), $signature)) { + /** @var RSA\PublicKey|false */ + $rsa = $x509->getPublicKey(); + if ($rsa === false) { + throw new InvalidSignatureException('Certificate does not provide valid public RSA key.'); + } + + $rsa = $rsa + ->withPadding(RSA::SIGNATURE_PSS) + ->withMGFHash('sha512') + ->withSaltLength(0); + + if (!$rsa->verify(json_encode($expectedHashes), (string)$signature)) { throw new InvalidSignatureException('Signature could not get verified.'); } diff --git a/lib/private/Security/Crypto.php b/lib/private/Security/Crypto.php index 711e9f98e9c..8db44ecee00 100644 --- a/lib/private/Security/Crypto.php +++ b/lib/private/Security/Crypto.php @@ -12,8 +12,8 @@ namespace OC\Security; use Exception; use OCP\IConfig; use OCP\Security\ICrypto; -use phpseclib\Crypt\AES; -use phpseclib\Crypt\Hash; +use phpseclib3\Crypt\AES; +use phpseclib3\Crypt\Hash; use SensitiveParameter; /** @@ -33,7 +33,7 @@ class Crypto implements ICrypto { public function __construct( private IConfig $config, ) { - $this->cipher = new AES(); + $this->cipher = new AES('cbc'); } /** @@ -77,7 +77,11 @@ class Crypto implements ICrypto { $password = $this->config->getSystemValueString('secret'); } $keyMaterial = hash_hkdf('sha512', $password); - $this->cipher->setPassword(substr($keyMaterial, 0, 32)); + // Pin the PBKDF2 parameters to phpseclib v2 defaults ('sha1' hash, 'phpseclib' salt). + // phpseclib v3 changed the default salt to 'phpseclib/salt', which would derive a + // different AES key and break decryption of previously stored ciphertexts. + // TODO: We should put our own salt into the HKDF derivation to avoid this dependency on phpseclib's defaults! + $this->cipher->setPassword(substr($keyMaterial, 0, 32), 'pbkdf2', 'sha1', 'phpseclib'); $iv = \random_bytes($this->ivLength); $this->cipher->setIV($iv); @@ -172,7 +176,9 @@ class Crypto implements ICrypto { $hmacKey = substr($keyMaterial, 32); } } - $this->cipher->setPassword($encryptionKey); + // Match the v2-compatible PBKDF2 parameters used in encrypt(), see the note there. + // TODO: We should put our own salt into the HKDF derivation to avoid this dependency on phpseclib's defaults! + $this->cipher->setPassword($encryptionKey, 'pbkdf2', 'sha1', 'phpseclib'); $this->cipher->setIV($iv); if ($isOwnCloudV2Migration) { diff --git a/tests/lib/IntegrityCheck/CheckerTest.php b/tests/lib/IntegrityCheck/CheckerTest.php index 6579c34c9b9..f69f5e9b52f 100644 --- a/tests/lib/IntegrityCheck/CheckerTest.php +++ b/tests/lib/IntegrityCheck/CheckerTest.php @@ -19,26 +19,27 @@ use OCP\IAppConfig; use OCP\ICacheFactory; use OCP\IConfig; use OCP\ServerVersion; -use phpseclib\Crypt\RSA; -use phpseclib\File\X509; +use phpseclib3\Crypt\RSA; +use phpseclib3\File\X509; +use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; class CheckerTest extends TestCase { - /** @var ServerVersion|\PHPUnit\Framework\MockObject\MockObject */ + /** @var ServerVersion&MockObject */ private $serverVersion; - /** @var EnvironmentHelper|\PHPUnit\Framework\MockObject\MockObject */ + /** @var EnvironmentHelper&MockObject */ private $environmentHelper; - /** @var FileAccessHelper|\PHPUnit\Framework\MockObject\MockObject */ + /** @var FileAccessHelper&MockObject */ private $fileAccessHelper; - /** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */ + /** @var IConfig&MockObject */ private $config; - /** @var IAppConfig|\PHPUnit\Framework\MockObject\MockObject */ + /** @var IAppConfig&MockObject */ private $appConfig; - /** @var ICacheFactory|\PHPUnit\Framework\MockObject\MockObject */ + /** @var ICacheFactory&MockObject */ private $cacheFactory; - /** @var IAppManager|\PHPUnit\Framework\MockObject\MockObject */ + /** @var IAppManager&MockObject */ private $appManager; - /** @var Detection|\PHPUnit\Framework\MockObject\MockObject */ + /** @var Detection&MockObject */ private $mimeTypeDetector; private Checker $checker; @@ -93,8 +94,7 @@ class CheckerTest extends TestCase { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.key'); - $rsa = new RSA(); - $rsa->loadKey($rsaPrivateKey); + $rsa = RSA::loadPrivateKey($rsaPrivateKey); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeAppSignature('NotExistingApp', $x509, $rsa); @@ -111,8 +111,8 @@ class CheckerTest extends TestCase { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.key'); - $rsa = new RSA(); - $rsa->loadKey($rsaPrivateKey); + $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.key'); + $rsa = RSA::loadPrivateKey($rsaPrivateKey); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeAppSignature(\OC::$SERVERROOT . '/tests/data/integritycheck/app/', $x509, $rsa); @@ -127,6 +127,14 @@ class CheckerTest extends TestCase { "signature": "Y5yvXvcGHVPuRRatKVDUONWq1FpLXugZd6Km\/+aEHsQj7coVl9FeMj9OsWamBf7yRIw3dtNLguTLlAA9QAv\/b0uHN3JnbNZN+dwFOve4NMtqXfSDlWftqKN00VS+RJXpG1S2IIx9Poyp2NoghL\/5AuTv4GHiNb7zU\/DT\/kt71pUGPgPR6IIFaE+zHOD96vjYkrH+GfWZzKR0FCdLib9yyNvk+EGrcjKM6qjs2GKfS\/XFjj\/\/neDnh\/0kcPuKE3ZbofnI4TIDTv0CGqvOp7PtqVNc3Vy\/UKa7uF1PT0MAUKMww6EiMUSFZdUVP4WWF0Y72W53Qdtf1hrAZa2kfKyoK5kd7sQmCSKUPSU8978AUVZlBtTRlyT803IKwMV0iHMkw+xYB1sN2FlHup\/DESADqxhdgYuK35bCPvgkb4SBe4B8Voz\/izTvcP7VT5UvkYdAO+05\/jzdaHEmzmsD92CFfvX0q8O\/Y\/29ubftUJsqcHeMDKgcR4eZOE8+\/QVc\/89QO6WnKNuNuV+5bybO6g6PAdC9ZPsCvnihS61O2mwRXHLR3jv2UleFWm+lZEquPKtkhi6SLtDiijA4GV6dmS+dzujSLb7hGeD5o1plZcZ94uhWljl+QIp82+zU\/lYB1Zfr4Mb4e+V7r2gv7Fbv7y6YtjE2GIQwRhC5jq56bD0ZB+I=", "certificate": "-----BEGIN CERTIFICATE-----\r\nMIIEwTCCAqmgAwIBAgIUWv0iujufs5lUr0svCf\/qTQvoyKAwDQYJKoZIhvcNAQEF\r\nBQAwIzEhMB8GA1UECgwYb3duQ2xvdWQgQ29kZSBTaWduaW5nIENBMB4XDTE1MTEw\r\nMzIyNDk1M1oXDTE2MTEwMzIyNDk1M1owEjEQMA4GA1UEAwwHU29tZUFwcDCCAiIw\r\nDQYJKoZIhvcNAQEBBQADggIPADCCAgoCggIBAK8q0x62agGSRBqeWsaeEwFfepMk\r\nF8cAobMMi50qHCv9IrOn\/ZH9l52xBrbIkErVmRjmly0d4JhD8Ymhidsh9ONKYl\/j\r\n+ishsZDM8eNNdp3Ew+fEYVvY1W7mR1qU24NWj0bzVsClI7hvPVIuw7AjfBDq1C5+\r\nA+ZSLSXYvOK2cEWjdxQfuNZwEZSjmA63DUllBIrm35IaTvfuyhU6BW9yHZxmb8+M\r\nw0xDv30D5UkE\/2N7Pa\/HQJLxCR+3zKibRK3nUyRDLSXxMkU9PnFNaPNX59VPgyj4\r\nGB1CFSToldJVPF4pzh7p36uGXZVxs8m3LFD4Ol8mhi7jkxDZjqFN46gzR0r23Py6\r\ndol9vfawGIoUwp9LvL0S7MvdRY0oazLXwClLP4OQ17zpSMAiCj7fgNT661JamPGj\r\nt5O7Zn2wA7I4ddDS\/HDTWCu98Zwc9fHIpsJPgCZ9awoqxi4Mnf7Pk9g5nnXhszGC\r\ncxxIASQKM+GhdzoRxKknax2RzUCwCzcPRtCj8AQT\/x\/mqN3PfRmlnFBNACUw9bpZ\r\nSOoNq2pCF9igftDWpSIXQ38pVpKLWowjjg3DVRmVKBgivHnUnVLyzYBahHPj0vaz\r\ntFtUFRaqXDnt+4qyUGyrT5h5pjZaTcHIcSB4PiarYwdVvgslgwnQzOUcGAzRWBD4\r\n6jV2brP5vFY3g6iPAgMBAAEwDQYJKoZIhvcNAQEFBQADggIBACTY3CCHC+Z28gCf\r\nFWGKQ3wAKs+k4+0yoti0qm2EKX7rSGQ0PHSas6uW79WstC4Rj+DYkDtIhGMSg8FS\r\nHVGZHGBCc0HwdX+BOAt3zi4p7Sf3oQef70\/4imPoKxbAVCpd\/cveVcFyDC19j1yB\r\nBapwu87oh+muoeaZxOlqQI4UxjBlR\/uRSMhOn2UGauIr3dWJgAF4pGt7TtIzt+1v\r\n0uA6FtN1Y4R5O8AaJPh1bIG0CVvFBE58esGzjEYLhOydgKFnEP94kVPgJD5ds9C3\r\npPhEpo1dRpiXaF7WGIV1X6DI\/ipWvfrF7CEy6I\/kP1InY\/vMDjQjeDnJ\/VrXIWXO\r\nyZvHXVaN\/m+1RlETsH7YO\/QmxRue9ZHN3gvvWtmpCeA95sfpepOk7UcHxHZYyQbF\r\n49\/au8j+5tsr4A83xzsT1JbcKRxkAaQ7WDJpOnE5O1+H0fB+BaLakTg6XX9d4Fo7\r\n7Gin7hVWX7pL+JIyxMzME3LhfI61+CRcqZQIrpyaafUziPQbWIPfEs7h8tCOWyvW\r\nUO8ZLervYCB3j44ivkrxPlcBklDCqqKKBzDP9dYOtS\/P4RB1NkHA9+NTvmBpTonS\r\nSFXdg9fFMD7VfjDE3Vnk+8DWkVH5wBYowTAD7w9Wuzr7DumiAULexnP\/Y7xwxLv7\r\n4B+pXTAcRK0zECDEaX3npS8xWzrB\r\n-----END CERTIFICATE-----" }'; + $this->fileAccessHelper + ->expects(self::any()) + ->method('is_writable') + ->with( + $this->equalTo(\OC::$SERVERROOT . '/tests/data/integritycheck/app//appinfo'), + ) + ->willReturn(true); + $this->fileAccessHelper ->expects($this->once()) ->method('file_put_contents') @@ -142,8 +150,7 @@ class CheckerTest extends TestCase { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.key'); - $rsa = new RSA(); - $rsa->loadKey($rsaPrivateKey); + $rsa = RSA::load($rsaPrivateKey); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeAppSignature(\OC::$SERVERROOT . '/tests/data/integritycheck/app/', $x509, $rsa); @@ -446,8 +453,7 @@ class CheckerTest extends TestCase { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.key'); - $rsa = new RSA(); - $rsa->loadKey($rsaPrivateKey); + $rsa = RSA::loadPrivateKey($rsaPrivateKey); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeCoreSignature($x509, $rsa, __DIR__); @@ -469,8 +475,7 @@ class CheckerTest extends TestCase { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.key'); - $rsa = new RSA(); - $rsa->loadKey($rsaPrivateKey); + $rsa = RSA::loadPrivateKey($rsaPrivateKey); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeCoreSignature($x509, $rsa, __DIR__); @@ -504,8 +509,7 @@ class CheckerTest extends TestCase { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/core.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/core.key'); - $rsa = new RSA(); - $rsa->loadKey($rsaPrivateKey); + $rsa = RSA::loadPrivateKey($rsaPrivateKey); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeCoreSignature($x509, $rsa, \OC::$SERVERROOT . '/tests/data/integritycheck/app/'); @@ -539,8 +543,7 @@ class CheckerTest extends TestCase { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/core.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/core.key'); - $rsa = new RSA(); - $rsa->loadKey($rsaPrivateKey); + $rsa = RSA::loadPrivateKey($rsaPrivateKey); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeCoreSignature($x509, $rsa, \OC::$SERVERROOT . '/tests/data/integritycheck/htaccessUnmodified/'); @@ -569,8 +572,7 @@ class CheckerTest extends TestCase { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/core.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/core.key'); - $rsa = new RSA(); - $rsa->loadKey($rsaPrivateKey); + $rsa = RSA::loadPrivateKey($rsaPrivateKey); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeCoreSignature($x509, $rsa, \OC::$SERVERROOT . '/tests/data/integritycheck/htaccessWithInvalidModifiedContent/'); @@ -605,8 +607,7 @@ class CheckerTest extends TestCase { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/core.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/core.key'); - $rsa = new RSA(); - $rsa->loadKey($rsaPrivateKey); + $rsa = RSA::loadPrivateKey($rsaPrivateKey); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeCoreSignature($x509, $rsa, \OC::$SERVERROOT . '/tests/data/integritycheck/htaccessWithValidModifiedContent'); @@ -976,7 +977,8 @@ class CheckerTest extends TestCase { } public function testRunInstanceVerification(): void { - $this->checker = $this->getMockBuilder('\OC\IntegrityCheck\Checker') + /** @var Checker&MockObject */ + $checker = $this->getMockBuilder('\OC\IntegrityCheck\Checker') ->setConstructorArgs([ $this->serverVersion, $this->environmentHelper, @@ -993,7 +995,7 @@ class CheckerTest extends TestCase { ]) ->getMock(); - $this->checker + $checker ->expects($this->once()) ->method('verifyCoreSignature'); $this->appManager @@ -1020,7 +1022,7 @@ class CheckerTest extends TestCase { 'calendar', 'dav', ]; - $this->checker + $checker ->expects($this->exactly(3)) ->method('verifyAppSignature') ->willReturnCallback(function ($app) use (&$calls) { @@ -1047,7 +1049,7 @@ class CheckerTest extends TestCase { ->method('deleteKey') ->with('core', 'oc.integritycheck.checker'); - $this->checker->runInstanceVerification(); + $checker->runInstanceVerification(); } public function testVerifyAppSignatureWithoutSignatureDataAndCodeCheckerDisabled(): void {