openssl_seal new param

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

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.

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.

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.

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

--

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

Hi Anatol

On Mon, Aug 31, 2015 at 10:16 PM, Anatol Belski anatol.php@belski.net
wrote:

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.

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.

We could maybe set a different RAND method in MINIT using
RAND_set_default_method . However this is not recommended as it might be
rewritten by the engine API (when/if we add support for loading engines).

There also is an alternative way of locking. Rather than use mutex around
the function call in openssl.c, we could use CRYPTO_set_locking_callback
and register locking there. That should be faster as it locks just places
when the race condition can happen. However not sure if that will work for
Win as there is some issue with polling entropy as documented in [1] and
more info including proposed patch can be found in [2].

I will try to think about it a bit more later to see if I can come up with
anything.

[1] https://wiki.openssl.org/index.php/Random_Numbers#Windows_Issues

[2] https://mta.openssl.org/pipermail/openssl-dev/2015-July/002210.html