Skip to content

Conversation

@paragonie-scott
Copy link
Collaborator

Differences from Crypto:

  • AES-CTR instead of AES-CBC
  • Operates on resources rather than strings
  • Decryption is slightly slow; it recalculates the MAC over the entire file and doesn't decrypt if it fails to validate.

Changes to Crypto:

  • All common denominators between the two classes have been moved to a separate class called Core.

Things we would like to test for before merging:

  • Is nonce reuse sufficiently mitigated?
  • Can we make the buffer size irrelevant to the decryption process?
  • Should we implement versioning in the regular library before adding this feature, and then make the ciphertext here versioned too?

This should close #39 (and #38?) when it's merged.

@pnowosie
Copy link

What's the point to moving to AES-CTR?

@paragonie-scott
Copy link
Collaborator Author

OpenSSL with CBC mode adds PKCS7 padding to each chunk (for us, 8192 bytes), which adds 16 bytes of overhead to each chunk.

We also have to transform the IV after each chunk and we'd end up making a sort of counter anyway.

src/File.php Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

might return FALSE

src/File.php Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe more informative: "Incorrect key length." ?

src/File.php Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

I'm worried about 32-bit PHP and files > 2GB. Wouldn't that overflow the integer and give us something that's potentially non-exact?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we error on file sizes >= (PHP_INT_MAX - 1) >> 1?

Now with return value checking

Remove nbproject
@paragonie-scott
Copy link
Collaborator Author

Okay, that commit satisfied the code warts you spotted (thanks @defuse!).

  • I still need to test incrementCounter() thoroughly to make sure it's doing The Right Thing (TM). Momma says nonce reuse is the devil.
  • If I can guarantee it works properly, I need to find out why changing the buffer size causes decryption to fail after the first buffer chunk. Then we can make it irrelevant or configurable.
  • We probably want to add version tagging to both libraries then rebase first. Version Tagging #34

If @zooko has any spare cycles to tell me, quickly, if I'm doing it wrong, that would be greatly appreciated too! (For quick reference, this is a "read the whole file, verify MAC, then decrypt" approach, so no random access within the file -- maybe a separate class can be made for that?)

@zooko
Copy link

zooko commented Jun 14, 2015 via email

@paragonie-scott paragonie-scott mentioned this pull request Jun 17, 2015
@paragonie-scott
Copy link
Collaborator Author

As per this comment in r/netsec I've added some basic documentation to this PR. I migrated the old documentation header from Crypto.php and wrote something similar for File.php.

Storing MACs in an array and re-verifying them on decryption (after the final HMAC has been verified) has been implemented, so race conditions against the filesystem should be effectively mitigated.

@paragonie-scott
Copy link
Collaborator Author

Should I resubmit or just force push to my branch?

@paragonie-scott
Copy link
Collaborator Author

Closing in favor of #78

@paragonie-scott paragonie-scott deleted the file_encryption branch July 22, 2015 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support API for Encrypting/Decrypting files.

6 participants