Skip to content

Bypass signature mismatch in the encrypted file#34710

Closed
sharidas wants to merge 1 commit intomasterfrom
bypass-signature-check
Closed

Bypass signature mismatch in the encrypted file#34710
sharidas wants to merge 1 commit intomasterfrom
bypass-signature-check

Conversation

@sharidas
Copy link
Contributor

@sharidas sharidas commented Mar 7, 2019

Bypass signature mismatch in the encrypted file.
This would be handy when files are transferred to
other users. In this changeset the HintException
is not re-thrown. Instead it is catched and the
exception is logged.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

The signature check done during decryption is bypassed. The reason/motivation behind this change set is, due to known reason ( yet to be investigated ), the files have encrypted column in the file cache with values which cause signature mismatch. And this was resulting in exception, HintException. So if the user tries to transfer files from say user1 to user2, then the transfer is aborted because of this. During this task, it is also observed that bypassing the signature does not prohibit the decryption of files. The files tested ( the txt files ) were successfully decrypted.

Few details regarding the implementation details of this changeset:

I have used Symfony events to let the transfer-ownership command know which are the files affected.
The symfony event files.aftersignaturemismatch is transmitted from :

  • lib/private/Files/Stream/Encryption.php

The listeners of this event in the order of priority are ( the higher positive number the higher priority ):

  • apps/encryption/lib/Crypto/Crypt.php, Priority => 30, Function signatureMismatchEvent
  • lib/private/Files/Storage/Wrapper/Encryption.php, Priority => 20, Function copyBetweenStorage
  • apps/files/lib/Command/TransferOwnership.php, Priority => 10, Function transfer

When the signature mismatch happens, an argument signatureMismatch is set to true. This is verified once the read operation is completed at copyBetweenStorage and the file which has signature mismatch issue is detected and an argument fileName is added to this event which has value of the file name. Also this is logged in the owncloud.log file. This is verified in the transfer ownership command and an array is populated to grab the affected files. Once the rename operation in the transfer ownership is completed, the list of affected files are shown to the admin, if any are available.

Related Issue

  • Fixes <issue_link>

Motivation and Context

Bypass the signature check in the Crypt and log the affected files which have signature mismatch.

How Has This Been Tested?

  • Create 2 users user1 and user2 ( kindly login to both users )
  • As user1 create a folder test and create files as under test
    • a.txt
    • b.txt
    • big.txt

Try to edit the database as shown below:

mysql> select fileid,encrypted from oc_filecache where path="files/test/a.txt";
+--------+-----------+
| fileid | encrypted |
+--------+-----------+
|     45 |         3 |
+--------+-----------+
1 row in set (0.00 sec)

mysql> select fileid,encrypted from oc_filecache where path="files/test/big.txt";
+--------+-----------+
| fileid | encrypted |
+--------+-----------+
|     66 |         1 |
+--------+-----------+
1 row in set (0.00 sec)

mysql> update oc_filecache set encrypted=7 where fileid=45;
Query OK, 1 row affected (0.00 sec)
Rows matched: 1  Changed: 1  Warnings: 0

mysql> update oc_filecache set encrypted=10 where fileid=66;
Query OK, 1 row affected (0.00 sec)
Rows matched: 1  Changed: 1  Warnings: 0

mysql> select fileid,encrypted from oc_filecache where path="files/test/a.txt";
+--------+-----------+
| fileid | encrypted |
+--------+-----------+
|     45 |         7 |
+--------+-----------+
1 row in set (0.00 sec)

mysql> select fileid,encrypted from oc_filecache where path="files/test/a.txt";
+--------+-----------+
| fileid | encrypted |
+--------+-----------+
|     45 |         7 |
+--------+-----------+
1 row in set (0.00 sec)

mysql> 
  • Now run transferownership command as shown below:
 sujith@sujith-ownCloud  ~/test/owncloud   master ●  ./occ files:transfer-ownership user1 user2
Cannot load Xdebug - it was already loaded
Analysing files of user1 ...
    4 [============================]
Collecting all share information for files and folder of user1 ...
    0 [>---------------------------]
Transferring files to user2/files/transferred from user1 on 20190307_114729 ...
The affected files are : files/test/a.txt, files/test/big.txt
Restoring shares ...
    0 [>---------------------------]
 sujith@sujith-ownCloud  ~/test/owncloud   bypass-signature-check 
  • The affected files were a.txt and big.txt were listed as shown above.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

Bypass signature mismatch in the encrypted file.
This would be handy when files are transferred to
other users. In this changeset the HintException
is not re-thrown. Instead it is catched and the
exception is logged.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@sharidas
Copy link
Contributor Author

sharidas commented Mar 7, 2019

I would like to have feedback here regarding the approach taken to achieve the list of affected files. Let me also know if there is a better approach here. Thanks.

@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #34710 into master will decrease coverage by 0.01%.
The diff coverage is 32%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #34710      +/-   ##
============================================
- Coverage     65.28%   65.27%   -0.02%     
- Complexity    18481    18497      +16     
============================================
  Files          1210     1210              
  Lines         69984    70008      +24     
  Branches       1280     1280              
============================================
+ Hits          45688    45696       +8     
- Misses        23924    23940      +16     
  Partials        372      372
Flag Coverage Δ Complexity Δ
#javascript 53.04% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.67% <32%> (-0.02%) 18497 <8> (+16)
Impacted Files Coverage Δ Complexity Δ
apps/files/lib/Command/TransferOwnership.php 0% <0%> (ø) 55 <5> (+9) ⬆️
lib/private/Files/Stream/Encryption.php 94.31% <100%> (+0.06%) 53 <0> (ø) ⬇️
lib/private/Files/Storage/Wrapper/Encryption.php 64.9% <66.66%> (+0.03%) 163 <3> (+7) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb21e34...75d111f. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #34710 into master will decrease coverage by 0.01%.
The diff coverage is 32%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #34710      +/-   ##
============================================
- Coverage     65.28%   65.27%   -0.02%     
- Complexity    18481    18497      +16     
============================================
  Files          1210     1210              
  Lines         69984    70008      +24     
  Branches       1280     1280              
============================================
+ Hits          45688    45696       +8     
- Misses        23924    23940      +16     
  Partials        372      372
Flag Coverage Δ Complexity Δ
#javascript 53.04% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.67% <32%> (-0.02%) 18497 <8> (+16)
Impacted Files Coverage Δ Complexity Δ
apps/files/lib/Command/TransferOwnership.php 0% <0%> (ø) 55 <5> (+9) ⬆️
lib/private/Files/Stream/Encryption.php 94.31% <100%> (+0.06%) 53 <0> (ø) ⬇️
lib/private/Files/Storage/Wrapper/Encryption.php 64.9% <66.66%> (+0.03%) 163 <3> (+7) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb21e34...75d111f. Read the comment docs.

@sharidas
Copy link
Contributor Author

sharidas commented Apr 5, 2019

Closing this PR in favour of owncloud/encryption#115

@sharidas sharidas closed this Apr 5, 2019
@sharidas sharidas deleted the bypass-signature-check branch April 5, 2019 08:00
@lock lock bot locked as resolved and limited conversation to collaborators Apr 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant