[pulseaudio-discuss] [PATCH v2] raop: add compatibility with openssl 1.1.0

[Tanu Kaskinen]

On Thu, 2016-11-03 at 09:54 -0300, Felipe Sateler wrote:
> On 3 November 2016 at 09:30, Tanu Kaskinen <tanuk at iki.fi> wrote:
> > Openssl 1.1.0 made all structs opaque, which caused a build failure in
> > raop_client.c. The struct member assignments are now replaced with a
> > call to RSA_set0_key().
> > 
> > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=96726
> > ---
> > 
> > Changes in v2:
> >  * More verbose comment.
> >  * Implement RSA_set0_key() as a function instead of a macro.
> >  * Use pa_assert_se() instead of pa_assert().
> > 
> > As before, I have tested that the code compiles against old and new openssl
> > versions, but I have not tested that the code works.
> > 
> >  src/modules/raop/raop_client.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/modules/raop/raop_client.c b/src/modules/raop/raop_client.c
> > index 3b6c36e..7a7bb8a 100644
> > --- a/src/modules/raop/raop_client.c
> > +++ b/src/modules/raop/raop_client.c
> > @@ -68,6 +68,21 @@
> > 
> >  #define RAOP_PORT 5000
> > 
> > +/* Openssl 1.1.0 broke compatibility. Before 1.1.0 we had to set RSA->n and
> > + * RSA->e manually, but after 1.1.0 the RSA struct is opaque and we have to use
> > + * RSA_set0_key(). RSA_set0_key() is a new function added in 1.1.0. We could
> > + * depend on openssl 1.1.0, but it may take some time before distributions will
> > + * be able to upgrade to the new openssl version. To insulate ourselves from
> > + * such transition problems, let's implement RSA_set0_key() ourselves if it's
> > + * not available. */
> > +#if OPENSSL_VERSION_NUMBER < 0x10100000L
> > +static int RSA_set0_key(RSA *r, BIGNUM *n, BIGNUM *e, BIGNUM *d) {
> > +    r->n = n;
> > +    r->e = e;
> > +    return 1;
> 
> 
> I'm sorry I did not point this out before. I've looked at the
> documentation of BN_bin2bn, and it can return NULL in case of failure.
> The real version of RSA_set0_key will return 0 if passed NULL 'n' and
> 'e', thus failing the assert. I'm not sure failing to 'BN_bin2bn' is a
> recoverable error so I don't think that it is bad that the assertion
> will fail. This new method does not fail the assert, even when it
> couldn't correctly set the key.
> 
> This code was buggy before this change, so maybe this code could go
> as-is and fix this later (either by adding checks, or requiring a
> new-enough openssl).

It would probably be safe to wrap the BN_bin2bn() calls in assertions,
because they should not need to talk to the kernel except when
allocating memory, but since the error conditions aren't documented, I
think it's more correct to not use assertions. This is true also for
RSA_set0_key().

My preference would be to handle errors gracefully, but it looks like
it's not trivial to implement, and I don't have motivation for spending
time on that. I think having assertions is better than nothing, so I'll
submit v3 with assertions for BN_bin2bn() and RSA_set0_key() plus a
FIXME comment.

-- 
Tanu

https://www.patreon.com/tanuk