From a696be6a2db0a6dedb87ba37959112ad394989b7 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Tue, 22 Mar 2022 20:24:46 +0200 Subject: [PATCH 1/8] Fix a crash by avoiding destroying TLS stream socket too early This commit fixes a crash in generic TLS stream code, which could be reproduced during some runs of the 'sslyze' tool. The intention of this commit is twofold. Firstly, it ensures that the TLS socket object cannot be destroyed too early. Now it is being deleted alongside the underlying TCP socket object. Secondly, it ensures that the TLS socket object cannot be destroyed as a result of calling 'tls_do_bio()' (the primary function which performs encryption/decryption during the IO) as the code did not expect that. This code path is fixed now. --- lib/isc/netmgr/netmgr-int.h | 1 + lib/isc/netmgr/tlsstream.c | 43 ++++++++++++++++++++----------------- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index bf6c3039ff..55626adf12 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -966,6 +966,7 @@ struct isc_nmsocket { worker */ size_t n_listener_tls_ctx; isc_nmsocket_t *tlslistener; + isc_nmsocket_t *tlssocket; atomic_bool result_updated; enum { TLS_INIT, diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index 2734bae7ae..e725159471 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -213,7 +213,6 @@ tls_failed_read_cb(isc_nmsocket_t *sock, const isc_result_t result) { if (destroy) { isc__nmsocket_prep_destroy(sock); - isc__nmsocket_detach(&sock); } } @@ -415,21 +414,7 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, send_data->cb.send(send_data->handle, result, send_data->cbarg); send_data = NULL; - /* This situation might occur only when SSL - * shutdown was already sent (see - * tls_send_outgoing()), and we are in the - * process of shutting down the connection (in - * this case tls_senddone() will be called), but - * some code tries to send data over the - * connection and called isc_tls_send(). The - * socket will be detached there, in - * tls_senddone().*/ - if (sent_shutdown || received_shutdown) { - return; - } else { - isc__nmsocket_detach(&sock); - return; - } + return; } } @@ -632,6 +617,12 @@ tlslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { tlssock->read_timeout = atomic_load(&handle->sock->mgr->init); tlssock->tid = tid; + /* + * Hold a reference to tlssock in the TCP socket: it will + * detached in isc__nm_tls_cleanup_data(). + */ + handle->sock->tlsstream.tlssocket = tlssock; + result = initialize_tls(tlssock, true); RUNTIME_CHECK(result == ISC_R_SUCCESS); /* TODO: catch failure code, detach tlssock, and log the error */ @@ -829,7 +820,7 @@ tls_close_direct(isc_nmsocket_t *sock) { isc__nmsocket_detach(&sock->listener); } - /* further cleanup performed in isc__nm_tls_cleanup_data() */ + /* Further cleanup performed in isc__nm_tls_cleanup_data() */ atomic_store(&sock->closed, true); atomic_store(&sock->active, false); sock->tlsstream.state = TLS_CLOSED; @@ -952,6 +943,12 @@ tcp_connected(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { isc_nmhandle_attach(handle, &tlssock->outerhandle); atomic_store(&tlssock->active, true); + /* + * Hold a reference to tlssock in the TCP socket: it will + * detached in isc__nm_tls_cleanup_data(). + */ + handle->sock->tlsstream.tlssocket = tlssock; + tls_do_bio(tlssock, NULL, NULL, false); return; error: @@ -1019,8 +1016,9 @@ void isc__nm_tls_cleanup_data(isc_nmsocket_t *sock) { if (sock->type == isc_nm_tcplistener && sock->tlsstream.tlslistener != NULL) { - REQUIRE(VALID_NMSOCK(sock->tlsstream.tlslistener)); isc__nmsocket_detach(&sock->tlsstream.tlslistener); + } else if (sock->type == isc_nm_tlslistener) { + tls_cleanup_listener_tlsctx(sock); } else if (sock->type == isc_nm_tlssocket) { if (sock->tlsstream.ctx != NULL) { isc_tlsctx_free(&sock->tlsstream.ctx); @@ -1031,8 +1029,13 @@ isc__nm_tls_cleanup_data(isc_nmsocket_t *sock) { sock->tlsstream.bio_out = NULL; sock->tlsstream.bio_in = NULL; } - } else if (sock->type == isc_nm_tlslistener) { - tls_cleanup_listener_tlsctx(sock); + } else if (sock->type == isc_nm_tcpsocket && + sock->tlsstream.tlssocket != NULL) { + /* + * The TLS socket can't be destroyed until its underlying TCP + * socket is, to avoid possible use-after-free errors. + */ + isc__nmsocket_detach(&sock->tlsstream.tlssocket); } } From 65c0de8e6fa9450520a61fbfead2b288a3587d44 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 23 Mar 2022 17:06:34 +0200 Subject: [PATCH 2/8] Add CHANGES entry for [GL #3216] --- CHANGES | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index 4f05877383..2367e6c143 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,6 @@ -5886. [placeholder] +5886. [security] Fix a crash in DNS-over-HTTPS (DoH) code caused by + premature TLS stream socket object deletion. + (CVE-2022-1183) [GL #3216] 5885. [bug] RPZ NSIP and NSDNAME rule processing didn't handle stub and static-stub zones at or above the query name. This From 6791500e96a258d0abef958d157ccf27626c7b10 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 24 Mar 2022 13:18:39 +0200 Subject: [PATCH 3/8] Add release note entry for [GL #3216] --- doc/notes/notes-current.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 660e0c5da0..1ba00c8088 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -17,6 +17,14 @@ Security Fixes - None. +- Previously, TLS socket objects could be destroyed prematurely, which + triggered assertion failures in :iscman:`named` instances serving + DNS-over-HTTPS (DoH) clients. This has been fixed. + + ISC would like to thank Thomas Amgarten from arcade solutions ag for + bringing this vulnerability to our attention. (CVE-2022-1183) + :gl:`#3216` + Known Issues ~~~~~~~~~~~~ From 7f1fbcb4e7f7c3a9f7fbeaf715964fa8044fbc93 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Thu, 5 May 2022 10:57:16 +0200 Subject: [PATCH 4/8] Prepare release notes for BIND 9.19.1 --- doc/arm/notes.rst | 2 +- .../{notes-current.rst => notes-9.19.1.rst} | 17 ----------------- 2 files changed, 1 insertion(+), 18 deletions(-) rename doc/notes/{notes-current.rst => notes-9.19.1.rst} (95%) diff --git a/doc/arm/notes.rst b/doc/arm/notes.rst index 5673978a16..59b037dd45 100644 --- a/doc/arm/notes.rst +++ b/doc/arm/notes.rst @@ -36,7 +36,7 @@ The latest versions of BIND 9 software can always be found at https://www.isc.org/download/. There you will find additional information about each release, and source code. -.. include:: ../notes/notes-current.rst +.. include:: ../notes/notes-9.19.1.rst .. include:: ../notes/notes-9.19.0.rst .. _relnotes_license: diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-9.19.1.rst similarity index 95% rename from doc/notes/notes-current.rst rename to doc/notes/notes-9.19.1.rst index 1ba00c8088..7733f23a8a 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-9.19.1.rst @@ -15,8 +15,6 @@ Notes for BIND 9.19.1 Security Fixes ~~~~~~~~~~~~~~ -- None. - - Previously, TLS socket objects could be destroyed prematurely, which triggered assertion failures in :iscman:`named` instances serving DNS-over-HTTPS (DoH) clients. This has been fixed. @@ -25,11 +23,6 @@ Security Fixes bringing this vulnerability to our attention. (CVE-2022-1183) :gl:`#3216` -Known Issues -~~~~~~~~~~~~ - -- None. - New Features ~~~~~~~~~~~~ @@ -61,16 +54,6 @@ New Features diagnosing the problem. :gl:`#3221` :gl:`#3222` :gl:`#3223` :gl:`#3224` :gl:`#3225` -Removed Features -~~~~~~~~~~~~~~~~ - -- None. - -Feature Changes -~~~~~~~~~~~~~~~ - -- None. - Bug Fixes ~~~~~~~~~ From 0cb80f7343ac10292aac2eb50e2ef11cbbbab8ff Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Thu, 5 May 2022 11:43:04 +0200 Subject: [PATCH 5/8] Tweak and reword release notes --- doc/notes/notes-9.19.1.rst | 48 +++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/doc/notes/notes-9.19.1.rst b/doc/notes/notes-9.19.1.rst index 7733f23a8a..e643c2e349 100644 --- a/doc/notes/notes-9.19.1.rst +++ b/doc/notes/notes-9.19.1.rst @@ -26,36 +26,40 @@ Security Fixes New Features ~~~~~~~~~~~~ -- Add DNS Extended Errors (:rfc:`8914`) when stale answers are returned from - cache. :gl:`#2267` - -- The Object Identifier (OID) embedded at the start of a PRIVATEOID public - key in a KEY, DNSKEY, CDNSKEY, or RKEY resource record is now checked to - ensure that it is valid when reading from zone files or receiving data - on the wire, and the OID is now printed when the ``dig +rrcomments`` - option is used. Similarly, the name embedded at the start of a PRIVATEDNS - public key is also checked for validity. :gl:`#3234` +- Support DNS Extended Errors (:rfc:`8914`) ``Stale Answer`` and + ``Stale NXDOMAIN Answer`` when stale answers are returned from cache. + :gl:`#2267` - The Object Identifier (OID) embedded at the start of a PRIVATEOID - signature in a SIG, or RRSIG resource record is now checked to + public key in a KEY, DNSKEY, CDNSKEY, or RKEY resource records is now + checked to ensure that it is valid when reading from zone files or + receiving data on the wire. The Object Identifier is now printed when + the ``dig +rrcomments`` option is used. Similarly, the name embedded + at the start of a PRIVATEDNS public key is also checked for validity. + :gl:`#3234` + +- The Object Identifier (OID) embedded at the start of a PRIVATEOID + signature in a SIG, or RRSIG resource records is now checked to ensure that it is valid when reading from zone files or receiving data on the wire. Similarly, the name embedded at the start of a PRIVATEDNS public key is also checked for validity. :gl:`#3296` -- Catalog Zones schema version 2, as described in the "DNS Catalog Zones" IETF - draft version 5 document, is now supported by :iscman:`named`. All of the - previously supported BIND-specific catalog zone custom properties - (``primaries``, ``allow-query``, and ``allow-transfer``), as well as the new - Change of Ownership (``coo``) property, are now implemented. Schema version 1 - is still supported, with some additional validation rules applied from - schema version 2: for example, the ``version`` property is mandatory, and a - member zone PTR RRset must not contain more than one record. In the event of a +- Catalog Zones schema version 2, as described in the + "DNS Catalog Zones" IETF draft version 5 document, is now supported by + :iscman:`named`. All of the previously supported BIND-specific catalog + zone custom properties (``primaries``, ``allow-query``, and + ``allow-transfer``), as well as the new Change of Ownership (``coo``) + property, are now implemented. Schema version 1 is still supported, + with some additional validation rules applied from schema version 2: + for example, the ``version`` property is mandatory, and a member zone + PTR RRset must not contain more than one record. In the event of a validation error, a corresponding error message is logged to help with - diagnosing the problem. :gl:`#3221` :gl:`#3222` :gl:`#3223` :gl:`#3224` - :gl:`#3225` + diagnosing the problem. :gl:`#3221` :gl:`#3222` :gl:`#3223` + :gl:`#3224` :gl:`#3225` Bug Fixes ~~~~~~~~~ -- CDS and CDNSKEY DELETE records are removed from the zone when configured with - 'auto-dnssec maintain;'. This has been fixed. :gl:`#2931`. +- Previously, CDS and CDNSKEY DELETE records were removed from the zone + when configured with the ``auto-dnssec maintain;`` option. This has + been fixed. :gl:`#2931` From d25977e50003ae3e4ad2f2dac443a764c92401f9 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Mon, 9 May 2022 09:44:59 +0200 Subject: [PATCH 6/8] Reorder release notes --- doc/notes/notes-9.19.1.rst | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/doc/notes/notes-9.19.1.rst b/doc/notes/notes-9.19.1.rst index e643c2e349..30ab7cf0fb 100644 --- a/doc/notes/notes-9.19.1.rst +++ b/doc/notes/notes-9.19.1.rst @@ -26,6 +26,19 @@ Security Fixes New Features ~~~~~~~~~~~~ +- Catalog Zones schema version 2, as described in the + "DNS Catalog Zones" IETF draft version 5 document, is now supported by + :iscman:`named`. All of the previously supported BIND-specific catalog + zone custom properties (``primaries``, ``allow-query``, and + ``allow-transfer``), as well as the new Change of Ownership (``coo``) + property, are now implemented. Schema version 1 is still supported, + with some additional validation rules applied from schema version 2: + for example, the ``version`` property is mandatory, and a member zone + PTR RRset must not contain more than one record. In the event of a + validation error, a corresponding error message is logged to help with + diagnosing the problem. :gl:`#3221` :gl:`#3222` :gl:`#3223` + :gl:`#3224` :gl:`#3225` + - Support DNS Extended Errors (:rfc:`8914`) ``Stale Answer`` and ``Stale NXDOMAIN Answer`` when stale answers are returned from cache. :gl:`#2267` @@ -44,19 +57,6 @@ New Features data on the wire. Similarly, the name embedded at the start of a PRIVATEDNS public key is also checked for validity. :gl:`#3296` -- Catalog Zones schema version 2, as described in the - "DNS Catalog Zones" IETF draft version 5 document, is now supported by - :iscman:`named`. All of the previously supported BIND-specific catalog - zone custom properties (``primaries``, ``allow-query``, and - ``allow-transfer``), as well as the new Change of Ownership (``coo``) - property, are now implemented. Schema version 1 is still supported, - with some additional validation rules applied from schema version 2: - for example, the ``version`` property is mandatory, and a member zone - PTR RRset must not contain more than one record. In the event of a - validation error, a corresponding error message is logged to help with - diagnosing the problem. :gl:`#3221` :gl:`#3222` :gl:`#3223` - :gl:`#3224` :gl:`#3225` - Bug Fixes ~~~~~~~~~ From b17f6ce5af0c4b1392774df76af25b28b8786107 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Mon, 9 May 2022 10:50:09 +0200 Subject: [PATCH 7/8] Add a CHANGES marker --- CHANGES | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES b/CHANGES index 2367e6c143..e804edbb95 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ + --- 9.19.1 released --- + 5886. [security] Fix a crash in DNS-over-HTTPS (DoH) code caused by premature TLS stream socket object deletion. (CVE-2022-1183) [GL #3216] From 8e122275805b2b5d908a8136b13ad06c5af7c315 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Mon, 9 May 2022 11:01:20 +0200 Subject: [PATCH 8/8] Update BIND 9 version for release --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index db46a9f83f..9fc390f348 100644 --- a/configure.ac +++ b/configure.ac @@ -17,7 +17,7 @@ m4_define([bind_VERSION_MAJOR], 9)dnl m4_define([bind_VERSION_MINOR], 19)dnl m4_define([bind_VERSION_PATCH], 1)dnl -m4_define([bind_VERSION_EXTRA], -dev)dnl +m4_define([bind_VERSION_EXTRA], )dnl m4_define([bind_DESCRIPTION], [(Development Release)])dnl m4_define([bind_SRCID], [m4_esyscmd_s([git rev-parse --short HEAD | cut -b1-7])])dnl m4_define([bind_PKG_VERSION], [[bind_VERSION_MAJOR.bind_VERSION_MINOR.bind_VERSION_PATCH]bind_VERSION_EXTRA])dnl