Re: openssl_seal new param

On Mon, Aug 31, 2015 at 5:16 PM, Anatol Belski <anatol.php@belski.net> wrote: > 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 > > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > 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>