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.

diff --git a/lib/dns/include/dns/message.h b/lib/dns/include/dns/message.h
index 81458aa68c4116b69a4fb689dcc1c5ee2e2d60c6..7ba445a1e4aa2145304bd9a55ea45208edf7936b 100644 (file)
--- a/lib/dns/include/dns/message.h
+++ b/lib/dns/include/dns/message.h
@@ -285,18 +285,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 5b4e17d90ed1a31463f178454620ceeaf01500c1..6ad65904b732f77bd73623f77570a17e820957d5 100644 (file)
--- a/lib/dns/include/dns/tsig.h
+++ b/lib/dns/include/dns/tsig.h
@@ -78,12 +78,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 */
@@ -234,6 +236,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 9eaeb990f01cc5652cc534ef49d76892208163bc..8d573d65e64b71025cc84a53fcf102a2e9f98b96 100644 (file)
--- a/lib/dns/message.c
+++ b/lib/dns/message.c
@@ -633,12 +633,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
@@ -659,11 +658,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 57da1846588cc26123221866ad8058ec53f2740d..b143a6136e34cccc2adb56efb5ffb2ed4b514f20 100644 (file)
--- a/lib/dns/tsig.c
+++ b/lib/dns/tsig.c
@@ -189,28 +189,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,
@@ -230,6 +208,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,
        };
 
@@ -247,8 +227,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);
@@ -432,7 +410,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,
@@ -605,7 +584,7 @@ dns_tsig_sign(dns_message_t *msg) {
        };
 
        dns_name_init(&tsig.algorithm);
-       dns_name_clone(key->algorithm, &tsig.algorithm);
+       dns_name_clone(dns_tsigkey_algorithm(key), &tsig.algorithm);
 
        isc_buffer_init(&databuf, data, sizeof(data));
 
@@ -962,12 +941,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;
                }
@@ -1091,7 +1075,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;
@@ -1539,7 +1523,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;
        }
@@ -1565,6 +1550,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 601bdbb3cbd44d0880cc04609b664ca27398a931..399c4b9fe00dd95b77941ecfbb41439e7a353f14 100644 (file)
--- a/tests/dns/tsig_test.c
+++ b/tests/dns/tsig_test.c
@@ -96,7 +96,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);
-       dns_name_clone(key->algorithm, &tsig.algorithm);
+       dns_name_clone(dns_tsigkey_algorithm(key), &tsig.algorithm);
 
        tsig.timesigned = now;
        tsig.fudge = DNS_TSIG_FUDGE;