[Security] Encrypt #686

piotrbaczek · 2016-04-26T13:01:44Z

According to: https://paragonie.com/blog/2015/05/if-you-re-typing-word-mcrypt-into-your-code-you-re-doing-it-wrong mcrypt is no longer a secure encryption mechanism.

Therefore I would suggest swapping from mcrypt to openssl, which is now considered a Core Internet Infastructure.

I would therefore suggest applying this patch: https://github.com/piotrgolasz/core/commit/b6769b4a1c49ac554ef7ffcfde48e715b22eedfb

This uses openssl in two modes: aes-128-cbc and aes-256-cbc, using the Encrypt-then-MAC scheme. It also allows using RSA encryption using Encrypt-then-Sign scheme. This exact method is used in Laravel framework.

Before, the encryption was not hmac-ed, meaning that crypto originating from it could be susceptible to padding oracle attacks. With this patch, first the check hash($ciphertext,$key,$hash) == $hmac is made, so that if the hashes don't match, we already know that this message is not valid, and therefore we don't have to decrypt.

rjd22 · 2016-05-03T07:36:40Z

@piotrgolasz why was this issue closed? Was it fixed?

piotrbaczek · 2016-05-18T18:26:31Z

I was going to make changes on a separate branch including changing phpunit tests.

piotrbaczek · 2016-05-20T17:17:26Z

Ok, so the main point of the change is to drop mcrypt in favour of openssl which is now well-tested and globally maintained software. The second reason for change is to use Encrypt-then-mac encryption, so that before we even try to decrypt anything we can validate the hmac, and therefore we won't accept anything that doesn't come from us.

Let me know if you have any suggestions.

piotrbaczek · 2016-05-24T08:46:45Z

Ok, so I have a test example to prove this.

``
public function action_test()
{
$this->auto_render = false;

    $encrypt = Encrypt::instance();

    $crypto = $encrypt->instance()->encode(serialize(['user' => 'asdf', 'admin' => true]));

    $list = array(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 'a', 'b', 'c', 'd', 'e', 'f');

    $get = unpack('H*', $crypto);

    for ($i = 0; $i < count($list); $i++)
    {
        for ($j = 0; $j < count($list); $j++)
        {
            //get the number
            $g2 = $get['1'];
            //subsitute next-to last position of IV
            $g2['31'] = $list[$i];
            //substitute last position of IV
            $g2['32'] = $list[$j];

            //echo the combination of bits that we're flipping
            echo $list[$i].$list[$j].' ';

            //After the bit-flipping, the result would be
            print_r(unserialize(Encrypt::instance()->decode(pack('H*', $g2))));
            echo '<br/>';
        }
    }
}

``

I am able to totally change the values of encrypted message, that give valid decryption output. This is because messages are not authenticated (hmaced)

The output is:
00 01 02 03 Array ( [up�r] => asdf [admin] => 1 ) 04 Array ( [up r] => asdf [admin] => 1 ) 05 Array ( [upMr] => asdf [admin] => 1 ) 06 Array ( [uper] => asdf [admin] => 1 ) 07 Array ( [up�r] => asdf [admin] => 1 ) 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 Array ( [uq�r] => asdf [admin] => 1 ) 14 Array ( [uq r] => asdf [admin] => 1 ) 15 Array ( [uqMr] => asdf [admin] => 1 ) 16 Array ( [uqer] => asdf [admin] => 1 ) 17 Array ( [uq�r] => asdf [admin] => 1 ) 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 Array ( [ur�r] => asdf [admin] => 1 ) 24 Array ( [ur r] => asdf [admin] => 1 ) 25 Array ( [urMr] => asdf [admin] => 1 ) 26 Array ( [urer] => asdf [admin] => 1 ) 27 Array ( [ur�r] => asdf [admin] => 1 ) 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 Array ( [us�r] => asdf [admin] => 1 ) 34 Array ( [us r] => asdf [admin] => 1 ) 35 Array ( [usMr] => asdf [admin] => 1 ) 36 Array ( [user] => asdf [admin] => 1 ) 37 Array ( [us�r] => asdf [admin] => 1 ) 38 39 3a 3b 3c 3d 3e 3f 40 41 42 43 Array ( [u|�r] => asdf [admin] => 1 ) 44 Array ( [u| r] => asdf [admin] => 1 ) 45 Array ( [u|Mr] => asdf [admin] => 1 ) 46 Array ( [u|er] => asdf [admin] => 1 ) 47 Array ( [u|�r] => asdf [admin] => 1 ) 48 49 4a 4b 4c 4d 4e 4f 50 51 52 53 Array ( [u}�r] => asdf [admin] => 1 ) 54 Array ( [u} r] => asdf [admin] => 1 ) 55 Array ( [u}Mr] => asdf [admin] => 1 ) 56 Array ( [u}er] => asdf [admin] => 1 ) 57 Array ( [u}�r] => asdf [admin] => 1 ) 58 59 5a 5b 5c 5d 5e 5f 60 61 62 63 Array ( [u~�r] => asdf [admin] => 1 ) 64 Array ( [u~ r] => asdf [admin] => 1 ) 65 Array ( [u~Mr] => asdf [admin] => 1 ) 66 Array ( [u~er] => asdf [admin] => 1 ) 67 Array ( [u~�r] => asdf [admin] => 1 ) 68 69 6a 6b 6c 6d 6e 6f 70 71 72 73 Array ( [u��r] => asdf [admin] => 1 ) 74 Array ( [u� r] => asdf [admin] => 1 ) 75 Array ( [u�Mr] => asdf [admin] => 1 ) 76 Array ( [u�er] => asdf [admin] => 1 ) 77 Array ( [u��r] => asdf [admin] => 1 ) 78 79 7a 7b 7c 7d 7e 7f 80 81 82 83 Array ( [ux�r] => asdf [admin] => 1 ) 84 Array ( [ux r] => asdf [admin] => 1 ) 85 Array ( [uxMr] => asdf [admin] => 1 ) 86 Array ( [uxer] => asdf [admin] => 1 ) 87 Array ( [ux�r] => asdf [admin] => 1 ) 88 89 8a 8b 8c 8d 8e 8f 90 91 92 93 Array ( [uy�r] => asdf [admin] => 1 ) 94 Array ( [uy r] => asdf [admin] => 1 ) 95 Array ( [uyMr] => asdf [admin] => 1 ) 96 Array ( [uyer] => asdf [admin] => 1 ) 97 Array ( [uy�r] => asdf [admin] => 1 ) 98 99 9a 9b 9c 9d 9e 9f a0 a1 a2 a3 a4 a5 a6 a7 a8 a9 aa ab ac ad ae af b0 b1 b2 b3 b4 b5 b6 b7 b8 b9 ba bb bc bd be bf c0 c1 c2 c3 c4 c5 c6 c7 c8 c9 ca cb cc cd ce cf d0 d1 d2 d3 d4 d5 d6 d7 d8 d9 da db dc dd de df e0 e1 e2 e3 e4 e5 e6 e7 e8 e9 ea eb ec ed ee ef f0 f1 f2 f3 f4 f5 f6 f7 f8 f9 fa fb fc fd fe ff

piotrbaczek · 2016-06-02T13:05:35Z

Hey @rjd22 what do you think?

rjd22 · 2016-06-02T15:52:06Z

@piotrgolasz I don't have enough experience with encryption to give good advise. I agree that replacing the functions for openssl is a good way to go but that most likely will break backwards compatibility so we only can release it with kohana 3.4 or 3.5 then.

@enov @acoulton what do you think?

acoulton · 2016-06-02T16:25:36Z

I think security is too important and fast moving for the community team to keep up with going forward, so it's a mistake to just push out a one-off upgrade that we can't guarantee to maintain.

We should heavily deprecate Encrypt in the next 3.3 release, and remove it altogether in the next breaking release. There must be decent, active, encryption packages in the composer ecosystem - we could find and recommend some, or leave that for the end user.

rjd22 · 2016-06-03T06:48:14Z

@acoulton I agree. There is no need for Kohana to offer encryption libraries.

piotrbaczek · 2016-06-09T07:49:44Z

@acoulton @rjd22 I think that having an encryption package is ok, as long as it has the latest security features. To be honest having mcrypt instead of openssl is not something big, however having encryption scheme where ciphertexts are not authenticated is a big security vulnerability, because then random or crafted modification of ciphertext can be decrypted as valid input.

So for me this change is not about removing the encryption or changing the underlaying library, but only making sure of ciphertext authenticity by signing ciphertext with hmac using Encrypt-then-hmac scheme.

As a reference I can pass the link to laravel's encryption class https://github.com/illuminate/encryption/blob/master/Encrypter.php

acoulton · 2016-06-09T08:18:40Z

@piotrgolasz

having an encryption package is ok, as long as it has the latest security features.

But who can guarantee that? I know what you're suggesting at the moment is quite a simple change, but who will make sure that the next security feature/issue that comes up is noticed and dealt with quickly?

Updating the package to whatever the latest security features are now implies that we are planning to keep it up to date in future. That might be our ambition, but I don't see how anyone can honestly claim that's likely to happen. There just aren't enough people actively using or developing Kohana on sites that use encryption.

So I think instead of a breaking change to update the latest version of our library which will soon be out of date again, we should make a breaking change to recommend people switch to a package they can rely on.

piotrbaczek · 2016-06-09T09:10:15Z

Well I would say that making and maintaining a PHP framework surely is not easy, however I think it should have a broad scope of functions so it can be used in wide range of sites.
AFAIK the Encrypt class can be used in Session after switching setting 'encrypted' to true, which encrypts cookies in the system.

If you would depreciate the encrypt class, then I think you should come up with some way of notifying developers about vulnerability in non-authenticated ciphertexts in past Kohana versions and recommend one or few available alternate libraries.

If so, then I think this can be closed.

enov · 2016-06-28T09:29:03Z

I am willing to take this. I am thinking if we could use a second key for the hmac authentication. If that key is empty, not configured, then the class would behave the same way as it currently behaves.

This way we can upgrade existing encrypted data. By keeping the old encryption profile with an empty auth key, and adding a new profile with the same encryption key but also with an auth key, the upgrade would be as easy as "decrypt with old profile then encrypt with the new one".

Let me know what you guys think.

piotrbaczek · 2016-07-29T10:51:05Z

+1

Also proper padding would be in need (such as one described here http://stackoverflow.com/questions/7314901/how-to-add-remove-pkcs7-padding-from-an-aes-encrypted-string)
kohana/kohana#99

enov · 2016-08-02T06:25:53Z

Thanks @piotrgolasz.

Adding proper padding scheme would make keeping BC even harder.

For that, we might need another config setting, and the code should pad the text if it is set to TRUE.

Not sure if I want to keep BC anymore. I guess a cleaner class + an example minion task in the docs would be much better.

@acoulton?

piotrbaczek · 2016-08-02T08:43:29Z

@enov

I think changing the config to:

<?php defined('SYSPATH') OR die('No direct script access.');

return array(

    'default' => array(
        /**
         * The following options must be set:
         *
         * string   key     secret passphrase
         * integer  mode    encryption mode, one of MCRYPT_MODE_*
         * integer  cipher  encryption cipher, one of the Mcrpyt cipher constants
         */
        'key' => NULL,
        'signing_key' => NULL,
        'padding' => FALSE,
        'hash' => FALSE,
        'cipher' => MCRYPT_RIJNDAEL_128,
        'mode'   => MCRYPT_MODE_NOFB,
    ),

From one side you don't want to make breaking changes, from the other side you don't want to have insecure default settings.
In this config example, signing key of 32 characters has to be filled to hash-hmac, hash has to be one of sha256, sha 384 or sha512, padding => true/false for pkcs7

acoulton · 2016-08-02T09:05:49Z

I still think we should just remove Encrypt altogether. However if you're keen to keep (and maintain) it then the proposal looks reasonable.

enov · 2016-08-02T09:46:34Z

just remove Encrypt altogether

In that case what should we do with Session using Encrypt?

enov · 2016-08-02T10:16:00Z

Maybe require https://github.com/defuse/php-encryption and write a wrapper that uses Kohana config?

acoulton · 2016-08-02T10:43:09Z

Ark. I forgot about Session::encrypted. I don't think we should add a compulsory dependency, but maybe the way forward is to add defuse/php-encryption (or another package) as a suggested dependency and as you say a wrapper class at our end that can throw a suitable exception if the package isn't installed.

That would give us the flexibility to provide a roughly-compatible interface without being responsible for maintaining the security implementation itself.

piotrbaczek · 2016-08-03T11:21:11Z

https://www.reddit.com/r/kohana/comments/4o6fsw/encryption_module_is_old_and_insecure/
@shadowhand's opinion

enov · 2016-08-04T11:16:05Z

* Edited to replace with the correct benchmark screenshot.

I want to share with you some of my findings as I played with the defuse/php-encryption package.

The library is simple, straight-forward to use and looks serious and promising. The main Crypto class that we need has 2 types of methods to encrypt:

one with a key generated by the library, that it can be saved in config for later
the other with a password/passphrase provided by the user, similar to what we do in config for Encrypt class in Kohana

I was first exploring the implementation with the user provided password, however it turned out the decryption process is (intentionally) very slow. It might not be adequate to use for sessions (that need to be encrypted/decrypted with every request).

In order for me to have an objective view of the difference between the two methods, I used PhpBench to benchmark:

Encryption Class used for benchmarking

namespace enov\BenchDefuse;

use Defuse\Crypto\Crypto as Crypto;
use Defuse\Crypto\Key as Key;

class Defuse
{
    protected $key;

    public function __construct()
    {
        $key = Key::createNewRandomKey();
    }

    public function encrypt_decrypt_pwd()
    {
        $password = 'Any password works here, even short ones';

        $text = 'This is the secret message';

        $ciphertext = Crypto::encryptWithPassword($text, $password);
        $plaintext = Crypto::decryptWithPassword($ciphertext, $password);
    }

    public function encrypt_decrypt_key()
    {
        $key = Key::createNewRandomKey();

        $text = 'This is the secret message';

        $ciphertext = Crypto::encrypt($text, $key);
        $plaintext = Crypto::decrypt($ciphertext, $key);
    }

    public function noop()
    {
        // do nothing
    }

    public function zleep()
    {
        usleep(100);
    }
}

PhpBench Benchmark class

<?php
use enov\BenchDefuse\Defuse;

class DefuseBench
{
    /**
     * @Revs(10)
     */
    public function benchEncryptDecryptPwd()
    {
       $defuse = new Defuse();
       $defuse->encrypt_decrypt_pwd();
    }

    /**
     * @Revs(10)
     */
    public function benchEncryptDecryptKey()
    {
       $defuse = new Defuse();
       $defuse->encrypt_decrypt_key();
    }

    /**
     * @Revs(1000)
     */
    public function benchNoop()
    {
       $defuse = new Defuse();
       $defuse->noop();
    }

    /**
     * @Revs(1000)
     */
    public function benchZleep()
    {
       $defuse = new Defuse();
       $defuse->zleep();
    }
}

Result

Note that each decryption takes more than half a second on average on my laptop.

Now using the second method requires the developer to generate a random key via the library and save it (most probably in config). So I wonder if that would result in too much support request afterwards, about how to use and upgrade.

Another thing that I was thinking, what if we remove the encryption feature completely from Session and ask the devs to encrypt it using their preferred library before using Session to save it to session?

Waiting for your comments.

Thanks!

piotrbaczek · 2016-08-04T12:40:58Z

Why don't you use native openssl methods, like laravel does: https://github.com/illuminate/encryption/blob/master/Encrypter.php

Or phpseclib for that matter?
https://github.com/phpseclib/phpseclib/tree/2.0
https://github.com/piotrgolasz/kohana-encrypt

shadowhand · 2016-08-04T13:16:42Z

Because Laravel does not follow best practices. OpenSSL is not the
recommended way to do encryption in PHP, defuse is.

On Thu, Aug 4, 2016, 07:41 Piotr G. [email protected] wrote:

Why don't you use native openssl methods, like laravel does:
https://github.com/illuminate/encryption/blob/master/Encrypter.php

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#686 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AACVO489Bfoz28wk33AW6OTnG86wxJE4ks5qcd3dgaJpZM4IP60h
.

piotrbaczek · 2016-08-05T08:34:12Z

@shadowhand can you elaborate on that or PM me with the answer @ [email protected]

shadowhand · 2016-08-05T13:11:51Z

@piotrgolasz @paragonie lays it all out in https://paragonie.com/white-paper/2015-secure-php-data-encryption

tl;dr: MCrypt is out. Defuse/libsodium is best. OpenSSL is fine, if you have nothing else.

piotrbaczek · 2016-08-08T14:03:04Z

@shadowhand Any difference between defuse and phpseclib?

acoulton · 2016-08-16T09:05:03Z

@enov thanks for the work on this. I think ideally we should keep support for encrypted sessions (using an external driver) but it does seem like password-based encryption will be too slow.

We could provide a minion task (or even just a small standalone script, since presumably Defuse just needs the composer autoloader) to generate a key if required, if that would help?

So long as we throw a sensible and helpful exception if key is empty / not configured then I think we should be able to point users in the right direction. They'll already have had to manually require defuse as well, so it's not unreasonable to expect them to configure it once installed.

piotrbaczek · 2016-09-15T22:46:25Z

Beside travis failing it looks good.

enov · 2016-09-16T10:47:46Z

Thanks @piotrgolasz for the review.

piotrbaczek · 2016-10-05T23:32:17Z

@enov when do you think this can be solved?

Daijobou · 2017-05-17T17:52:46Z

http://php.net/manual/de/function.mcrypt-get-key-size.php this function is deprecated. Is here a valid php 7.1 solution without warnings? :)

shadowhand · 2017-05-17T18:04:25Z

@Daijobou the right solution is to stop using the Encrypt class. It's bad and probably not secure.

Daijobou · 2017-05-18T07:25:20Z

Correct, but not only I use Encrypt, kohana too. See https://github.com/kohana/core/blob/3.3/master/classes/Kohana/Session.php#L146 or https://github.com/kohana/core/blob/3.3/master/classes/Kohana/Session.php#L305

UPDATE: I found a working solution with koseven (kohana for php7) project and openssl as encrypt type.

shadowhand · 2017-05-21T00:37:26Z

Very easy to stop using Session class too.

piotrbaczek closed this as completed May 2, 2016

rjd22 reopened this May 3, 2016

piotrbaczek closed this as completed Jun 9, 2016

piotrbaczek reopened this Jul 29, 2016

enov mentioned this issue Aug 17, 2016

[WIP] Use external libraries for encrypt #698

Open

neo22s mentioned this issue Jan 11, 2017

PHP 7.1: Deprecated Mcrypt koseven/koseven#14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Encrypt #686

[Security] Encrypt #686

piotrbaczek commented Apr 26, 2016 •

edited

Loading

rjd22 commented May 3, 2016

piotrbaczek commented May 18, 2016

piotrbaczek commented May 20, 2016

piotrbaczek commented May 24, 2016 •

edited

Loading

piotrbaczek commented Jun 2, 2016

rjd22 commented Jun 2, 2016

acoulton commented Jun 2, 2016

rjd22 commented Jun 3, 2016

piotrbaczek commented Jun 9, 2016 •

edited

Loading

acoulton commented Jun 9, 2016

piotrbaczek commented Jun 9, 2016

enov commented Jun 28, 2016

piotrbaczek commented Jul 29, 2016 •

edited

Loading

enov commented Aug 2, 2016

piotrbaczek commented Aug 2, 2016

acoulton commented Aug 2, 2016

enov commented Aug 2, 2016

enov commented Aug 2, 2016

acoulton commented Aug 2, 2016

piotrbaczek commented Aug 3, 2016

enov commented Aug 4, 2016 •

edited

Loading

piotrbaczek commented Aug 4, 2016 •

edited

Loading

shadowhand commented Aug 4, 2016

piotrbaczek commented Aug 5, 2016

shadowhand commented Aug 5, 2016

piotrbaczek commented Aug 8, 2016 •

edited

Loading

acoulton commented Aug 16, 2016

piotrbaczek commented Sep 15, 2016

enov commented Sep 16, 2016

piotrbaczek commented Oct 5, 2016

Daijobou commented May 17, 2017

shadowhand commented May 17, 2017

Daijobou commented May 18, 2017 •

edited

Loading

shadowhand commented May 21, 2017

[Security] Encrypt #686

[Security] Encrypt #686

Comments

piotrbaczek commented Apr 26, 2016 • edited Loading

rjd22 commented May 3, 2016

piotrbaczek commented May 18, 2016

piotrbaczek commented May 20, 2016

piotrbaczek commented May 24, 2016 • edited Loading

piotrbaczek commented Jun 2, 2016

rjd22 commented Jun 2, 2016

acoulton commented Jun 2, 2016

rjd22 commented Jun 3, 2016

piotrbaczek commented Jun 9, 2016 • edited Loading

acoulton commented Jun 9, 2016

piotrbaczek commented Jun 9, 2016

enov commented Jun 28, 2016

piotrbaczek commented Jul 29, 2016 • edited Loading

enov commented Aug 2, 2016

piotrbaczek commented Aug 2, 2016

acoulton commented Aug 2, 2016

enov commented Aug 2, 2016

enov commented Aug 2, 2016

acoulton commented Aug 2, 2016

piotrbaczek commented Aug 3, 2016

enov commented Aug 4, 2016 • edited Loading

Encryption Class used for benchmarking

PhpBench Benchmark class

Result

piotrbaczek commented Aug 4, 2016 • edited Loading

shadowhand commented Aug 4, 2016

piotrbaczek commented Aug 5, 2016

shadowhand commented Aug 5, 2016

piotrbaczek commented Aug 8, 2016 • edited Loading

acoulton commented Aug 16, 2016

piotrbaczek commented Sep 15, 2016

enov commented Sep 16, 2016

piotrbaczek commented Oct 5, 2016

Daijobou commented May 17, 2017

shadowhand commented May 17, 2017

Daijobou commented May 18, 2017 • edited Loading

shadowhand commented May 21, 2017

piotrbaczek commented Apr 26, 2016 •

edited

Loading

piotrbaczek commented May 24, 2016 •

edited

Loading

piotrbaczek commented Jun 9, 2016 •

edited

Loading

piotrbaczek commented Jul 29, 2016 •

edited

Loading

enov commented Aug 4, 2016 •

edited

Loading

piotrbaczek commented Aug 4, 2016 •

edited

Loading

piotrbaczek commented Aug 8, 2016 •

edited

Loading

Daijobou commented May 18, 2017 •

edited

Loading