Prevent assertion when processing TSIG algorithm

In a previous change, the "algorithm" value passed to
dns_tsigkey_create() was changed from a DNS name to an integer;
the name was then chosen from a table of known algorithms. A
side effect of this change was that a query using an unknown TSIG
algorithm was no longer handled correctly, and could trigger an
assertion failure.  This has been corrected.

The dns_tsigkey struct now stores the signing algorithm
as dst_algorithm_t value 'alg' instead of as a dns_name,
but retains an 'algname' field, which is used only when the
algorithm is DST_ALG_UNKNOWN.  This allows the name of the
unrecognized algorithm name to be returned in a BADKEY
response.

(cherry picked from commit decf461d68846d6754c1f64790c3f9006d158a1d)

diff --git a/lib/dns/include/dns/message.h b/lib/dns/include/dns/message.h
index 89e20b8306993907980c723a2f7f2f8409be2eea..6a70665ba7c2c5d562f9532b869bc2c8622950c2 100644 (file)
--- a/lib/dns/include/dns/message.h
+++ b/lib/dns/include/dns/message.h
@@ -287,18 +287,16 @@ struct dns_message {
        ISC_LIST(dns_rdata_t) freerdata;
        ISC_LIST(dns_rdatalist_t) freerdatalist;
 
-       dns_rcode_t tsigstatus;
-       dns_rcode_t querytsigstatus;
-       dns_name_t *tsigname; /* Owner name of TSIG, if any
-                              * */
+       dns_rcode_t     tsigstatus;
+       dns_rcode_t     querytsigstatus;
+       dns_name_t     *tsigname; /* Owner name of TSIG, if any */
        dns_rdataset_t *querytsig;
        dns_tsigkey_t  *tsigkey;
        dst_context_t  *tsigctx;
        int             sigstart;
        int             timeadjust;
 
-       dns_name_t *sig0name; /* Owner name of SIG0, if any
-                              * */
+       dns_name_t  *sig0name; /* Owner name of SIG0, if any */
        dst_key_t   *sig0key;
        dns_rcode_t  sig0status;
        isc_region_t query;

diff --git a/lib/dns/include/dns/tsig.h b/lib/dns/include/dns/tsig.h
index edd0694df049238a0a3d3aa2bd5981de53cc86df..1f1f4af7339816812d5d7a2c782d221d47aee548 100644 (file)
--- a/lib/dns/include/dns/tsig.h
+++ b/lib/dns/include/dns/tsig.h
@@ -79,12 +79,14 @@ struct dns_tsigkeyring {
 
 struct dns_tsigkey {
        /* Unlocked */
-       unsigned int       magic; /*%< Magic number. */
-       isc_mem_t         *mctx;
-       dst_key_t         *key; /*%< Key */
-       dns_fixedname_t    fn;
-       dns_name_t        *name;          /*%< Key name */
-       const dns_name_t  *algorithm;     /*%< Algorithm name */
+       unsigned int    magic; /*%< Magic number. */
+       isc_mem_t      *mctx;
+       dst_key_t      *key; /*%< Key */
+       dns_fixedname_t fn;
+       dns_name_t     *name;             /*%< Key name */
+       dst_algorithm_t alg;              /*< Algorithm */
+       dns_name_t      algname;          /*< Algorithm name, only used if
+                                           algorithm is DST_ALG_UNKNOWN */
        dns_name_t        *creator;       /*%< name that created secret */
        bool               generated : 1; /*%< key was auto-generated */
        bool               restored  : 1; /*%< key was restored at startup */
@@ -240,6 +242,16 @@ dns_tsigkey_find(dns_tsigkey_t **tsigkeyp, const dns_name_t *name,
  *\li          #ISC_R_NOTFOUND
  */
 
+const dns_name_t *
+dns_tsigkey_algorithm(dns_tsigkey_t *tkey);
+/*%<
+ *     Returns the key algorithm associated with a tsigkey object.
+ *
+ *     Note that when a tsigkey object is created with algorithm
+ *     DST_ALG_UNKNOWN, the unknown algorithm's name must be cloned
+ *     into tsigkey->algname.
+ */
+
 void
 dns_tsigkeyring_create(isc_mem_t *mctx, dns_tsigkeyring_t **ringp);
 /*%<

diff --git a/lib/dns/message.c b/lib/dns/message.c
index 80babc99a9ef17ca069e3d7abc9f3ebf83c9b572..3b4757e5050d71a8dc74cf41968082879815f9ec 100644 (file)
--- a/lib/dns/message.c
+++ b/lib/dns/message.c
@@ -687,12 +687,11 @@ msgreset(dns_message_t *msg, bool everything) {
 
 static unsigned int
 spacefortsig(dns_tsigkey_t *key, int otherlen) {
-       isc_region_t r1, r2;
-       unsigned int x;
-       isc_result_t result;
+       isc_region_t r1 = { 0 }, r2 = { 0 };
+       unsigned int x = 0;
 
        /*
-        * The space required for an TSIG record is:
+        * The space required for a TSIG record is:
         *
         *      n1 bytes for the name
         *      2 bytes for the type
@@ -713,11 +712,11 @@ spacefortsig(dns_tsigkey_t *key, int otherlen) {
         */
 
        dns_name_toregion(key->name, &r1);
-       dns_name_toregion(key->algorithm, &r2);
-       if (key->key == NULL) {
-               x = 0;
-       } else {
-               result = dst_key_sigsize(key->key, &x);
+       if (key->alg != DST_ALG_UNKNOWN) {
+               dns_name_toregion(dns_tsigkey_algorithm(key), &r2);
+       }
+       if (key->key != NULL) {
+               isc_result_t result = dst_key_sigsize(key->key, &x);
                if (result != ISC_R_SUCCESS) {
                        x = 0;
                }

diff --git a/lib/dns/tsig.c b/lib/dns/tsig.c
index a6b839cdcf54b736aaf0418de2429f0b4603f4bd..25b9c7f50824ca10feb149c56834000a8774b39f 100644 (file)
--- a/lib/dns/tsig.c
+++ b/lib/dns/tsig.c
@@ -205,28 +205,6 @@ adjust_lru(dns_tsigkey_t *tkey) {
        }
 }
 
-static const dns_name_t *
-namefromalg(dst_algorithm_t alg) {
-       switch (alg) {
-       case DST_ALG_HMACMD5:
-               return dns_tsig_hmacmd5_name;
-       case DST_ALG_HMACSHA1:
-               return dns_tsig_hmacsha1_name;
-       case DST_ALG_HMACSHA224:
-               return dns_tsig_hmacsha224_name;
-       case DST_ALG_HMACSHA256:
-               return dns_tsig_hmacsha256_name;
-       case DST_ALG_HMACSHA384:
-               return dns_tsig_hmacsha384_name;
-       case DST_ALG_HMACSHA512:
-               return dns_tsig_hmacsha512_name;
-       case DST_ALG_GSSAPI:
-               return dns_tsig_gssapi_name;
-       default:
-               return NULL;
-       }
-}
-
 isc_result_t
 dns_tsigkey_createfromkey(const dns_name_t *name, dst_algorithm_t algorithm,
                          dst_key_t *dstkey, bool generated, bool restored,
@@ -246,6 +224,8 @@ dns_tsigkey_createfromkey(const dns_name_t *name, dst_algorithm_t algorithm,
                .restored = restored,
                .inception = inception,
                .expire = expire,
+               .alg = algorithm,
+               .algname = DNS_NAME_INITEMPTY,
                .link = ISC_LINK_INITIALIZER,
        };
 
@@ -263,8 +243,6 @@ dns_tsigkey_createfromkey(const dns_name_t *name, dst_algorithm_t algorithm,
                goto cleanup_name;
        }
 
-       tkey->algorithm = namefromalg(algorithm);
-
        if (creator != NULL) {
                tkey->creator = isc_mem_get(mctx, sizeof(dns_name_t));
                dns_name_init(tkey->creator, NULL);
@@ -448,7 +426,8 @@ dump_key(dns_tsigkey_t *tkey, FILE *fp) {
 
        dns_name_format(tkey->name, namestr, sizeof(namestr));
        dns_name_format(tkey->creator, creatorstr, sizeof(creatorstr));
-       dns_name_format(tkey->algorithm, algorithmstr, sizeof(algorithmstr));
+       dns_name_format(dns_tsigkey_algorithm(tkey), algorithmstr,
+                       sizeof(algorithmstr));
        result = dst_key_dump(tkey->key, tkey->mctx, &buffer, &length);
        if (result == ISC_R_SUCCESS) {
                fprintf(fp, "%s %s %u %u %s %.*s\n", namestr, creatorstr,
@@ -621,7 +600,7 @@ dns_tsig_sign(dns_message_t *msg) {
        };
 
        dns_name_init(&tsig.algorithm, NULL);
-       dns_name_clone(key->algorithm, &tsig.algorithm);
+       dns_name_clone(dns_tsigkey_algorithm(key), &tsig.algorithm);
 
        isc_buffer_init(&databuf, data, sizeof(data));
 
@@ -978,12 +957,17 @@ dns_tsig_verify(isc_buffer_t *source, dns_message_t *msg,
                }
                if (result != ISC_R_SUCCESS) {
                        msg->tsigstatus = dns_tsigerror_badkey;
-                       result = dns_tsigkey_create(
-                               keyname, dns__tsig_algfromname(&tsig.algorithm),
-                               NULL, 0, mctx, &msg->tsigkey);
+                       alg = dns__tsig_algfromname(&tsig.algorithm);
+                       result = dns_tsigkey_create(keyname, alg, NULL, 0, mctx,
+                                                   &msg->tsigkey);
                        if (result != ISC_R_SUCCESS) {
                                return result;
                        }
+                       if (alg == DST_ALG_UNKNOWN) {
+                               dns_name_clone(&tsig.algorithm,
+                                              &msg->tsigkey->algname);
+                       }
+
                        tsig_log(msg->tsigkey, 2, "unknown key");
                        return DNS_R_TSIGVERIFYFAILURE;
                }
@@ -1107,7 +1091,7 @@ dns_tsig_verify(isc_buffer_t *source, dns_message_t *msg,
                /*
                 * Digest the key algorithm.
                 */
-               dns_name_toregion(tsigkey->algorithm, &r);
+               dns_name_toregion(dns_tsigkey_algorithm(tsigkey), &r);
                result = dst_context_adddata(ctx, &r);
                if (result != ISC_R_SUCCESS) {
                        goto cleanup_context;
@@ -1555,7 +1539,8 @@ again:
                RWUNLOCK(&ring->lock, locktype);
                return result;
        }
-       if (algorithm != NULL && !dns_name_equal(key->algorithm, algorithm)) {
+
+       if (algorithm != NULL && key->alg != dns__tsig_algfromname(algorithm)) {
                RWUNLOCK(&ring->lock, locktype);
                return ISC_R_NOTFOUND;
        }
@@ -1581,6 +1566,39 @@ again:
        return ISC_R_SUCCESS;
 }
 
+const dns_name_t *
+dns_tsigkey_algorithm(dns_tsigkey_t *tkey) {
+       REQUIRE(VALID_TSIGKEY(tkey));
+
+       switch (tkey->alg) {
+       case DST_ALG_HMACMD5:
+               return dns_tsig_hmacmd5_name;
+       case DST_ALG_HMACSHA1:
+               return dns_tsig_hmacsha1_name;
+       case DST_ALG_HMACSHA224:
+               return dns_tsig_hmacsha224_name;
+       case DST_ALG_HMACSHA256:
+               return dns_tsig_hmacsha256_name;
+       case DST_ALG_HMACSHA384:
+               return dns_tsig_hmacsha384_name;
+       case DST_ALG_HMACSHA512:
+               return dns_tsig_hmacsha512_name;
+       case DST_ALG_GSSAPI:
+               return dns_tsig_gssapi_name;
+
+       case DST_ALG_UNKNOWN:
+               /*
+                * If the tsigkey object was created with an
+                * unknown algorithm, then we cloned
+                * the algorithm name here.
+                */
+               return &tkey->algname;
+
+       default:
+               UNREACHABLE();
+       }
+}
+
 void
 dns_tsigkeyring_create(isc_mem_t *mctx, dns_tsigkeyring_t **ringp) {
        dns_tsigkeyring_t *ring = NULL;

diff --git a/tests/dns/tsig_test.c b/tests/dns/tsig_test.c
index 48adbab5a330466979014a952d94e22bdc047959..ef0a5b3c26af9b67b2ca6336b4baa8c602eca2f2 100644 (file)
--- a/tests/dns/tsig_test.c
+++ b/tests/dns/tsig_test.c
@@ -118,7 +118,7 @@ add_tsig(dst_context_t *tsigctx, dns_tsigkey_t *key, isc_buffer_t *target,
        tsig.common.rdtype = dns_rdatatype_tsig;
        ISC_LINK_INIT(&tsig.common, link);
        dns_name_init(&tsig.algorithm, NULL);
-       dns_name_clone(key->algorithm, &tsig.algorithm);
+       dns_name_clone(dns_tsigkey_algorithm(key), &tsig.algorithm);
 
        tsig.timesigned = now;
        tsig.fudge = DNS_TSIG_FUDGE;