Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87962
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain belski.net from 85.214.73.107 cause and error)
To: "'Jakub Zelenka'" <bukka@php.net>
Cc: "'PHP internals list'" <internals@lists.php.net>,
	"'Ferenc Kovacs'" <tyra3l@gmail.com>,
	"'Pierre Joye'" <pierre.php@gmail.com>
References: <CAEKnhAF4DZ45_nku8GoRo1s=1ni1M-8krVyqc2FT0-6macH0Mw@mail.gmail.com>	<020801d0e3de$a8be40f0$fa3ac2d0$@belski.net> <CAEKnhAG_5z6c3Ou2+qkkMtFZ7-U2HrYeUBYVJNLciag4PvVFbA@mail.gmail.com>
In-Reply-To: <CAEKnhAG_5z6c3Ou2+qkkMtFZ7-U2HrYeUBYVJNLciag4PvVFbA@mail.gmail.com>
Date: Mon, 31 Aug 2015 23:16:53 +0200
Message-ID: <030d01d0e432$5e002570$1a007050$@belski.net>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQKlTIs5g27BLw3cmuRcFI770qdriwJnZjvZAjEefeOcWP9lEA==
Content-Language: en-us
Subject: RE: [PHP-DEV] openssl_seal new param
From: anatol.php@belski.net ("Anatol Belski")

Hi Jakub,

> -----Original Message-----
> From: jakub.php@gmail.com [mailto:jakub.php@gmail.com] On Behalf Of =
Jakub
> 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
>=20
> Hi Anatol,
>=20
> 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 =
required 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
>=20
> php -r 'print_r(openssl_get_cipher_methods());'
>=20
> you will see that there are more than 80% of cipher algs requiring IV.
>=20
> The main thing is that openssl_seal doesn't have a practical usage at =
the
> moment if you encrypt (seal) a message longer than one block size =
because
> there is a risk of reply attack.
>=20
> 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?
>=20
> About 5.6 I'm not really sure. I'm not a big fun of changing signature =
in the later
> stages of point release. It could be also a small motivation for =
updating 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.
>=20
Yeah, just reread the docs - CBC, CFB, OFB, CTR all require an IV. And =
the ECB mode which is the only available is weak. IMHO doing the =
necessary change in 7.0 is legit because the only usable cipher mode is =
weak, and any other mode will lead to crash. I were ok with the =
additional optional argument you suggest if there are no other =
objections.

>=20
> > > 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_bytes.
> > 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 =
functions.
> >
> >
> >
> Yeah I was thinking about that after I sent an email yesterday. I will =
check if
> there are more places where this can be an issue and try to put =
together a patch.
>=20
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 =
always external. After yet looking at the APIs, maybe one could setup =
the default rand in MINIT, or force it to be always RAND_SSLeay(), but =
not sure it is a better idea than locking.

Regards

Anatol