From d323e1bda8a9ee37b958a2247ed9003a995b2764 Mon Sep 17 00:00:00 2001 From: Ralph Dolmans Date: Fri, 5 Jul 2019 16:52:03 +0200 Subject: [PATCH 1/6] - Fix for possible assertion failure when answering respip CNAME from cache. --- daemon/worker.c | 14 +++++--------- doc/Changelog | 4 ++++ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/daemon/worker.c b/daemon/worker.c index 661f6967d..bc2ca5aa0 100644 --- a/daemon/worker.c +++ b/daemon/worker.c @@ -721,8 +721,6 @@ answer_from_cache(struct worker* worker, struct query_info* qinfo, if(encode_rep != rep) secure = 0; /* if rewritten, it can't be considered "secure" */ if(!encode_rep || *alias_rrset) { - sldns_buffer_clear(repinfo->c->buffer); - sldns_buffer_flip(repinfo->c->buffer); if(!encode_rep) *need_drop = 1; else { @@ -762,17 +760,14 @@ answer_from_cache(struct worker* worker, struct query_info* qinfo, return 0; } -/** Reply to client and perform prefetch to keep cache up to date. - * If the buffer for the reply is empty, it indicates that only prefetch is - * necessary and the reply should be suppressed (because it's dropped or - * being deferred). */ +/** Reply to client and perform prefetch to keep cache up to date. */ static void reply_and_prefetch(struct worker* worker, struct query_info* qinfo, - uint16_t flags, struct comm_reply* repinfo, time_t leeway) + uint16_t flags, struct comm_reply* repinfo, time_t leeway, int noreply) { /* first send answer to client to keep its latency * as small as a cachereply */ - if(sldns_buffer_limit(repinfo->c->buffer) != 0) { + if(!noreply) { if(repinfo->c->tcp_req_info) { sldns_buffer_copy( repinfo->c->tcp_req_info->spool_buffer, @@ -1484,7 +1479,8 @@ worker_handle_request(struct comm_point* c, void* arg, int error, lock_rw_unlock(&e->lock); reply_and_prefetch(worker, lookup_qinfo, sldns_buffer_read_u16_at(c->buffer, 2), - repinfo, leeway); + repinfo, leeway, + (partial_rep || need_drop)); if(!partial_rep) { rc = 0; regional_free_all(worker->scratchpad); diff --git a/doc/Changelog b/doc/Changelog index c3fceb6ba..6343ab565 100644 --- a/doc/Changelog +++ b/doc/Changelog @@ -1,3 +1,7 @@ +5 July 2019: Ralph + - Fix for possible assertion failure when answering respip CNAME from + cache. + 25 June 2019: Wouter - For #45, check that 127.0.0.1 and ::1 are not used in unbound.conf when do-not-query-localhost is turned on, or at default on, From d5ebc63addab841d2096b2151f3f0e56efd5080a Mon Sep 17 00:00:00 2001 From: Ralph Dolmans Date: Tue, 9 Jul 2019 14:58:36 +0200 Subject: [PATCH 2/6] - Fix in respip addrtree selection. Absence of addr_tree_init_parents() call made it impossible to go up the tree when the matching netmask is too specific. --- doc/Changelog | 5 +++++ respip/respip.c | 1 + 2 files changed, 6 insertions(+) diff --git a/doc/Changelog b/doc/Changelog index 6343ab565..7b26c410a 100644 --- a/doc/Changelog +++ b/doc/Changelog @@ -1,3 +1,8 @@ +9 July 2019: Ralph + - Fix in respip addrtree selection. Absence of addr_tree_init_parents() + call made it impossible to go up the tree when the matching netmask is + too specific. + 5 July 2019: Ralph - Fix for possible assertion failure when answering respip CNAME from cache. diff --git a/respip/respip.c b/respip/respip.c index d61877b55..36a1c9726 100644 --- a/respip/respip.c +++ b/respip/respip.c @@ -361,6 +361,7 @@ respip_set_apply_cfg(struct respip_set* set, char* const* tagname, int num_tags, free(pd); pd = np; } + addr_tree_init_parents(&set->ip_tree); return 1; } From 368386c01130e02b0705401f8d420b26bb9dc814 Mon Sep 17 00:00:00 2001 From: "W.C.A. Wijngaards" Date: Fri, 12 Jul 2019 14:34:35 +0200 Subject: [PATCH 3/6] - Fix #48: Unbound returns additional records on NODATA response, if minimal-responses is enabled, also the additional for negative responses is removed. --- doc/Changelog | 5 ++ testdata/fwd_minimal.rpl | 125 +++++++++++++++++++++++++++++++++++++++ util/data/msgencode.c | 66 ++++++++++++++------- 3 files changed, 175 insertions(+), 21 deletions(-) create mode 100644 testdata/fwd_minimal.rpl diff --git a/doc/Changelog b/doc/Changelog index 7b26c410a..3b9618cac 100644 --- a/doc/Changelog +++ b/doc/Changelog @@ -1,3 +1,8 @@ +12 July 2019: Wouter + - Fix #48: Unbound returns additional records on NODATA response, + if minimal-responses is enabled, also the additional for negative + responses is removed. + 9 July 2019: Ralph - Fix in respip addrtree selection. Absence of addr_tree_init_parents() call made it impossible to go up the tree when the matching netmask is diff --git a/testdata/fwd_minimal.rpl b/testdata/fwd_minimal.rpl new file mode 100644 index 000000000..e85d7124b --- /dev/null +++ b/testdata/fwd_minimal.rpl @@ -0,0 +1,125 @@ +; This is a comment. +; config options go here. +server: + ; the snoop is to elicit a referral and check the additional + ; is fine for that, not removed by minimal-responses. + access-control: 127.0.0.1 allow_snoop + minimal-responses: yes +forward-zone: name: "." forward-addr: 216.0.0.1 +CONFIG_END + +SCENARIO_BEGIN Test minimal-responses +RANGE_BEGIN 0 100 + ENTRY_BEGIN + MATCH opcode qtype qname + ADJUST copy_id + REPLY QR RD RA NOERROR + SECTION QUESTION +www.example.com. IN A + SECTION ANSWER +www.example.com. IN A 10.20.30.40 + SECTION AUTHORITY +example.com. IN NS ns.example.com. + SECTION ADDITIONAL +ns.example.com. IN A 10.20.30.50 +txt.example.com. IN TXT "foo" + ENTRY_END + + ENTRY_BEGIN + MATCH opcode qtype qname + ADJUST copy_id + REPLY QR RD RA NOERROR + SECTION QUESTION +a.example.com. IN A + SECTION ANSWER + SECTION AUTHORITY +example.com. IN SOA host.example.com. ns.example.com. 1 2 3 4 5 +example.com. IN NS ns.example.com. + SECTION ADDITIONAL +ns.example.com. IN A 10.20.30.50 +txt.example.com. IN TXT "foo" + ENTRY_END + + ENTRY_BEGIN + MATCH opcode qtype qname + ADJUST copy_id + REPLY QR RD RA NXDOMAIN + SECTION QUESTION +b.example.com. IN A + SECTION ANSWER + SECTION AUTHORITY +example.com. IN SOA host.example.com. ns.example.com. 1 2 3 4 5 + SECTION ADDITIONAL +ns.example.com. IN A 10.20.30.50 +txt.example.com. IN TXT "foo" + ENTRY_END +RANGE_END + +STEP 1 QUERY +ENTRY_BEGIN +REPLY RD +SECTION QUESTION +www.example.com. IN A +ENTRY_END +STEP 4 CHECK_ANSWER +ENTRY_BEGIN +MATCH opcode qname qtype all +REPLY QR RD RA +SECTION QUESTION +www.example.com. IN A +SECTION ANSWER +www.example.com. IN A 10.20.30.40 +ENTRY_END + +STEP 11 QUERY +ENTRY_BEGIN +REPLY RD +SECTION QUESTION +a.example.com. IN A +ENTRY_END +STEP 14 CHECK_ANSWER +ENTRY_BEGIN +MATCH opcode qname qtype all +REPLY QR RD RA +SECTION QUESTION +a.example.com. IN A +SECTION AUTHORITY +example.com. IN SOA host.example.com. ns.example.com. 1 2 3 4 5 +ENTRY_END + +STEP 21 QUERY +ENTRY_BEGIN +REPLY RD +SECTION QUESTION +b.example.com. IN A +ENTRY_END +STEP 24 CHECK_ANSWER +ENTRY_BEGIN +MATCH opcode qname qtype all +REPLY QR RD RA NXDOMAIN +SECTION QUESTION +b.example.com. IN A +SECTION AUTHORITY +example.com. IN SOA host.example.com. ns.example.com. 1 2 3 4 5 +ENTRY_END + +; get a referral, the additional is not removed. +STEP 31 QUERY +ENTRY_BEGIN +REPLY +SECTION QUESTION +c.example.com. IN A +ENTRY_END +STEP 34 CHECK_ANSWER +ENTRY_BEGIN +MATCH opcode qname qtype all +REPLY QR RA NOERROR +SECTION QUESTION +c.example.com. IN A +SECTION AUTHORITY +example.com. IN NS ns.example.com. + SECTION ADDITIONAL +ns.example.com. IN A 10.20.30.50 +ENTRY_END + +SCENARIO_END diff --git a/util/data/msgencode.c b/util/data/msgencode.c index 4c0a5550b..0be99c04f 100644 --- a/util/data/msgencode.c +++ b/util/data/msgencode.c @@ -639,15 +639,37 @@ positive_answer(struct reply_info* rep, uint16_t qtype) { return 0; } -int -reply_info_encode(struct query_info* qinfo, struct reply_info* rep, - uint16_t id, uint16_t flags, sldns_buffer* buffer, time_t timenow, +static int +negative_answer(struct reply_info* rep) { + size_t i; + int ns_seen = 0; + if(FLAGS_GET_RCODE(rep->flags) == LDNS_RCODE_NXDOMAIN) + return 1; + if(FLAGS_GET_RCODE(rep->flags) == LDNS_RCODE_NOERROR && + rep->an_numrrsets != 0) + return 0; /* positive */ + if(FLAGS_GET_RCODE(rep->flags) != LDNS_RCODE_NOERROR && + FLAGS_GET_RCODE(rep->flags) != LDNS_RCODE_NXDOMAIN) + return 0; + for(i=rep->an_numrrsets; ian_numrrsets+rep->ns_numrrsets; i++){ + if(ntohs(rep->rrsets[i]->rk.type) == LDNS_RR_TYPE_SOA) + return 1; + if(ntohs(rep->rrsets[i]->rk.type) == LDNS_RR_TYPE_NS) + ns_seen = 1; + } + if(ns_seen) return 0; /* could be referral, NS, but no SOA */ + return 1; +} + +int +reply_info_encode(struct query_info* qinfo, struct reply_info* rep, + uint16_t id, uint16_t flags, sldns_buffer* buffer, time_t timenow, struct regional* region, uint16_t udpsize, int dnssec) { uint16_t ancount=0, nscount=0, arcount=0; struct compress_tree_node* tree = 0; int r; - size_t rr_offset; + size_t rr_offset; sldns_buffer_clear(buffer); if(udpsize < sldns_buffer_limit(buffer)) @@ -663,7 +685,7 @@ reply_info_encode(struct query_info* qinfo, struct reply_info* rep, /* insert query section */ if(rep->qdcount) { - if((r=insert_query(qinfo, &tree, buffer, region)) != + if((r=insert_query(qinfo, &tree, buffer, region)) != RETVAL_OK) { if(r == RETVAL_TRUNC) { /* create truncated message */ @@ -707,8 +729,8 @@ reply_info_encode(struct query_info* qinfo, struct reply_info* rep, } /* insert answer section */ - if((r=insert_section(rep, rep->an_numrrsets, &ancount, buffer, - 0, timenow, region, &tree, LDNS_SECTION_ANSWER, qinfo->qtype, + if((r=insert_section(rep, rep->an_numrrsets, &ancount, buffer, + 0, timenow, region, &tree, LDNS_SECTION_ANSWER, qinfo->qtype, dnssec, rr_offset)) != RETVAL_OK) { if(r == RETVAL_TRUNC) { /* create truncated message */ @@ -724,7 +746,7 @@ reply_info_encode(struct query_info* qinfo, struct reply_info* rep, /* if response is positive answer, auth/add sections are not required */ if( ! (MINIMAL_RESPONSES && positive_answer(rep, qinfo->qtype)) ) { /* insert auth section */ - if((r=insert_section(rep, rep->ns_numrrsets, &nscount, buffer, + if((r=insert_section(rep, rep->ns_numrrsets, &nscount, buffer, rep->an_numrrsets, timenow, region, &tree, LDNS_SECTION_AUTHORITY, qinfo->qtype, dnssec, rr_offset)) != RETVAL_OK) { @@ -739,20 +761,22 @@ reply_info_encode(struct query_info* qinfo, struct reply_info* rep, } sldns_buffer_write_u16_at(buffer, 8, nscount); - /* insert add section */ - if((r=insert_section(rep, rep->ar_numrrsets, &arcount, buffer, - rep->an_numrrsets + rep->ns_numrrsets, timenow, region, - &tree, LDNS_SECTION_ADDITIONAL, qinfo->qtype, - dnssec, rr_offset)) != RETVAL_OK) { - if(r == RETVAL_TRUNC) { - /* no need to set TC bit, this is the additional */ - sldns_buffer_write_u16_at(buffer, 10, arcount); - sldns_buffer_flip(buffer); - return 1; + if(! (MINIMAL_RESPONSES && negative_answer(rep))) { + /* insert add section */ + if((r=insert_section(rep, rep->ar_numrrsets, &arcount, buffer, + rep->an_numrrsets + rep->ns_numrrsets, timenow, region, + &tree, LDNS_SECTION_ADDITIONAL, qinfo->qtype, + dnssec, rr_offset)) != RETVAL_OK) { + if(r == RETVAL_TRUNC) { + /* no need to set TC bit, this is the additional */ + sldns_buffer_write_u16_at(buffer, 10, arcount); + sldns_buffer_flip(buffer); + return 1; + } + return 0; } - return 0; + sldns_buffer_write_u16_at(buffer, 10, arcount); } - sldns_buffer_write_u16_at(buffer, 10, arcount); } sldns_buffer_flip(buffer); return 1; @@ -763,7 +787,7 @@ calc_edns_field_size(struct edns_data* edns) { size_t rdatalen = 0; struct edns_option* opt; - if(!edns || !edns->edns_present) + if(!edns || !edns->edns_present) return 0; for(opt = edns->opt_list; opt; opt = opt->next) { rdatalen += 4 + opt->opt_len; From c94e13220b6eea8e18c8cbbea51e4e663cb079e6 Mon Sep 17 00:00:00 2001 From: "W.C.A. Wijngaards" Date: Fri, 19 Jul 2019 08:18:06 +0200 Subject: [PATCH 4/6] - Fix #49: Set no renegotiation on the SSL context to stop client session renegotiation. --- doc/Changelog | 4 ++++ smallapp/unbound-control.c | 8 +++++++- util/net_help.c | 16 ++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/doc/Changelog b/doc/Changelog index 3b9618cac..75d1a900a 100644 --- a/doc/Changelog +++ b/doc/Changelog @@ -1,3 +1,7 @@ +19 July 2019: Wouter + - Fix #49: Set no renegotiation on the SSL context to stop client + session renegotiation. + 12 July 2019: Wouter - Fix #48: Unbound returns additional records on NODATA response, if minimal-responses is enabled, also the additional for negative diff --git a/smallapp/unbound-control.c b/smallapp/unbound-control.c index 3ea6aa033..eba7f5814 100644 --- a/smallapp/unbound-control.c +++ b/smallapp/unbound-control.c @@ -498,7 +498,13 @@ setup_ctx(struct config_file* cfg) ssl_err("could not set SSL_OP_NO_SSLv2"); if((SSL_CTX_set_options(ctx, SSL_OP_NO_SSLv3) & SSL_OP_NO_SSLv3) != SSL_OP_NO_SSLv3) - ssl_err("could not set SSL_OP_NO_SSLv3"); + ssl_err("could not set SSL_O P_NO_SSLv3"); +#if defined(SSL_OP_NO_RENEGOTIATION) + /* disable client renegotiation */ + if((SSL_CTX_set_options(ctx, SSL_OP_NO_RENEGOTIATION) & + SSL_OP_NO_RENEGOTIATION) != SSL_OP_NO_RENEGOTIATION) + ssl_err("could not set SSL_OP_NO_RENEGOTIATION"); +#endif if(!SSL_CTX_use_certificate_chain_file(ctx,c_cert)) ssl_path_err("Error setting up SSL_CTX client cert", c_cert); if (!SSL_CTX_use_PrivateKey_file(ctx,c_key,SSL_FILETYPE_PEM)) diff --git a/util/net_help.c b/util/net_help.c index 13bcdf808..88bfc225a 100644 --- a/util/net_help.c +++ b/util/net_help.c @@ -744,6 +744,14 @@ listen_sslctx_setup(void* ctxt) return 0; } #endif +#if defined(SSL_OP_NO_RENEGOTIATION) + /* disable client renegotiation */ + if((SSL_CTX_set_options(ctx, SSL_OP_NO_RENEGOTIATION) & + SSL_OP_NO_RENEGOTIATION) != SSL_OP_NO_RENEGOTIATION) { + log_crypto_err("could not set SSL_OP_NO_RENEGOTIATION"); + return 0; + } +#endif #if defined(SHA256_DIGEST_LENGTH) && defined(USE_ECDSA) /* if we have sha256, set the cipher list to have no known vulns */ if(!SSL_CTX_set_cipher_list(ctx, "TLS13-CHACHA20-POLY1305-SHA256:TLS13-AES-256-GCM-SHA384:TLS13-AES-128-GCM-SHA256:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256")) @@ -962,6 +970,14 @@ void* connect_sslctx_create(char* key, char* pem, char* verifypem, int wincert) SSL_CTX_free(ctx); return NULL; } +#if defined(SSL_OP_NO_RENEGOTIATION) + /* disable client renegotiation */ + if((SSL_CTX_set_options(ctx, SSL_OP_NO_RENEGOTIATION) & + SSL_OP_NO_RENEGOTIATION) != SSL_OP_NO_RENEGOTIATION) { + log_crypto_err("could not set SSL_OP_NO_RENEGOTIATION"); + return 0; + } +#endif if(key && key[0]) { if(!SSL_CTX_use_certificate_chain_file(ctx, pem)) { log_err("error in client certificate %s", pem); From b4b00655542024111bc2eb044f2029008cf9a6ae Mon Sep 17 00:00:00 2001 From: "W.C.A. Wijngaards" Date: Fri, 19 Jul 2019 12:51:37 +0200 Subject: [PATCH 5/6] Fixup space in error message. --- smallapp/unbound-control.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/smallapp/unbound-control.c b/smallapp/unbound-control.c index eba7f5814..01e2385fa 100644 --- a/smallapp/unbound-control.c +++ b/smallapp/unbound-control.c @@ -498,7 +498,7 @@ setup_ctx(struct config_file* cfg) ssl_err("could not set SSL_OP_NO_SSLv2"); if((SSL_CTX_set_options(ctx, SSL_OP_NO_SSLv3) & SSL_OP_NO_SSLv3) != SSL_OP_NO_SSLv3) - ssl_err("could not set SSL_O P_NO_SSLv3"); + ssl_err("could not set SSL_OP_NO_SSLv3"); #if defined(SSL_OP_NO_RENEGOTIATION) /* disable client renegotiation */ if((SSL_CTX_set_options(ctx, SSL_OP_NO_RENEGOTIATION) & From 5f5c00203e87d5c7cd1152476edb8fc9778d0be3 Mon Sep 17 00:00:00 2001 From: "W.C.A. Wijngaards" Date: Tue, 23 Jul 2019 14:01:59 +0200 Subject: [PATCH 6/6] - Fix question section mismatch in local zone redirect. --- doc/Changelog | 3 +++ services/mesh.c | 17 +++++++++-------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/doc/Changelog b/doc/Changelog index 75d1a900a..089c4de65 100644 --- a/doc/Changelog +++ b/doc/Changelog @@ -1,3 +1,6 @@ +23 July 2019: Wouter + - Fix question section mismatch in local zone redirect. + 19 July 2019: Wouter - Fix #49: Set no renegotiation on the SSL context to stop client session renegotiation. diff --git a/services/mesh.c b/services/mesh.c index 59ee9a08e..0d7118af9 100644 --- a/services/mesh.c +++ b/services/mesh.c @@ -1340,14 +1340,15 @@ int mesh_state_add_reply(struct mesh_state* s, struct edns_data* edns, log_assert(!qinfo->local_alias->next && dsrc->count == 1 && qinfo->local_alias->rrset->rk.type == htons(LDNS_RR_TYPE_CNAME)); - /* Technically, we should make a local copy for the owner - * name of the RRset, but in the case of the first (and - * currently only) local alias RRset, the owner name should - * point to the qname of the corresponding query, which should - * be valid throughout the lifetime of this mesh_reply. So - * we can skip copying. */ - log_assert(qinfo->local_alias->rrset->rk.dname == - sldns_buffer_at(rep->c->buffer, LDNS_HEADER_SIZE)); + /* we should make a local copy for the owner name of + * the RRset */ + r->local_alias->rrset->rk.dname_len = + qinfo->local_alias->rrset->rk.dname_len; + r->local_alias->rrset->rk.dname = regional_alloc_init( + s->s.region, qinfo->local_alias->rrset->rk.dname, + qinfo->local_alias->rrset->rk.dname_len); + if(!r->local_alias->rrset->rk.dname) + return 0; /* the rrset is not packed, like in the cache, but it is * individualy allocated with an allocator from localzone. */