You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Graham Leggett <mi...@sharp.fm> on 2018/07/22 15:54:42 UTC
Re: svn commit: r1833359 - in /apr/apr/trunk: CHANGES CMakeLists.txt
build.conf build/crypto.m4 crypto/apr_crypto.c crypto/apr_crypto_prng.c
include/apr.h.in include/apr_crypto.h test/testcrypto.c
threadproc/unix/proc.c
On 12 Jun 2018, at 12:06 AM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Mon Jun 11 22:06:09 2018
> New Revision: 1833359
>
> URL: http://svn.apache.org/viewvc?rev=1833359&view=rev
> Log:
> Cryptographic Pseudo Random Number Generator (CPRNG).
>
> 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.
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.
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.
Alternatively add the apr_crypto_prng_* functions to the drivers, with APR_ENOTIMPL for NSS and CommonCrypto until the time comes where they are supported.
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.
Regards,
Graham
—
Re: svn commit: r1833359 - in /apr/apr/trunk: CHANGES CMakeLists.txt
build.conf build/crypto.m4 crypto/apr_crypto.c crypto/apr_crypto_prng.c
include/apr.h.in include/apr_crypto.h test/testcrypto.c
threadproc/unix/proc.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 24 Jul 2018, at 14:25, Yann Ylavic <yl...@gmail.com> wrote:
>> I’m not seeing that this functionality being PRNG, or the apparent
>> simplicity of it being relevant in any way.
>>
>> We’ve created a hard link between APR and OpenSSL, in a system where
>> we already have a clean DSO based system to interface with crypto and
>> other libraries.
>
> How is DSO harder or softer than linking when --with-crypto (and
> --with-openssl or any other crypto lib) is configured?
> To me, --with-openssl means link with openssl, just like
> --with-libxml2 or --with-expat.
It doesn’t mean that no, I think this is the source of the confusion.
To explain this, look at how Redhat packages APR into discrete RPMs, each limiting the scope of the transitive dependencies that get dragged in.
apr-util-ldap.x86_64 : APR utility library LDAP support
apr-util-mysql.x86_64 : APR utility library MySQL DBD driver
apr-util-nss.x86_64 : APR utility library NSS crytpo support
apr-util-odbc.x86_64 : APR utility library ODBC DBD driver
apr-util-openssl.x86_64 : APR utility library OpenSSL crytpo support
apr-util-pgsql.x86_64 : APR utility library PostgreSQL DBD driver
apr-util-sqlite.x86_64 : APR utility library SQLite DBD driver
apr-util.x86_64 : Apache Portable Runtime Utility library
apr.x86_64 : Apache Portable Runtime library
APR itself is tiny, and has very few library dependencies:
[root@tools ~]# ls -al /usr/lib64/libapr-1.so.0.6.3
-rwxr-xr-x. 1 root root 240504 Jun 9 13:03 /usr/lib64/libapr-1.so.0.6.3
[root@tools ~]# ldd /usr/lib64/libapr-1.so.0.6.3
linux-vdso.so.1 => (0x00007ffcd61ae000)
libuuid.so.1 => /lib64/libuuid.so.1 (0x00007f98f382a000)
librt.so.1 => /lib64/librt.so.1 (0x00007f98f3622000)
libcrypt.so.1 => /lib64/libcrypt.so.1 (0x00007f98f33eb000)
libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f98f31cf000)
libdl.so.2 => /lib64/libdl.so.2 (0x00007f98f2fcb000)
libc.so.6 => /lib64/libc.so.6 (0x00007f98f2bfe000)
/lib64/ld-linux-x86-64.so.2 (0x00007f98f3c68000)
libfreebl3.so => /lib64/libfreebl3.so (0x00007f98f29fb000)
APR-util is also tiny, and is bound to a similar set of libraries, most notably due to some very old legacy code, we hard bind the whole apr-util library to expat:
[root@tools ~]# ls -al /usr/lib64/libaprutil-1.so.0.5.2
-rwxr-xr-x. 1 root root 172288 Jun 10 2014 /usr/lib64/libaprutil-1.so.0.5.2
[root@tools ~]# ldd /usr/lib64/libaprutil-1.so.0.5.2
linux-vdso.so.1 => (0x00007ffc0027f000)
libuuid.so.1 => /lib64/libuuid.so.1 (0x00007f35578cf000)
libcrypt.so.1 => /lib64/libcrypt.so.1 (0x00007f3557698000)
libexpat.so.1 => /lib64/libexpat.so.1 (0x00007f355746e000) <------
libdb-5.3.so => /lib64/libdb-5.3.so (0x00007f35570af000)
libapr-1.so.0 => /lib64/libapr-1.so.0 (0x00007f3556e76000)
libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f3556c5a000)
libdl.so.2 => /lib64/libdl.so.2 (0x00007f3556a56000)
libc.so.6 => /lib64/libc.so.6 (0x00007f3556689000)
/lib64/ld-linux-x86-64.so.2 (0x00007f3557cfd000)
libfreebl3.so => /lib64/libfreebl3.so (0x00007f3556486000)
librt.so.1 => /lib64/librt.so.1 (0x00007f355627e000)
Now lets bring in the postgresql driver for the apr_dbd library:
[root@tools ~]# rpm -q -l apr-util-pgsql.x86_64
/usr/lib64/apr-util-1/apr_dbd_pgsql-1.so
/usr/lib64/apr-util-1/apr_dbd_pgsql.so
[root@tools ~]# ldd /usr/lib64/apr-util-1/apr_dbd_pgsql-1.so
linux-vdso.so.1 => (0x00007ffdf077f000)
libpq.so.5 => /lib64/libpq.so.5 (0x00007f66eb1d8000)
libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f66eafbc000)
libc.so.6 => /lib64/libc.so.6 (0x00007f66eabef000)
libssl.so.10 => /lib64/libssl.so.10 (0x00007f66ea97d000) <— openssl
libcrypto.so.10 => /lib64/libcrypto.so.10 (0x00007f66ea51c000)
libkrb5.so.3 => /lib64/libkrb5.so.3 (0x00007f66ea234000)
libcom_err.so.2 => /lib64/libcom_err.so.2 (0x00007f66ea030000)
libgssapi_krb5.so.2 => /lib64/libgssapi_krb5.so.2 (0x00007f66e9de3000)
libldap_r-2.4.so.2 => /lib64/libldap_r-2.4.so.2 (0x00007f66e9b84000)
/lib64/ld-linux-x86-64.so.2 (0x00007f66eb60d000)
libk5crypto.so.3 => /lib64/libk5crypto.so.3 (0x00007f66e9951000)
libdl.so.2 => /lib64/libdl.so.2 (0x00007f66e974d000)
libz.so.1 => /lib64/libz.so.1 (0x00007f66e9537000)
libkrb5support.so.0 => /lib64/libkrb5support.so.0 (0x00007f66e9329000)
libkeyutils.so.1 => /lib64/libkeyutils.so.1 (0x00007f66e9125000)
libresolv.so.2 => /lib64/libresolv.so.2 (0x00007f66e8f0c000)
liblber-2.4.so.2 => /lib64/liblber-2.4.so.2 (0x00007f66e8cfd000)
libsasl2.so.3 => /lib64/libsasl2.so.3 (0x00007f66e8ae0000)
libssl3.so => /lib64/libssl3.so (0x00007f66e888e000) <— NSS
libsmime3.so => /lib64/libsmime3.so (0x00007f66e8667000)
libnss3.so => /lib64/libnss3.so (0x00007f66e833a000)
libnssutil3.so => /lib64/libnssutil3.so (0x00007f66e810b000)
libplds4.so => /lib64/libplds4.so (0x00007f66e7f07000)
libplc4.so => /lib64/libplc4.so (0x00007f66e7d02000)
libnspr4.so => /lib64/libnspr4.so (0x00007f66e7ac4000)
libselinux.so.1 => /lib64/libselinux.so.1 (0x00007f66e789d000)
libcrypt.so.1 => /lib64/libcrypt.so.1 (0x00007f66e7666000)
librt.so.1 => /lib64/librt.so.1 (0x00007f66e745e000)
libpcre.so.1 => /lib64/libpcre.so.1 (0x00007f66e71fc000)
libfreebl3.so => /lib64/libfreebl3.so (0x00007f66e6ff9000)
There is a lot going on. apr-util-pgsql links to libpq, which brings in all sorts of libraries, including both NSS and openssl at the same time.
The DSO mechanism insulates our end users from this - if you don’t want postgresql, you simply don’t dynamically load it and you’re not impacted. But if you do need it, there it is.
Now, let’s bring in the apr-util-openssl driver for the apr_crypto library:
[root@tools ~]# rpm -q -l apr-util-openssl.x86_64
/usr/lib64/apr-util-1/apr_crypto_openssl-1.so
/usr/lib64/apr-util-1/apr_crypto_openssl.so
[root@tools ~]# ldd /usr/lib64/apr-util-1/apr_crypto_openssl-1.so
linux-vdso.so.1 => (0x00007ffcaa789000)
libssl.so.10 => /lib64/libssl.so.10 (0x00007fedb74cd000)
libcrypto.so.10 => /lib64/libcrypto.so.10 (0x00007fedb706c000)
libpthread.so.0 => /lib64/libpthread.so.0 (0x00007fedb6e50000)
libc.so.6 => /lib64/libc.so.6 (0x00007fedb6a83000)
libgssapi_krb5.so.2 => /lib64/libgssapi_krb5.so.2 (0x00007fedb6836000)
libkrb5.so.3 => /lib64/libkrb5.so.3 (0x00007fedb654e000)
libcom_err.so.2 => /lib64/libcom_err.so.2 (0x00007fedb634a000)
libk5crypto.so.3 => /lib64/libk5crypto.so.3 (0x00007fedb6117000)
libdl.so.2 => /lib64/libdl.so.2 (0x00007fedb5f13000)
libz.so.1 => /lib64/libz.so.1 (0x00007fedb5cfd000)
/lib64/ld-linux-x86-64.so.2 (0x00007fedb7944000)
libkrb5support.so.0 => /lib64/libkrb5support.so.0 (0x00007fedb5aef000)
libkeyutils.so.1 => /lib64/libkeyutils.so.1 (0x00007fedb58eb000)
libresolv.so.2 => /lib64/libresolv.so.2 (0x00007fedb56d2000)
libselinux.so.1 => /lib64/libselinux.so.1 (0x00007fedb54ab000)
libpcre.so.1 => /lib64/libpcre.so.1 (0x00007fedb5249000)
Again, there is a lot going on. Redhat users are going to be mighty peeved if they install APR and in the process get an enormous list of transitive libraries that they don’t want or need.
This shows the next problem: we’re trying to make changes to apr_util_openssl that binds to openssl, but we’re completely ignoring apr_util_pgsql which also binds to openssl, and apr_util_mysql which binds to openssl, etc etc etc.
Whatever changes we try to make to apr_crypto also need to be made to apr_dbd, and apr_ldap, and possibly other libraries, otherwise we’re solving nothing.
>> Remember this is the Apache Portable Runtime, and currently there is
>> nothing portable about the PRNG implementation.
>
> Do you mean openssl isn't portable?
Yes.
Openssl is deprecated on MacOS, and is currently still stuck at v0.9.8 in High Sierra with no plans for an upgrade:
Little-Net:~ minfrin$ ls -al /usr/lib/libssl.0.*
-rwxr-xr-x 1 root wheel 392912 Jul 4 07:57 /usr/lib/libssl.0.9.7.dylib
-rwxr-xr-x 1 root wheel 630144 Jul 4 07:57 /usr/lib/libssl.0.9.8.dylib
This is why we have commoncrypto.
Openssl is entirely missing on Windows. Sure, httpd builds typically package it, but we’re not httpd, we’re APR.
> (BTW, I still struggle on how to do
> simple/run-this-cipher-with-this-key crypto with libnss, or even find
> a stream cipher with commoncrypto).
I struggled so you don’t have to. (To give credit where it is due, apr_crypto was based largely on https://www.aleksey.com/xmlsec/)
A quick google for CHACHA support shows NSS has it in RHEL6/7 since here: https://bugzilla.redhat.com/show_bug.cgi?id=1373158
On the Mac, I would stick to using their randomisation libraries: https://developer.apple.com/documentation/security/randomization_services?language=objc
>> This needs to be changed to follow our existing convention.
>
> I think there isn't a single convention in APR, libxml2 is not dlopen()ed right?
There is a convention as described above, with expat/libxml2 being the exception due to the age of the code.
Regards,
Graham
—
Re: svn commit: r1833359 - in /apr/apr/trunk: CHANGES CMakeLists.txt
build.conf build/crypto.m4 crypto/apr_crypto.c crypto/apr_crypto_prng.c
include/apr.h.in include/apr_crypto.h test/testcrypto.c threadproc/unix/proc.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Jul 24, 2018 at 10:12 AM, Joe Orton <jo...@redhat.com> wrote:
> On Tue, Jul 24, 2018 at 02:25:57PM +0200, Yann Ylavic wrote:
> > On Tue, Jul 24, 2018 at 11:43 AM, Graham Leggett <mi...@sharp.fm>
> wrote:
> > > Throughout APR we have a long established convention where
> > > “expensive” libraries are bound to APR through DSOs. All the SQL
> > > database drivers are like this, LDAP is like this, and so is crypto.
> >
> > I think this is a mistake (for crypto at least) and that it solves
> nothing.
> > As I already said, DSOs don't work better if the external lib is not
> > installed on the system in the first place (same dependency), and so
> > will libapr-util.
>
> It's much worse than simply being pointless, pointless DSO abstractions
> make dependency management both more complex and more fragile for
> distributors. It is a total misfeature in both the crypto and LDAP
> cases. When I have a direct linkage (i.e. ELF DT_NEEDED tags):
>
> mod_ldap -> libaprldap.so -> libldap.so
> mod_whatever -> libapr.so -> libssl.so
>
> my (RPM/.deb/whatever) package manager can detect library dependencies
> and reflect that in package dependencies to ensure system consistency.
>
> When you stick a DSO abstraction in the middle that goes away, and now I
> have to track the dependencies manually (i.e. it's now fragile), even
> though I know that 100% of users of mod_ldap CAN ONLY and MUST use
> exactly the same libldap.so as if they had directly linked against
> -lldap. Rinse and repeat for libcrypto.
>
> This is obviously completely different to db-style APIs where users
> *actually care* because their runtime environments are completely
> different (pgsql/mysql/...), hence run-time selection (and
> configuration) is well matched by a DSO-based abstraction.
>
While this is a valid argument, as a distributor, you present the option of
installing multiple DB providers, multiple crypto providers, etc. Due to the
flexible nature of this API, they can pick, say, mariadb, and have only the
single db provider loaded by libldl. Were APR to insist on 1:1 providers,
the user could not switch to using postgresql, or could only do so by
mapping both .so provider stacks into memory.
Based on the way most modules are architected, they are completely
abstract. The underlying mod_authn_dbd doesn't care about which
provider is loaded. Only the libaprmodule-1.so does.
The only fragile piece is aprldap-1.so, which completely exposes which
ldap provider was chosen to the consuming application. This is why it
was evicted from apr-2. If that is ever revisited and made entirely
abstract,
it would be welcome back into the fold.
Yes, the manual tracking of dependency chains is a headache, but not
insurmountable, and gives the users and packagers additional choices.
Re: svn commit: r1833359 - in /apr/apr/trunk: CHANGES CMakeLists.txt
build.conf build/crypto.m4 crypto/apr_crypto.c crypto/apr_crypto_prng.c
include/apr.h.in include/apr_crypto.h test/testcrypto.c
threadproc/unix/proc.c
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Jul 24, 2018 at 02:25:57PM +0200, Yann Ylavic wrote:
> On Tue, Jul 24, 2018 at 11:43 AM, Graham Leggett <mi...@sharp.fm> wrote:
> > Throughout APR we have a long established convention where
> > “expensive” libraries are bound to APR through DSOs. All the SQL
> > database drivers are like this, LDAP is like this, and so is crypto.
>
> I think this is a mistake (for crypto at least) and that it solves nothing.
> As I already said, DSOs don't work better if the external lib is not
> installed on the system in the first place (same dependency), and so
> will libapr-util.
It's much worse than simply being pointless, pointless DSO abstractions
make dependency management both more complex and more fragile for
distributors. It is a total misfeature in both the crypto and LDAP
cases. When I have a direct linkage (i.e. ELF DT_NEEDED tags):
mod_ldap -> libaprldap.so -> libldap.so
mod_whatever -> libapr.so -> libssl.so
my (RPM/.deb/whatever) package manager can detect library dependencies
and reflect that in package dependencies to ensure system consistency.
When you stick a DSO abstraction in the middle that goes away, and now I
have to track the dependencies manually (i.e. it's now fragile), even
though I know that 100% of users of mod_ldap CAN ONLY and MUST use
exactly the same libldap.so as if they had directly linked against
-lldap. Rinse and repeat for libcrypto.
This is obviously completely different to db-style APIs where users
*actually care* because their runtime environments are completely
different (pgsql/mysql/...), hence run-time selection (and
configuration) is well matched by a DSO-based abstraction.
Re: svn commit: r1833359 - in /apr/apr/trunk: CHANGES CMakeLists.txt
build.conf build/crypto.m4 crypto/apr_crypto.c crypto/apr_crypto_prng.c
include/apr.h.in include/apr_crypto.h test/testcrypto.c threadproc/unix/proc.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Jul 24, 2018 at 11:43 AM, Graham Leggett <mi...@sharp.fm> wrote:
> On 23 Jul 2018, at 01:10, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> 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.
>
> I’m not seeing that this functionality being PRNG, or the apparent
> simplicity of it being relevant in any way.
>
> We’ve created a hard link between APR and OpenSSL, in a system where
> we already have a clean DSO based system to interface with crypto and
> other libraries.
How is DSO harder or softer than linking when --with-crypto (and
--with-openssl or any other crypto lib) is configured?
To me, --with-openssl means link with openssl, just like
--with-libxml2 or --with-expat.
You don't want (or can't have) APR crypto, just don't ./configure it,
otherwise you need a crypto lib on the system in any case, be it
dlopen()ed or linked.
>
> Remember this is the Apache Portable Runtime, and currently there is
> nothing portable about the PRNG implementation.
Do you mean openssl isn't portable?
(BTW, I still struggle on how to do
simple/run-this-cipher-with-this-key crypto with libnss, or even find
a stream cipher with commoncrypto).
>
> This needs to be changed to follow our existing convention.
I think there isn't a single convention in APR, libxml2 is not dlopen()ed right?
>
> To be clear I totally support the addition of the PRNG code, it’s
> just the breaking of established conventions that we need to fix.
Thanks for your support, but IMHO here a key point is simplicity (as
opposed to complexity) like for any crypto code.
The more complex, the less secure, let's not introduce attack vectors
in our implementation.
>
>>> 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.
>
> That’s not how it works, no.
That's how it works, though not how *you* want it to work.
>
> Throughout APR we have a long established convention where
> “expensive” libraries are bound to APR through DSOs. All the SQL
> database drivers are like this, LDAP is like this, and so is crypto.
I think this is a mistake (for crypto at least) and that it solves nothing.
As I already said, DSOs don't work better if the external lib is not
installed on the system in the first place (same dependency), and so
will libapr-util.
>
> Now, an “expensive” openssl library, and very soon an “expensive” NSS
> library are going to be tightly bound to APR itself. This part is
> broken and needs fixing.
I disagree, APR offers crypto but does not want to implement crypto
primitives (rightly), so it depends on a crypto lib for --with-crypto.
They way we link to this lib is irrelevent.
>
> Remember OpenSSL is deprecated on MacOS (thus commoncrypto), and
> entirely missing on Windows (thus a potential future Windows specific
> driver), so binding to openssl is not portable.
Come on, openssl is highly portable and exists on all the OSes you are
citing here.
If one wants/needs APR PRNG, it needs openssl which shouldn't be
difficult to achieve.
Once I (or any of us) figure out how to use nss or cc for the PRNG
needs, this will improve (no different than the ENOTIMPLs for the two
ms* API in current apr_crypto code right?).
>
>>> 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…
>
> Again, not seeing what the problem is here.
>
> Either use what’s there, or if it’s not there add it, or
> alternatively add a dedicated function to apr_crypto_internal.h for
> whatever PRNG call you need and then do whatever library specific
> thing you need to do in that function as you’ve described above.
That's a bit of bikeshedding...
I could implement _init/_encrypt/_finish for the two stream cipher
used by the PRNG, in a separate file (looks overkill with openssl only
for now) or in the crypto driver (could be useful outside the PRNG,
though the "block" terminology doesn't help there). At least with
openssl for now still.
Yet I won't use them for the PRNG because I can do better with
internal/low-level/self-contained code in the PRNG itself (my concerns
are non-failure and speed, which doesn't match allocations for every
crypto operation).
>
> Binding to crypto libraries is a solved problem, what we’ve done here
> is unsolve that problem.
Again, I see no problem with linking to a lib when --with-that-lib is
used. What problem exactly?
Anyway, if this is really an issue for the team, an other option I can
see is to put the PRNG outside of the APR(-util) crypto tree/linking,
i.e. directly in APR "core" as a replacement of apr_random, and
implement CHACHA20 by ourself (which is light code).
>
>>> 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?
>
> Still segfaults for me on apr-trunk (it originally didn’t build). I
> have not yet had a chance to investigate this, I needed to get the
> test suite to the point where it just fails and didn’t crash so I can
> get work done.
Possibly addressed in r1836539, we really need to crypto_init() on the
global pool (as before) since openssl can't cleanup than re-init (no
DSO unload/reload to clears it internals...).
Regards,
Yann.
Re: svn commit: r1833359 - in /apr/apr/trunk: CHANGES CMakeLists.txt
build.conf build/crypto.m4 crypto/apr_crypto.c crypto/apr_crypto_prng.c
include/apr.h.in include/apr_crypto.h test/testcrypto.c
threadproc/unix/proc.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 23 Jul 2018, at 01:10, Yann Ylavic <yl...@gmail.com> wrote:
>>> +/* 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.
I’m not seeing that this functionality being PRNG, or the apparent simplicity of it being relevant in any way.
We’ve created a hard link between APR and OpenSSL, in a system where we already have a clean DSO based system to interface with crypto and other libraries.
Remember this is the Apache Portable Runtime, and currently there is nothing portable about the PRNG implementation.
This needs to be changed to follow our existing convention.
To be clear I totally support the addition of the PRNG code, it’s just the breaking of established conventions that we need to fix.
>> 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.
That’s not how it works, no.
Throughout APR we have a long established convention where “expensive” libraries are bound to APR through DSOs. All the SQL database drivers are like this, LDAP is like this, and so is crypto.
Now, an “expensive” openssl library, and very soon an “expensive” NSS library are going to be tightly bound to APR itself. This part is broken and needs fixing.
Remember OpenSSL is deprecated on MacOS (thus commoncrypto), and entirely missing on Windows (thus a potential future Windows specific driver), so binding to openssl is not portable.
>> 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…
Again, not seeing what the problem is here.
Either use what’s there, or if it’s not there add it, or alternatively add a dedicated function to apr_crypto_internal.h for whatever PRNG call you need and then do whatever library specific thing you need to do in that function as you’ve described above.
Binding to crypto libraries is a solved problem, what we’ve done here is unsolve that problem.
>> 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?
Still segfaults for me on apr-trunk (it originally didn’t build). I have not yet had a chance to investigate this, I needed to get the test suite to the point where it just fails and didn’t crash so I can get work done.
Regards,
Graham
—
Re: svn commit: r1833359 - in /apr/apr/trunk: CHANGES CMakeLists.txt
build.conf build/crypto.m4 crypto/apr_crypto.c crypto/apr_crypto_prng.c
include/apr.h.in include/apr_crypto.h test/testcrypto.c threadproc/unix/proc.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Sun, Jul 22, 2018 at 5:54 PM, Graham Leggett <mi...@sharp.fm> wrote:
> On 12 Jun 2018, at 12:06 AM, ylavic@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.