Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87964
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain paragonie.com from 209.85.212.178 cause and error)
MIME-Version: 1.0
In-Reply-To: <030d01d0e432$5e002570$1a007050$@belski.net>
References: <CAEKnhAF4DZ45_nku8GoRo1s=1ni1M-8krVyqc2FT0-6macH0Mw@mail.gmail.com>
	<020801d0e3de$a8be40f0$fa3ac2d0$@belski.net>
	<CAEKnhAG_5z6c3Ou2+qkkMtFZ7-U2HrYeUBYVJNLciag4PvVFbA@mail.gmail.com>
	<030d01d0e432$5e002570$1a007050$@belski.net>
Date: Mon, 31 Aug 2015 17:56:36 -0400
Message-ID: <CAKws9z2YaWWeM=E6wdk_aW0rwUGP3Ypvm6EN+8KbuewNpjgeYA@mail.gmail.com>
To: Anatol Belski <anatol.php@belski.net>
Cc: Jakub Zelenka <bukka@php.net>, PHP internals list <internals@lists.php.net>, 
	Ferenc Kovacs <tyra3l@gmail.com>, Pierre Joye <pierre.php@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] openssl_seal new param
From: scott@paragonie.com (Scott Arciszewski)

On Mon, Aug 31, 2015 at 5:16 PM, Anatol Belski <anatol.php@belski.net> wrot=
e:
> Hi Jakub,
>
>> -----Original Message-----
>> From: jakub.php@gmail.com [mailto:jakub.php@gmail.com] On Behalf Of Jaku=
b
>> Zelenka
>> Sent: Monday, August 31, 2015 10:13 PM
>> To: Anatol Belski <anatol.php@belski.net>
>> Cc: PHP internals list <internals@lists.php.net>; Ferenc Kovacs
>> <tyra3l@gmail.com>; Pierre Joye <pierre.php@gmail.com>
>> Subject: Re: [PHP-DEV] openssl_seal new param
>>
>> Hi Anatol,
>>
>> On Mon, Aug 31, 2015 at 12:17 PM, Anatol Belski <anatol.php@belski.net>
>> wrote:
>> >
>> > > Hi,
>> > >
>> > > I have been looking to https://bugs.php.net/bug.php?id=3D60632 which
>> > > is
>> > about
>> > > failing (segfaulting) openssl_seal when used with cipher alg that
>> > requires IV (e.g.
>> > > AES-128-CBC). I think that the patch looks reasonable from the quick
>> > look.
>> > >
>> > > The only question and the reason why I'm sending this here is if
>> > everyone (and
>> > > mainly Ferenc ) is ok with adding new ref arg to openssl_seal that
>> > > will
>> > return iv
>> > > to 5.6? So the definition is:
>> > >
>> > > int openssl_seal ( string $data , string &$sealed_data , array
>> > &$env_keys , array
>> > > $pub_key_ids [, string $method[, string &$iv ]] )
>> > >
>> > > (the last iv is new). There would be also a new param for
>> > > openssl_open
>> > that
>> > > would allow to pass that IV for opening sealed data.
>> > >
>> > > Alternatively we could just disable IV ciphers in 5.6 to at least
>> > prevent the
>> > > segfault and add it to 7 if Anatol and Kalle are ok with that or 7.1
>> > > if
>> > not :) ?
>> > >
>> > Were it possible to list the ciphers that are affected? IMHO adding
>> > the IV arg were fine for 7.0 if there are many ciphers affected (say
>> > 1/3 of them), otherwise just checking EVP_CIPHER_iv_length() and
>> > returning false were appropriate with a subsequent proper fix flowing =
into 7.1.
>> >
>> >
>> It's actually about cipher algorithms (cipher + mode) where the IV is re=
quired by
>> the mode en/decryption. Basically all modes except ECB require IV. There=
 also
>> are some archaic ciphers that don't have mode defined which is equal ECB
>> (example is default example cipher for rc4). If you do
>>
>> php -r 'print_r(openssl_get_cipher_methods());'
>>
>> you will see that there are more than 80% of cipher algs requiring IV.
>>
>> The main thing is that openssl_seal doesn't have a practical usage at th=
e
>> moment if you encrypt (seal) a message longer than one block size becaus=
e
>> there is a risk of reply attack.
>>
>> The patch is self contained so I think that it shouldn't be a big issue =
for
>> 7.0 if you are fine with that?
>>
>> About 5.6 I'm not really sure. I'm not a big fun of changing signature i=
n the later
>> stages of point release. It could be also a small motivation for updatin=
g to 7. So
>> probably best to disable IV ciphers for 5.6 just to fix the segfault. If=
 anyone
>> doesn't have any objection and you are ok with adding new param to 7, I =
will go
>> for it.
>>
> Yeah, just reread the docs - CBC, CFB, OFB, CTR all require an IV. And th=
e ECB mode which is the only available is weak. IMHO doing the necessary ch=
ange in 7.0 is legit because the only usable cipher mode is weak, and any o=
ther mode will lead to crash. I were ok with the additional optional argume=
nt you suggest if there are no other objections.
>
>>
>> > > There also is an another thing for TS Win build (probably question
>> > > for
>> > Anatol and
>> > > Pierre :) ). The thing is that EVP_SealInit uses internally RAND_byt=
es.
>> > IIRC there is
>> > > some locking issue with openssl RAND on TS win ( the reason why
>> > > openssl_random_pseudo_bytes uses Win random) so I was wondering if
>> > > it should be disabled on win? The thing is that it is already a case
>> > > for
>> > other
>> > > functions. One example is generating key params in
>> > > openssl_pkey_new:
>> > >
>> > > openssl_pkey_new(array( 'dh'=3D> array( 'p' =3D> $bin_prime, 'g' =3D=
> '2'
>> > > )));
>> > >
>> > > This will also call RAND_bytes when generating priv key.
>> > >
>> > If you look into crypto/rand/rand_lib.c within the openssl src - it is
>> > not thread safe even in 1.0.2 branch. It is a good catch, but has only
>> > to do with the thread safety. And the issue is exactly that there is
>> > no locking around RAND_bytes() affected codes in PHP. So the issue is =
cross
>> platform.
>> >
>> > A quick solution could be using the mutex functionality exported from
>> > TSRM
>> > (tsrm_mutex_*() functions) in the thread safe builds for affected func=
tions.
>> >
>> >
>> >
>> Yeah I was thinking about that after I sent an email yesterday. I will c=
heck if
>> there are more places where this can be an issue and try to put together=
 a patch.
>>
> TSRM is probably the best we have in the core right now. Mutex will sure =
have a performance impact, but probably no way around it as openssl is alwa=
ys external. After yet looking at the APIs, maybe one could setup the defau=
lt rand in MINIT, or force it to be always RAND_SSLeay(), but not sure it i=
s a better idea than locking.
>
> Regards
>
> Anatol
>
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>

At the risk of sounding silly, can we just use random_bytes()? :)

Additionally, I'd like to (for example) be able to use ChaCha20
instead of RC4. Is that what the (currently undocumented) $method
parameter is for?

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com>