From b84fa122ced4a58cdbf25166d9fc7ebae7aa9223 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Mon, 7 Jun 2021 16:38:39 +0300 Subject: [PATCH] Make BIND refuse to serve XFRs over DoH We cannot use DoH for zone transfers. According to RFC8484 a DoH request contains exactly one DNS message (see Section 6: Definition of the "application/dns-message" Media Type, https://datatracker.ietf.org/doc/html/rfc8484#section-6). This makes DoH unsuitable for zone transfers as often (and usually!) these need more than one DNS message, especially for larger zones. As zone transfers over DoH are not (yet) standardised, nor discussed in RFC8484, the best thing we can do is to return "not implemented." Technically DoH can be used to transfer small zones which fit in one message, but that is not enough for the generic case. Also, this commit makes the server-side DoH code ensure that no multiple responses could be attempted to be sent over one HTTP/2 stream. In HTTP/2 one stream is mapped to one request/response transaction. Now the write callback will be called with failure error code in such a case. --- bin/tests/system/doth/tests.sh | 6 +++--- lib/isc/include/isc/netmgr.h | 3 +++ lib/isc/netmgr/http.c | 19 +++++++++++++++++++ lib/isc/netmgr/netmgr-int.h | 1 + lib/ns/query.c | 19 ++++++++++++++++++- 5 files changed, 44 insertions(+), 4 deletions(-) diff --git a/bin/tests/system/doth/tests.sh b/bin/tests/system/doth/tests.sh index de1fa2ffd3..3ef4016ac3 100644 --- a/bin/tests/system/doth/tests.sh +++ b/bin/tests/system/doth/tests.sh @@ -114,7 +114,7 @@ n=$((n + 1)) echo_i "checking DoH XFR (POST) (failure expected) ($n)" ret=0 dig_with_https_opts +comm @10.53.0.1 . AXFR > dig.out.test$n -grep "status: FORMERR" dig.out.test$n > /dev/null || ret=1 +grep "; Transfer failed." dig.out.test$n > /dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) @@ -154,7 +154,7 @@ n=$((n + 1)) echo_i "checking DoH XFR (GET) (failure expected) ($n)" ret=0 dig_with_https_opts +https-get +comm @10.53.0.1 . AXFR > dig.out.test$n -grep "status: FORMERR" dig.out.test$n > /dev/null || ret=1 +grep "; Transfer failed." dig.out.test$n > /dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) @@ -178,7 +178,7 @@ n=$((n + 1)) echo_i "checking unencrypted DoH XFR (failure expected) ($n)" ret=0 dig_with_http_opts +comm @10.53.0.1 . AXFR > dig.out.test$n -grep "status: FORMERR" dig.out.test$n > /dev/null || ret=1 +grep "; Transfer failed." dig.out.test$n > /dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) diff --git a/lib/isc/include/isc/netmgr.h b/lib/isc/include/isc/netmgr.h index b8f19bf15a..e2ce614df4 100644 --- a/lib/isc/include/isc/netmgr.h +++ b/lib/isc/include/isc/netmgr.h @@ -522,6 +522,9 @@ isc_result_t isc_nm_http_endpoint(isc_nmsocket_t *sock, const char *uri, isc_nm_recv_cb_t cb, void *cbarg, size_t extrahandlesize); +bool +isc_nm_is_http_handle(isc_nmhandle_t *handle); + void isc_nm_task_enqueue(isc_nm_t *mgr, isc_task_t *task, int threadid); /*%< diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index e38ff13092..d087507d47 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1910,6 +1910,15 @@ server_send_response(nghttp2_session *ngsession, int32_t stream_id, nghttp2_data_provider data_prd; int rv; + if (socket->h2.response_submitted) { + /* NGHTTP2 will gladly accept new response (write request) + * from us even though we cannot send more than one over the + * same HTTP/2 stream. Thus, we need to handle this case + * manually. We will return failure code so that it will be + * passed to the write callback. */ + return (ISC_R_FAILURE); + } + data_prd.source.ptr = socket; data_prd.read_callback = server_read_callback; @@ -1918,6 +1927,8 @@ server_send_response(nghttp2_session *ngsession, int32_t stream_id, if (rv != 0) { return (ISC_R_FAILURE); } + + socket->h2.response_submitted = true; return (ISC_R_SUCCESS); } @@ -2695,6 +2706,14 @@ failed_read_cb(isc_result_t result, isc_nm_http_session_t *session) { } } +bool +isc_nm_is_http_handle(isc_nmhandle_t *handle) { + REQUIRE(VALID_NMHANDLE(handle)); + REQUIRE(VALID_NMSOCK(handle->sock)); + + return (handle->sock->type == isc_nm_httpsocket); +} + static const bool base64url_validation_table[256] = { false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index 03cf316677..b61c925bc3 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -823,6 +823,7 @@ typedef struct isc_nmsocket_h2 { ISC_LIST(isc_nm_httpcbarg_t) handler_cbargs; isc_rwlock_t lock; + bool response_submitted; struct { char *uri; bool post; diff --git a/lib/ns/query.c b/lib/ns/query.c index 6639a75451..c5dd5cae3c 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -12043,7 +12043,24 @@ ns_query_start(ns_client_t *client, isc_nmhandle_t *handle) { break; /* Let the query logic handle it. */ case dns_rdatatype_ixfr: case dns_rdatatype_axfr: - ns_xfr_start(client, rdataset->type); + if (isc_nm_is_http_handle(handle)) { + /* We cannot use DoH for zone transfers. + * According to RFC8484 a DoH request contains + * exactly one DNS message (see Section 6: + * Definition of the "application/dns-message" + * Media Type, + * https://datatracker.ietf.org/doc/html/rfc8484#section-6). + * This makes DoH unsuitable for zone transfers + * as often (and usually!) these need more than + * one DNS message, especially for larger zones. + * As zone transfers over DoH are not (yet) + * standardised, nor discussed in the RFC8484, + * the best thing we can do is to return "not + * implemented". */ + query_error(client, DNS_R_NOTIMP, __LINE__); + } else { + ns_xfr_start(client, rdataset->type); + } return; case dns_rdatatype_maila: case dns_rdatatype_mailb: