Page MenuHomePhabricator

Implement elementary file operations with encryption-decryption layer
ClosedPublic

Authored by marcin on Jun 28 2022, 8:37 AM.
Tags
None
Referenced Files
F3383944: D4385.id14466.diff
Thu, Nov 28, 6:28 PM
F3383941: D4385.id14164.diff
Thu, Nov 28, 6:27 PM
Unknown Object (File)
Tue, Nov 26, 12:52 AM
Unknown Object (File)
Mon, Nov 25, 3:51 AM
Unknown Object (File)
Mon, Nov 25, 3:16 AM
Unknown Object (File)
Mon, Nov 25, 1:31 AM
Unknown Object (File)
Sun, Nov 24, 10:53 AM
Unknown Object (File)
Mon, Nov 11, 7:32 PM

Details

Summary

Implements file operations that work with encryption-decryotion layer defined in previous diffs

Test Plan

Modify NotificationService and AppDelegate so that the first writes to the file upon notification and the second read it on AppLaunch. Examine in the debugger that the content read by AppDelegate is consistent sent notifications.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested changes to this revision.Jun 29 2022, 8:19 AM

A test plan could contain a description of a code that was written to test this, e.g. a modification to AppDelegate that is run when the app starts, writes something to a file, reads it, decrypt, and checks the result.

As a question:
We have two options to write to the file

  1. We write from the beginning
  2. We append to it

At the same time, when decrypting, we do that for the whole content at the same time.
Is the decryption algorithm able to recognize where one encrypted block ends and another begins? Should we read and decrypt block-by-block? Should we separate blocks by some char, e.g. a new line? Have you tested if after writing and appending we are able to decrypt both parts?

This revision now requires changes to proceed.Jun 29 2022, 8:19 AM
In D4385#124349, @palys-swm wrote:

A test plan could contain a description of a code that was written to test this, e.g. a modification to AppDelegate that is run when the app starts, writes something to a file, reads it, decrypt, and checks the result.

I agree it is not a good practice to postpone writing test plan for an API-diff until we have a diff with its usage. I will provide test plans for all "No test plan" I have already submitted. However it is not a first time I submit "Not test plan. Future diffs will provide test plan", but it is the first time I am advised against it. Perhaps it we should persist this rule somewhere so that no one repeats this mistake in the future.

As a question:
We have two options to write to the file

  1. We write from the beginning
  2. We append to it

At the same time, when decrypting, we do that for the whole content at the same time.
Is the decryption algorithm able to recognize where one encrypted block ends and another begins? Should we read and decrypt block-by-block? Should we separate blocks by some char, e.g. a new line? Have you tested if after writing and appending we are able to decrypt both parts?

Thanks for pointing it out! I wanted to use "comm.encryptionKey" for encryption, which is 512 bit long. The only algorithm supported by CCCrypt that accepts 512 key is RC4, which happens to be stream-oriented encryption algorithm (https://pl.wikipedia.org/wiki/RC4). This is why I was able encrypt in chunks and receive exactly the same content after decrypting as a whole. This article however raises some concerns whether RC4 is a secure algorithm. Perhaps we will have to use block-oriented encryption and read in chunks as well.

Should we separate blocks by some char, e.g. a new line?

Newline separation takes place in further diffs. Each usage of [EncryptedFileUtils appendData:toFileAtPAth:] passes message with prepended newline separator. But if we decide to use block-oriented encryption we will have to move it to EncryptedFileUtils.

We could use AES-256 with the first 256 bits. Not familiar with RC4 but if it's an efficient/safe symmetric encryption algorithm, we're probably fine

Introduce files to project.pbxproj

Hit plan changes after rebase to keep it out of reviewers queue.

return earlier in readFile method

I decided to give up on RC4 since it is not considered secure and most sources I found advise against using it in modern projects. I went with AES working in ECB mode. AES in ECB mode encrypts data in 128 byte blocks treating each block independently. Therefore, by implementing padding with newline characters we can encrypt single messages independently and decrypt entire file content at once, which gives us convenient api to work with file. However AES working in CBC mode is considered more secure but every block is encrypted with previous block, so encryption operation is not additive. That would require to split file content according to some separator and decrypt each message independently. Implementation and API would be less convenient but we would align with top-notch security standards. @ashoat , @karol-bisztyga , @palys-swm what are your opinions on this matter?

I think we can do whatever TLS does

According to this response of Apple staff member: https://developer.apple.com/forums/thread/129611, we should be compliant with TLS 1.2 which according to this: https://docs.microsoft.com/en-us/power-platform/admin/server-cipher-tls-requirements does not support AES ECB, but supports AES CBC which is provided by iOS. Therefore I will reimplement file operations to use AES CBC.

Re-implement read operation to return list of appended contents

Now file operations work as follows:

  • Write:
    1. Message represented as string (NSString) is encrypted with AES CBC to binary (NSData)
    2. Encrypted binary is encoded to base64.
    3. Newline character is appended to a file
    4. Encrypted binary is appended to a file
  • Read:
    1. File content is read into a string
    2. String is split with spearator being newline character
    3. Each slice is independently decrypted and decoded from base64 to the original message

Advantages:

  • We use AES CBC wich is accepted by TLS 1.2
  • No need to worry about messages containing separator characters.
  • If one message was written partially (due to error) it does not affect other messages being correctly decrypted and read
  • If it appears that reading entire file content into memory is erroneous (too large) this code can be easily refactored to load messages into memory one-by-one as separate lines

Remove clear content method implementation as it is not needed

Rename Temporal to Temporary

Looks great! Just a couple of questions inline

native/ios/Comm/TemporaryMessageStorage/EncryptedFileUtils.mm
19–21 ↗(On Diff #14273)

When this can be null?

38 ↗(On Diff #14273)

Should we close a file in finally?

This revision is now accepted and ready to land.Jul 7 2022, 8:07 AM
native/ios/Comm/TemporaryMessageStorage/EncryptedFileUtils.mm
19–21 ↗(On Diff #14273)

It can be null in two cases:

  1. Encryption key was not generated
  2. Cryptographic operation failed - cryptographic operation (previous diff) is expected to be very stable, I tested and debugged it carefully. But if it ever happens that it fails the method returns null and err variable is overwritten with description and an error code from CommonCrypto.h. This header file contains explanations for all error codes that CCCrypt method can return.
38 ↗(On Diff #14273)

closeFile method of NSFileHandle can throw an exception itself - https://developer.apple.com/documentation/foundation/nsfilehandle/1413393-closefile. It can be noted here that I am using deprecated NSFileHandle API, but I do it on purpose since newer methods are not available before iOS 13. Never version of those methods do the same things but the only difference is that they do not throw exception but overwrite NSError variable.

This revision was landed with ongoing or failed builds.Jul 14 2022, 5:10 AM
This revision was automatically updated to reflect the committed changes.