Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87959
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.213.178 as permitted sender)
MIME-Version: 1.0
Sender: jakub.php@gmail.com
In-Reply-To: <020801d0e3de$a8be40f0$fa3ac2d0$@belski.net>
References: <CAEKnhAF4DZ45_nku8GoRo1s=1ni1M-8krVyqc2FT0-6macH0Mw@mail.gmail.com>
	<020801d0e3de$a8be40f0$fa3ac2d0$@belski.net>
Date: Mon, 31 Aug 2015 21:13:26 +0100
Message-ID: <CAEKnhAG_5z6c3Ou2+qkkMtFZ7-U2HrYeUBYVJNLciag4PvVFbA@mail.gmail.com>
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>
Content-Type: multipart/alternative; boundary=047d7b414316f357a3051ea110dd
Subject: Re: [PHP-DEV] openssl_seal new param
From: bukka@php.net (Jakub Zelenka)

--047d7b414316f357a3051ea110dd
Content-Type: text/plain; charset=UTF-8

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=60632 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

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 the
moment if you encrypt (seal) a message longer than one block size because
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 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.



> > 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'=> array( 'p' => $bin_prime, 'g' => '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.

Cheers

Jakub

--047d7b414316f357a3051ea110dd--