v2.0.remove-unnecessary-inheritance.0 #116

defuse · 2015-10-16T21:50:49Z

See the commit messages.

Refactoring =================== Removes unnecessary inheritance. Using inheritance adds an extra mental burden of remembering where methods and variables are defined. With static inheritance, 'self::' can either mean "the file you're looking at" or "some other file." Moves the encoding functions into Encoding.php. I'm guessing the purpose of the inheritance was to be able to use 'protected' to prevent the user of the library from calling internal functions. This patch uses a different technique: There are only public and private methods, and the user is allowed to any public method of "user-usable" classes. So: We tell the user about Crypto and the new Encoding, but we don't tell them Core exists. The user should never be calling functions on Core, but our classes (and test cases) are allowed to. PHP doesn't let you make classes private to a namespace, so there's currently nothing *stopping* the user from doing that, but I think it's a better trade-off than the extra mental burden of using static inheritance. De-duplicates the magic number by defining Core::HEADER_MAGIC and Core::HEADER_MAGIC_FILE constants. The normal encryption and file encryption's config getters are now completely independent. This reduces coupling and increases cohesion, at the cost of duplication of the settings (but, conceptually, we may want to vary the file and normal encryption settings independently.) Increases security to downgrade attacks. The old code would simply fail if the version header was legacy. The new code allows you to pass a $min_ver_header to getVersionConfigFromHeader(), which means "I'm giving you a header, and I want you to give me the settings for that header, but I also want you to check that the version in that header is bigger than $min_ver_header." This is used by decrypt to ensure that it isn't decrypting a legacy ciphertext (i.e. the user has to call legacyDecrypt). This will be useful in the future if, say, the 2.0 configuration has a security vulnerability such that changing a 2.1 ciphertext's header to say 2.0 would make 2.1 ciphertexts vulnerable. (This downgrade prevention hasn't been implemented for the File class, but it should be soon.) Currently, there is no API for checking whether a ciphertext is legacy. That should be added in the future. In a couple places in the old code, $config was set to some hard-coded version if it wasn't passed in to the function. For each of those cases, I removed the code that did that, and ensured that a valid $config is always passed in. Protocol changes: =================== Different HKDF infos are used for the three different things: - Legacy encryption and decryption (keeps the old values) - 2.0 encryption and decryption (adds "V2") - File encryption and decryption (adds "V2File") This ensures that if the same key is re-used for all three, the same keys aren't re-used (potentially avoiding subtle interactions that could lead to a break). I got rid of the timing-safe code in getVersionConfig(). The differences in configuration returned (CBC vs CTR, etc.) reveal which version it is anyway, so there's no point trying to timing-safe-ize those functions.

defuse · 2015-10-16T21:51:51Z

@paragonie-scott: It would be wonderful if you could review this PR, and, at your leisure, argue against removing the inheritance if you think it's valuable.

defuse · 2015-10-16T21:52:30Z

(What it does is written in the commit message)

paragonie-scott · 2015-10-17T03:13:47Z

Aside from the failure caused by #22 (and possibly any errors that failure is hiding), this looks very good to me. I like the way you organize everything. 👍

(I am of course going to re-read the whole PR as soon as it's ready to be finalized.)

paragonie-scott · 2015-10-17T03:29:52Z

It would be wonderful if you could review this PR, and, at your leisure, argue against removing the inheritance if you think it's valuable.

The only reason I used inheritance in my initial PR #78 was to expose common methods that the derived classes (Crypto and File) could use, but were not public. It's not a feature I feel super strongly about, and someone could always make

class CryptoProxy extends \Defuse\Crypto\Core {
    public function getInaccessibleMethod($method, $args = []) {
        return parent::$method(...$args);
    }
}

and there's nothing inheritance can do to stop this.

…id config to be returned.

paragonie-scott · 2015-10-18T19:06:00Z

src/File.php

For this, shouldn't we use the header length constant?

paragonie-scott · 2015-10-18T19:10:25Z

Aside from a few very minor things this looks good to me. I do have one suggestion but I'll open an issue for that.

defuse · 2015-10-19T06:20:43Z

Okay, fixed those minor issues, and the travis testing is passing. I think it's time to land this!

defuse added 2 commits October 16, 2015 14:59

Whitespace fix.

5a729ed

defuse added the review needed label Oct 16, 2015

defuse added 4 commits October 16, 2015 16:03

Fix silly mistakes to make the tests pass.

e7008c0

Remove debug echos I accidentally left in.

6da7367

Add backwards compatibility attack prevention to File.

0202401

Move the runtime tests into their own file.

3d84dbc

defuse mentioned this pull request Oct 16, 2015

Split the Runtime tests out of Crypto.php #118

Closed

defuse added 2 commits October 16, 2015 16:20

Fix documentation of get*Config() methods.

a2521bc

Fix method documentation.

aa6a5d4

This was referenced Oct 16, 2015

Make the HKDF derivation depend on the version #111

Closed

Fix magic number discrepancy #107

Closed

SECURITY CRITICAL: Fix CTR mode nonce incrementor.

9074411

defuse mentioned this pull request Oct 16, 2015

incrementCounter is incorrect #120

Closed

Make incrementCounter more robust, improve tests.

ace418e

defuse mentioned this pull request Oct 17, 2015

Add some defense-in-depth to incrementCounter() #121

Closed

defuse added 6 commits October 16, 2015 18:31

Move legacy test to PHPUnit.

5551dba

Remove the empty output test (no longer checking accurately)

5e832aa

Fix bug uncovered by testing with mbstring.func_overload=7

ff87a53

Run tests properly.

5c15b10

Move annoying files into an 'other/' directory.

bbcc1ae

Put proper credit to Scott in phpunit.sh

ee6abfd

defuse mentioned this pull request Oct 17, 2015

Use PHPUnit for tests #119

Closed

Make all exceptions use consistent names and namespacing.

7fca4ae

defuse mentioned this pull request Oct 17, 2015

Wrong Exception names #113

Closed

Test that our nonce format is the same as OpenSSL's

b310ac9

This was referenced Oct 17, 2015

Unit test the shit out of the nonce incrementer. #109

Closed

Distinguish normal ciphertext and file ciphertext with versions. #105

Closed

defuse mentioned this pull request Oct 17, 2015

Fix documentation to say CTR #102

Closed

Get rid of the terrible ExceptionHandler hack.

3b41509

defuse mentioned this pull request Oct 17, 2015

Remove the exception handler override #97

Closed

Switch to AES-256.

aa6810a

defuse mentioned this pull request Oct 17, 2015

Switch to AES-256. #99

Closed

defuse added 2 commits October 16, 2015 21:20

Get older PHPUnit on older versions of PHP.

533ad7d

Fix syntax to parse on older versions of PHP.

82a7599

paragonie-scott mentioned this pull request Oct 17, 2015

Harden the autoloader against unserialize -> LFI risks. #96

Closed

Fix bug by baking 'sha256' in the mac-the-mac comparison.

26db090

defuse added 3 commits October 16, 2015 21:31

Fix bug caused by moving createNewRandomKey() to Crypto.

2986ab1

Don't allow Travis-CI tests of PHP 7.0 to fail.

9b9a732

Add a 'Key' "firewall" to prevent use of insecurely-generated keys.

0211202

defuse mentioned this pull request Oct 17, 2015

Create Key container class that prevents user from stupid mistakes #80

Closed

defuse added 5 commits October 17, 2015 12:19

SECURITY CRITICAL: Fix bug where bad version header could cause inval…

6e72941

…id config to be returned.

Turn RangeExceptions into our exceptions upon hex decoding failure.

56dd2d2

Add test for backwards compatibility attacks.

fec7f5f

Add more invalid version number tests.

4691ab2

Use an object for config instead of an array.

6f99c97

defuse mentioned this pull request Oct 17, 2015

Use a type to store the config, not an array. #112

Closed

paragonie-scott reviewed Oct 18, 2015
View reviewed changes

src/File.php Outdated

Copy link

Collaborator

paragonie-scott Oct 18, 2015

Choose a reason for hiding this comment

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

For this, shouldn't we use the header length constant?

defuse added 2 commits October 19, 2015 00:03

Use HEADER_VERSION_SIZE instead of magic number 4.

35c618a

Make Key not use an error-prone array for config options.

6c1d4bc

defuse merged commit 6c1d4bc into v2.0 Oct 19, 2015

paragonie-scott mentioned this pull request Oct 21, 2015

Malformed ciphertext files can cause fread with negative values upon decryption #115

Closed

paragonie-scott deleted the v2.0.remove-unnecessary-inheritance.0 branch May 16, 2016 21:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

v2.0.remove-unnecessary-inheritance.0 #116

v2.0.remove-unnecessary-inheritance.0 #116

Uh oh!

defuse commented Oct 16, 2015

Uh oh!

defuse commented Oct 16, 2015

Uh oh!

defuse commented Oct 16, 2015

Uh oh!

paragonie-scott commented Oct 17, 2015

Uh oh!

paragonie-scott commented Oct 17, 2015

Uh oh!

paragonie-scott Oct 18, 2015

Uh oh!

paragonie-scott commented Oct 18, 2015

Uh oh!

defuse commented Oct 19, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

v2.0.remove-unnecessary-inheritance.0 #116

v2.0.remove-unnecessary-inheritance.0 #116

Uh oh!

Conversation

defuse commented Oct 16, 2015

Uh oh!

defuse commented Oct 16, 2015

Uh oh!

defuse commented Oct 16, 2015

Uh oh!

paragonie-scott commented Oct 17, 2015

Uh oh!

paragonie-scott commented Oct 17, 2015

Uh oh!

paragonie-scott Oct 18, 2015

Choose a reason for hiding this comment

Uh oh!

paragonie-scott commented Oct 18, 2015

Uh oh!

defuse commented Oct 19, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants