Hi Jakub, > -----Original Message----- > From: jakub....@gmail.com [mailto:jakub....@gmail.com] On Behalf Of Jakub > Zelenka > Sent: Monday, August 31, 2015 10:13 PM > To: Anatol Belski <anatol....@belski.net> > Cc: PHP internals list <internals@lists.php.net>; Ferenc Kovacs > <tyr...@gmail.com>; Pierre Joye <pierre....@gmail.com> > Subject: Re: [PHP-DEV] openssl_seal new param > > Hi Anatol, > > On Mon, Aug 31, 2015 at 12:17 PM, Anatol Belski <anatol....@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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
