Skip to content

Conversation

@paragonie-scott
Copy link
Collaborator

Satisfies the proposal outlined in #71

Supersedes #63
Closes #34
Closes #39

src/Crypto.php Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing whitespace ;)

@sstok
Copy link
Contributor

sstok commented Jul 22, 2015

https://github.com/paragonie/php-encryption/blob/files/src/Crypto.php#L322
if (\in_array(self::CIPHER_METHOD, \openssl_get_cipher_methods()) === FALSE) { was this left unchanged intentionally?

src/Crypto.php Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug code

@paragonie-scott
Copy link
Collaborator Author

The latest test verifies that our nonce incrementing strategy is functioning correctly. Aside from the danger of birthday collisions, nonce collisions are mitigated.

Mitigating nonce reuse is nontrivial, since we're limited to 128 bits. I think we would first need to create a subkey for each file and store the random HKDF or PBKDF2 salt in the file data to make this impractical.

@paragonie-scott
Copy link
Collaborator Author

Since HKDF accepts a salt parameter, I added a random per-file salt to the file encrypted file header in addition to the IV.

Without the salt, the odds of a birthday collision after 2^64 files is approximately 50%. Depending on the file size, a nonce reuse becomes a real problem.

Using a random salt and a nonrandom nonce (e.g. all 0's) is virtually identical to the existing situation.

Using a random salt AND a random nonce raises our birthday resistance to require 2^128 files for a 50% chance of birthday collision. (This is effectively similar to a 256-bit nonce.)

Default to hex encoding (since base64 is way too complicated)
Files: Store a salt in the file data to create a per-file subkey (via HKDF) to prevent nonce reuse via birthday collisions.
Use salts in HKDF everywhere. Affects Crypto.php and File.php. Also: MAC the salt.
Authenticate the version tag
@paragonie-scott
Copy link
Collaborator Author

The lastest commit should satisfy #93

@paragonie-scott
Copy link
Collaborator Author

Forgot about the runtime tests. Need new test vectors.

Copy link
Owner

Choose a reason for hiding this comment

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

This is now CTR.

@paragonie-scott
Copy link
Collaborator Author

One thing that the File class doesn't have, but the Crypto class does, is the runtime tests. Would you prefer that they be run in that instance as well? (I'd wager your answer is, "Yes".)

Copy link
Owner

Choose a reason for hiding this comment

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

Awesome!

Copy link
Owner

Choose a reason for hiding this comment

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

$if doesn't get closed here.

@defuse
Copy link
Owner

defuse commented Oct 16, 2015

I haven't looked too closely at the issues, but I'm going to merge this into a v2.0 branch so that it's easier to work with, and then open tickets for the issues I found.

@paragonie-scott
Copy link
Collaborator Author

Okay, sounds good to me. Need me to resubmit the PR?

@defuse
Copy link
Owner

defuse commented Oct 16, 2015

@paragonie-scott: I just created the v2.0 branch, so feel free to submit PRs to it if you feel like fixing any of those things. :)

@defuse
Copy link
Owner

defuse commented Oct 16, 2015

Hang on, I'm still trying to figure out how to push that branch to github.

@defuse
Copy link
Owner

defuse commented Oct 16, 2015

There we go.

@paragonie-scott
Copy link
Collaborator Author

This has been merged in another branch.

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.

3 participants