phpseclib / phpseclib

PHP Secure Communications Library
http://phpseclib.com/
MIT License
5.36k stars 887 forks source link

CBC with openssl #907

Open nimasdj opened 8 years ago

nimasdj commented 8 years ago

sorry, I got confused if I use CBC with openssl for AES and if I don't use setIV() will I use warning because of null IV param of openssl_encrypt? Or if I don't call setIV(), phpseclib generates one IV to avoid warning?

terrafrost commented 8 years ago

Yes. By default, phpseclib uses an IV of null characters if one isn't explicitly provided to it. Behavior like this isn't exactly without precedent either. Both OpenSSL and mcrypt null pad keys if they're not long enough.

nimasdj commented 8 years ago

If I use a random number for IV, where should I store it to get it for decryption? Should I append it with a separator at the end of encoded string? if yes, is it secure as everyone can see that number at the end of that encoded string?

terrafrost commented 8 years ago

That's fine. Quoting wikipedia,

An initialization vector has different security requirements than a key, so the IV usually does not need to be secret. However, in most cases, it is important that an initialization vector is never reused under the same key. For CBC and CFB, reusing an IV leaks some information about the first block of plaintext, and about any common prefix shared by the two messages.

https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation#Initialization_vector_.28IV.29

Joey3000 commented 8 years ago

By default, phpseclib uses an IV of null characters if one isn't explicitly provided to it. Behavior like this isn't exactly without precedent either. Both OpenSSL and mcrypt null pad keys if they're not long enough.

The internets seem to be full of warnings to avoid using a constant IV with the CBC mode, while many application devs nevertheless use incorrect library defaults. E.g., some research results:

Maybe a random IV could be made used by default in future?

terrafrost commented 8 years ago

Maybe a random IV could be made used by default in future?

I'd then have to add a new method - getIV().

I think the ideal approach would just be to throw an exception if setIV() wasn't called and you're using any mode other than ECB.

Joey3000 commented 8 years ago

I agree. It would make a user investigate and fall over the many recommendations to not use a constant IV. And it's how mcrypt_encrypt() does it:

iv Used for the initialization in CBC, CFB, OFB modes, and in some algorithms in STREAM mode. If the provided IV size is not supported by the chaining mode or no IV was provided, but the chaining mode requires one, the function will emit a warning and return FALSE.

It might also be good to additionally recommend usage of a randomly generated IV in:

terrafrost commented 8 years ago

Looks like PHP 5.6.0 introduced that as a change to mcrypt_encrypt. Interesting - did not know that!

I'm going to re-open this to keep track of this.

nimasdj commented 8 years ago

I suggest by default it should use a random IV (I did a Pull Request), and prepend it before encoded data and use it when decrypting, then implement a method disableIV() to use all-NULLs for IV (as it is now), if a user does not want to use IV.

    var $disabledIV = false;
    var $IV = "";

    function disableIV()
    {
        $this->disabledIV = true;
    }

    function getIV($ciphertext)
    {
        $iv = substr($ciphertext, 0, $this->getIVLength());
        $this->setIV($iv);
        return $iv;
    }

    function getCiphertext($ciphertext)
    {
        return substr($ciphertext, $this->getIVLength());
    }

    function encrypt($plaintext)
    {
        if (!$this->disabledIV && $this->mode != self::MODE_ECB) {
            $iv = $this->createIV();
            $this->setIV($iv);
            $this->IV = $iv;
        }

        // do encrypting job....

        return $this->IV.$ciphertext;
    }

    function decrypt($ciphertext)
    {
        if (!$this->disabledIV && $this->mode != self::MODE_ECB) {
            $iv = $this->getIV($ciphertext);
            $ciphertext = $this->getCiphertext($ciphertext);
        }

        // do decrypting job...

        return $this->paddable ? $this->_unpad($plaintext) : $plaintext;
    }

If you like this code, let me know your improvements and I'd do pull request it. createIV() and getIVLength() methods are already in my current pull request.

terrafrost commented 8 years ago

I'm not a big fan of it tbh. If end users want to prepend or postpend or do whatever with their IV then that's on them. But I don't think it's appropriate for the library to do so.

If you manually set the IV should it still prepend the manually set IV to the ciphertext? Is Net/SSH2.php going to have to do substr($ciphertext, $cipher->getBlockLength()) every time it wants to send another packet to the server?

In my mind, stuff like AES.php, DES.php, etc, should be seen as cryptographic primitives. They provide a low level API that you can have very fine tuned control over. But if you don't know what you're doing w.r.t. crypto then they're probably not the optimal API for you.

Like until AES-GCM and the like are implemented it'd probably be good to append an HMAC as well. And maybe the result ought to be base64 encoded too. But the more stuff like that I do and the less low level it becomes. And stuff like Net/SSH2.php needs a low level API.

Maybe in the future I can do something like... Crypt/Cipher.php that'll implement a higher level API for people who don't even know what HMAC is. It could chose the algorithm for you, block cipher mode, etc. But that can be a project for a later date!

terrafrost commented 8 years ago

But I do like the idea of throwing exceptions. Just need to implement that..

nimasdj commented 8 years ago

I was thiking the same. But for those two methods in my PR I think you should consider to merge and release a version as it does not cause BC.