diff --git a/lib/dns/dnssec.c b/lib/dns/dnssec.c index e619366..2f3d23b 100644 --- a/lib/dns/dnssec.c +++ b/lib/dns/dnssec.c @@ -1070,6 +1070,8 @@ dns_dnssec_verifymessage(isc_buffer_t *source, dns_message_t *msg, mctx = msg->mctx; msg->verify_attempted = 1; + msg->verified_sig = 0; + msg->sig0status = dns_tsigerror_badsig; if (is_response(msg)) { if (msg->query.base == NULL) @@ -1165,6 +1167,7 @@ dns_dnssec_verifymessage(isc_buffer_t *source, dns_message_t *msg, } msg->verified_sig = 1; + msg->sig0status = dns_rcode_noerror; dst_context_destroy(&ctx); dns_rdata_freestruct(&sig); diff --git a/lib/dns/message.c b/lib/dns/message.c index 08d2ffd..a531b6f 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -3052,12 +3052,19 @@ dns_message_signer(dns_message_t *msg, dns_name_t *signer) { result = dns_rdata_tostruct(&rdata, &tsig, NULL); INSIST(result == ISC_R_SUCCESS); - if (msg->tsigstatus != dns_rcode_noerror) + if (msg->verified_sig && + msg->tsigstatus == dns_rcode_noerror && + tsig.error == dns_rcode_noerror) + { + result = ISC_R_SUCCESS; + } else if ((!msg->verified_sig) || + (msg->tsigstatus != dns_rcode_noerror)) + { result = DNS_R_TSIGVERIFYFAILURE; - else if (tsig.error != dns_rcode_noerror) + } else { + INSIST(tsig.error != dns_rcode_noerror); result = DNS_R_TSIGERRORSET; - else - result = ISC_R_SUCCESS; + } dns_rdata_freestruct(&tsig); if (msg->tsigkey == NULL) { diff --git a/lib/dns/tsig.c b/lib/dns/tsig.c index 8055013..b68c445 100644 --- 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,7 +979,7 @@ dns_tsig_sign(dns_message_t *msg) { return (ret); /* - * If this is a response, digest the query signature. + * If this is a response, digest the request's MAC. */ if (response) { dns_rdata_t querytsigrdata = DNS_RDATA_INIT; @@ -1100,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; @@ -1209,6 +1229,8 @@ dns_tsig_verify(isc_buffer_t *source, dns_message_t *msg, REQUIRE(tsigkey == NULL || VALID_TSIG_KEY(tsigkey)); msg->verify_attempted = 1; + msg->verified_sig = 0; + msg->tsigstatus = dns_tsigerror_badsig; if (msg->tcp_continuation) { if (tsigkey == NULL || msg->querytsig == NULL) @@ -1307,19 +1329,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. */ alg = dst_key_alg(key); @@ -1332,31 +1341,19 @@ dns_tsig_verify(isc_buffer_t *source, dns_message_t *msg, #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); + alg == DST_ALG_HMACSHA384 || alg == DST_ALG_HMACSHA512) + { if (tsig.siglen > siglen) { tsig_log(msg->tsigkey, 2, "signature length too big"); return (DNS_R_FORMERR); } if (tsig.siglen > 0 && - (tsig.siglen < 10 || tsig.siglen < ((siglen + 1) / 2))) { + (tsig.siglen < 10 || tsig.siglen < ((siglen + 1) / 2))) + { tsig_log(msg->tsigkey, 2, "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) { @@ -1471,34 +1468,92 @@ 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)"); goto cleanup_context; - } else if (ret != ISC_R_SUCCESS) + } else if (ret != ISC_R_SUCCESS) { goto cleanup_context; - - dst_context_destroy(&ctx); + } } 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); } - msg->tsigstatus = dns_rcode_noerror; + /* + * 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) - return (DNS_R_CLOCKSKEW); + ret = DNS_R_CLOCKSKEW; else - return (DNS_R_TSIGERRORSET); + ret = DNS_R_TSIGERRORSET; + goto cleanup_context; } + msg->tsigstatus = dns_rcode_noerror; msg->verified_sig = 1; - - return (ISC_R_SUCCESS); + ret = ISC_R_SUCCESS; cleanup_context: if (ctx != NULL) @@ -1523,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); @@ -1530,12 +1587,16 @@ tsig_verify_tcp(isc_buffer_t *source, dns_message_t *msg) { REQUIRE(msg->tcp_continuation == 1); REQUIRE(msg->querytsig != NULL); + msg->verified_sig = 0; + msg->tsigstatus = dns_tsigerror_badsig; + if (!is_response(msg)) return (DNS_R_EXPECTEDRESPONSE); mctx = msg->mctx; tsigkey = dns_message_gettsigkey(msg); + key = tsigkey->key; /* * Extract and parse the previous TSIG @@ -1568,7 +1629,8 @@ tsig_verify_tcp(isc_buffer_t *source, dns_message_t *msg) { * Do the key name and algorithm match that of the query? */ if (!dns_name_equal(keyname, &tsigkey->name) || - !dns_name_equal(&tsig.algorithm, &querytsig.algorithm)) { + !dns_name_equal(&tsig.algorithm, &querytsig.algorithm)) + { msg->tsigstatus = dns_tsigerror_badkey; ret = DNS_R_TSIGVERIFYFAILURE; tsig_log(msg->tsigkey, 2, @@ -1577,27 +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; - goto cleanup_querystruct; - } 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; + alg = dst_key_alg(key); + ret = dst_key_sigsize(key, &siglen); + if (ret != ISC_R_SUCCESS) goto cleanup_querystruct; + 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) + { + 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, @@ -1693,10 +1768,12 @@ tsig_verify_tcp(isc_buffer_t *source, dns_message_t *msg) { sig_r.length = tsig.siglen; if (tsig.siglen == 0) { if (tsig.error != dns_rcode_noerror) { - if (tsig.error == dns_tsigerror_badtime) + msg->tsigstatus = tsig.error; + if (tsig.error == dns_tsigerror_badtime) { ret = DNS_R_CLOCKSKEW; - else + } else { ret = DNS_R_TSIGERRORSET; + } } else { tsig_log(msg->tsigkey, 2, "signature is empty"); @@ -1707,29 +1784,111 @@ 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; goto cleanup_context; + } else if (ret != ISC_R_SUCCESS) { + 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; } - else if (ret != ISC_R_SUCCESS) + + 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); - dst_context_destroy(&msg->tsigctx); + /* + * 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) + ret = DNS_R_CLOCKSKEW; + else + ret = DNS_R_TSIGERRORSET; + goto cleanup_context; + } } msg->tsigstatus = dns_rcode_noerror; - return (ISC_R_SUCCESS); + msg->verified_sig = 1; + ret = ISC_R_SUCCESS; cleanup_context: - dst_context_destroy(&msg->tsigctx); + if (msg->tsigctx != NULL) + dst_context_destroy(&msg->tsigctx); cleanup_querystruct: dns_rdata_freestruct(&querytsig); return (ret); - } isc_result_t