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. > > 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
