From 67e52ea9c5815333152e6a25a20a8fe902499346 Mon Sep 17 00:00:00 2001 From: Natalie Reece Date: Tue, 11 Jul 2023 17:01:26 -0600 Subject: [PATCH 1/2] Exclude EDE before other EDNS options when there isn't enough space --- util/data/msgencode.c | 49 +++++++++++++++++++++++++++++++++++-------- util/data/msgencode.h | 8 +++++++ 2 files changed, 48 insertions(+), 9 deletions(-) diff --git a/util/data/msgencode.c b/util/data/msgencode.c index fe21cfb86..c394cceba 100644 --- a/util/data/msgencode.c +++ b/util/data/msgencode.c @@ -806,6 +806,24 @@ calc_edns_field_size(struct edns_data* edns) return 1 + 2 + 2 + 4 + 2 + rdatalen; } +uint16_t +calc_edns_option_size(struct edns_data* edns, uint16_t code) +{ + size_t rdatalen = 0; + struct edns_option* opt; + if(!edns || !edns->edns_present) + return 0; + for(opt = edns->opt_list_inplace_cb_out; opt; opt = opt->next) { + if (opt->opt_code == code) + rdatalen += 4 + opt->opt_len; + } + for(opt = edns->opt_list_out; opt; opt = opt->next) { + if (opt->opt_code == code) + rdatalen += 4 + opt->opt_len; + } + return rdatalen; +} + static void attach_edns_record_max_msg_sz(sldns_buffer* pkt, struct edns_data* edns, uint16_t max_msg_sz) @@ -918,23 +936,30 @@ reply_info_answer_encode(struct query_info* qinf, struct reply_info* rep, return 0; if(sldns_buffer_capacity(pkt) < udpsize) udpsize = sldns_buffer_capacity(pkt); - if(udpsize < LDNS_HEADER_SIZE + calc_edns_field_size(edns)) { + if(udpsize < LDNS_HEADER_SIZE + calc_edns_field_size(edns) - + calc_edns_option_size(edns, LDNS_EDNS_EDE)) { /* packet too small to contain edns, omit it. */ attach_edns = 0; } else { /* reserve space for edns record */ - attach_edns = (unsigned int)calc_edns_field_size(edns); - udpsize -= attach_edns; + attach_edns = (unsigned int)calc_edns_field_size(edns) - + calc_edns_option_size(edns, LDNS_EDNS_EDE); } if(!reply_info_encode(qinf, rep, id, flags, pkt, timenow, region, - udpsize, dnssec, MINIMAL_RESPONSES)) { + udpsize - attach_edns, dnssec, MINIMAL_RESPONSES)) { log_err("reply encode: out of memory"); return 0; } - if(attach_edns && sldns_buffer_capacity(pkt) >= - sldns_buffer_limit(pkt)+attach_edns) - attach_edns_record_max_msg_sz(pkt, edns, udpsize+attach_edns); + if(attach_edns) { + if(udpsize >= sldns_buffer_limit(pkt)+calc_edns_field_size(edns)) + attach_edns_record_max_msg_sz(pkt, edns, udpsize); + else if(udpsize >= sldns_buffer_limit(pkt) + attach_edns) { + edns_opt_list_remove(&edns->opt_list_inplace_cb_out, LDNS_EDNS_EDE); + edns_opt_list_remove(&edns->opt_list_out, LDNS_EDNS_EDE); + attach_edns_record_max_msg_sz(pkt, edns, udpsize); + } + } return 1; } @@ -996,8 +1021,14 @@ error_encode(sldns_buffer* buf, int r, struct query_info* qinfo, es.ext_rcode = 0; es.bits &= EDNS_DO; if(sldns_buffer_limit(buf) + calc_edns_field_size(&es) > - edns->udp_size) - return; + edns->udp_size) { + edns_opt_list_remove(&es.opt_list_inplace_cb_out, LDNS_EDNS_EDE); + edns_opt_list_remove(&es.opt_list_out, LDNS_EDNS_EDE); + if(sldns_buffer_limit(buf) + calc_edns_field_size(&es) > + edns->udp_size) { + return; + } + } attach_edns_record(buf, &es); } } diff --git a/util/data/msgencode.h b/util/data/msgencode.h index 30dc515cb..7dd1772dd 100644 --- a/util/data/msgencode.h +++ b/util/data/msgencode.h @@ -108,6 +108,14 @@ void qinfo_query_encode(struct sldns_buffer* pkt, struct query_info* qinfo); */ uint16_t calc_edns_field_size(struct edns_data* edns); +/** + * Calculate the size of a specific EDNS option in packet. + * @param edns: edns data or NULL. + * @param code: the opt code to get the size of. + * @return octets the option will take up. + */ +uint16_t calc_edns_option_size(struct edns_data* edns, uint16_t code); + /** * Attach EDNS record to buffer. Buffer has complete packet. There must * be enough room left for the EDNS record. From 08e11284fbd805556e8eefdbb01a372ba623a346 Mon Sep 17 00:00:00 2001 From: George Thessalonikefs Date: Tue, 1 Aug 2023 09:55:28 +0200 Subject: [PATCH 2/2] - For #911: Try to trim EXTRA-TEXT (and LDNS_EDE_OTHER options altogether) before giving up on attaching EDE options. --- testcode/unitmain.c | 214 ++++++++++++++++++++++++++++++++++++++++++ util/data/msgencode.c | 93 ++++++++++++++++-- util/data/msgencode.h | 13 +++ 3 files changed, 312 insertions(+), 8 deletions(-) diff --git a/testcode/unitmain.c b/testcode/unitmain.c index b6dac5507..b0bf6bb54 100644 --- a/testcode/unitmain.c +++ b/testcode/unitmain.c @@ -839,6 +839,218 @@ static void respip_test(void) respip_conf_actions_test(); } +#include "util/regional.h" +#include "sldns/sbuffer.h" +#include "util/data/dname.h" +#include "util/data/msgreply.h" +#include "util/data/msgencode.h" +#include "sldns/str2wire.h" + +static void edns_ede_encode_setup(struct edns_data* edns, + struct regional* region) +{ + memset(edns, 0, sizeof(*edns)); + edns->edns_present = 1; + edns->edns_version = EDNS_ADVERTISED_VERSION; + edns->udp_size = EDNS_ADVERTISED_SIZE; + edns->bits &= EDNS_DO; + /* Fill up opt_list_out with EDEs */ + unit_assert( + edns_opt_list_append_ede(&edns->opt_list_out, region, + LDNS_EDE_OTHER, "Too long other text")); + unit_assert( + edns_opt_list_append_ede(&edns->opt_list_out, region, + LDNS_EDE_OTHER, "Too long other text")); + unit_assert( + edns_opt_list_append_ede(&edns->opt_list_out, region, + LDNS_EDE_BLOCKED, "Too long blocked text")); + unit_assert( + edns_opt_list_append_ede(&edns->opt_list_out, region, + LDNS_EDE_OTHER, "Too long other text")); + unit_assert( + edns_opt_list_append_ede(&edns->opt_list_out, region, + LDNS_EDE_BLOCKED, "Too long blocked text")); + /* Fill up opt_list_inplace_cb_out with EDEs */ + unit_assert( + edns_opt_list_append_ede(&edns->opt_list_inplace_cb_out, region, + LDNS_EDE_OTHER, "Too long other text")); + unit_assert( + edns_opt_list_append_ede(&edns->opt_list_inplace_cb_out, region, + LDNS_EDE_OTHER, "Too long other text")); + unit_assert( + edns_opt_list_append_ede(&edns->opt_list_inplace_cb_out, region, + LDNS_EDE_BLOCKED, "Too long blocked text")); + unit_assert( + edns_opt_list_append_ede(&edns->opt_list_inplace_cb_out, region, + LDNS_EDE_OTHER, "Too long other text")); + unit_assert( + edns_opt_list_append_ede(&edns->opt_list_inplace_cb_out, region, + LDNS_EDE_BLOCKED, "Too long blocked text")); + /* append another EDNS option to both lists */ + unit_assert( + edns_opt_list_append(&edns->opt_list_out, + LDNS_EDNS_UNBOUND_CACHEDB_TESTFRAME_TEST, 0, NULL, region)); + unit_assert( + edns_opt_list_append(&edns->opt_list_inplace_cb_out, + LDNS_EDNS_UNBOUND_CACHEDB_TESTFRAME_TEST, 0, NULL, region)); + /* append LDNS_EDE_OTHER at the end of both lists */ + unit_assert( + edns_opt_list_append_ede(&edns->opt_list_out, region, + LDNS_EDE_OTHER, "Too long other text")); + unit_assert( + edns_opt_list_append_ede(&edns->opt_list_inplace_cb_out, region, + LDNS_EDE_OTHER, "Too long other text")); +} + +static void edns_ede_encode_encodedecode(struct query_info* qinfo, + struct reply_info* rep, struct regional* region, + struct edns_data* edns, sldns_buffer* pkt) +{ + /* encode */ + unit_assert( + reply_info_answer_encode(qinfo, rep, 1, rep->flags, pkt, + 0, 0, region, 65535, edns, 0, 0)); + /* buffer ready for reading; skip after the question section */ + sldns_buffer_skip(pkt, LDNS_HEADER_SIZE); + (void)query_dname_len(pkt); + sldns_buffer_skip(pkt, 2 + 2); + /* decode */ + unit_assert( + parse_edns_from_query_pkt(pkt, edns, NULL, NULL, region)==0); +} + +static void edns_ede_encode_check(struct edns_data* edns, int* found_ede, + int* found_ede_other, int* found_ede_txt, int* found_other_edns) +{ + struct edns_option* opt; + for(opt = edns->opt_list_in; opt; opt = opt->next) { + if(opt->opt_code == LDNS_EDNS_EDE) { + (*found_ede)++; + if(opt->opt_len > 2) + (*found_ede_txt)++; + if(opt->opt_len >= 2 && sldns_read_uint16( + opt->opt_data) == LDNS_EDE_OTHER) + (*found_ede_other)++; + } else { + (*found_other_edns)++; + } + } + +} + +static void edns_ede_encode_fit_test(struct query_info* qinfo, + struct reply_info* rep, struct regional* region) +{ + struct edns_data edns; + int found_ede = 0, found_ede_other = 0, found_ede_txt = 0; + int found_other_edns = 0; + sldns_buffer* pkt = sldns_buffer_new(65535); + unit_assert(pkt); + edns_ede_encode_setup(&edns, region); + /* leave the pkt buffer as is; everything should fit */ + edns_ede_encode_encodedecode(qinfo, rep, region, &edns, pkt); + edns_ede_encode_check(&edns, &found_ede, &found_ede_other, + &found_ede_txt, &found_other_edns); + unit_assert(found_ede == 12); + unit_assert(found_ede_other == 8); + unit_assert(found_ede_txt == 12); + unit_assert(found_other_edns == 2); + /* cleanup */ + sldns_buffer_free(pkt); +} + +static void edns_ede_encode_notxt_fit_test( struct query_info* qinfo, + struct reply_info* rep, struct regional* region) +{ + struct edns_data edns; + sldns_buffer* pkt; + uint16_t edns_field_size, ede_txt_size; + int found_ede = 0, found_ede_other = 0, found_ede_txt = 0; + int found_other_edns = 0; + edns_ede_encode_setup(&edns, region); + /* pkt buffer should fit everything if the ede txt is cropped. + * OTHER EDE should not be there since it is useless without text. */ + edns_field_size = calc_edns_field_size(&edns); + (void)calc_ede_option_size(&edns, &ede_txt_size); + pkt = sldns_buffer_new(LDNS_HEADER_SIZE + + qinfo->qname_len + + 2 + 2 /* qtype + qclass */ + + 11 /* opt record */ + + edns_field_size + - ede_txt_size); + unit_assert(pkt); + edns_ede_encode_encodedecode(qinfo, rep, region, &edns, pkt); + edns_ede_encode_check(&edns, &found_ede, &found_ede_other, + &found_ede_txt, &found_other_edns); + unit_assert(found_ede == 4); + unit_assert(found_ede_other == 0); + unit_assert(found_ede_txt == 0); + unit_assert(found_other_edns == 2); + /* cleanup */ + sldns_buffer_free(pkt); +} + +static void edns_ede_encode_no_fit_test( struct query_info* qinfo, + struct reply_info* rep, struct regional* region) +{ + struct edns_data edns; + sldns_buffer* pkt; + uint16_t edns_field_size, ede_size, ede_txt_size; + int found_ede = 0, found_ede_other = 0, found_ede_txt = 0; + int found_other_edns = 0; + edns_ede_encode_setup(&edns, region); + /* pkt buffer should fit only non-EDE options. */ + edns_field_size = calc_edns_field_size(&edns); + ede_size = calc_ede_option_size(&edns, &ede_txt_size); + pkt = sldns_buffer_new(LDNS_HEADER_SIZE + + qinfo->qname_len + + 2 + 2 /* qtype + qclass */ + + 11 /* opt record */ + + edns_field_size + - ede_size); + unit_assert(pkt); + edns_ede_encode_encodedecode(qinfo, rep, region, &edns, pkt); + edns_ede_encode_check(&edns, &found_ede, &found_ede_other, + &found_ede_txt, &found_other_edns); + unit_assert(found_ede == 0); + unit_assert(found_ede_other == 0); + unit_assert(found_ede_txt == 0); + unit_assert(found_other_edns == 2); + /* cleanup */ + sldns_buffer_free(pkt); +} + +/** test optional EDE encoding with various buffer + * available sizes */ +static void edns_ede_answer_encode_test(void) +{ + struct regional* region = regional_create(); + struct reply_info* rep; + struct query_info qinfo; + unit_show_feature("edns ede optional encoding"); + unit_assert(region); + rep = construct_reply_info_base(region, + LDNS_RCODE_NOERROR | BIT_QR, 1, + 3600, 3600, 3600, + 0, 0, 0, 0, + sec_status_unchecked, LDNS_EDE_NONE); + unit_assert(rep); + memset(&qinfo, 0, sizeof(qinfo)); + qinfo.qname = sldns_str2wire_dname("encode.ede.", &qinfo.qname_len); + unit_assert(qinfo.qname); + qinfo.qtype = LDNS_RR_TYPE_TXT; + qinfo.qclass = LDNS_RR_CLASS_IN; + + edns_ede_encode_fit_test(&qinfo, rep, region); + edns_ede_encode_notxt_fit_test(&qinfo, rep, region); + edns_ede_encode_no_fit_test(&qinfo, rep, region); + + /* cleanup */ + free(qinfo.qname); + regional_free_all(region); + regional_destroy(region); +} + void unit_show_func(const char* file, const char* func) { printf("test %s:%s\n", file, func); @@ -852,6 +1064,7 @@ void unit_show_feature(const char* feature) #ifdef USE_ECDSA_EVP_WORKAROUND void ecdsa_evp_workaround_init(void); #endif + /** * Main unit test program. Setup, teardown and report errors. * @param argc: arg count. @@ -909,6 +1122,7 @@ main(int argc, char* argv[]) zonemd_test(); tcpreuse_test(); msgparse_test(); + edns_ede_answer_encode_test(); #ifdef CLIENT_SUBNET ecs_test(); #endif /* CLIENT_SUBNET */ diff --git a/util/data/msgencode.c b/util/data/msgencode.c index c394cceba..19a5a6ec0 100644 --- a/util/data/msgencode.c +++ b/util/data/msgencode.c @@ -814,16 +814,85 @@ calc_edns_option_size(struct edns_data* edns, uint16_t code) if(!edns || !edns->edns_present) return 0; for(opt = edns->opt_list_inplace_cb_out; opt; opt = opt->next) { - if (opt->opt_code == code) + if(opt->opt_code == code) rdatalen += 4 + opt->opt_len; } for(opt = edns->opt_list_out; opt; opt = opt->next) { - if (opt->opt_code == code) + if(opt->opt_code == code) rdatalen += 4 + opt->opt_len; } return rdatalen; } +uint16_t +calc_ede_option_size(struct edns_data* edns, uint16_t* txt_size) +{ + size_t rdatalen = 0; + struct edns_option* opt; + *txt_size = 0; + if(!edns || !edns->edns_present) + return 0; + for(opt = edns->opt_list_inplace_cb_out; opt; opt = opt->next) { + if(opt->opt_code == LDNS_EDNS_EDE) + rdatalen += 4 + opt->opt_len; + if(opt->opt_len > 2) + *txt_size += opt->opt_len - 2; + if(opt->opt_len >= 2 && sldns_read_uint16( + opt->opt_data) == LDNS_EDE_OTHER) + *txt_size += 4 + 2; + } + for(opt = edns->opt_list_out; opt; opt = opt->next) { + if(opt->opt_code == LDNS_EDNS_EDE) + rdatalen += 4 + opt->opt_len; + if(opt->opt_len > 2) + *txt_size += opt->opt_len - 2; + if(opt->opt_len >= 2 && sldns_read_uint16( + opt->opt_data) == LDNS_EDE_OTHER) + *txt_size += 4 + 2; + } + return rdatalen; +} + +/* Trims the EDE OPTION-DATA to not include any EXTRA-TEXT data. + * Also removes any LDNS_EDE_OTHER options from the list since they are useless + * without the extra text. */ +static void +ede_trim_text(struct edns_option** list) +{ + struct edns_option* curr, *prev = NULL; + if(!list || !(*list)) return; + /* Unlink and repoint if LDNS_EDE_OTHER are first in list */ + while(list && *list && (*list)->opt_code == LDNS_EDNS_EDE + && (*list)->opt_len >= 2 + && sldns_read_uint16((*list)->opt_data) == LDNS_EDE_OTHER ) { + *list = (*list)->next; + } + if(!list || !(*list)) return; + curr = *list; + while(curr) { + if(curr->opt_code == LDNS_EDNS_EDE) { + if(curr->opt_len >= 2 && sldns_read_uint16( + curr->opt_data) == LDNS_EDE_OTHER) { + /* LDNS_EDE_OTHER cannot be the first option in + * this while, so prev is always initialized at + * this point from the other branches; + * cut this option off */ + prev->next = curr->next; + curr = curr->next; + } else if(curr->opt_len > 2) { + /* trim this option's EXTRA-TEXT */ + curr->opt_len = 2; + prev = curr; + curr = curr->next; + } + } else { + /* continue */ + prev = curr; + curr = curr->next; + } + } +} + static void attach_edns_record_max_msg_sz(sldns_buffer* pkt, struct edns_data* edns, uint16_t max_msg_sz) @@ -912,6 +981,7 @@ reply_info_answer_encode(struct query_info* qinf, struct reply_info* rep, { uint16_t flags; unsigned int attach_edns = 0; + uint16_t edns_field_size, ede_size, ede_txt_size; if(!cached || rep->authoritative) { /* original flags, copy RD and CD bits from query. */ @@ -934,16 +1004,19 @@ reply_info_answer_encode(struct query_info* qinf, struct reply_info* rep, log_assert(flags & BIT_QR); /* QR bit must be on in our replies */ if(udpsize < LDNS_HEADER_SIZE) return 0; + /* currently edns does not change during calculations; + * calculate sizes once here */ + edns_field_size = calc_edns_field_size(edns); + ede_size = calc_ede_option_size(edns, &ede_txt_size); if(sldns_buffer_capacity(pkt) < udpsize) udpsize = sldns_buffer_capacity(pkt); - if(udpsize < LDNS_HEADER_SIZE + calc_edns_field_size(edns) - - calc_edns_option_size(edns, LDNS_EDNS_EDE)) { + /* EDEs are optional, try to fit anything else before them */ + if(udpsize < LDNS_HEADER_SIZE + edns_field_size - ede_size) { /* packet too small to contain edns, omit it. */ attach_edns = 0; } else { /* reserve space for edns record */ - attach_edns = (unsigned int)calc_edns_field_size(edns) - - calc_edns_option_size(edns, LDNS_EDNS_EDE); + attach_edns = (unsigned int)edns_field_size - ede_size; } if(!reply_info_encode(qinf, rep, id, flags, pkt, timenow, region, @@ -952,9 +1025,13 @@ reply_info_answer_encode(struct query_info* qinf, struct reply_info* rep, return 0; } if(attach_edns) { - if(udpsize >= sldns_buffer_limit(pkt)+calc_edns_field_size(edns)) + if(udpsize >= sldns_buffer_limit(pkt) + edns_field_size) + attach_edns_record_max_msg_sz(pkt, edns, udpsize); + else if(udpsize >= sldns_buffer_limit(pkt) + edns_field_size - ede_txt_size) { + ede_trim_text(&edns->opt_list_inplace_cb_out); + ede_trim_text(&edns->opt_list_out); attach_edns_record_max_msg_sz(pkt, edns, udpsize); - else if(udpsize >= sldns_buffer_limit(pkt) + attach_edns) { + } else if(udpsize >= sldns_buffer_limit(pkt) + edns_field_size - ede_size) { edns_opt_list_remove(&edns->opt_list_inplace_cb_out, LDNS_EDNS_EDE); edns_opt_list_remove(&edns->opt_list_out, LDNS_EDNS_EDE); attach_edns_record_max_msg_sz(pkt, edns, udpsize); diff --git a/util/data/msgencode.h b/util/data/msgencode.h index 7dd1772dd..d92bd3b03 100644 --- a/util/data/msgencode.h +++ b/util/data/msgencode.h @@ -116,6 +116,19 @@ uint16_t calc_edns_field_size(struct edns_data* edns); */ uint16_t calc_edns_option_size(struct edns_data* edns, uint16_t code); +/** + * Calculate the size of the EDE option(s) in packet. Also calculate seperately + * the size of the EXTRA-TEXT field(s) in case we can trim them to fit. + * In this case include any LDNS_EDE_OTHER options in their entirety since they + * are useless without extra text. + * @param edns: edns data or NULL. + * @param txt_size: the size of the EXTRA-TEXT field(s); this includes + * LDNS_EDE_OTHER in their entirety since they are useless without + * extra text. + * @return octets the option will take up. + */ +uint16_t calc_ede_option_size(struct edns_data* edns, uint16_t* txt_size); + /** * Attach EDNS record to buffer. Buffer has complete packet. There must * be enough room left for the EDNS record.