From bd41100295ac4ddd194885acc5889152e1ccbc78 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. (cherry picked from commit a696be6a2db0a6dedb87ba37959112ad394989b7) --- 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 3c31b2cc76..650ee7aa49 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -972,6 +972,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 11b895bab1..4fd909d228 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -214,7 +214,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); } } @@ -416,21 +415,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; } } @@ -634,6 +619,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 */ @@ -834,7 +825,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; @@ -958,6 +949,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: @@ -1025,8 +1022,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); @@ -1037,8 +1035,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 6edbd0692f53e645d8cef7c3e5c7f4f26f0e6e99 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] (cherry picked from commit 65c0de8e6fa9450520a61fbfead2b288a3587d44) --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index ccec1b2cf5..e9e3ee9ffd 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +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 has now been addressed. [GL #3232] From ea223b9b444c687a0f554f04e83fd79be76c00d4 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] (cherry picked from commit 6791500e96a258d0abef958d157ccf27626c7b10) --- 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 fd7a2d5acd..c934af9de6 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 46819373e3ad1e64d98f13c047952bd4295d3457 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Thu, 5 May 2022 15:53:35 +0200 Subject: [PATCH 4/8] Prepare release notes for BIND 9.18.3 --- doc/arm/notes.rst | 2 +- doc/notes/{notes-current.rst => notes-9.18.3.rst} | 12 ------------ 2 files changed, 1 insertion(+), 13 deletions(-) rename doc/notes/{notes-current.rst => notes-9.18.3.rst} (96%) diff --git a/doc/arm/notes.rst b/doc/arm/notes.rst index c1260b48e5..94bd141573 100644 --- a/doc/arm/notes.rst +++ b/doc/arm/notes.rst @@ -33,7 +33,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.18.3.rst .. include:: ../notes/notes-9.18.2.rst .. include:: ../notes/notes-9.18.1.rst .. include:: ../notes/notes-9.18.0.rst diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-9.18.3.rst similarity index 96% rename from doc/notes/notes-current.rst rename to doc/notes/notes-9.18.3.rst index c934af9de6..0365500501 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-9.18.3.rst @@ -15,8 +15,6 @@ Notes for BIND 9.18.3 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. @@ -61,16 +59,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 b2e3ecf8591c9719e61397bd00b5905a04875883 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Fri, 6 May 2022 17:01:46 +0200 Subject: [PATCH 5/8] Tweak and reword release notes --- doc/notes/notes-9.18.3.rst | 56 ++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/doc/notes/notes-9.18.3.rst b/doc/notes/notes-9.18.3.rst index 0365500501..05d2c71cb2 100644 --- a/doc/notes/notes-9.18.3.rst +++ b/doc/notes/notes-9.18.3.rst @@ -26,41 +26,45 @@ Security Fixes Known Issues ~~~~~~~~~~~~ -- According to RFC 8310, Section 8.1, the Subject field MUST NOT be - inspected when verifying a remote certificate while establishing a - DNS-over-TLS connection. Only SubjectAltName must be checked +- According to :rfc:`8310`, Section 8.1, the ``Subject`` field MUST NOT + be inspected when verifying a remote certificate while establishing a + DNS-over-TLS connection. Only ``subjectAltName`` must be checked instead. Unfortunately, some quite old versions of cryptographic - libraries might lack the functionality to ignore the Subject - field. It should have minimal production use consequences, as most - of the production-ready certificates issued by certificate - authorities will have SubjectAltNames set. In such a case, the - Subject field is ignored. Only old platforms are affected by this, - e.g., those supplied with OpenSSL versions older than 1.1.1. + libraries might lack the ability to ignore the ``Subject`` field. This + should have minimal production-use consequences, as most of the + production-ready certificates issued by certificate authorities will + have ``subjectAltName`` set. In such cases, the ``Subject`` field is + ignored. Only old platforms are affected by this, e.g. those supplied + with OpenSSL versions older than 1.1.1. :gl:`#3163` New Features ~~~~~~~~~~~~ -- Add DNS Extended Errors (:rfc:`8914`) when stale answers are returned from - cache. :gl:`#2267` +- Support DNS Extended Errors (:rfc:`8914`) ``Stale Answer`` and + ``Stale NXDOMAIN Answer`` when stale answers are returned from cache. + :gl:`#2267` -- Add support for remote TLS certificates verification, both to BIND - and ``dig``, making it possible to implement Strict and Mutual TLS - authentication, as described in RFC 9103, Section 9.3. :gl:`#3163` +- Add support for remote TLS certificate verification, both to + :iscman:`named` and :iscman:`dig`, making it possible to implement + Strict and Mutual TLS authentication, as described in :rfc:`9103`, + Section 9.3. :gl:`#3163` -- 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 87f017bec3b49034975c179f0e86dcd9ebc9250c Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Mon, 9 May 2022 11:29:18 +0200 Subject: [PATCH 6/8] Reorder release notes --- doc/notes/notes-9.18.3.rst | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/doc/notes/notes-9.18.3.rst b/doc/notes/notes-9.18.3.rst index 05d2c71cb2..e24bbcfc7e 100644 --- a/doc/notes/notes-9.18.3.rst +++ b/doc/notes/notes-9.18.3.rst @@ -40,15 +40,6 @@ Known Issues New Features ~~~~~~~~~~~~ -- Support DNS Extended Errors (:rfc:`8914`) ``Stale Answer`` and - ``Stale NXDOMAIN Answer`` when stale answers are returned from cache. - :gl:`#2267` - -- Add support for remote TLS certificate verification, both to - :iscman:`named` and :iscman:`dig`, making it possible to implement - Strict and Mutual TLS authentication, as described in :rfc:`9103`, - Section 9.3. :gl:`#3163` - - 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 @@ -62,6 +53,15 @@ New Features 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` + +- Add support for remote TLS certificate verification, both to + :iscman:`named` and :iscman:`dig`, making it possible to implement + Strict and Mutual TLS authentication, as described in :rfc:`9103`, + Section 9.3. :gl:`#3163` + Bug Fixes ~~~~~~~~~ From eea876716f4e054b3676a22589969742d851cff5 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Mon, 9 May 2022 18:06:43 +0200 Subject: [PATCH 7/8] Add a CHANGES marker --- CHANGES | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES b/CHANGES index e9e3ee9ffd..e6b1ba0d0f 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ + --- 9.18.3 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 16aefa34b0e410c0f8aa98c26c67a7ebc06304d8 Mon Sep 17 00:00:00 2001 From: Michal Nowak Date: Mon, 9 May 2022 18:07:17 +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 d4c0fbe658..ddc754d70e 100644 --- a/configure.ac +++ b/configure.ac @@ -17,7 +17,7 @@ m4_define([bind_VERSION_MAJOR], 9)dnl m4_define([bind_VERSION_MINOR], 18)dnl m4_define([bind_VERSION_PATCH], 3)dnl -m4_define([bind_VERSION_EXTRA], -dev)dnl +m4_define([bind_VERSION_EXTRA], )dnl m4_define([bind_DESCRIPTION], [(Stable 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