From c4544e3e5e2891eaea2c04b998f320d93b2e552a Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 9 Apr 2024 15:14:36 +0200 Subject: [PATCH 1/5] feat(deps): Upgrade phpseclib to v3 Signed-off-by: Ferdinand Thiessen --- 3rdparty | 2 +- apps/encryption/lib/Crypto/Crypt.php | 4 +- .../lib/Controller/AjaxController.php | 18 ++-- .../lib/Lib/Auth/PublicKey/RSA.php | 28 +++---- .../lib/Lib/Auth/PublicKey/RSAPrivateKey.php | 21 +++-- apps/files_external/lib/Lib/Storage/SFTP.php | 17 ++-- .../lib/Lib/Storage/SFTPReadStream.php | 6 +- .../lib/Lib/Storage/SFTPWriteStream.php | 8 +- .../lib/Service/EncryptionService.php | 26 +++++- .../tests/Controller/AjaxControllerTest.php | 45 ++++++---- .../tests/Service/EncryptionServiceTest.php | 82 +++++++++++++++++++ build/psalm-baseline.xml | 21 ----- core/Command/Integrity/SignApp.php | 7 +- core/Command/Integrity/SignCore.php | 7 +- lib/private/Installer.php | 2 +- lib/private/IntegrityCheck/Checker.php | 58 +++++++------ lib/private/Security/Crypto.php | 16 ++-- tests/lib/IntegrityCheck/CheckerTest.php | 66 +++++++-------- 18 files changed, 271 insertions(+), 163 deletions(-) create mode 100644 apps/files_external/tests/Service/EncryptionServiceTest.php diff --git a/3rdparty b/3rdparty index 23b76aa5251ad..5935defbd4b61 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 806b6b954b723..238db34b37967 100644 --- a/apps/encryption/lib/Crypto/Crypt.php +++ b/apps/encryption/lib/Crypto/Crypt.php @@ -17,7 +17,7 @@ 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 @@ public function useLegacyBase64Encoding(): bool { */ 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 @@ private function rc4Decrypt(string $data, string $secret): string { */ 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 5e3ceb91842d8..64a9fe141d0e1 100644 --- a/apps/files_external/lib/Controller/AjaxController.php +++ b/apps/files_external/lib/Controller/AjaxController.php @@ -78,10 +78,15 @@ public function getApplicableEntities(string $pattern = '', ?int $limit = null, */ 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 @@ private function generateSshKeys($keyLength) { 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 807445f0e27d2..c55bb5c4ec3c7 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 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 @@ public function __construct( */ #[\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 @@ public function manipulateStorageConfig(StorageConfig &$storage, ?IUser $user = * 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 a954467ebe96c..16260c0d074f2 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 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 @@ public function __construct( */ #[\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 f2b8924673a61..67a38f7019fd3 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\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 @@ public function __construct(array $parameters) { /** * 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 @@ public function getConnection(): \phpseclib\Net\SFTP { $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 @@ public function fopen(string $path, string $mode) { 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 @@ public function copy(string $source, string $target): bool { $absTarget = $this->absPath($target); $connection = $this->getConnection(); - $size = $connection->size($absSource); + $size = $connection->filesize($absSource); if ($size === false) { return false; } @@ -498,7 +497,7 @@ public function copy(string $source, string $target): bool { 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 5007c251a47eb..78457a7a48a42 100644 --- a/apps/files_external/lib/Lib/Storage/SFTPReadStream.php +++ b/apps/files_external/lib/Lib/Storage/SFTPReadStream.php @@ -10,13 +10,13 @@ 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 @@ protected function loadContext(string $name) { } 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 4673573589047..5dfdead4e73a4 100644 --- a/apps/files_external/lib/Lib/Storage/SFTPWriteStream.php +++ b/apps/files_external/lib/Lib/Storage/SFTPWriteStream.php @@ -10,13 +10,13 @@ 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 @@ protected function loadContext(string $name) { } 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 @@ public function stream_open($path, $mode, $options, &$opened_path) { 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 c36f6d0613269..6a36cdad4e231 100644 --- a/apps/files_external/lib/Service/EncryptionService.php +++ b/apps/files_external/lib/Service/EncryptionService.php @@ -12,7 +12,7 @@ use OCP\IConfig; use OCP\Security\ISecureRandom; -use phpseclib\Crypt\AES; +use phpseclib3\Crypt\AES; class EncryptionService { public function __construct( @@ -78,8 +78,28 @@ private function decryptPassword(string $encryptedPassword): string { * 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 b186c48b29ced..510fd886c751c 100644 --- a/apps/files_external/tests/Controller/AjaxControllerTest.php +++ b/apps/files_external/tests/Controller/AjaxControllerTest.php @@ -21,6 +21,7 @@ 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 @@ public function testGetApplicableEntitiesWithNoResults(): void { } 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 0000000000000..a9bffb539c3f0 --- /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 dbd7cb5ac8be2..7e59e0d212c8e 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 e4d61e8cc5b08..1c0aa18a41ad4 100644 --- a/core/Command/Integrity/SignApp.php +++ b/core/Command/Integrity/SignApp.php @@ -11,8 +11,8 @@ 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 @@ protected function execute(InputInterface $input, OutputInterface $output): int 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 73b7e76b4ba18..b718b6c3a6f37 100644 --- a/core/Command/Integrity/SignCore.php +++ b/core/Command/Integrity/SignCore.php @@ -10,8 +10,8 @@ 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 @@ protected function execute(InputInterface $input, OutputInterface $output): int 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 5990e93277da3..b1e444f6b984a 100644 --- a/lib/private/Installer.php +++ b/lib/private/Installer.php @@ -31,7 +31,7 @@ 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 8425a96bff30a..7c03274a3a9c6 100644 --- a/lib/private/IntegrityCheck/Checker.php +++ b/lib/private/IntegrityCheck/Checker.php @@ -22,8 +22,8 @@ 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 @@ private function generateHashes(\RecursiveIteratorIterator $iterator, * * @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 @@ private function createSignatureData(array $hashes, * * @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 @@ public function writeAppSignature($path, 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 @@ public function writeAppSignature($path, * 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 @@ private function verify(string $signaturePath, string $basePath, string $certifi $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 @@ private function verify(string $signaturePath, string $basePath, string $certifi } // 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 711e9f98e9ca1..8db44ecee00b6 100644 --- a/lib/private/Security/Crypto.php +++ b/lib/private/Security/Crypto.php @@ -12,8 +12,8 @@ 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 @@ public function encrypt( $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 @@ private function decryptWithoutSecret(string $authenticatedCiphertext, string $p $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 6579c34c9b9e2..f69f5e9b52f48 100644 --- a/tests/lib/IntegrityCheck/CheckerTest.php +++ b/tests/lib/IntegrityCheck/CheckerTest.php @@ -19,26 +19,27 @@ 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 @@ public function testWriteAppSignatureOfNotExistingApp(): void { $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 @@ public function testWriteAppSignatureWrongPermissions(): void { $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 @@ public function testWriteAppSignature(): void { "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 @@ public function testWriteAppSignature(): void { $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 @@ public function testWriteCoreSignatureWithException(): void { $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 @@ public function testWriteCoreSignatureWrongPermissions(): void { $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 @@ public function testWriteCoreSignature(): void { $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 @@ public function testWriteCoreSignatureWithUnmodifiedHtaccess(): void { $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 @@ public function testWriteCoreSignatureWithInvalidModifiedHtaccess(): void { $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 @@ public function testWriteCoreSignatureWithValidModifiedHtaccess(): void { $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 @@ public function testVerifyCoreWithDifferentScope(): void { } 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 @@ public function testRunInstanceVerification(): void { ]) ->getMock(); - $this->checker + $checker ->expects($this->once()) ->method('verifyCoreSignature'); $this->appManager @@ -1020,7 +1022,7 @@ public function testRunInstanceVerification(): void { 'calendar', 'dav', ]; - $this->checker + $checker ->expects($this->exactly(3)) ->method('verifyAppSignature') ->willReturnCallback(function ($app) use (&$calls) { @@ -1047,7 +1049,7 @@ public function testRunInstanceVerification(): void { ->method('deleteKey') ->with('core', 'oc.integritycheck.checker'); - $this->checker->runInstanceVerification(); + $checker->runInstanceVerification(); } public function testVerifyAppSignatureWithoutSignatureDataAndCodeCheckerDisabled(): void { From fbd39500b44afe927f5d62f8424d6e8f03cdcd2b Mon Sep 17 00:00:00 2001 From: Josh Date: Sat, 6 Sep 2025 22:50:39 -0400 Subject: [PATCH 2/5] fix: adjust hash to v2 library default Signed-off-by: Josh --- lib/private/IntegrityCheck/Checker.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/private/IntegrityCheck/Checker.php b/lib/private/IntegrityCheck/Checker.php index 7c03274a3a9c6..3f3d135b90226 100644 --- a/lib/private/IntegrityCheck/Checker.php +++ b/lib/private/IntegrityCheck/Checker.php @@ -178,6 +178,7 @@ private function createSignatureData( $signature = $privateKey ->withPadding(RSA::SIGNATURE_PSS) + ->withHash('sha1') ->withMGFHash('sha512') ->withSaltLength(0) ->sign(json_encode($hashes)); @@ -318,6 +319,7 @@ private function verify(string $signaturePath, string $basePath, string $certifi $rsa = $rsa ->withPadding(RSA::SIGNATURE_PSS) + ->withHash('sha1') ->withMGFHash('sha512') ->withSaltLength(0); From fb2a62d35f4f92e7852da05516daacbf75a61bf8 Mon Sep 17 00:00:00 2001 From: Josh Date: Thu, 25 Sep 2025 23:34:18 -0400 Subject: [PATCH 3/5] fix: set PSS padding / options for RSA keys for v2->v3 consistency Updated RSA key loading to use PSS padding and options. Signed-off-by: Josh --- tests/lib/IntegrityCheck/CheckerTest.php | 47 +++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/tests/lib/IntegrityCheck/CheckerTest.php b/tests/lib/IntegrityCheck/CheckerTest.php index f69f5e9b52f48..f86f2acc4eaac 100644 --- a/tests/lib/IntegrityCheck/CheckerTest.php +++ b/tests/lib/IntegrityCheck/CheckerTest.php @@ -95,6 +95,11 @@ public function testWriteAppSignatureOfNotExistingApp(): void { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.key'); $rsa = RSA::loadPrivateKey($rsaPrivateKey); + // After loading the key, always set the PSS padding and options: + $rsa = $rsa + ->withPadding(RSA::SIGNATURE_PSS) + ->withMGFHash('sha512') + ->withSaltLength(0); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeAppSignature('NotExistingApp', $x509, $rsa); @@ -113,6 +118,11 @@ public function testWriteAppSignatureWrongPermissions(): void { $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.key'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.key'); $rsa = RSA::loadPrivateKey($rsaPrivateKey); + // After loading the key, always set the PSS padding and options: + $rsa = $rsa + ->withPadding(RSA::SIGNATURE_PSS) + ->withMGFHash('sha512') + ->withSaltLength(0); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeAppSignature(\OC::$SERVERROOT . '/tests/data/integritycheck/app/', $x509, $rsa); @@ -150,7 +160,12 @@ public function testWriteAppSignature(): void { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.key'); - $rsa = RSA::load($rsaPrivateKey); + $rsa = RSA::loadPrivateKey($rsaPrivateKey); + // After loading the key, always set the PSS padding and options: + $rsa = $rsa + ->withPadding(RSA::SIGNATURE_PSS) + ->withMGFHash('sha512') + ->withSaltLength(0); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeAppSignature(\OC::$SERVERROOT . '/tests/data/integritycheck/app/', $x509, $rsa); @@ -454,6 +469,11 @@ public function testWriteCoreSignatureWithException(): void { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.key'); $rsa = RSA::loadPrivateKey($rsaPrivateKey); + // After loading the key, always set the PSS padding and options: + $rsa = $rsa + ->withPadding(RSA::SIGNATURE_PSS) + ->withMGFHash('sha512') + ->withSaltLength(0); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeCoreSignature($x509, $rsa, __DIR__); @@ -476,6 +496,11 @@ public function testWriteCoreSignatureWrongPermissions(): void { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/SomeApp.key'); $rsa = RSA::loadPrivateKey($rsaPrivateKey); + // After loading the key, always set the PSS padding and options: + $rsa = $rsa + ->withPadding(RSA::SIGNATURE_PSS) + ->withMGFHash('sha512') + ->withSaltLength(0); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeCoreSignature($x509, $rsa, __DIR__); @@ -510,6 +535,11 @@ public function testWriteCoreSignature(): void { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/core.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/core.key'); $rsa = RSA::loadPrivateKey($rsaPrivateKey); + // After loading the key, always set the PSS padding and options: + $rsa = $rsa + ->withPadding(RSA::SIGNATURE_PSS) + ->withMGFHash('sha512') + ->withSaltLength(0); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeCoreSignature($x509, $rsa, \OC::$SERVERROOT . '/tests/data/integritycheck/app/'); @@ -544,6 +574,11 @@ public function testWriteCoreSignatureWithUnmodifiedHtaccess(): void { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/core.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/core.key'); $rsa = RSA::loadPrivateKey($rsaPrivateKey); + // After loading the key, always set the PSS padding and options: + $rsa = $rsa + ->withPadding(RSA::SIGNATURE_PSS) + ->withMGFHash('sha512') + ->withSaltLength(0); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeCoreSignature($x509, $rsa, \OC::$SERVERROOT . '/tests/data/integritycheck/htaccessUnmodified/'); @@ -573,6 +608,11 @@ public function testWriteCoreSignatureWithInvalidModifiedHtaccess(): void { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/core.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/core.key'); $rsa = RSA::loadPrivateKey($rsaPrivateKey); + // After loading the key, always set the PSS padding and options: + $rsa = $rsa + ->withPadding(RSA::SIGNATURE_PSS) + ->withMGFHash('sha512') + ->withSaltLength(0); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeCoreSignature($x509, $rsa, \OC::$SERVERROOT . '/tests/data/integritycheck/htaccessWithInvalidModifiedContent/'); @@ -608,6 +648,11 @@ public function testWriteCoreSignatureWithValidModifiedHtaccess(): void { $keyBundle = file_get_contents(__DIR__ . '/../../data/integritycheck/core.crt'); $rsaPrivateKey = file_get_contents(__DIR__ . '/../../data/integritycheck/core.key'); $rsa = RSA::loadPrivateKey($rsaPrivateKey); + // After loading the key, always set the PSS padding and options: + $rsa = $rsa + ->withPadding(RSA::SIGNATURE_PSS) + ->withMGFHash('sha512') + ->withSaltLength(0); $x509 = new X509(); $x509->loadX509($keyBundle); $this->checker->writeCoreSignature($x509, $rsa, \OC::$SERVERROOT . '/tests/data/integritycheck/htaccessWithValidModifiedContent'); From a38f3484cfb62ed0a82b0430d887ae6a183aa0b9 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Sun, 19 Jul 2026 21:13:02 +0200 Subject: [PATCH 4/5] fix: salt size needs to be 16, 24 or 32 bytes not random 30 bytes Signed-off-by: Ferdinand Thiessen --- lib/private/Repair/NC25/AddMissingSecretJob.php | 2 +- lib/private/Setup.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/private/Repair/NC25/AddMissingSecretJob.php b/lib/private/Repair/NC25/AddMissingSecretJob.php index 5dabe2577369a..eb9302a3bab4b 100644 --- a/lib/private/Repair/NC25/AddMissingSecretJob.php +++ b/lib/private/Repair/NC25/AddMissingSecretJob.php @@ -32,7 +32,7 @@ public function run(IOutput $output): void { $passwordSalt = $this->config->getSystemValueString('passwordsalt', ''); if ($passwordSalt === '') { try { - $this->config->setSystemValue('passwordsalt', $this->random->generate(30)); + $this->config->setSystemValue('passwordsalt', $this->random->generate(32)); } catch (HintException $e) { $output->warning('passwordsalt is missing from your config.php and your config.php is read only. Please fix it manually.'); } diff --git a/lib/private/Setup.php b/lib/private/Setup.php index 05a55a149fa2e..7f598f7abc843 100644 --- a/lib/private/Setup.php +++ b/lib/private/Setup.php @@ -57,7 +57,7 @@ use Psr\Log\LoggerInterface; class Setup { - public const MIN_PASSWORD_SALT_LENGTH = 30; + public const MIN_PASSWORD_SALT_LENGTH = 32; public const MIN_SECRET_LENGTH = 48; protected IL10N $l10n; From a7ae6df4fbe4aaba0f406fdd4595f24350500934 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Sun, 19 Jul 2026 23:43:58 +0200 Subject: [PATCH 5/5] fix(files_external): adjust custom sftp stream for phpseclib v3 The implementation was using private fields of v2, which are now properly marked as private. So either drop the custom streams or use reflection. Assisted-by: ClaudeCode:claude-opus-4-8 Signed-off-by: Ferdinand Thiessen --- .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + .../lib/Lib/Storage/SFTPReadStream.php | 33 +++++++------ .../lib/Lib/Storage/SFTPReflection.php | 47 +++++++++++++++++++ .../lib/Lib/Storage/SFTPWriteStream.php | 22 +++++---- 5 files changed, 82 insertions(+), 22 deletions(-) create mode 100644 apps/files_external/lib/Lib/Storage/SFTPReflection.php diff --git a/apps/files_external/composer/composer/autoload_classmap.php b/apps/files_external/composer/composer/autoload_classmap.php index e8bf64ac1688d..6cb11dc66fe85 100644 --- a/apps/files_external/composer/composer/autoload_classmap.php +++ b/apps/files_external/composer/composer/autoload_classmap.php @@ -96,6 +96,7 @@ 'OCA\\Files_External\\Lib\\Storage\\OwnCloud' => $baseDir . '/../lib/Lib/Storage/OwnCloud.php', 'OCA\\Files_External\\Lib\\Storage\\SFTP' => $baseDir . '/../lib/Lib/Storage/SFTP.php', 'OCA\\Files_External\\Lib\\Storage\\SFTPReadStream' => $baseDir . '/../lib/Lib/Storage/SFTPReadStream.php', + 'OCA\\Files_External\\Lib\\Storage\\SFTPReflection' => $baseDir . '/../lib/Lib/Storage/SFTPReflection.php', 'OCA\\Files_External\\Lib\\Storage\\SFTPWriteStream' => $baseDir . '/../lib/Lib/Storage/SFTPWriteStream.php', 'OCA\\Files_External\\Lib\\Storage\\SMB' => $baseDir . '/../lib/Lib/Storage/SMB.php', 'OCA\\Files_External\\Lib\\Storage\\StreamWrapper' => $baseDir . '/../lib/Lib/Storage/StreamWrapper.php', diff --git a/apps/files_external/composer/composer/autoload_static.php b/apps/files_external/composer/composer/autoload_static.php index 9b0a1df75b051..132da2a2eff10 100644 --- a/apps/files_external/composer/composer/autoload_static.php +++ b/apps/files_external/composer/composer/autoload_static.php @@ -111,6 +111,7 @@ class ComposerStaticInitFiles_External 'OCA\\Files_External\\Lib\\Storage\\OwnCloud' => __DIR__ . '/..' . '/../lib/Lib/Storage/OwnCloud.php', 'OCA\\Files_External\\Lib\\Storage\\SFTP' => __DIR__ . '/..' . '/../lib/Lib/Storage/SFTP.php', 'OCA\\Files_External\\Lib\\Storage\\SFTPReadStream' => __DIR__ . '/..' . '/../lib/Lib/Storage/SFTPReadStream.php', + 'OCA\\Files_External\\Lib\\Storage\\SFTPReflection' => __DIR__ . '/..' . '/../lib/Lib/Storage/SFTPReflection.php', 'OCA\\Files_External\\Lib\\Storage\\SFTPWriteStream' => __DIR__ . '/..' . '/../lib/Lib/Storage/SFTPWriteStream.php', 'OCA\\Files_External\\Lib\\Storage\\SMB' => __DIR__ . '/..' . '/../lib/Lib/Storage/SMB.php', 'OCA\\Files_External\\Lib\\Storage\\StreamWrapper' => __DIR__ . '/..' . '/../lib/Lib/Storage/StreamWrapper.php', diff --git a/apps/files_external/lib/Lib/Storage/SFTPReadStream.php b/apps/files_external/lib/Lib/Storage/SFTPReadStream.php index 78457a7a48a42..0547bf25c59a2 100644 --- a/apps/files_external/lib/Lib/Storage/SFTPReadStream.php +++ b/apps/files_external/lib/Lib/Storage/SFTPReadStream.php @@ -13,6 +13,8 @@ use phpseclib3\Net\SSH2; class SFTPReadStream implements File { + use SFTPReflection; + /** @var resource */ public $context; @@ -73,27 +75,25 @@ public function stream_open($path, $mode, $options, &$opened_path) { $this->loadContext('sftp'); - if (!($this->sftp->bitmap & SSH2::MASK_LOGIN)) { + if (!($this->getSftpProperty($this->sftp, 'bitmap') & SSH2::MASK_LOGIN)) { return false; } - $remote_file = $this->sftp->_realpath($path); + $remote_file = $this->sftp->realpath($path); if ($remote_file === false) { return false; } $packet = pack('Na*N2', strlen($remote_file), $remote_file, NET_SFTP_OPEN_READ, 0); - if (!$this->sftp->_send_sftp_packet(NET_SFTP_OPEN, $packet)) { - return false; - } + $this->invokeSftp($this->sftp, 'send_sftp_packet', [NET_SFTP_OPEN, $packet]); - $response = $this->sftp->_get_sftp_packet(); - switch ($this->sftp->packet_type) { + $response = $this->invokeSftp($this->sftp, 'get_sftp_packet'); + switch ($this->getSftpProperty($this->sftp, 'packet_type')) { case NET_SFTP_HANDLE: $this->handle = substr($response, 4); break; case NET_SFTP_STATUS: // presumably SSH_FX_NO_SUCH_FILE or SSH_FX_PERMISSION_DENIED - $this->sftp->_logError($response); + $this->invokeSftp($this->sftp, 'logError', [$response]); return false; default: user_error('Expected SSH_FXP_HANDLE or SSH_FXP_STATUS'); @@ -152,19 +152,24 @@ public function stream_read($count) { private function request_chunk(int $size) { if ($this->pendingRead) { - $this->sftp->_get_sftp_packet(); + $this->invokeSftp($this->sftp, 'get_sftp_packet'); } $packet = pack('Na*N3', strlen($this->handle), $this->handle, $this->internalPosition / 4294967296, $this->internalPosition, $size); $this->pendingRead = true; - return $this->sftp->_send_sftp_packet(NET_SFTP_READ, $packet); + try { + $this->invokeSftp($this->sftp, 'send_sftp_packet', [NET_SFTP_READ, $packet]); + return true; + } catch (\Throwable) { + return false; + } } private function read_chunk() { $this->pendingRead = false; - $response = $this->sftp->_get_sftp_packet(); + $response = $this->invokeSftp($this->sftp, 'get_sftp_packet'); - switch ($this->sftp->packet_type) { + switch ($this->getSftpProperty($this->sftp, 'packet_type')) { case NET_SFTP_DATA: $temp = substr($response, 4); $len = strlen($temp); @@ -220,9 +225,9 @@ public function stream_eof() { public function stream_close() { // we still have a read request incoming that needs to be handled before we can close if ($this->pendingRead) { - $this->sftp->_get_sftp_packet(); + $this->invokeSftp($this->sftp, 'get_sftp_packet'); } - if (!$this->sftp->_close_handle($this->handle)) { + if (!$this->invokeSftp($this->sftp, 'close_handle', [$this->handle])) { return false; } return true; diff --git a/apps/files_external/lib/Lib/Storage/SFTPReflection.php b/apps/files_external/lib/Lib/Storage/SFTPReflection.php new file mode 100644 index 0000000000000..830d28351dd24 --- /dev/null +++ b/apps/files_external/lib/Lib/Storage/SFTPReflection.php @@ -0,0 +1,47 @@ + */ + private static array $sftpReflectionMethods = []; + /** @var array */ + private static array $sftpReflectionProperties = []; + + /** + * Invoke a method that is private in phpseclib v3. + */ + private function invokeSftp(SFTP $sftp, string $method, array $arguments = []): mixed { + self::$sftpReflectionMethods[$method] ??= new \ReflectionMethod(SFTP::class, $method); + return self::$sftpReflectionMethods[$method]->invokeArgs($sftp, $arguments); + } + + /** + * Read a property that is private/protected in phpseclib v3. + */ + private function getSftpProperty(SFTP $sftp, string $property): mixed { + self::$sftpReflectionProperties[$property] ??= new \ReflectionProperty(SFTP::class, $property); + return self::$sftpReflectionProperties[$property]->getValue($sftp); + } +} diff --git a/apps/files_external/lib/Lib/Storage/SFTPWriteStream.php b/apps/files_external/lib/Lib/Storage/SFTPWriteStream.php index 5dfdead4e73a4..d9e63650b2ab4 100644 --- a/apps/files_external/lib/Lib/Storage/SFTPWriteStream.php +++ b/apps/files_external/lib/Lib/Storage/SFTPWriteStream.php @@ -13,6 +13,8 @@ use phpseclib3\Net\SSH2; class SFTPWriteStream implements File { + use SFTPReflection; + /** @var resource */ public $context; @@ -70,7 +72,7 @@ public function stream_open($path, $mode, $options, &$opened_path) { $this->loadContext('sftp'); - if (!($this->sftp->bitmap & SSH2::MASK_LOGIN)) { + if (!($this->getSftpProperty($this->sftp, 'bitmap') & SSH2::MASK_LOGIN)) { return false; } @@ -82,17 +84,19 @@ public function stream_open($path, $mode, $options, &$opened_path) { } $packet = pack('Na*N2', strlen($remote_file), $remote_file, NET_SFTP_OPEN_WRITE | NET_SFTP_OPEN_CREATE | NET_SFTP_OPEN_TRUNCATE, 0); - if (!$this->sftp->_send_sftp_packet(NET_SFTP_OPEN, $packet)) { + try { + $this->invokeSftp($this->sftp, 'send_sftp_packet', [NET_SFTP_OPEN, $packet]); + } catch (\Throwable) { return false; } - $response = $this->sftp->_get_sftp_packet(); - switch ($this->sftp->packet_type) { + $response = $this->invokeSftp($this->sftp, 'get_sftp_packet'); + switch ($this->getSftpProperty($this->sftp, 'packet_type')) { case NET_SFTP_HANDLE: $this->handle = substr($response, 4); break; case NET_SFTP_STATUS: // presumably SSH_FX_NO_SUCH_FILE or SSH_FX_PERMISSION_DENIED - $this->sftp->_logError($response); + $this->invokeSftp($this->sftp, 'logError', [$response]); return false; default: user_error('Expected SSH_FXP_HANDLE or SSH_FXP_STATUS'); @@ -157,13 +161,15 @@ public function stream_lock($operation) { public function stream_flush() { $size = strlen($this->buffer); $packet = pack('Na*N3a*', strlen($this->handle), $this->handle, $this->internalPosition / 4294967296, $this->internalPosition, $size, $this->buffer); - if (!$this->sftp->_send_sftp_packet(NET_SFTP_WRITE, $packet)) { + try { + $this->invokeSftp($this->sftp, 'send_sftp_packet', [NET_SFTP_WRITE, $packet]); + } catch (\Throwable) { return false; } $this->internalPosition += $size; $this->buffer = ''; - return $this->sftp->_read_put_responses(1); + return $this->invokeSftp($this->sftp, 'read_put_responses', [1]); } #[\Override] @@ -174,7 +180,7 @@ public function stream_eof() { #[\Override] public function stream_close() { $this->stream_flush(); - if (!$this->sftp->_close_handle($this->handle)) { + if (!$this->invokeSftp($this->sftp, 'close_handle', [$this->handle])) { return false; } $this->sftp->touch($this->path, time(), time());