Skip to content

Commit e423fb4

Browse files
come-nccome-nc
authored
Merge pull request #53666 from nextcloud/backport/53665/stable31
[stable31] fix(encryption): Catch exceptions in encrypt-all command and continue
2 parents 0eed4ba + e2861b4 commit e423fb4

File tree

3 files changed

+89
-71
lines changed

3 files changed

+89
-71
lines changed

‎apps/encryption/lib/Crypto/EncryptAll.php‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
use OCP\Mail\Headers\AutoSubmitted;
2121
use OCP\Mail\IMailer;
2222
use OCP\Security\ISecureRandom;
23+
use Psr\Log\LoggerInterface;
2324
use Symfony\Component\Console\Helper\ProgressBar;
2425
use Symfony\Component\Console\Helper\QuestionHelper;
2526
use Symfony\Component\Console\Helper\Table;
@@ -50,6 +51,7 @@ public function __construct(
5051
protected IFactory $l10nFactory,
5152
protected QuestionHelper $questionHelper,
5253
protected ISecureRandom $secureRandom,
54+
protected LoggerInterface $logger,
5355
) {
5456
// store one time passwords for the users
5557
$this->userPasswords = [];
@@ -207,9 +209,22 @@ protected function encryptUsersFiles($uid, ProgressBar $progress, $userCount) {
207209
} else {
208210
$progress->setMessage("encrypt files for user $userCount: $path");
209211
$progress->advance();
210-
if ($this->encryptFile($path) === false) {
211-
$progress->setMessage("encrypt files for user $userCount: $path (already encrypted)");
212+
try {
213+
if ($this->encryptFile($path) === false) {
214+
$progress->setMessage("encrypt files for user $userCount: $path (already encrypted)");
215+
$progress->advance();
216+
}
217+
} catch (\Exception $e) {
218+
$progress->setMessage("Failed to encrypt path $path: " . $e->getMessage());
212219
$progress->advance();
220+
$this->logger->error(
221+
'Failed to encrypt path {path}',
222+
[
223+
'user' => $uid,
224+
'path' => $path,
225+
'exception' => $e,
226+
]
227+
);
213228
}
214229
}
215230
}
@@ -234,7 +249,14 @@ protected function encryptFile($path) {
234249
$target = $path . '.encrypted.' . time();
235250

236251
try {
237-
$this->rootView->copy($source, $target);
252+
$copySuccess = $this->rootView->copy($source, $target);
253+
if ($copySuccess === false) {
254+
/* Copy failed, abort */
255+
if ($this->rootView->file_exists($target)) {
256+
$this->rootView->unlink($target);
257+
}
258+
throw new \Exception('Copy failed for ' . $source);
259+
}
238260
$this->rootView->rename($target, $source);
239261
} catch (DecryptionFailedException $e) {
240262
if ($this->rootView->file_exists($target)) {

‎apps/encryption/tests/Crypto/EncryptAllTest.php‎

Lines changed: 33 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
use OCP\Mail\IMailer;
2121
use OCP\Security\ISecureRandom;
2222
use OCP\UserInterface;
23+
use PHPUnit\Framework\MockObject\MockObject;
24+
use Psr\Log\LoggerInterface;
2325
use Symfony\Component\Console\Formatter\OutputFormatterInterface;
2426
use Symfony\Component\Console\Helper\ProgressBar;
2527
use Symfony\Component\Console\Helper\QuestionHelper;
@@ -28,48 +30,21 @@
2830
use Test\TestCase;
2931

3032
class EncryptAllTest extends TestCase {
31-
32-
/** @var \PHPUnit\Framework\MockObject\MockObject|KeyManager */
33-
protected $keyManager;
34-
35-
/** @var \PHPUnit\Framework\MockObject\MockObject|Util */
36-
protected $util;
37-
38-
/** @var \PHPUnit\Framework\MockObject\MockObject|IUserManager */
39-
protected $userManager;
40-
41-
/** @var \PHPUnit\Framework\MockObject\MockObject|Setup */
42-
protected $setupUser;
43-
44-
/** @var \PHPUnit\Framework\MockObject\MockObject|View */
45-
protected $view;
46-
47-
/** @var \PHPUnit\Framework\MockObject\MockObject|IConfig */
48-
protected $config;
49-
50-
/** @var \PHPUnit\Framework\MockObject\MockObject|IMailer */
51-
protected $mailer;
52-
53-
/** @var \PHPUnit\Framework\MockObject\MockObject|IL10N */
54-
protected $l;
55-
56-
/** @var \PHPUnit\Framework\MockObject\MockObject | IFactory */
57-
protected $l10nFactory;
58-
59-
/** @var \PHPUnit\Framework\MockObject\MockObject | \Symfony\Component\Console\Helper\QuestionHelper */
60-
protected $questionHelper;
61-
62-
/** @var \PHPUnit\Framework\MockObject\MockObject | \Symfony\Component\Console\Input\InputInterface */
63-
protected $inputInterface;
64-
65-
/** @var \PHPUnit\Framework\MockObject\MockObject | \Symfony\Component\Console\Output\OutputInterface */
66-
protected $outputInterface;
67-
68-
/** @var \PHPUnit\Framework\MockObject\MockObject|UserInterface */
69-
protected $userInterface;
70-
71-
/** @var \PHPUnit\Framework\MockObject\MockObject|ISecureRandom */
72-
protected $secureRandom;
33+
protected KeyManager&MockObject $keyManager;
34+
protected Util&MockObject $util;
35+
protected IUserManager&MockObject $userManager;
36+
protected Setup&MockObject $setupUser;
37+
protected View&MockObject $view;
38+
protected IConfig&MockObject $config;
39+
protected IMailer&MockObject $mailer;
40+
protected IL10N&MockObject $l;
41+
protected IFactory&MockObject $l10nFactory;
42+
protected \Symfony\Component\Console\Helper\QuestionHelper&MockObject $questionHelper;
43+
protected \Symfony\Component\Console\Input\InputInterface&MockObject $inputInterface;
44+
protected \Symfony\Component\Console\Output\OutputInterface&MockObject $outputInterface;
45+
protected UserInterface&MockObject $userInterface;
46+
protected ISecureRandom&MockObject $secureRandom;
47+
protected LoggerInterface&MockObject $logger;
7348

7449
/** @var EncryptAll */
7550
protected $encryptAll;
@@ -101,6 +76,7 @@ protected function setUp(): void {
10176
->disableOriginalConstructor()->getMock();
10277
$this->userInterface = $this->getMockBuilder(UserInterface::class)
10378
->disableOriginalConstructor()->getMock();
79+
$this->logger = $this->createMock(LoggerInterface::class);
10480

10581
/**
10682
* We need format method to return a string
@@ -131,12 +107,13 @@ protected function setUp(): void {
131107
$this->l,
132108
$this->l10nFactory,
133109
$this->questionHelper,
134-
$this->secureRandom
110+
$this->secureRandom,
111+
$this->logger,
135112
);
136113
}
137114

138115
public function testEncryptAll(): void {
139-
/** @var EncryptAll | \PHPUnit\Framework\MockObject\MockObject $encryptAll */
116+
/** @var EncryptAll&MockObject $encryptAll */
140117
$encryptAll = $this->getMockBuilder(EncryptAll::class)
141118
->setConstructorArgs(
142119
[
@@ -150,7 +127,8 @@ public function testEncryptAll(): void {
150127
$this->l,
151128
$this->l10nFactory,
152129
$this->questionHelper,
153-
$this->secureRandom
130+
$this->secureRandom,
131+
$this->logger,
154132
]
155133
)
156134
->setMethods(['createKeyPairs', 'encryptAllUsersFiles', 'outputPasswords'])
@@ -165,7 +143,7 @@ public function testEncryptAll(): void {
165143
}
166144

167145
public function testEncryptAllWithMasterKey(): void {
168-
/** @var EncryptAll | \PHPUnit\Framework\MockObject\MockObject $encryptAll */
146+
/** @var EncryptAll&MockObject $encryptAll */
169147
$encryptAll = $this->getMockBuilder(EncryptAll::class)
170148
->setConstructorArgs(
171149
[
@@ -179,7 +157,8 @@ public function testEncryptAllWithMasterKey(): void {
179157
$this->l,
180158
$this->l10nFactory,
181159
$this->questionHelper,
182-
$this->secureRandom
160+
$this->secureRandom,
161+
$this->logger,
183162
]
184163
)
185164
->setMethods(['createKeyPairs', 'encryptAllUsersFiles', 'outputPasswords'])
@@ -195,7 +174,7 @@ public function testEncryptAllWithMasterKey(): void {
195174
}
196175

197176
public function testCreateKeyPairs(): void {
198-
/** @var EncryptAll | \PHPUnit\Framework\MockObject\MockObject $encryptAll */
177+
/** @var EncryptAll&MockObject $encryptAll */
199178
$encryptAll = $this->getMockBuilder(EncryptAll::class)
200179
->setConstructorArgs(
201180
[
@@ -209,7 +188,8 @@ public function testCreateKeyPairs(): void {
209188
$this->l,
210189
$this->l10nFactory,
211190
$this->questionHelper,
212-
$this->secureRandom
191+
$this->secureRandom,
192+
$this->logger,
213193
]
214194
)
215195
->setMethods(['setupUserFS', 'generateOneTimePassword'])
@@ -259,7 +239,8 @@ public function testEncryptAllUsersFiles(): void {
259239
$this->l,
260240
$this->l10nFactory,
261241
$this->questionHelper,
262-
$this->secureRandom
242+
$this->secureRandom,
243+
$this->logger,
263244
]
264245
)
265246
->setMethods(['encryptUsersFiles'])
@@ -295,7 +276,8 @@ public function testEncryptUsersFiles(): void {
295276
$this->l,
296277
$this->l10nFactory,
297278
$this->questionHelper,
298-
$this->secureRandom
279+
$this->secureRandom,
280+
$this->logger,
299281
]
300282
)
301283
->setMethods(['encryptFile', 'setupUserFS'])

‎lib/private/Files/Storage/Wrapper/Encryption.php‎

Lines changed: 31 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
use OCP\Encryption\IManager;
2424
use OCP\Encryption\Keys\IStorage;
2525
use OCP\Files\Cache\ICacheEntry;
26+
use OCP\Files\GenericFileException;
2627
use OCP\Files\Mount\IMountPoint;
2728
use OCP\Files\Storage;
2829
use Psr\Log\LoggerInterface;
@@ -185,11 +186,11 @@ public function unlink(string $path): bool {
185186
public function rename(string $source, string $target): bool {
186187
$result = $this->storage->rename($source, $target);
187188

188-
if ($result &&
189+
if ($result
189190
// versions always use the keys from the original file, so we can skip
190191
// this step for versions
191-
$this->isVersion($target) === false &&
192-
$this->encryptionManager->isEnabled()) {
192+
&& $this->isVersion($target) === false
193+
&& $this->encryptionManager->isEnabled()) {
193194
$sourcePath = $this->getFullPath($source);
194195
if (!$this->util->isExcluded($sourcePath)) {
195196
$targetPath = $this->getFullPath($target);
@@ -210,9 +211,9 @@ public function rename(string $source, string $target): bool {
210211
public function rmdir(string $path): bool {
211212
$result = $this->storage->rmdir($path);
212213
$fullPath = $this->getFullPath($path);
213-
if ($result &&
214-
$this->util->isExcluded($fullPath) === false &&
215-
$this->encryptionManager->isEnabled()
214+
if ($result
215+
&& $this->util->isExcluded($fullPath) === false
216+
&& $this->encryptionManager->isEnabled()
216217
) {
217218
$this->keyStorage->deleteAllFileKeys($fullPath);
218219
}
@@ -225,9 +226,9 @@ public function isReadable(string $path): bool {
225226

226227
$metaData = $this->getMetaData($path);
227228
if (
228-
!$this->is_dir($path) &&
229-
isset($metaData['encrypted']) &&
230-
$metaData['encrypted'] === true
229+
!$this->is_dir($path)
230+
&& isset($metaData['encrypted'])
231+
&& $metaData['encrypted'] === true
231232
) {
232233
$fullPath = $this->getFullPath($path);
233234
$module = $this->getEncryptionModule($path);
@@ -384,9 +385,9 @@ protected function verifyUnencryptedSize(string $path, int $unencryptedSize): in
384385
$size = $this->storage->filesize($path);
385386
$result = $unencryptedSize;
386387

387-
if ($unencryptedSize < 0 ||
388-
($size > 0 && $unencryptedSize === $size) ||
389-
$unencryptedSize > $size
388+
if ($unencryptedSize < 0
389+
|| ($size > 0 && $unencryptedSize === $size)
390+
|| $unencryptedSize > $size
390391
) {
391392
// check if we already calculate the unencrypted size for the
392393
// given path to avoid recursions
@@ -622,8 +623,8 @@ private function copyBetweenStorage(
622623
): bool {
623624
// for versions we have nothing to do, because versions should always use the
624625
// key from the original file. Just create a 1:1 copy and done
625-
if ($this->isVersion($targetInternalPath) ||
626-
$this->isVersion($sourceInternalPath)) {
626+
if ($this->isVersion($targetInternalPath)
627+
|| $this->isVersion($sourceInternalPath)) {
627628
// remember that we try to create a version so that we can detect it during
628629
// fopen($sourceInternalPath) and by-pass the encryption in order to
629630
// create a 1:1 copy of the file
@@ -673,12 +674,16 @@ private function copyBetweenStorage(
673674
try {
674675
$source = $sourceStorage->fopen($sourceInternalPath, 'r');
675676
$target = $this->fopen($targetInternalPath, 'w');
676-
[, $result] = \OC_Helper::streamCopy($source, $target);
677+
if ($source === false || $target === false) {
678+
$result = false;
679+
} else {
680+
[, $result] = \OC_Helper::streamCopy($source, $target);
681+
}
677682
} finally {
678-
if (is_resource($source)) {
683+
if (isset($source) && $source !== false) {
679684
fclose($source);
680685
}
681-
if (is_resource($target)) {
686+
if (isset($target) && $target !== false) {
682687
fclose($target);
683688
}
684689
}
@@ -728,6 +733,9 @@ public function stat(string $path): array|false {
728733

729734
public function hash(string $type, string $path, bool $raw = false): string|false {
730735
$fh = $this->fopen($path, 'rb');
736+
if ($fh === false) {
737+
return false;
738+
}
731739
$ctx = hash_init($type);
732740
hash_update_stream($ctx, $fh);
733741
fclose($fh);
@@ -752,6 +760,9 @@ protected function readFirstBlock(string $path): string {
752760
$firstBlock = '';
753761
if ($this->storage->is_file($path)) {
754762
$handle = $this->storage->fopen($path, 'r');
763+
if ($handle === false) {
764+
return '';
765+
}
755766
$firstBlock = fread($handle, $this->util->getHeaderSize());
756767
fclose($handle);
757768
}
@@ -882,6 +893,9 @@ protected function shouldEncrypt(string $path): bool {
882893
public function writeStream(string $path, $stream, ?int $size = null): int {
883894
// always fall back to fopen
884895
$target = $this->fopen($path, 'w');
896+
if ($target === false) {
897+
throw new GenericFileException("Failed to open $path for writing");
898+
}
885899
[$count, $result] = \OC_Helper::streamCopy($stream, $target);
886900
fclose($stream);
887901
fclose($target);

0 commit comments

Comments
 (0)