Bug#859784: validns: Please migrate to openssl1.1 in Buster

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Bug#859784: validns: Please migrate to openssl1.1 in Buster

Sebastian Andrzej Siewior
Package: validns
Version: 0.8+git20160720-2
Severity: important
Tags: sid buster
User: [hidden email]
Usertags: openssl-1.1-trans

Please migrate to libssl-dev in the Buster cycle. The bug report about
the FTBFS is #828591. The log of the FTBFS can be found at
        https://breakpoint.cc/openssl-1.1-rebuild-2016-05-29/Attempted/validns_0.8-2_amd64-20160529-1548

Sebastian

Reply | Threaded
Open this post in threaded view
|

Bug#859784: validns: Please migrate to openssl1.1 in Buster

Sebastian Andrzej Siewior
Hi,

this is a remainder about the openssl transition [0]. We really want to
remove libssl1.0-dev from unstable for Buster. I will raise the severity
of this bug to serious in a month. Please react before that happens.

[0] https://bugs.debian.org/871056#55

Sebastian

Reply | Threaded
Open this post in threaded view
|

Bug#897882: Bug#859784: NMU for validns

Sebastian Andrzej Siewior
In reply to this post by Sebastian Andrzej Siewior
On 2018-10-27 18:36:12 [+0200], Christoph Biedl wrote:

> +--- a/ipseckey.c
> ++++ b/ipseckey.c
> +@@ -111,8 +111,11 @@
> + default:
> + strcpy(gw, "??");
> + }
> ++#pragma GCC diagnostic push
> ++#pragma GCC diagnostic ignored "-Wformat-truncation"
> +     snprintf(s, 1024, "( %d %d %d %s ... )",
> +     rr->precedence, rr->gateway_type, rr->algorithm, gw);
> ++#pragma GCC diagnostic pop

This looks odd. There has to be a better way of dealing with this than
just shutting off the warning so things compile again.

> diff -Nru validns-0.8+git20160720/debian/patches/use-openssl-1.1.patch validns-0.8+git20160720/debian/patches/use-openssl-1.1.patch
> --- validns-0.8+git20160720/debian/patches/use-openssl-1.1.patch 1970-01-01 01:00:00.000000000 +0100
> +++ validns-0.8+git20160720/debian/patches/use-openssl-1.1.patch 2018-10-27 18:13:35.000000000 +0200
> +--- a/dnskey.c
> ++++ b/dnskey.c
> +@@ -154,6 +154,7 @@
> + unsigned int e_bytes;
> + unsigned char *pk;
> + int l;
> ++               BIGNUM *n, *e;
> +
> + rsa = RSA_new();
> + if (!rsa)
> +@@ -174,11 +175,12 @@
> + if (l < e_bytes) /* public key is too short */
> + goto done;
> +
> +- rsa->e = BN_bin2bn(pk, e_bytes, NULL);
> ++               e = BN_bin2bn(pk, e_bytes, NULL);

BN_bin2bn() and EVP_MD_CTX_new() which were introduced as part of this
patch may return NULL. Not a single instance in the patch checks the
return value. This is just sloppy.

> + pk += e_bytes;
> + l -= e_bytes;
> +
> +- rsa->n = BN_bin2bn(pk, l, NULL);
> ++               n = BN_bin2bn(pk, l, NULL);
> ++               RSA_set0_key(rsa, n, e, NULL);
> +
> + pkey = EVP_PKEY_new();
> + if (!pkey)
> +--- a/rrsig.c
> ++++ b/rrsig.c
> +@@ -374,7 +374,7 @@
> + static pthread_mutex_t *lock_cs;
> + static long *lock_count;
> +
> +-static unsigned long pthreads_thread_id(void)
> ++unsigned long pthreads_thread_id(void)

not only there is no need for this hunk IMHO the thread locking used
here is not required for openssl 1.1.0+.

> + {
> + unsigned long ret;
> +

Sebastian

Reply | Threaded
Open this post in threaded view
|

Bug#859784: NMU for validns

Christoph Biedl-7
Control: tags 859784 -pending
Control: tags 897882 -pending

Sebastian Andrzej Siewior wrote...

> On 2018-10-27 18:36:12 [+0200], Christoph Biedl wrote:
> > +--- a/ipseckey.c
> > ++++ b/ipseckey.c
> > +@@ -111,8 +111,11 @@
> > + default:
> > + strcpy(gw, "??");
> > + }
> > ++#pragma GCC diagnostic push
> > ++#pragma GCC diagnostic ignored "-Wformat-truncation"
> > +     snprintf(s, 1024, "( %d %d %d %s ... )",
> > +     rr->precedence, rr->gateway_type, rr->algorithm, gw);
> > ++#pragma GCC diagnostic pop
>
> This looks odd. There has to be a better way of dealing with this than
> just shutting off the warning so things compile again.
Well, i could move the fix for the underlying problem around, resulting
in a more subtle way to deal with it. Otherwise there is no difference:
Upstream took into account an information loss might happen - by using a
limit on the inet_ntop invocations a few lines above, and eventually
that snprintf. Although basically a good idea, the strict gcc checking
brings trouble, especially since the warning is treated as an error.
So instead of disabling that globally, possibly introducing real issues,
I decided to do that only at that particular place.

Another solution (not checked) was to limit gw[] and inet_ntop to, say,
768 characters. Shouldn't do any change.

> > +--- a/dnskey.c
> > ++++ b/dnskey.c
> > +@@ -154,6 +154,7 @@
> > + unsigned int e_bytes;
> > + unsigned char *pk;
> > + int l;
> > ++               BIGNUM *n, *e;
> > +
> > + rsa = RSA_new();
> > + if (!rsa)
> > +@@ -174,11 +175,12 @@
> > + if (l < e_bytes) /* public key is too short */
> > + goto done;
> > +
> > +- rsa->e = BN_bin2bn(pk, e_bytes, NULL);
> > ++               e = BN_bin2bn(pk, e_bytes, NULL);
>
> BN_bin2bn() and EVP_MD_CTX_new() which were introduced as part of this
> patch may return NULL. Not a single instance in the patch checks the
> return value. This is just sloppy.
These are worse, though. I'll cancel the upload so there's time for
improvement. The original submitter already got some feedback (probably
by you) upstream.

    Christoph

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#859784: NMU for validns

Sebastian Andrzej Siewior
In reply to this post by Sebastian Andrzej Siewior
On 2018-11-03 18:12:07 [+0100], Christoph Biedl wrote:

> Subject: Build against openssl 1.1.
> Author: Chris West <[hidden email]>
> Bug: https://github.com/tobez/validns/pull/64
> Bug-Debian: https://bugs.debian.org/859784
> Last-Update: 2018-11-03
>
> --- a/dnskey.c
> +++ b/dnskey.c
> @@ -154,6 +154,7 @@
>   unsigned int e_bytes;
>   unsigned char *pk;
>   int l;
> + BIGNUM *n, *e;
>  
>   rsa = RSA_new();
>   if (!rsa)
> @@ -174,11 +175,14 @@
>   if (l < e_bytes) /* public key is too short */
>   goto done;
>  
> - rsa->e = BN_bin2bn(pk, e_bytes, NULL);
> + e = BN_bin2bn(pk, e_bytes, NULL);
> + if (e == NULL) goto done;

putting the goto into a new line would look better and match the coding
style.

>   pk += e_bytes;
>   l -= e_bytes;
>  
> - rsa->n = BN_bin2bn(pk, l, NULL);
> + n = BN_bin2bn(pk, l, NULL);
> + if (n == NULL) goto done;
> + RSA_set0_key(rsa, n, e, NULL);
>  
>   pkey = EVP_PKEY_new();
>   if (!pkey)

> --- a/rrsig.c
> +++ b/rrsig.c
> @@ -374,7 +374,7 @@
>  static pthread_mutex_t *lock_cs;
>  static long *lock_count;
>  
> -static unsigned long pthreads_thread_id(void)
> +unsigned long pthreads_thread_id(void)
>  {
>   unsigned long ret;
>  
> @@ -382,7 +382,7 @@
>   return(ret);
>  }
>  
> -static void pthreads_locking_callback(int mode, int type, char *file, int line)
> +void pthreads_locking_callback(int mode, int type, char *file, int line)

This is noise. Plus lock_cs, lock_count, CRYPTO_set_id_callback(),
CRYPTO_set_locking_callback() is not required since OpenSSL 1.1.0:
|git grep CRYPTO_set_locking_callback include/
|include/openssl/crypto.h:#  define CRYPTO_set_locking_callback(func)

I would suggest to put it behind a version ifdef so it is left out. I
_assume_ the static has been removed to avoid "defined but not used
warning".

>  {
>   if (mode & CRYPTO_LOCK) {
>   pthread_mutex_lock(&(lock_cs[type]));
> @@ -446,6 +446,7 @@
>   if (k->to_verify[i].openssl_error != 0)
>   e = k->to_verify[i].openssl_error;
>   }
> + EVP_MD_CTX_free(k->to_verify[i].ctx);
>   }
>   if (!ok) {
>   struct named_rr *named_rr;

Otherwise it looks okay, thank you.

Sebastian

Reply | Threaded
Open this post in threaded view
|

Bug#859784: Bug #859784: validns: Please migrate to openssl1.1 in Buster

Joost van Baal-Ilić-77
In reply to this post by Sebastian Andrzej Siewior
Hi Moritz,

On Thu, Jan 10, 2019 at 08:33:05PM +0100, Moritz Mühlenhoff wrote:

> On Mon, Nov 05, 2018 at 03:13:08PM +0100, Joost van Baal-Ilić wrote:
> >
> > FWIW, this work:
> >
> > > validns (0.8+git20170804-1) unstable; urgency=medium
>
> [..]
>
> > is available from
> > https://non-gnu.uvt.nl/debian/stretch/validns/validns_0.8+git20170804-1.dsc .
> > I _*might*_ have time to merge this back with what's in the Debian archive,
> > before the freeze.
>
> What's the status?
Thanks for your interest.  Haven't done any work on it yet.

I'll visit https://wiki.debian.org/BSP/2019/01/nl/Venlo and might get to doing
some work on it, this weekend.  However, if _you're_ interested in working on
it: don't wait for me; go for it.

Bye,

Joost


signature.asc (836 bytes) Download Attachment