Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:48313
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: softfail (pb1.pair.com: domain cdatazone.org does not designate 207.58.142.213 as permitted sender)
Message-ID: <4BF27252.4030503@cdatazone.org>
Date: Tue, 18 May 2010 06:56:18 -0400
User-Agent: Thunderbird 2.0.0.24 (Macintosh/20100228)
MIME-Version: 1.0
To: Sara Golemon <pollita@php.net>
CC: internals@lists.php.net
References: <D0.2B.05421.113F1FB4@pb1.pair.com>
In-Reply-To: <D0.2B.05421.113F1FB4@pb1.pair.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] openssl_(en|de)crypt missing IV
From: rrichards@cdatazone.org (Rob Richards)

Sara Golemon wrote:
> I was just looking through the implementation of openssl_encrypt() 
> (and openssl_decrypt()) today because I need to make some encrypted 
> payloads, but the prototype didn't have anywhere to place an 
> initialization vector.
>
> On opening ext/openssl/openssl.c, I noticed line 4620 which simply 
> hardcodes IV as a string of NULL bytes.
>
> This is a bad idea roughly equivalent to hashing passwords without 
> salt; Worse, it prevents interoperability at the application layer by 
> preventing the decryption of a data stream where the generator used an 
> IV other than all-null.
>
> Fixing this is a simple matter, but I wanted to bounce approaches for 
> BC (or lack thereof) off everyone else since this version of 
> openssl_encrypt() is already "in the wild".
>
> The most direct and obvious solution is to add a fifth, optional 
> parameter to openssl_encrypt() and openssl_decrypt() to take IV as a 
> string.  The problems with this are that it: (A) Places the IV in an 
> odd argument location, and (B) Does not enforce the passing of an IV 
> (since raw is already optional).  As stated above, IV really *should* 
> be enforced, given what it does to soften the security normally 
> offered by a chaining block method.
>
> That said, I'd like to propose something unpopular;   Change the 
> signature for these methods entirely, deliberately breaking BC.  I 
> know, I know.... spare me your rotten tomatoes.  I think it's 
> justified in this case because, as they are now, these functions are 
> useless at best, and possibly dangerous in terms of encouraging unsafe 
> practices with regards to cryptography.
>
> I think it's worth a BC break.  Comments?
>
> -Sara
>
> P.S. - Here's the signature I'd go with: openssl_encrypt($data, 
> $method, $iv, $key, $raw=false)
>

Personally I would like to see the signature changed to be even more 
aligned with the ones from mcrypt - since thats what people are most 
used to using. Currently without the iv, these methods are useless imo. 
The biggest downfall from changing the signature where the parameter is 
not optional, is that it cant be done until a major release, which will 
drag out the time where the functionality will be useful to anyone.

When the iv parameter is added (however it is done), also add 
functionality for creating an iv and getting iv length based on method. 
These methods are crucial for the function to be useful in real world apps.

Rob