Bug#859784: NMU for validns
On 2018-11-03 18:12:07 [+0100], Christoph Biedl wrote:
> Subject: Build against openssl 1.1.
> Author: Chris West
> 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
Bug#859784: NMU for validns
Sebastian Andrzej Siewior wrote...
> 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.
As I'd still like to fix validns for Debian: Mind I ask you for review
of the updated patch below? Are there more flaky things in the openssl
adjustment patch beside those you've mentioned?
As this is a NMU, I'd like to keep the change small, hence I blatantly
ignored "The whole locking callbacks and memory allocation is not
required for 1.1" remark since, as far as I understand it, this does
no harm. Marking pthreads_thread_id and pthreads_locking_callback as
non-static is required to avoid a compiler error.
Changes:
* Adjust whitespace to upstream form
* Catch failure from library calls in three places
By the way, #897882 will see a nicer solution as well.
Regards,
Christoph
Subject: Build against openssl 1.1.
Author: Chris West
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;
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/nsec3checks.c
+++ b/nsec3checks.c
@@ -28,7 +28,7 @@
static struct binary_data name2hash(char *name, struct rr *param)
{
struct rr_nsec3param *p = (struct rr_nsec3param *)param;
- EVP_MD_CTX ctx;
+ EVP_MD_CTX *ctx;
unsigned char md0[EVP_MAX_MD_SIZE];
unsigned char md1[EVP_MAX_MD_SIZE];
unsigned char *md[2];
@@ -45,22 +45,24 @@
/* XXX Maybe use Init_ex and Final_ex for speed? */
- EVP_MD_CTX_init(&ctx);
- if (EVP_DigestInit(&ctx, EVP_sha1()) != 1)
+ ctx = EVP_MD_CTX_new();
+ if (ctx == NULL) return r;
+ if (EVP_DigestInit(ctx, EVP_sha1()) != 1)
return r;
- digest_size = EVP_MD_CTX_size(&ctx);
- EVP_DigestUpdate(&ctx, wire_name.data, wire_name.length);
- EVP_DigestUpdate(&ctx, p->salt.data, p->salt.length);
- EVP_DigestFinal(&ctx, md[mdi], NULL);
+ digest_size = EVP_MD_CTX_size(ctx);
+ EVP_DigestUpdate(ctx, wire_name.data, wire_name.length);
+ EVP_DigestUpdate(ctx, p->salt.data, p->salt.length);
+ EVP_DigestFinal(ctx, md[mdi], NULL);
for (i = 0; i < p->iterations; i++) {
- if (EVP_DigestInit(&ctx, EVP_sha1()) != 1)
+ if (EVP_DigestInit(ctx, EVP_sha1()) != 1)
return r;
- EVP_DigestUpdate(&ctx, md[mdi], digest_size);
+ EVP_DigestUpdate(ctx, md[mdi], digest_size);
mdi = (mdi + 1) % 2;
- EVP_DigestUpdate(&ctx, p->salt.data, p->salt.length);
- EVP_DigestFinal(&ctx, md[mdi], NULL);
+ EVP_DigestUpdate(ctx, p->salt.data, p->salt.length);
+ EVP_DigestFinal(ctx, md[mdi], NULL);
}
+ EVP_MD_CTX_free(ctx);
r.length = digest_size;
r.data = getmem(digest_size);
--- a/rrsig.c
+++ b/rrsig.c
@@ -26,7 +26,7 @@
struct verification_data
{
struct verification_data *next;
- EVP_MD_CTX ctx;
+ EVP_MD_CTX *ctx;
struct rr_dnskey *key;
struct rr_rrsig *rr;
int ok;
@@ -180,7 +180,7 @@
if (d) {
int r;
d->next = NULL;
- r = EVP_VerifyFinal(&d->ctx, (unsigned char
*)d->rr->signature.data, d->rr->signature.length, d->key->pkey);
+ r = EVP_VerifyFinal(d->ctx, (unsigned char
*)d->rr->signature.data, d->rr->signature.length, d->key->pkey);
if (r == 1) {
d->ok = 1;
} else {
@@ -232,7 +232,7 @@
} else {
int r;
G.stats.signatures_verified++;
- r = EVP_VerifyFinal(&d->ctx, (unsigned char
*)d->rr->signature.data, d->rr->signature.length, d->key->pkey);
+ r = EVP_VerifyFinal(d->ctx, (unsigned char
*)d->rr->signature.data, d->rr->signature.length, d->key->pkey);
if (r == 1) {
d->ok = 1;
} else {
@@ -250,21 +250,21 @@
struct rr *signed_rr;
int i;
- EVP_MD_CTX_init
Bug#859784: NMU for validns
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 Description: PGP signature
Bug#897882: Bug#859784: NMU for validns
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.0 +0100
> +++ validns-0.8+git20160720/debian/patches/use-openssl-1.1.patch
> 2018-10-27 18:13:35.0 +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
