From bef3d2cca3552100bbe44790c8c1a4f5bef06798 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0pa=C4=8Dek?= Date: Thu, 16 May 2024 12:10:41 +0200 Subject: [PATCH 1/7] Remove support for SIG(0) message verification --- lib/dns/message.c | 99 +++-------------------------------------------- lib/ns/client.c | 7 ++++ 2 files changed, 13 insertions(+), 93 deletions(-) diff --git a/lib/dns/message.c b/lib/dns/message.c index f8e45ecf92..1d3b6fe2d3 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -3292,111 +3292,24 @@ dns_message_dumpsig(dns_message_t *msg, char *txt1) { isc_result_t dns_message_checksig(dns_message_t *msg, dns_view_t *view) { - isc_buffer_t b, msgb; + isc_buffer_t msgb; REQUIRE(DNS_MESSAGE_VALID(msg)); - if (msg->tsigkey == NULL && msg->tsig == NULL && msg->sig0 == NULL) { + if (msg->tsigkey == NULL && msg->tsig == NULL) { return (ISC_R_SUCCESS); } INSIST(msg->saved.base != NULL); isc_buffer_init(&msgb, msg->saved.base, msg->saved.length); isc_buffer_add(&msgb, msg->saved.length); - if (msg->tsigkey != NULL || msg->tsig != NULL) { #ifdef SKAN_MSG_DEBUG - dns_message_dumpsig(msg, "dns_message_checksig#1"); + dns_message_dumpsig(msg, "dns_message_checksig#1"); #endif /* ifdef SKAN_MSG_DEBUG */ - if (view != NULL) { - return (dns_view_checksig(view, &msgb, msg)); - } else { - return (dns_tsig_verify(&msgb, msg, NULL, NULL)); - } + if (view != NULL) { + return (dns_view_checksig(view, &msgb, msg)); } else { - dns_rdata_t rdata = DNS_RDATA_INIT; - dns_rdata_sig_t sig; - dns_rdataset_t keyset; - isc_result_t result; - - result = dns_rdataset_first(msg->sig0); - INSIST(result == ISC_R_SUCCESS); - dns_rdataset_current(msg->sig0, &rdata); - - /* - * This can occur when the message is a dynamic update, since - * the rdata length checking is relaxed. This should not - * happen in a well-formed message, since the SIG(0) is only - * looked for in the additional section, and the dynamic update - * meta-records are in the prerequisite and update sections. - */ - if (rdata.length == 0) { - return (ISC_R_UNEXPECTEDEND); - } - - result = dns_rdata_tostruct(&rdata, &sig, NULL); - if (result != ISC_R_SUCCESS) { - return (result); - } - - dns_rdataset_init(&keyset); - if (view == NULL) { - result = DNS_R_KEYUNAUTHORIZED; - goto freesig; - } - result = dns_view_simplefind(view, &sig.signer, - dns_rdatatype_key /* SIG(0) */, 0, - 0, false, &keyset, NULL); - - if (result != ISC_R_SUCCESS) { - /* XXXBEW Should possibly create a fetch here */ - result = DNS_R_KEYUNAUTHORIZED; - goto freesig; - } else if (keyset.trust < dns_trust_secure) { - /* XXXBEW Should call a validator here */ - result = DNS_R_KEYUNAUTHORIZED; - goto freesig; - } - result = dns_rdataset_first(&keyset); - INSIST(result == ISC_R_SUCCESS); - for (; result == ISC_R_SUCCESS; - result = dns_rdataset_next(&keyset)) - { - dst_key_t *key = NULL; - - dns_rdata_reset(&rdata); - dns_rdataset_current(&keyset, &rdata); - isc_buffer_init(&b, rdata.data, rdata.length); - isc_buffer_add(&b, rdata.length); - - result = dst_key_fromdns(&sig.signer, rdata.rdclass, &b, - view->mctx, &key); - if (result != ISC_R_SUCCESS) { - continue; - } - if (dst_key_alg(key) != sig.algorithm || - dst_key_id(key) != sig.keyid || - !(dst_key_proto(key) == DNS_KEYPROTO_DNSSEC || - dst_key_proto(key) == DNS_KEYPROTO_ANY)) - { - dst_key_free(&key); - continue; - } - result = dns_dnssec_verifymessage(&msgb, msg, key); - dst_key_free(&key); - if (result == ISC_R_SUCCESS) { - break; - } - } - if (result == ISC_R_NOMORE) { - result = DNS_R_KEYUNAUTHORIZED; - } - - freesig: - if (dns_rdataset_isassociated(&keyset)) { - dns_rdataset_disassociate(&keyset); - } - dns_rdata_freestruct(&sig); - return (result); + return (dns_tsig_verify(&msgb, msg, NULL, NULL)); } } diff --git a/lib/ns/client.c b/lib/ns/client.c index 590859f020..002185d642 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -2167,6 +2167,13 @@ ns__client_request(isc_nmhandle_t *handle, isc_result_t eresult, ns_client_log(client, DNS_LOGCATEGORY_SECURITY, NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(3), "request is signed by a nonauthoritative key"); + } else if (result == DNS_R_NOTVERIFIEDYET && + client->message->sig0 != NULL) + { + ns_client_log(client, DNS_LOGCATEGORY_SECURITY, + NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(3), + "request has a SIG(0) signature but its support " + "was removed (CVE-2024-1975)"); } else { char tsigrcode[64]; isc_buffer_t b; From 33007e302d2e5b4550fa8c9d5cd1bffaaffb6819 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0pa=C4=8Dek?= Date: Thu, 16 May 2024 12:15:23 +0200 Subject: [PATCH 2/7] Document SIG(0) verification removal --- doc/arm/general.rst | 6 ++---- doc/arm/intro-security.inc.rst | 2 +- doc/arm/reference.rst | 4 ++-- doc/arm/security.inc.rst | 4 ++-- doc/arm/sig0.inc.rst | 16 ++-------------- 5 files changed, 9 insertions(+), 23 deletions(-) diff --git a/doc/arm/general.rst b/doc/arm/general.rst index 09bcaa9725..cabf5c8591 100644 --- a/doc/arm/general.rst +++ b/doc/arm/general.rst @@ -391,10 +391,8 @@ Notes .. [#rfc1035_2] CLASS ANY queries are not supported. This is considered a feature. -.. [#rfc2931] When receiving a query signed with a SIG(0), the server is - only able to verify the signature if it has the key in its local - authoritative data; it cannot do recursion or validation to - retrieve unknown keys. +.. [#rfc2931] Support for SIG(0) message verification was removed + as part of the mitigation of CVE-2024-1975. .. [#rfc2874] Compliance is with loading and serving of A6 records only. A6 records were moved to the experimental category by :rfc:`3363`. diff --git a/doc/arm/intro-security.inc.rst b/doc/arm/intro-security.inc.rst index 87db9701da..996e910a6d 100644 --- a/doc/arm/intro-security.inc.rst +++ b/doc/arm/intro-security.inc.rst @@ -47,7 +47,7 @@ or ports come preconfigured with local (loopback address) security preconfigured If ``rndc`` is being invoked from a remote host, further configuration is required. The ``nsupdate`` tool uses **Dynamic DNS (DDNS)** features and allows users to dynamically change the contents of the zone file(s). ``nsupdate`` access and security may be controlled -using ``named.conf`` :ref:`statements or using TSIG or SIG(0) cryptographic methods `. +using ``named.conf`` :ref:`statements or via the TSIG cryptographic method `. Clearly, if the remote hosts used for either ``rndc`` or DDNS lie within a network entirely under the user's control, the security threat may be regarded as non-existent. Any implementation requirements, therefore, depend on the site's security policy. diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index 2ddc368514..d6dd932948 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -7450,7 +7450,7 @@ the zone's filename, unless :any:`inline-signing` is enabled. updates are allowed. It specifies a set of rules, in which each rule either grants or denies permission for one or more names in the zone to be updated by one or more identities. Identity is determined by the key - that signed the update request, using either TSIG or SIG(0). In most + that signed the update request, using TSIG. In most cases, :any:`update-policy` rules only apply to key-based identities. There is no way to specify update permissions based on the client source address. @@ -7507,7 +7507,7 @@ the zone's filename, unless :any:`inline-signing` is enabled. field. Details for each rule type are described below. The ``identity`` field must be set to a fully qualified domain name. In - most cases, this represents the name of the TSIG or SIG(0) key that + most cases, this represents the name of the TSIG key that must be used to sign the update request. If the specified name is a wildcard, it is subject to DNS wildcard expansion, and the rule may apply to multiple identities. When a TKEY exchange has been used to diff --git a/doc/arm/security.inc.rst b/doc/arm/security.inc.rst index 878fa379fe..8fc65d3820 100644 --- a/doc/arm/security.inc.rst +++ b/doc/arm/security.inc.rst @@ -85,7 +85,7 @@ Limiting access to the server by outside parties can help prevent spoofing and denial of service (DoS) attacks against the server. ACLs match clients on the basis of up to three characteristics: 1) The -client's IP address; 2) the TSIG or SIG(0) key that was used to sign the +client's IP address; 2) the TSIG key that was used to sign the request, if any; and 3) an address prefix encoded in an EDNS Client-Subnet option, if any. @@ -126,7 +126,7 @@ and no queries at all from the networks specified in ``bogusnets``. In addition to network addresses and prefixes, which are matched against the source address of the DNS request, ACLs may include ``key`` -elements, which specify the name of a TSIG or SIG(0) key. +elements, which specify the name of a TSIG key. When BIND 9 is built with GeoIP support, ACLs can also be used for geographic access restrictions. This is done by specifying an ACL diff --git a/doc/arm/sig0.inc.rst b/doc/arm/sig0.inc.rst index 048dbeae7c..6e6fc3219e 100644 --- a/doc/arm/sig0.inc.rst +++ b/doc/arm/sig0.inc.rst @@ -12,17 +12,5 @@ SIG(0) ------ -BIND partially supports DNSSEC SIG(0) transaction signatures as -specified in :rfc:`2535` and :rfc:`2931`. SIG(0) uses public/private keys to -authenticate messages. Access control is performed in the same manner as with -TSIG keys; privileges can be granted or denied in ACL directives based -on the key name. - -When a SIG(0) signed message is received, it is only verified if -the key is known and trusted by the server. The server does not attempt -to recursively fetch or validate the key. - -SIG(0) signing of multiple-message TCP streams is not supported. - -The only tool shipped with BIND 9 that generates SIG(0) signed messages -is :iscman:`nsupdate`. +Support for DNSSEC SIG(0) transaction signatures has been removed. +This is a countermeasure for CVE-2024-1975. From 4b1f026ad903ec997471e219c98d9a804dfa0822 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 21 May 2024 08:45:48 +0000 Subject: [PATCH 3/7] Enable stdout autoflush in authsock.pl With enabled buffering the output gets lost when the process receives a TERM signal. Disable the buffering. --- bin/tests/system/tsiggss/authsock.pl | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/bin/tests/system/tsiggss/authsock.pl b/bin/tests/system/tsiggss/authsock.pl index 4c76bf8d56..103d3b0be2 100644 --- a/bin/tests/system/tsiggss/authsock.pl +++ b/bin/tests/system/tsiggss/authsock.pl @@ -33,6 +33,10 @@ if (!defined($path)) { exit(1); } +# Enable output autoflush so that it's not lost when the parent sends TERM. +select STDOUT; +$| = 1; + unlink($path); my $server = IO::Socket::UNIX->new(Local => $path, Type => SOCK_STREAM, Listen => 8) or die "unable to create socket $path"; From 02dffb63a84662b19da4e0efda26e061676f85a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0pa=C4=8Dek?= Date: Fri, 17 May 2024 12:23:05 +0200 Subject: [PATCH 4/7] Adapt the tsiggss test to the SIG(0) removal Test that SIG(0) signer is NOT sent to the external socket for authorization. It MUST NOT be considered a valid signature by any chance. Also check that the signer's name does not appear in authsock.pl output. --- bin/tests/system/tsiggss/authsock.pl | 1 + bin/tests/system/tsiggss/tests.sh | 12 +++++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/bin/tests/system/tsiggss/authsock.pl b/bin/tests/system/tsiggss/authsock.pl index 103d3b0be2..972252aa99 100644 --- a/bin/tests/system/tsiggss/authsock.pl +++ b/bin/tests/system/tsiggss/authsock.pl @@ -54,6 +54,7 @@ if ($timeout != 0) { } while (my $client = $server->accept()) { + printf("accept()\n"); $client->recv(my $buf, 8, 0); my ($version, $req_len) = unpack('N N', $buf); diff --git a/bin/tests/system/tsiggss/tests.sh b/bin/tests/system/tsiggss/tests.sh index c37f32ed39..004ad83f94 100644 --- a/bin/tests/system/tsiggss/tests.sh +++ b/bin/tests/system/tsiggss/tests.sh @@ -117,7 +117,7 @@ status=$((status + ret)) echo_i "testing external update policy (CNAME) with auth sock ($n)" ret=0 -$PERL ./authsock.pl --type=CNAME --path=ns1/auth.sock --pidfile=authsock.pid --timeout=120 >/dev/null 2>&1 & +$PERL ./authsock.pl --type=CNAME --path=ns1/auth.sock --pidfile=authsock.pid --timeout=120 >authsock.log 2>&1 & sleep 1 test_update $n testcname.example.nil. CNAME "86400 CNAME testdenied.example.nil" "testdenied" || ret=1 n=$((n + 1)) @@ -131,17 +131,19 @@ n=$((n + 1)) if [ "$ret" -ne 0 ]; then echo_i "failed"; fi status=$((status + ret)) -echo_i "testing external policy with SIG(0) key ($n)" +echo_i "testing external policy with unsupported SIG(0) key ($n)" ret=0 -$NSUPDATE -k ns1/Kkey.example.nil.*.private </dev/null 2>&1 || ret=1 +$NSUPDATE -d -k ns1/Kkey.example.nil.*.private <nsupdate.out${n} 2>&1 || true +debug server 10.53.0.1 ${PORT} zone example.nil update add fred.example.nil 120 cname foo.bar. send END output=$($DIG $DIGOPTS +short cname fred.example.nil.) -[ -n "$output" ] || ret=1 -[ $ret -eq 0 ] || echo_i "failed" +# update must have failed - SIG(0) signer is not supported +[ -n "$output" ] && ret=1 +grep -F "signer=key.example.nil" authsock.log >/dev/null && ret=1 n=$((n + 1)) if [ "$ret" -ne 0 ]; then echo_i "failed"; fi status=$((status + ret)) From 227f9aa0646cdf521e0db0d472f8bcc1e2bd6154 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 21 May 2024 09:29:35 +0000 Subject: [PATCH 5/7] Adapt the upforwd test to the SIG(0) removal Change the check so that update with SIG(0) is expected to fail. --- bin/tests/system/upforwd/tests.sh | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/bin/tests/system/upforwd/tests.sh b/bin/tests/system/upforwd/tests.sh index 25919f81e3..20758a6926 100644 --- a/bin/tests/system/upforwd/tests.sh +++ b/bin/tests/system/upforwd/tests.sh @@ -229,10 +229,12 @@ fi n=$((n + 1)) if test -f keyname; then - echo_i "checking update forwarding to with sig0 ($n)" + echo_i "checking update forwarding to with sig0 (expected to fail) ($n)" ret=0 keyname=$(cat keyname) - $NSUPDATE -k $keyname.private -- - <nsupdate.out.$n 2>&1 && ret=1 $DIG -p ${PORT} unsigned.example2 A @10.53.0.1 >dig.out.ns1.test$n || ret=1 - grep "status: NOERROR" dig.out.ns1.test$n >/dev/null || ret=1 + grep "status: NOERROR" dig.out.ns1.test$n >/dev/null && ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) n=$((n + 1)) From 8acd71b9cc3d46618319f8c2195d857a8f79744b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0pa=C4=8Dek?= Date: Thu, 6 Jun 2024 17:43:20 +0200 Subject: [PATCH 6/7] Add CHANGES note for [GL #4480] --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index a77b8cea2a..655694c8d1 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +6402. [security] Remove SIG(0) support from named as a countermeasure + for CVE-2024-1975. [GL #4480] + 6401. [security] An excessively large number of rrtypes per owner can slow down database query processing, so a limit has been placed on the number of rrtypes that can be stored per From 5aef5f1fb9215f6c6976b4304d9703501495012b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0pa=C4=8Dek?= Date: Thu, 6 Jun 2024 17:59:50 +0200 Subject: [PATCH 7/7] Add release note for GL #4480 --- doc/notes/notes-current.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 9440dd6d8c..fbd682b01b 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -34,6 +34,11 @@ Security Fixes ISC would like to thank Toshifumi Sakaguchi who independently discovered and responsibly reported the issue to ISC. :gl:`#4548` +- Validating DNS messages signed using the SIG(0) protocol (:rfc:`2931`) could + cause excessive CPU load, leading to a denial-of-service condition. + Support for SIG(0) message validation was removed from this version of + :iscman:`named`. :cve:`2024-1975` :gl:`#4480` + - Named could trigger an assertion failure when looking up the NS records of parent zones as part of looking up DS records. This has been fixed. :gl:`#4661`