Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix encrypted version to 0 when finding unencrypted file #28373

Merged
merged 2 commits into from
Aug 26, 2021
Merged

Fix encrypted version to 0 when finding unencrypted file #28373

merged 2 commits into from
Aug 26, 2021

Conversation

PVince81
Copy link
Member

@PVince81 PVince81 commented Aug 11, 2021 •
edited
Loading

Whenever the command is run and a "legacy cipher" seems to be detected
when the legacy option is disabled, it's highly likely that the file is
actually unencrypted but the database contains a encrypted version
higher than 0 for some reason.

The command now detects this case and automatically sets the encrypted
version to 0 so that the file can be read again.
⚠️ This assumes that there are no more legacy encrypted files because the admin has not set the legacy option.

  • add unit tests

Steps to test

  1. Setup NC and login as "admin"
  2. Upload a picture and rename it to "unencrypted.jpg"
  3. Enable encryption with occ app:enable encryption && occ encryption:enable
  4. Upload another picture and rename it to "encrypted.jpg"
  5. Set encrypted flag in database: update oc_filecache set encrypted=2 where name like '%unencrypted%';
  6. Try downloading the files: "unencrypted.jpg" shows an error
  7. Run occ encryption:fix-encrypted-version admin
  8. Try downloading the files: both can be downloaded properly

@PVince81 PVince81 added this to the Nextcloud 22.1.1 milestone Aug 11, 2021
@PVince81 PVince81 self-assigned this Aug 11, 2021
@PVince81
Copy link
Member Author

/backport to stable22

@PVince81
Copy link
Member Author

/backport to stable21

@PVince81
Copy link
Member Author

/backport to stable20

@PVince81 PVince81 force-pushed the enh/noid/fix-encrypted-versions-detect-unencrypted branch from 6dad182 to 532b01b Compare August 12, 2021 12:40
@PVince81
Copy link
Member Author

tests fail for me locally even if I comment out all my code changes, it's only if I delete the rows that they pass
weird shit...

@PVince81 PVince81 force-pushed the enh/noid/fix-encrypted-versions-detect-unencrypted branch from 532b01b to 8c3392f Compare August 13, 2021 09:43
@PVince81
Copy link
Member Author

I've added unit tests now

@PVince81 PVince81 marked this pull request as ready for review August 13, 2021 09:43
@PVince81
Copy link
Member Author

I've added steps to test in the original post

@PVince81 PVince81 added 3. to review Waiting for reviews feature: encryption (server-side) and removed 2. developing Work in progress labels Aug 13, 2021
@J0WI
Copy link
Contributor

J0WI commented Aug 15, 2021

Please keep CVE-2020-8150 in mind. Silently overwriting encrypted files should not be allowed.

@PVince81
Copy link
Member Author

Please keep CVE-2020-8150 in mind. Silently overwriting encrypted files should not be allowed.

not sure if this applies. @LukasReschke can you check ?

the command is only for admins to run on the CLI and it doesn't touch the files
it only makes the database flag reflect the actual situation that exists on disk, so if a file is actually unencrypted (for whatever reason, maybe restoring from unencrypted backup) but the database says it is, it will tell the database that the file is not encrypted

after that, the admin can still run "occ encrypt-all" to reencrypt those files.

@PVince81 PVince81 requested a review from LukasReschke August 16, 2021 06:15
@PVince81
Copy link
Member Author

okay, so the CVE mentions the legacy encryption scheme
this PR/modification only works if the legacy encryption is disabled (opted-out) and does not operate when it is enabled

artonge
artonge approved these changes Aug 16, 2021
View reviewed changes
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@juliusknorr
Copy link
Member

Tests do not seem to be happy, though I cannot see why that change should cause such a failure


There were 2 errors:

1) OCA\Files_Sharing\Tests\EncryptedSizePropagationTest::testSizePropagationWhenOwnerChangesFile
stream_wrapper_register(): Protocol ocencryption:// is already defined.

/drone/src/lib/private/Files/Stream/Encryption.php:208
/drone/src/lib/private/Files/Stream/Encryption.php:188
/drone/src/lib/private/Files/Storage/Wrapper/Encryption.php:471
/drone/src/lib/private/Files/Storage/Wrapper/Encryption.php:240
/drone/src/lib/private/Files/View.php:1169
/drone/src/lib/private/Files/View.php:706
/drone/src/apps/files_sharing/tests/SizePropagationTest.php:55

2) OCA\Files_Sharing\Tests\EncryptedSizePropagationTest::testSizePropagationWhenRecipientChangesFile
stream_wrapper_register(): Protocol ocencryption:// is already defined.

/drone/src/lib/private/Files/Stream/Encryption.php:208
/drone/src/lib/private/Files/Stream/Encryption.php:188
/drone/src/lib/private/Files/Storage/Wrapper/Encryption.php:471
/drone/src/lib/private/Files/Storage/Wrapper/Encryption.php:240
/drone/src/lib/private/Files/View.php:1169
/drone/src/lib/private/Files/View.php:706
/drone/src/apps/files_sharing/tests/SizePropagationTest.php:90

--

@PVince81
Copy link
Member Author

PVince81 commented Aug 17, 2021 •
edited
Loading

and yet the tests pass locally for me... might be a cleanup issue revealed by different test order

edit: wait, these tests are not even related. probably cleanup issue, yes

@PVince81
Copy link
Member Author

even when running all tests on this branch locally I couldn't reproduce this issue :-(

@PVince81 PVince81 force-pushed the enh/noid/fix-encrypted-versions-detect-unencrypted branch from 8c3392f to 75d5348 Compare August 19, 2021 12:01
@skjnldsv skjnldsv mentioned this pull request Aug 19, 2021
Merged
7 tasks
@skjnldsv
Copy link
Member

Nope, tests still fails :)

@PVince81
Copy link
Member Author

Nope, tests still fails :)

image

@PVince81 PVince81 marked this pull request as draft August 25, 2021 07:45
@PVince81
Copy link
Member Author

switched back to WIP as I'm going to use CI for debugging this strange side effect (with a reduced drone config)

@PVince81
Copy link
Member Author

oh... so even deleting my new test has the side effect 🤔

@PVince81
Copy link
Member Author

ohh, locally when I run all tests there are only 18

but here in CI there's way more, and it even runs "/drone/src/apps/files_versions/tests/VersioningTest.php:690" even though I deleted those a few commits ago... I wonder if it's a setup issue / Drone caching issue...

@PVince81 PVince81 mentioned this pull request Aug 25, 2021
Closed
@PVince81
Copy link
Member Author

let's see if a resend in a separate PR works: #28593

@PVince81
Copy link
Member Author

alright, I was looking at an old test results.
conflicts solved and the sharing test pass when encryption tests are deleted

now bringing back the tests for closer inspection...

@PVince81
Copy link
Member Author

mega facepalm

so it seems I forgot to enable the encryption app locally before running all the tests, I didn't know this was necessary and thought the test scripts would do that at some stage ?! now I'm able to see the failure locally

@PVince81
Copy link
Member Author

FINALLY some results:

stream_wrapper_unregister ocencryption in exception OC\ServerNotAvailableException: Legacy cipher is no longer supported! in /srv/www/htdocs/server/apps/encryption/lib/Crypto/Crypt.php:319
Stack trace:
#0 /srv/www/htdocs/server/apps/encryption/lib/Crypto/Encryption.php(237): OCA\Encryption\Crypto\Crypt->getLegacyCipher()
#1 /srv/www/htdocs/server/lib/private/Files/Stream/Encryption.php(294): OCA\Encryption\Crypto\Encryption->begin()
#2 [internal function]: OC\Files\Stream\Encryption->stream_open()
#3 /srv/www/htdocs/server/lib/private/Files/Stream/Encryption.php(215): fopen()
#4 /srv/www/htdocs/server/lib/private/Files/Stream/Encryption.php(189): OC\Files\Stream\Encryption::wrapSource()
#5 /srv/www/htdocs/server/lib/private/Files/Storage/Wrapper/Encryption.php(471): OC\Files\Stream\Encryption::wrap()
#6 /srv/www/htdocs/server/lib/private/Files/View.php(1169): OC\Files\Storage\Wrapper\Encryption->fopen()
#7 /srv/www/htdocs/server/lib/private/Files/View.php(1005): OC\Files\View->basicOperation()
#8 /srv/www/htdocs/server/apps/encryption/lib/Command/FixEncryptedVersion.php(188): OC\Files\View->fopen()
#9 /srv/www/htdocs/server/apps/encryption/lib/Command/FixEncryptedVersion.php(167): OCA\Encryption\Command\FixEncryptedVersion->verifyFileContent()
#10 /srv/www/htdocs/server/apps/encryption/lib/Command/FixEncryptedVersion.php(136): OCA\Encryption\Command\FixEncryptedVersion->walkPathOfUser()
#11 /srv/www/htdocs/server/3rdparty/symfony/console/Command/Command.php(255): OCA\Encryption\Command\FixEncryptedVersion->execute()
#12 /srv/www/htdocs/server/3rdparty/symfony/console/Tester/CommandTester.php(76): Symfony\Component\Console\Command\Command->run()
#13 /srv/www/htdocs/server/apps/encryption/tests/Command/FixEncryptedVersionTest.php(273): Symfony\Component\Console\Tester\CommandTester->execute()
#14 /var/lib/wwwrun/.composer/vendor/phpunit/phpunit/src/Framework/TestCase.php(1415): OCA\Encryption\Tests\Command\FixEncryptedVersionTest->testRepairUnencryptedFileWhenVersionIsSet()
#15 /var/lib/wwwrun/.composer/vendor/phpunit/phpunit/src/Framework/TestCase.php(1035): PHPUnit\Framework\TestCase->runTest()
#16 /var/lib/wwwrun/.composer/vendor/phpunit/phpunit/src/Framework/TestResult.php(691): PHPUnit\Framework\TestCase->runBare()
#17 /var/lib/wwwrun/.composer/vendor/phpunit/phpunit/src/Framework/TestCase.php(763): PHPUnit\Framework\TestResult->run()
#18 /var/lib/wwwrun/.composer/vendor/phpunit/phpunit/src/Framework/TestSuite.php(601): PHPUnit\Framework\TestCase->run()
#19 /var/lib/wwwrun/.composer/vendor/phpunit/phpunit/src/Framework/TestSuite.php(601): PHPUnit\Framework\TestSuite->run()
#20 /var/lib/wwwrun/.composer/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(633): PHPUnit\Framework\TestSuite->run()
#21 /var/lib/wwwrun/.composer/vendor/phpunit/phpunit/src/TextUI/Command.php(204): PHPUnit\TextUI\TestRunner->doRun()
#22 /var/lib/wwwrun/.composer/vendor/phpunit/phpunit/src/TextUI/Command.php(163): PHPUnit\TextUI\Command->run()
#23 /var/lib/wwwrun/.composer/vendor/phpunit/phpunit/phpunit(61): PHPUnit\TextUI\Command::main()
#24 {main}

it turns out that stream_wrapper_unregister is not called for a generic exception, just for a BadMethodCall.
so even in case of an expected exception like OC\ServerNotAvailableException, it would not clean up even though the test passes... I'll fix this

@PVince81
Fix encrypted version to 0 when finding unencrypted file
60e4407 
Whenever the command is run and a "legacy cipher" seems to be detected
when the legacy option is disabled, it's highly likely that the file is
actually unencrypted but the database contains a encrypted version
higher than 0 for some reason.

The command now detects this case and automatically sets the encrypted
version to 0 so that the file can be read again.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81 PVince81 force-pushed the enh/noid/fix-encrypted-versions-detect-unencrypted branch from 87713f9 to 28dc915 Compare August 26, 2021 08:46
@PVince81
Unregister enc stream wrapper for any exception
9c6bbfa 
This prevents side effects in tests by properly cleaning up
even with expected exceptions.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81 PVince81 force-pushed the enh/noid/fix-encrypted-versions-detect-unencrypted branch from 28dc915 to 9c6bbfa Compare August 26, 2021 08:52
@PVince81 PVince81 marked this pull request as ready for review August 26, 2021 08:53
@PVince81
Copy link
Member Author

finally green! please review the last commit which fixes the test issue, then we can merge 😄

@PVince81 PVince81 merged commit fd93aa8 into master Aug 26, 2021
@PVince81 PVince81 deleted the enh/noid/fix-encrypted-versions-detect-unencrypted branch August 26, 2021 10:51
This was referenced Aug 26, 2021
Merged
Merged
Merged
@asheroto asheroto mentioned this pull request Dec 3, 2023
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@LukasReschke LukasReschke Awaiting requested review from LukasReschke

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@artonge artonge Awaiting requested review from artonge

Assignees

@PVince81 PVince81

Labels
3. to review Waiting for reviews enhancement feature: encryption (server-side)
Projects
None yet
Milestone
Nextcloud 22.1.1
Development

Successfully merging this pull request may close these issues.
6 participants
@PVince81 @J0WI @juliusknorr @skjnldsv @artonge @CarlSchwan