[v9_11] complete change #4643

diff --git a/lib/dns/tsig.c b/lib/dns/tsig.c
index 4b662ddff69e52e559948c805c8bc7b432547cdc..d2e7a02a9198239ff3c6e6ed9bea65f00d8b2b8a 100644 (file)
--- a/lib/dns/tsig.c
+++ b/lib/dns/tsig.c
@@ -958,11 +958,20 @@ dns_tsig_sign(dns_message_t *msg) {
                isc_buffer_putuint48(&otherbuf, tsig.timesigned);
        }
 
-       if (key->key != NULL && tsig.error != dns_tsigerror_badsig) {
+       if ((key->key != NULL) &&
+           (tsig.error != dns_tsigerror_badsig) &&
+           (tsig.error != dns_tsigerror_badkey))
+       {
                unsigned char header[DNS_MESSAGE_HEADERLEN];
                isc_buffer_t headerbuf;
                isc_uint16_t digestbits;
 
+               /*
+                * If it is a response, we assume that the request MAC
+                * has validated at this point. This is why we include a
+                * MAC length > 0 in the reply.
+                */
+
                ret = dst_context_create3(key->key, mctx,
                                          DNS_LOGCATEGORY_DNSSEC,
                                          ISC_TRUE, &ctx);
@@ -970,10 +979,9 @@ dns_tsig_sign(dns_message_t *msg) {
                        return (ret);
 
                /*
-                * If this is a response and the query's signature
-                * validated, digest the query signature.
+                * If this is a response, digest the request's MAC.
                 */
-               if (response && (tsig.error == dns_rcode_noerror)) {
+               if (response) {
                        dns_rdata_t querytsigrdata = DNS_RDATA_INIT;
 
                        ret = dns_rdataset_first(msg->querytsig);
@@ -1101,6 +1109,17 @@ dns_tsig_sign(dns_message_t *msg) {
                dst_context_destroy(&ctx);
                digestbits = dst_key_getbits(key->key);
                if (digestbits != 0) {
+                       /*
+                        * XXXRAY: Is this correct? What is the
+                        * expected behavior when digestbits is not an
+                        * integral multiple of 8? It looks like bytes
+                        * should either be (digestbits/8) or
+                        * (digestbits+7)/8.
+                        *
+                        * In any case, for current algorithms,
+                        * digestbits are an integral multiple of 8, so
+                        * it has the same effect as (digestbits/8).
+                        */
                        unsigned int bytes = (digestbits + 1) / 8;
                        if (response && bytes < querytsig.siglen)
                                bytes = querytsig.siglen;
@@ -1309,19 +1328,6 @@ dns_tsig_verify(isc_buffer_t *source, dns_message_t *msg,
 
        key = tsigkey->key;
 
-       /*
-        * Is the time ok?
-        */
-       if (now + msg->timeadjust > tsig.timesigned + tsig.fudge) {
-               msg->tsigstatus = dns_tsigerror_badtime;
-               tsig_log(msg->tsigkey, 2, "signature has expired");
-               return (DNS_R_CLOCKSKEW);
-       } else if (now + msg->timeadjust < tsig.timesigned - tsig.fudge) {
-               msg->tsigstatus = dns_tsigerror_badtime;
-               tsig_log(msg->tsigkey, 2, "signature is in the future");
-               return (DNS_R_CLOCKSKEW);
-       }
-
        /*
         * Check digest length.
         */
@@ -1337,7 +1343,6 @@ dns_tsig_verify(isc_buffer_t *source, dns_message_t *msg,
            alg == DST_ALG_HMACSHA224 || alg == DST_ALG_HMACSHA256 ||
            alg == DST_ALG_HMACSHA384 || alg == DST_ALG_HMACSHA512)
        {
-               isc_uint16_t digestbits = dst_key_getbits(key);
                if (tsig.siglen > siglen) {
                        tsig_log(msg->tsigkey, 2, "signature length too big");
                        return (DNS_R_FORMERR);
@@ -1349,21 +1354,6 @@ dns_tsig_verify(isc_buffer_t *source, dns_message_t *msg,
                                 "signature length below minimum");
                        return (DNS_R_FORMERR);
                }
-               if (tsig.siglen > 0 && digestbits != 0 &&
-                   tsig.siglen < ((digestbits + 1) / 8))
-               {
-                       msg->tsigstatus = dns_tsigerror_badtrunc;
-                       tsig_log(msg->tsigkey, 2,
-                                "truncated signature length too small");
-                       return (DNS_R_TSIGVERIFYFAILURE);
-               }
-               if (tsig.siglen > 0 && digestbits == 0 &&
-                   tsig.siglen < siglen)
-               {
-                       msg->tsigstatus = dns_tsigerror_badtrunc;
-                       tsig_log(msg->tsigkey, 2, "signature length too small");
-                       return (DNS_R_TSIGVERIFYFAILURE);
-               }
        }
 
        if (tsig.siglen > 0) {
@@ -1378,7 +1368,7 @@ dns_tsig_verify(isc_buffer_t *source, dns_message_t *msg,
                if (ret != ISC_R_SUCCESS)
                        return (ret);
 
-               if (response && (tsig.error == dns_rcode_noerror)) {
+               if (response) {
                        isc_buffer_init(&databuf, data, sizeof(data));
                        isc_buffer_putuint16(&databuf, querytsig.siglen);
                        isc_buffer_usedregion(&databuf, &r);
@@ -1478,7 +1468,6 @@ dns_tsig_verify(isc_buffer_t *source, dns_message_t *msg,
 
                ret = dst_context_verify(ctx, &sig_r);
                if (ret == DST_R_VERIFYFAILURE) {
-                       msg->tsigstatus = dns_tsigerror_badsig;
                        ret = DNS_R_TSIGVERIFYFAILURE;
                        tsig_log(msg->tsigkey, 2,
                                 "signature failed to verify(1)");
@@ -1488,11 +1477,71 @@ dns_tsig_verify(isc_buffer_t *source, dns_message_t *msg,
                }
        } else if (tsig.error != dns_tsigerror_badsig &&
                   tsig.error != dns_tsigerror_badkey) {
-               msg->tsigstatus = dns_tsigerror_badsig;
                tsig_log(msg->tsigkey, 2, "signature was empty");
                return (DNS_R_TSIGVERIFYFAILURE);
        }
 
+       /*
+        * Here at this point, the MAC has been verified. Even if any of
+        * the following code returns a TSIG error, the reply will be
+        * signed and WILL always include the request MAC in the digest
+        * computation.
+        */
+
+       /*
+        * Is the time ok?
+        */
+       if (now + msg->timeadjust > tsig.timesigned + tsig.fudge) {
+               msg->tsigstatus = dns_tsigerror_badtime;
+               tsig_log(msg->tsigkey, 2, "signature has expired");
+               ret = DNS_R_CLOCKSKEW;
+               goto cleanup_context;
+       } else if (now + msg->timeadjust < tsig.timesigned - tsig.fudge) {
+               msg->tsigstatus = dns_tsigerror_badtime;
+               tsig_log(msg->tsigkey, 2, "signature is in the future");
+               ret = DNS_R_CLOCKSKEW;
+               goto cleanup_context;
+       }
+
+       if (
+#ifndef PK11_MD5_DISABLE
+           alg == DST_ALG_HMACMD5 ||
+#endif
+           alg == DST_ALG_HMACSHA1 ||
+           alg == DST_ALG_HMACSHA224 || alg == DST_ALG_HMACSHA256 ||
+           alg == DST_ALG_HMACSHA384 || alg == DST_ALG_HMACSHA512)
+       {
+               isc_uint16_t digestbits = dst_key_getbits(key);
+
+               /*
+                * XXXRAY: Is this correct? What is the expected
+                * behavior when digestbits is not an integral multiple
+                * of 8? It looks like bytes should either be
+                * (digestbits/8) or (digestbits+7)/8.
+                *
+                * In any case, for current algorithms, digestbits are
+                * an integral multiple of 8, so it has the same effect
+                * as (digestbits/8).
+                */
+               if (tsig.siglen > 0 && digestbits != 0 &&
+                   tsig.siglen < ((digestbits + 1) / 8))
+               {
+                       msg->tsigstatus = dns_tsigerror_badtrunc;
+                       tsig_log(msg->tsigkey, 2,
+                                "truncated signature length too small");
+                       ret = DNS_R_TSIGVERIFYFAILURE;
+                       goto cleanup_context;
+               }
+               if (tsig.siglen > 0 && digestbits == 0 &&
+                   tsig.siglen < siglen)
+               {
+                       msg->tsigstatus = dns_tsigerror_badtrunc;
+                       tsig_log(msg->tsigkey, 2, "signature length too small");
+                       ret = DNS_R_TSIGVERIFYFAILURE;
+                       goto cleanup_context;
+               }
+       }
+
        if (tsig.error != dns_rcode_noerror) {
                msg->tsigstatus = tsig.error;
                if (tsig.error == dns_tsigerror_badtime)
@@ -1529,6 +1578,8 @@ tsig_verify_tcp(isc_buffer_t *source, dns_message_t *msg) {
        isc_uint16_t addcount, id;
        isc_boolean_t has_tsig = ISC_FALSE;
        isc_mem_t *mctx;
+       unsigned int siglen;
+       unsigned int alg;
 
        REQUIRE(source != NULL);
        REQUIRE(msg != NULL);
@@ -1545,6 +1596,7 @@ tsig_verify_tcp(isc_buffer_t *source, dns_message_t *msg) {
        mctx = msg->mctx;
 
        tsigkey = dns_message_gettsigkey(msg);
+       key = tsigkey->key;
 
        /*
         * Extract and parse the previous TSIG
@@ -1587,28 +1639,40 @@ tsig_verify_tcp(isc_buffer_t *source, dns_message_t *msg) {
                }
 
                /*
-                * Is the time ok?
+                * Check digest length.
                 */
-               isc_stdtime_get(&now);
-
-               if (now + msg->timeadjust > tsig.timesigned + tsig.fudge) {
-                       msg->tsigstatus = dns_tsigerror_badtime;
-                       tsig_log(msg->tsigkey, 2, "signature has expired");
-                       ret = DNS_R_CLOCKSKEW;
+               alg = dst_key_alg(key);
+               ret = dst_key_sigsize(key, &siglen);
+               if (ret != ISC_R_SUCCESS)
                        goto cleanup_querystruct;
-               } else if (now + msg->timeadjust <
-                          tsig.timesigned - tsig.fudge)
+               if (
+#ifndef PK11_MD5_DISABLE
+                       alg == DST_ALG_HMACMD5 ||
+#endif
+                       alg == DST_ALG_HMACSHA1 ||
+                       alg == DST_ALG_HMACSHA224 ||
+                       alg == DST_ALG_HMACSHA256 ||
+                       alg == DST_ALG_HMACSHA384 ||
+                       alg == DST_ALG_HMACSHA512)
                {
-                       msg->tsigstatus = dns_tsigerror_badtime;
-                       tsig_log(msg->tsigkey, 2,
-                                "signature is in the future");
-                       ret = DNS_R_CLOCKSKEW;
-                       goto cleanup_querystruct;
+                       if (tsig.siglen > siglen) {
+                               tsig_log(tsigkey, 2,
+                                        "signature length too big");
+                               ret = DNS_R_FORMERR;
+                               goto cleanup_querystruct;
+                       }
+                       if (tsig.siglen > 0 &&
+                           (tsig.siglen < 10 ||
+                            tsig.siglen < ((siglen + 1) / 2)))
+                       {
+                               tsig_log(tsigkey, 2,
+                                        "signature length below minimum");
+                               ret = DNS_R_FORMERR;
+                               goto cleanup_querystruct;
+                       }
                }
        }
 
-       key = tsigkey->key;
-
        if (msg->tsigctx == NULL) {
                ret = dst_context_create3(key, mctx,
                                          DNS_LOGCATEGORY_DNSSEC,
@@ -1720,7 +1784,6 @@ tsig_verify_tcp(isc_buffer_t *source, dns_message_t *msg) {
 
                ret = dst_context_verify(msg->tsigctx, &sig_r);
                if (ret == DST_R_VERIFYFAILURE) {
-                       msg->tsigstatus = dns_tsigerror_badsig;
                        tsig_log(msg->tsigkey, 2,
                                 "signature failed to verify(2)");
                        ret = DNS_R_TSIGVERIFYFAILURE;
@@ -1729,6 +1792,81 @@ tsig_verify_tcp(isc_buffer_t *source, dns_message_t *msg) {
                        goto cleanup_context;
                }
 
+               /*
+                * Here at this point, the MAC has been verified. Even
+                * if any of the following code returns a TSIG error,
+                * the reply will be signed and WILL always include the
+                * request MAC in the digest computation.
+                */
+
+               /*
+                * Is the time ok?
+                */
+               isc_stdtime_get(&now);
+
+               if (now + msg->timeadjust > tsig.timesigned + tsig.fudge) {
+                       msg->tsigstatus = dns_tsigerror_badtime;
+                       tsig_log(msg->tsigkey, 2, "signature has expired");
+                       ret = DNS_R_CLOCKSKEW;
+                       goto cleanup_context;
+               } else if (now + msg->timeadjust <
+                          tsig.timesigned - tsig.fudge)
+               {
+                       msg->tsigstatus = dns_tsigerror_badtime;
+                       tsig_log(msg->tsigkey, 2,
+                                "signature is in the future");
+                       ret = DNS_R_CLOCKSKEW;
+                       goto cleanup_context;
+               }
+
+               alg = dst_key_alg(key);
+               ret = dst_key_sigsize(key, &siglen);
+               if (ret != ISC_R_SUCCESS)
+                       goto cleanup_context;
+               if (
+#ifndef PK11_MD5_DISABLE
+                       alg == DST_ALG_HMACMD5 ||
+#endif
+                       alg == DST_ALG_HMACSHA1 ||
+                       alg == DST_ALG_HMACSHA224 ||
+                       alg == DST_ALG_HMACSHA256 ||
+                       alg == DST_ALG_HMACSHA384 ||
+                       alg == DST_ALG_HMACSHA512)
+               {
+                       isc_uint16_t digestbits = dst_key_getbits(key);
+
+                       /*
+                        * XXXRAY: Is this correct? What is the
+                        * expected behavior when digestbits is not an
+                        * integral multiple of 8? It looks like bytes
+                        * should either be (digestbits/8) or
+                        * (digestbits+7)/8.
+                        *
+                        * In any case, for current algorithms,
+                        * digestbits are an integral multiple of 8, so
+                        * it has the same effect as (digestbits/8).
+                        */
+                       if (tsig.siglen > 0 && digestbits != 0 &&
+                           tsig.siglen < ((digestbits + 1) / 8))
+                       {
+                               msg->tsigstatus = dns_tsigerror_badtrunc;
+                               tsig_log(msg->tsigkey, 2,
+                                        "truncated signature length "
+                                        "too small");
+                               ret = DNS_R_TSIGVERIFYFAILURE;
+                               goto cleanup_context;
+                       }
+                       if (tsig.siglen > 0 && digestbits == 0 &&
+                           tsig.siglen < siglen)
+                       {
+                               msg->tsigstatus = dns_tsigerror_badtrunc;
+                               tsig_log(msg->tsigkey, 2,
+                                        "signature length too small");
+                               ret = DNS_R_TSIGVERIFYFAILURE;
+                               goto cleanup_context;
+                       }
+               }
+
                if (tsig.error != dns_rcode_noerror) {
                        msg->tsigstatus = tsig.error;
                        if (tsig.error == dns_tsigerror_badtime)