On Sun, Jul 22, 2018 at 5:54 PM, Graham Leggett <minf...@sharp.fm> wrote:
> On 12 Jun 2018, at 12:06 AM, yla...@apache.org wrote:
>>
>> New apr_crypto_prng API and apr_crypto[_thread]_random_bytes() functions.
>
>> Added: apr/apr/trunk/crypto/apr_crypto_prng.c
>> + EVP_EncryptInit_ex(ctx, NULL, NULL, key, NULL);
>> + EVP_CIPHER_CTX_set_padding(ctx, 0);
>> +
>> + memset(key, 0, APR_CRYPTO_PRNG_KEY_SIZE);
>> + EVP_EncryptUpdate(ctx, key, &len, key, APR_CRYPTO_PRNG_KEY_SIZE);
>> + EVP_EncryptUpdate(ctx, to, &len, z, n);
>> +
>> + return APR_SUCCESS;
>> +}
>> +
>> +#else /* APU_HAVE_OPENSSL */
>> +
>> +/* XXX: APU_HAVE_CRYPTO_PRNG shoudn't be defined! */
>> +#error apr_crypto_prng implemented with OpenSSL only for now
>
> The layout of the code goes against the established structure of the
> apr_crypto API, all of this openssl specific code should go into
> crypto/apr_crypto_openssl.c.
No thanks, this is APR crypto *PRNG* API, no DSO, no block cipher, no
PBKDF2, no HMAC, no MD2/4/5 (really?)..
It needs a stream cipher with a quite specific interface, no
_init/_encrypt_/_finish each time a keystream is to be generated, no
mutiple indirections, no allocation once the initial context is
created (i.e. it never fails after apr_crypto_prng_init(), which is
nice for a PRNG).
I know how to do this with openssl, not with the other libs yet but I
guess it should be as simple.
I don't see why we'd need all the crypto
block/passphrase/digest/hmac/whatever complexity for the 5 lines of
the above function.
>
> We shouldn’t be ignoring the caller’s choice of crypto library and
> then hard coding these calls to openssl, especially on platforms like
> Linux where openssl might be installed by default. Platforms like
> MacOS where openssl is deprecated would also be a problem.
Without openssl, the CPRNG is not configured/available, it's that
simple and no different than other APR parts.
We don't ignore anything, but simply implement it only with openssl for now.
When cprng_stream_ctx_bytes() is implemented with other libraries, the
CPRNG becomes available with that libraries.
>
> The apr_crypto_block_encrypt_init / apr_crypto_block_encrypt /
> apr_crypto_block_encrypt_finish functions already implement the above
> for you, so they could be used instead.
No, the CPRNG does not use a block cipher, and does not allocate data
for encryption (at least with openssl, provided we don't use
_init/_encrypt/_finish), and is constant time with chacha20 and with
AES256-CTR if aesni instructions are available on the CPU (provided we
use the simple functions self-contained in "apr_crypto_prng.c". These
are not negligeable points for a PRNG...
>
> The tests keep segfaulting for me in apr-trunk and apr-util v1.7, I
> think this code needs more tuning to get it right before it’s
> backported to apr_util v1.7.
Could you please be more specific? Is it what you fixed in r1836438
("Make sure we don't segfault if the PRNG does not initialise") or
does it still segfault for you?
Regards,
Yann.
