From 506138ec0f21efb2625e21127aeb4b5465f9dffc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 25 Jul 2024 20:30:03 +0200 Subject: [PATCH 1/3] Fix the assertion failure when putting 48-bit number to buffer When putting the 48-bit number into a fixed-size buffer that's exactly 6 bytes, the assertion failure would occur as the 48-bit number is internally represented as 64-bit number and the code was checking if there is enough space for `sizeof(val)`. This causes assertion failure when otherwise valid TSIG signature has a bad timing information. Specify the size of the argument explicitly, so the 48-bit number doesn't require 8-byte long buffer. (cherry picked from commit 37dbd57c163049cea87d9984b1b413b923aed23c) --- lib/isc/include/isc/buffer.h | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/lib/isc/include/isc/buffer.h b/lib/isc/include/isc/buffer.h index 8a2beac8f1..9c4a27d1d3 100644 --- a/lib/isc/include/isc/buffer.h +++ b/lib/isc/include/isc/buffer.h @@ -857,22 +857,21 @@ isc_buffer_getuint8(isc_buffer_t *restrict b) { return (val); } -#define ISC_BUFFER_PUT_RESERVE(b, v) \ - { \ - REQUIRE(ISC_BUFFER_VALID(b)); \ - \ - if (b->mctx) { \ - isc_result_t result = isc_buffer_reserve(b, \ - sizeof(val)); \ - ENSURE(result == ISC_R_SUCCESS); \ - } \ - \ - REQUIRE(isc_buffer_availablelength(b) >= sizeof(val)); \ +#define ISC_BUFFER_PUT_RESERVE(b, v, s) \ + { \ + REQUIRE(ISC_BUFFER_VALID(b)); \ + \ + if (b->mctx) { \ + isc_result_t result = isc_buffer_reserve(b, s); \ + ENSURE(result == ISC_R_SUCCESS); \ + } \ + \ + REQUIRE(isc_buffer_availablelength(b) >= s); \ } static inline void isc_buffer_putuint8(isc_buffer_t *restrict b, const uint8_t val) { - ISC_BUFFER_PUT_RESERVE(b, val); + ISC_BUFFER_PUT_RESERVE(b, val, sizeof(val)); uint8_t *cp = isc_buffer_used(b); b->used += sizeof(val); @@ -900,7 +899,7 @@ isc_buffer_getuint16(isc_buffer_t *restrict b) { static inline void isc_buffer_putuint16(isc_buffer_t *restrict b, const uint16_t val) { - ISC_BUFFER_PUT_RESERVE(b, val); + ISC_BUFFER_PUT_RESERVE(b, val, sizeof(val)); uint8_t *cp = isc_buffer_used(b); b->used += sizeof(val); @@ -928,7 +927,7 @@ isc_buffer_getuint32(isc_buffer_t *restrict b) { static inline void isc_buffer_putuint32(isc_buffer_t *restrict b, const uint32_t val) { - ISC_BUFFER_PUT_RESERVE(b, val); + ISC_BUFFER_PUT_RESERVE(b, val, sizeof(val)); uint8_t *cp = isc_buffer_used(b); b->used += sizeof(val); @@ -957,7 +956,7 @@ isc_buffer_getuint48(isc_buffer_t *restrict b) { static inline void isc_buffer_putuint48(isc_buffer_t *restrict b, const uint64_t val) { - ISC_BUFFER_PUT_RESERVE(b, val); + ISC_BUFFER_PUT_RESERVE(b, val, 6); /* 48-bits */ uint8_t *cp = isc_buffer_used(b); b->used += 6; From 0e1d47c171ec98b1aff658ba8f38bfdd45f77dec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 26 Jul 2024 02:05:43 +0200 Subject: [PATCH 2/3] Add tsig unit test for bad time and bad signatures The tsig unit test was only testing if everything went ok, but it was not testing whether the error paths work. Add two more unit tests - one uses the time outside of the TSIG skew, and the second trashes the signature with random data. (cherry picked from commit 3835d75f003a73c8b90567e0f4af182d302f4f5b) --- tests/dns/tsig_test.c | 64 +++++++++++++++++++++++++++++++++---------- 1 file changed, 49 insertions(+), 15 deletions(-) diff --git a/tests/dns/tsig_test.c b/tests/dns/tsig_test.c index b2efb5407b..95e2c0b52d 100644 --- a/tests/dns/tsig_test.c +++ b/tests/dns/tsig_test.c @@ -24,6 +24,8 @@ #include #include +#include +#include #include #include @@ -94,7 +96,8 @@ cleanup: } static isc_result_t -add_tsig(dst_context_t *tsigctx, dns_tsigkey_t *key, isc_buffer_t *target) { +add_tsig(dst_context_t *tsigctx, dns_tsigkey_t *key, isc_buffer_t *target, + isc_stdtime_t now, bool mangle_sig) { dns_compress_t cctx; dns_rdata_any_tsig_t tsig; dns_rdata_t rdata = DNS_RDATA_INIT; @@ -119,7 +122,7 @@ add_tsig(dst_context_t *tsigctx, dns_tsigkey_t *key, isc_buffer_t *target) { dns_name_init(&tsig.algorithm, NULL); dns_name_clone(key->algorithm, &tsig.algorithm); - tsig.timesigned = isc_stdtime_now(); + tsig.timesigned = now; tsig.fudge = DNS_TSIG_FUDGE; tsig.originalid = 50; tsig.error = dns_rcode_noerror; @@ -138,6 +141,9 @@ add_tsig(dst_context_t *tsigctx, dns_tsigkey_t *key, isc_buffer_t *target) { CHECK(dst_context_sign(tsigctx, &sigbuf)); tsig.siglen = isc_buffer_usedlength(&sigbuf); assert_int_equal(sigsize, tsig.siglen); + if (mangle_sig) { + isc_random_buf(tsig.signature, tsig.siglen); + } isc_buffer_allocate(mctx, &dynbuf, 512); CHECK(dns_rdata_fromstruct(&rdata, dns_rdataclass_any, @@ -260,13 +266,8 @@ render(isc_buffer_t *buf, unsigned int flags, dns_tsigkey_t *key, dns_message_detach(&msg); } -/* - * Test tsig tcp-continuation validation: - * Check that a simulated three message TCP sequence where the first - * and last messages contain TSIGs but the intermediate message doesn't - * correctly verifies. - */ -ISC_RUN_TEST_IMPL(tsig_tcp) { +static void +tsig_tcp(isc_stdtime_t now, isc_result_t expected_result, bool mangle_sig) { const dns_name_t *tsigowner = NULL; dns_fixedname_t fkeyname; dns_message_t *msg = NULL; @@ -282,8 +283,6 @@ ISC_RUN_TEST_IMPL(tsig_tcp) { dst_context_t *tsigctx = NULL; dst_context_t *outctx = NULL; - UNUSED(state); - /* isc_log_setdebuglevel(lctx, 99); */ keyname = dns_fixedname_initname(&fkeyname); @@ -409,7 +408,7 @@ ISC_RUN_TEST_IMPL(tsig_tcp) { isc_buffer_allocate(mctx, &buf, 65535); render(buf, DNS_MESSAGEFLAG_QR, key, &tsigout, &tsigout, outctx); - result = add_tsig(outctx, key, buf); + result = add_tsig(outctx, key, buf, now, mangle_sig); assert_int_equal(result, ISC_R_SUCCESS); /* @@ -438,9 +437,20 @@ ISC_RUN_TEST_IMPL(tsig_tcp) { dns_message_setquerytsig(msg, tsigin); result = dns_tsig_verify(buf, msg, NULL, NULL); - assert_int_equal(result, ISC_R_SUCCESS); - assert_int_equal(msg->verified_sig, 1); - assert_int_equal(msg->tsigstatus, dns_rcode_noerror); + switch (expected_result) { + case ISC_R_SUCCESS: + assert_int_equal(result, ISC_R_SUCCESS); + assert_int_equal(msg->verified_sig, 1); + assert_int_equal(msg->tsigstatus, dns_rcode_noerror); + break; + case DNS_R_CLOCKSKEW: + assert_int_equal(result, DNS_R_CLOCKSKEW); + assert_int_equal(msg->verified_sig, 1); + assert_int_equal(msg->tsigstatus, dns_tsigerror_badtime); + break; + default: + UNREACHABLE(); + } if (tsigin != NULL) { isc_buffer_free(&tsigin); @@ -470,6 +480,29 @@ ISC_RUN_TEST_IMPL(tsig_tcp) { } } +/* + * Test tsig tcp-continuation validation: + * Check that a simulated three message TCP sequence where the first + * and last messages contain TSIGs but the intermediate message doesn't + * correctly verifies. + */ +ISC_RUN_TEST_IMPL(tsig_tcp) { + /* Run with correct current time */ + tsig_tcp(isc_stdtime_now(), ISC_R_SUCCESS, false); +} + +ISC_RUN_TEST_IMPL(tsig_badtime) { + /* Run with time outside of the fudge */ + tsig_tcp(isc_stdtime_now() - 2 * DNS_TSIG_FUDGE, DNS_R_CLOCKSKEW, + false); + tsig_tcp(isc_stdtime_now() + 2 * DNS_TSIG_FUDGE, DNS_R_CLOCKSKEW, + false); +} + +ISC_RUN_TEST_IMPL(tsig_badsig) { + tsig_tcp(isc_stdtime_now(), DNS_R_TSIGERRORSET, true); +} + /* Tests the dns__tsig_algvalid function */ ISC_RUN_TEST_IMPL(algvalid) { UNUSED(state); @@ -487,6 +520,7 @@ ISC_RUN_TEST_IMPL(algvalid) { ISC_TEST_LIST_START ISC_TEST_ENTRY_CUSTOM(tsig_tcp, setup_test, teardown_test) +ISC_TEST_ENTRY_CUSTOM(tsig_badtime, setup_test, teardown_test) ISC_TEST_ENTRY(algvalid) ISC_TEST_LIST_END From ac170e8c5b14709811324f628a92deaef7d0660a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 26 Jul 2024 02:21:39 +0200 Subject: [PATCH 3/3] Add a system test that sends TSIG with bad time Add a system test that sets TSIG fudge to 0, waits three seconds and then sends signed message to the server. This tests the path where the time difference between the client and the server is outside of the TSIG fudge value. (cherry picked from commit 8def0c3b125bb915f304ce8e2e4ce89c7ffef777) --- bin/tests/system/tsig/tests_badtime.py | 63 ++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 bin/tests/system/tsig/tests_badtime.py diff --git a/bin/tests/system/tsig/tests_badtime.py b/bin/tests/system/tsig/tests_badtime.py new file mode 100644 index 0000000000..2f6d251151 --- /dev/null +++ b/bin/tests/system/tsig/tests_badtime.py @@ -0,0 +1,63 @@ +#!/usr/bin/python3 + +# Copyright (C) Internet Systems Consortium, Inc. ("ISC") +# +# SPDX-License-Identifier: MPL-2.0 +# +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, you can obtain one at https://mozilla.org/MPL/2.0/. +# +# See the COPYRIGHT file distributed with this work for additional +# information regarding copyright ownership. + +# pylint: disable=unused-variable + +import socket +import time + +import pytest + +pytest.importorskip("dns", minversion="2.0.0") +import dns.message +import dns.query +import dns.tsigkeyring + +TIMEOUT = 10 + + +def create_msg(qname, qtype, edns=-1): + msg = dns.message.make_query(qname, qtype, use_edns=edns) + return msg + + +def timeout(): + return time.time() + TIMEOUT + + +def create_socket(host, port): + sock = socket.create_connection((host, port), timeout=10) + sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, True) + return sock + + +def test_tsig_badtime(named_port): + with create_socket("10.53.0.1", named_port) as sock: + msg = create_msg("a.example.", "A") + + keyring = dns.tsigkeyring.from_text( + { + "sha256": "R16NojROxtxH/xbDl//ehDsHm5DjWTQ2YXV+hGC2iBY=", + } + ) + + msg.use_tsig(keyring, keyname="sha256", fudge=0) + + wire = msg.to_wire() + assert len(wire) > 0 + + time.sleep(3) + + (sbytes, stime) = dns.query.send_tcp(sock, wire, timeout()) + with pytest.raises(dns.tsig.PeerBadTime): + (response, rtime) = dns.query.receive_tcp(sock, timeout(), keyring=keyring)