[Security] Encrypt about core HOT 34 OPEN

Thanks @piotrgolasz for the review.

from core.

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

from core.

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

from core.

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.

from core.

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

from core.

Hey @rjd22 what do you think?

from core.

@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?

from core.

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.

from core.

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

from core.

@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

from core.

@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.

from core.

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.

from core.

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.

from core.

+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

from core.

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?

from core.

@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

from core.

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

from core.

just remove Encrypt altogether

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

from core.

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

from core.

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.

from core.

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

from core.

* 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!

from core.

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

from core.

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
.

from core.

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

from core.

@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.

from core.

@shadowhand Any difference between defuse and phpseclib?

from core.

@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.

from core.

Beside travis failing it looks good.

from core.

@enov when do you think this can be solved?

from core.

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? :)

from core.

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

from core.

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.

from core.

Very easy to stop using Session class too.

from core.