Refactor Encrypter to use openssl instead of mcrypt (Breaking Change)

[jenschude]

No description provided.

[Seldaek]

This is a BC break as it'll make all encrypted cookies unreadable. So either it should be added as a new optional flag, or only in 3.x..

[jenschude]

Yep. That's true. tried to make it backwards compatible, but didn't worked out. But i could create an openssl encrypter, which could be enabled by config

[jenschude]

Now the encrypted cookie functionality is backwards compatible.

[romainneutron]

Please use Nelmio Security Bundle headers here

[romainneutron]

that's cool. Do you guys see a benefit of providing a tool that decrypts a mcrypt-encrypted cookie to an openssl one ?

[jenschude]

I would not go this way. I think it would be better to ensure, that cookies which could not be decrypted just get invalidated.

[relaxnow]

Note that openssl_random_pseudo_bytes may not be a proper CSPRNG depending on your setup: paragonie/random_compat#96

[romainneutron]

Thanks for the hint @relaxnow . True that, let's do that using pagagonie's random bytes

[jenschude]

Yep. Thx @relaxnow.

[sstok]

Please don't create your own encryption implementation unless you really know what you are doing!! 💥
https://paragonie.com/blog/2015/05/using-encryption-and-authentication-correctly
https://paragonie.com/blog/2015/08/you-wouldnt-base64-a-password-cryptography-decoded
https://paragonie.com/blog/2015/05/if-you-re-typing-word-mcrypt-into-your-code-you-re-doing-it-wrong (part about: III. Null Padding)

When it comes to encryption it's always better to use a library written by a crypto expert.
https://github.com/defuse/php-encryption (requires PHP 5.4)
https://github.com/paragonie/halite (GPL (commercial license available), requires libsodium)

@paragonie-scott can properly tell more about this then I can 😄

"This is a BC break as it'll make all encrypted cookies unreadable."

And when I clear the browser cache I also loos my cookies 👍
Cookies are cheap so it's safer to just recreate them then relying on unsafe data decryption.

[jenschude]

@sstok I see your point. But actually it's not directly related to the PR itself.

The current encrypter uses the mcrypt library which should be removed. The PR uses openssl which does it better as stated in the articles. And as the PR uses libraries, it's not creating an own encryption. But any objections to problems in the code would be cool.

[sstok]

The problem with the current implementation is that it only encrypts but does not guarantee that when you decrypt, the data is correct (you eg. get the original data or gibberish (when the data is tampered with), there is no way to check if what you get back is correct. Even worse is when the attacker is able to perform a Padding-oracle attack https://en.wikipedia.org/wiki/Padding_oracle_attack | http://www.zimuel.it/slides/dpc2014.html#/7 )

https://paragonie.com/blog/2015/05/using-encryption-and-authentication-correctly (How to Attack Unauthenticated Encryption) actually shows how dangerous unauthenticated encryption is.

Yes mcrypt is highly insecure, but to replace one insecure implementation with another insecure implementation doesn't solve anything. Cryptography is complex stuff and easy to get wrong, the numerous amount of (good intended but) insecure examples of OpenSSL/MCrypt usage don't help either.
Even Zend Framework was not free from crypto problems.

Note that I'm not a cryptographic expert 😃 I only know what is insecure (from reading articles), not what is exactly secure 😄

"And as the PR uses libraries, it's not creating an own encryption. But any objections to problems in the code would be cool."

No you are using an API which provides cryptographic functionality using a system library.
By library I meant something written in php (OK, package or end-user implementation would be a better description), the OpenSSL extension provides a thin binding of the C API for the PHP language, even so thin it inherits all the problem like a shared error stack or unreadable error messages... it's provided for cryptographers to build an encryption library in PHP, as much as libssl provides a library for implementers in C.

Defuse/php-encryption and Halite actually provide the best API possible for safe encryption.
You only have the data you want to encrypt/decrypt and the key to make this possible, no details like padding blocks, cipher choice (which one should I use?), authenticating the message, side channel attacks, exception stack key exposure, broken crypto engine risk (does a run-time test to ensure a sane crypto system), and other stuff I properly forgot about 😨 you get the point :)

[jenschude]

The question is, what is necessary. PlayFramework for example stores the session in a signed cookie. So it's highly encouraged to not store sensitive information to the session. But this way they are able to create stateless web applications.

[paragonie-scott]

The question is, what is necessary.

Use authenticated encryption.

👍 for using OpenSSL instead of mcrypt (less side-channels, better performance), but you really should consider just using defuse/php-encryption. It does the correct thing here and has a very intuitive interface. Give it a message and a key, you get authenticated ciphertext (with version tagging).

[jenschude]

Okay I finally got your point @sstok and @paragonie-scott

I will take a look at the suggested libraries.

[jenschude]

Just for your information. Made some good progress and the tests are running. But found an issue when trying to use the new encrypter in a real project.

Cause the cookie session handler sends one cookie with the session id and one with the encrypted data. The cookie listener now tries to decrypt the unencrypted cookie with the session id and fails as it's not encrypted. So I still have to find a proper solution.

[jenschude]

Now it's really a breaking change as defuse/php-encryption needs minimum php 5.4

I adjusted the composer.json and the travis.yml as well as the title of the PR

[jenschude]

To verify if the package is really working backwards compatible, I was just installing all packages with composer update --prefer-lowest and the unit tests are not running through (Fatal error: Uncaught Error: Class 'Symfony\Component\Yaml\Parser' not found). So we already have some breaking changes in the bundle itself.

Also tried to fix the breaking symfony/yaml to be at least v2.3. But still a lot of other tests are breaking with prefer-lowest.

[jenschude]

Played a little bit more with the master branch.

The minimum versions needed for running the unit tests are:

    "require": {
        "symfony/framework-bundle": "~2.4|~3.0",
        "symfony/security": "~2.3|~3.0",
        "symfony/yaml": "^2.0.5|~3.0",
        "paragonie/random_compat": "~1.0|~2.0"
    },
    "require-dev": {
        "phpunit/phpunit": "^4.5",
        "twig/twig": "^1.24",
        "ua-parser/uap-php": "^3.4.1"
    },

[Sander-Toonen]

Bump. With PHP 7.1 deprecating the mcrypt extension this PR might need some love.
@Seldaek What would be needed to move this forward?

[Seldaek]

IMO we could do a new major version of the bundle that drops symofny <2.7 and requires php 5.5.9 like symfony. Then it'd be good if the openssl functionality could migrate mcrypt-encrypted cookies to the new openssl ones, so users can upgrade and their site visitors will transparently get upgraded slowly, then once you migrate to php 7.1 if mcrypt is missing this would stop working and old cookies are discarded. That would give a good upgrade path, but might be some work..

[oleg-andreyev]

@jenschude any updates on this PR? Maybe you need some help?

[jenschude]

Most likely this PR would have to be rewritten with for the changes requested. So I will close this one for now.