From c8874b0d858e7261382895a01786c732fde14ffa Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 1 Feb 2022 09:58:20 +0100 Subject: [PATCH 1/3] mdns: Fix null-service issue when parsing packets Closes https://github.com/espressif/esp-idf/issues/8307 --- components/mdns/mdns.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index f119ad67a9..a94c782174 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -149,6 +149,9 @@ static char * _mdns_mangle_name(char* in) { static bool _mdns_service_match(const mdns_service_t * srv, const char * service, const char * proto, const char * hostname) { + if (!service || !proto) { + return false; + } return !strcasecmp(srv->service, service) && !strcasecmp(srv->proto, proto) && (_str_null_or_empty(hostname) || !strcasecmp(srv->hostname, hostname)); } @@ -262,6 +265,12 @@ static bool _mdns_instance_name_match(const char *lhs, const char *rhs) static bool _mdns_service_match_instance(const mdns_service_t *srv, const char *instance, const char *service, const char *proto, const char *hostname) { + // service and proto must be supplied, if not this instance won't match + if (!service || !proto) { + return false; + } + // instance==NULL -> _mdns_instance_name_match() will check the default instance + // hostname==NULL -> matches if instance, service and proto matches return !strcasecmp(srv->service, service) && _mdns_instance_name_match(srv->instance, instance) && !strcasecmp(srv->proto, proto) && (_str_null_or_empty(hostname) || !strcasecmp(srv->hostname, hostname)); } @@ -1480,7 +1489,7 @@ static void _mdns_create_answer_from_parsed_packet(mdns_parsed_packet_t *parsed_ shared = q->type == MDNS_TYPE_PTR || q->type == MDNS_TYPE_SDPTR || !parsed_packet->probe; if (q->type == MDNS_TYPE_SRV || q->type == MDNS_TYPE_TXT) { mdns_srv_item_t *service = _mdns_get_service_item_instance(q->host, q->service, q->proto, NULL); - if (!_mdns_create_answer_from_service(packet, service->service, q, shared, send_flush)) { + if (service == NULL || !_mdns_create_answer_from_service(packet, service->service, q, shared, send_flush)) { _mdns_free_tx_packet(packet); return; } @@ -3226,8 +3235,7 @@ void mdns_parse_packet(mdns_rx_packet_t * packet) _mdns_search_result_add_ptr(search_result, name->host, name->service, name->proto, packet->tcpip_if, packet->ip_protocol, ttl); } else if ((discovery || ours) && !name->sub && _mdns_name_is_ours(name)) { - if (discovery) { - service = _mdns_get_service_item(name->service, name->proto, NULL); + if (discovery && (service = _mdns_get_service_item(name->service, name->proto, NULL))) { _mdns_remove_parsed_question(parsed_packet, MDNS_TYPE_SDPTR, service); } else if (service && parsed_packet->questions && !parsed_packet->probe) { _mdns_remove_parsed_question(parsed_packet, type, service); From d6ad597b0f2a49617bf873bcb452e7c20d9069b3 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Wed, 2 Feb 2022 08:35:04 +0100 Subject: [PATCH 2/3] mdns: Fix memleak when adding delegated host --- components/mdns/mdns.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index a94c782174..b58bd19038 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -2587,10 +2587,17 @@ static bool _hostname_is_ours(const char * hostname) return false; } +/** + * @brief Adds a delegated hostname to the linked list + * @param hostname Host name pointer + * @param address_list Address list + * @return true on success + * false if the host wasn't attached (this is our hostname, or alloc failure) so we have to free the structs + */ static bool _mdns_delegate_hostname_add(const char * hostname, mdns_ip_addr_t * address_list) { if (_hostname_is_ours(hostname)) { - return true; + return false; } mdns_host_item_t * host = (mdns_host_item_t *)malloc(sizeof(mdns_host_item_t)); @@ -2638,6 +2645,18 @@ static mdns_ip_addr_t * copy_address_list(const mdns_ip_addr_t * address_list) return head; } +static void free_delegated_hostnames(void) +{ + mdns_host_item_t * host = _mdns_host_list; + while (host != NULL) { + free_address_list(host->address_list); + free((char *)host->hostname); + mdns_host_item_t *item = host; + host = host->next; + free(item); + } +} + static bool _mdns_delegate_hostname_remove(const char * hostname) { mdns_srv_item_t * srv = _mdns_server->services; @@ -4482,8 +4501,11 @@ static void _mdns_execute_action(mdns_action_t * action) _mdns_packet_free(action->data.rx_handle.packet); break; case ACTION_DELEGATE_HOSTNAME_ADD: - _mdns_delegate_hostname_add(action->data.delegate_hostname.hostname, - action->data.delegate_hostname.address_list); + if (!_mdns_delegate_hostname_add(action->data.delegate_hostname.hostname, + action->data.delegate_hostname.address_list)) { + free((char *)action->data.delegate_hostname.hostname); + free_address_list(action->data.delegate_hostname.address_list); + } break; case ACTION_DELEGATE_HOSTNAME_REMOVE: _mdns_delegate_hostname_remove(action->data.delegate_hostname.hostname); @@ -4839,6 +4861,7 @@ void mdns_free(void) #endif mdns_service_remove_all(); + free_delegated_hostnames(); _mdns_service_task_stop(); for (i=0; i Date: Wed, 2 Feb 2022 12:21:12 +0100 Subject: [PATCH 3/3] mdns: Fix potential read behind parsed packet --- components/mdns/mdns.c | 73 ++++++++++++-------- examples/protocols/mdns/mdns_example_test.py | 1 + 2 files changed, 46 insertions(+), 28 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index b58bd19038..03977706c8 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -299,10 +299,11 @@ static mdns_srv_item_t *_mdns_get_service_item_instance(const char *instance, co * * @return the address after the parsed FQDN in the packet or NULL on error */ -static const uint8_t * _mdns_read_fqdn(const uint8_t * packet, const uint8_t * start, mdns_name_t * name, char * buf) +static const uint8_t * _mdns_read_fqdn(const uint8_t * packet, const uint8_t * start, mdns_name_t * name, char * buf, size_t packet_len) { size_t index = 0; - while (start[index]) { + const uint8_t * packet_end = packet + packet_len; + while (start + index < packet_end && start[index]) { if (name->parts == 4) { name->invalid = true; } @@ -314,6 +315,9 @@ static const uint8_t * _mdns_read_fqdn(const uint8_t * packet, const uint8_t * s } uint8_t i; for (i=0; i= packet_end) { + return NULL; + } buf[i] = start[index++]; } buf[len] = '\0'; @@ -336,7 +340,7 @@ static const uint8_t * _mdns_read_fqdn(const uint8_t * packet, const uint8_t * s //reference address can not be after where we are return NULL; } - if (_mdns_read_fqdn(packet, packet + address, name, buf)) { + if (_mdns_read_fqdn(packet, packet + address, name, buf, packet_len)) { return start + index; } return NULL; @@ -536,7 +540,7 @@ static inline int append_one_txt_record_entry(uint8_t * packet, uint16_t * index * * @return length of added data: 0 on error or length on success */ -static uint16_t _mdns_append_fqdn(uint8_t * packet, uint16_t * index, const char * strings[], uint8_t count) +static uint16_t _mdns_append_fqdn(uint8_t * packet, uint16_t * index, const char * strings[], uint8_t count, size_t packet_len) { if (!count) { //empty string so terminate @@ -563,7 +567,7 @@ search_next: name.service[0] = 0; name.proto[0] = 0; name.domain[0] = 0; - const uint8_t * content = _mdns_read_fqdn(packet, len_location, &name, buf); + const uint8_t * content = _mdns_read_fqdn(packet, len_location, &name, buf, packet_len); if (!content) { //not a readable fqdn? return 0; @@ -589,7 +593,7 @@ search_next: return 0; } //run the same for the other strings in the name - return written + _mdns_append_fqdn(packet, index, &strings[1], count - 1); + return written + _mdns_append_fqdn(packet, index, &strings[1], count - 1, packet_len); } //we have found the string so let's insert a pointer to it instead @@ -623,7 +627,7 @@ static uint16_t _mdns_append_ptr_record(uint8_t * packet, uint16_t * index, cons str[2] = proto; str[3] = MDNS_DEFAULT_DOMAIN; - part_length = _mdns_append_fqdn(packet, index, str + 1, 3); + part_length = _mdns_append_fqdn(packet, index, str + 1, 3, MDNS_MAX_PACKET_SIZE); if (!part_length) { return 0; } @@ -636,7 +640,7 @@ static uint16_t _mdns_append_ptr_record(uint8_t * packet, uint16_t * index, cons record_length += part_length; uint16_t data_len_location = *index - 2; - part_length = _mdns_append_fqdn(packet, index, str, 4); + part_length = _mdns_append_fqdn(packet, index, str, 4, MDNS_MAX_PACKET_SIZE); if (!part_length) { return 0; } @@ -675,7 +679,7 @@ static uint16_t _mdns_append_sdptr_record(uint8_t * packet, uint16_t * index, md str[1] = service->proto; str[2] = MDNS_DEFAULT_DOMAIN; - part_length = _mdns_append_fqdn(packet, index, sd_str, 4); + part_length = _mdns_append_fqdn(packet, index, sd_str, 4, MDNS_MAX_PACKET_SIZE); record_length += part_length; @@ -686,7 +690,7 @@ static uint16_t _mdns_append_sdptr_record(uint8_t * packet, uint16_t * index, md record_length += part_length; uint16_t data_len_location = *index - 2; - part_length = _mdns_append_fqdn(packet, index, str, 3); + part_length = _mdns_append_fqdn(packet, index, str, 3, MDNS_MAX_PACKET_SIZE); if (!part_length) { return 0; } @@ -724,7 +728,7 @@ static uint16_t _mdns_append_txt_record(uint8_t * packet, uint16_t * index, mdns return 0; } - part_length = _mdns_append_fqdn(packet, index, str, 4); + part_length = _mdns_append_fqdn(packet, index, str, 4, MDNS_MAX_PACKET_SIZE); if (!part_length) { return 0; } @@ -788,7 +792,7 @@ static uint16_t _mdns_append_srv_record(uint8_t * packet, uint16_t * index, mdns return 0; } - part_length = _mdns_append_fqdn(packet, index, str, 4); + part_length = _mdns_append_fqdn(packet, index, str, 4, MDNS_MAX_PACKET_SIZE); if (!part_length) { return 0; } @@ -821,7 +825,7 @@ static uint16_t _mdns_append_srv_record(uint8_t * packet, uint16_t * index, mdns return 0; } - part_length = _mdns_append_fqdn(packet, index, str, 2); + part_length = _mdns_append_fqdn(packet, index, str, 2, MDNS_MAX_PACKET_SIZE); if (!part_length) { return 0; } @@ -854,7 +858,7 @@ static uint16_t _mdns_append_a_record(uint8_t * packet, uint16_t * index, const return 0; } - part_length = _mdns_append_fqdn(packet, index, str, 2); + part_length = _mdns_append_fqdn(packet, index, str, 2, MDNS_MAX_PACKET_SIZE); if (!part_length) { return 0; } @@ -906,7 +910,7 @@ static uint16_t _mdns_append_aaaa_record(uint8_t * packet, uint16_t * index, con } - part_length = _mdns_append_fqdn(packet, index, str, 2); + part_length = _mdns_append_fqdn(packet, index, str, 2, MDNS_MAX_PACKET_SIZE); if (!part_length) { return 0; } @@ -954,7 +958,7 @@ static uint16_t _mdns_append_question(uint8_t * packet, uint16_t * index, mdns_o str[str_index++] = q->domain; } - part_length = _mdns_append_fqdn(packet, index, str, str_index); + part_length = _mdns_append_fqdn(packet, index, str, str_index, MDNS_MAX_PACKET_SIZE); if (!part_length) { return 0; } @@ -2804,7 +2808,7 @@ static inline uint32_t _mdns_read_u32(const uint8_t * packet, uint16_t index) * * @return the address after the parsed FQDN in the packet or NULL on error */ -static const uint8_t * _mdns_parse_fqdn(const uint8_t * packet, const uint8_t * start, mdns_name_t * name) +static const uint8_t * _mdns_parse_fqdn(const uint8_t * packet, const uint8_t * start, mdns_name_t * name, size_t packet_len) { name->parts = 0; name->sub = 0; @@ -2816,7 +2820,7 @@ static const uint8_t * _mdns_parse_fqdn(const uint8_t * packet, const uint8_t * static char buf[MDNS_NAME_BUF_LEN]; - const uint8_t * next_data = (uint8_t*)_mdns_read_fqdn(packet, start, name, buf); + const uint8_t * next_data = (uint8_t*)_mdns_read_fqdn(packet, start, name, buf, packet_len); if (!next_data) { return 0; } @@ -3089,6 +3093,10 @@ void mdns_parse_packet(mdns_rx_packet_t * packet) mdns_name_t * name = &n; memset(name, 0, sizeof(mdns_name_t)); + if (len <= MDNS_HEAD_ADDITIONAL_OFFSET) { + free(parsed_packet); + return; + } header.id = _mdns_read_u16(data, MDNS_HEAD_ID_OFFSET); header.flags.value = _mdns_read_u16(data, MDNS_HEAD_FLAGS_OFFSET); header.questions = _mdns_read_u16(data, MDNS_HEAD_QUESTIONS_OFFSET); @@ -3120,7 +3128,7 @@ void mdns_parse_packet(mdns_rx_packet_t * packet) uint8_t qs = header.questions; while (qs--) { - content = _mdns_parse_fqdn(data, content, name); + content = _mdns_parse_fqdn(data, content, name, len); if (!content) { header.answers = 0; header.additional = 0; @@ -3128,6 +3136,9 @@ void mdns_parse_packet(mdns_rx_packet_t * packet) goto clear_rx_packet;//error } + if (content + MDNS_CLASS_OFFSET + 1 >= data + len) { + goto clear_rx_packet; // malformed packet, won't read behind it + } uint16_t type = _mdns_read_u16(content, MDNS_TYPE_OFFSET); uint16_t mdns_class = _mdns_read_u16(content, MDNS_CLASS_OFFSET); bool unicast = !!(mdns_class & 0x8000); @@ -3197,11 +3208,14 @@ void mdns_parse_packet(mdns_rx_packet_t * packet) while (content < (data + len)) { - content = _mdns_parse_fqdn(data, content, name); + content = _mdns_parse_fqdn(data, content, name, len); if (!content) { goto clear_rx_packet;//error } + if (content + MDNS_LEN_OFFSET + 1 >= data + len) { + goto clear_rx_packet; // malformed packet, won't read behind it + } uint16_t type = _mdns_read_u16(content, MDNS_TYPE_OFFSET); uint16_t mdns_class = _mdns_read_u16(content, MDNS_CLASS_OFFSET); uint32_t ttl = _mdns_read_u32(content, MDNS_TTL_OFFSET); @@ -3247,7 +3261,7 @@ void mdns_parse_packet(mdns_rx_packet_t * packet) } if (type == MDNS_TYPE_PTR) { - if (!_mdns_parse_fqdn(data, data_ptr, name)) { + if (!_mdns_parse_fqdn(data, data_ptr, name, len)) { continue;//error } if (search_result) { @@ -3286,9 +3300,12 @@ void mdns_parse_packet(mdns_rx_packet_t * packet) } } - if (!_mdns_parse_fqdn(data, data_ptr + MDNS_SRV_FQDN_OFFSET, name)) { + if (!_mdns_parse_fqdn(data, data_ptr + MDNS_SRV_FQDN_OFFSET, name, len)) { continue;//error } + if (data_ptr + MDNS_SRV_PORT_OFFSET + 1 >= data + len) { + goto clear_rx_packet; // malformed packet, won't read behind it + } uint16_t priority = _mdns_read_u16(data_ptr, MDNS_SRV_PRIORITY_OFFSET); uint16_t weight = _mdns_read_u16(data_ptr, MDNS_SRV_WEIGHT_OFFSET); uint16_t port = _mdns_read_u16(data_ptr, MDNS_SRV_PORT_OFFSET); @@ -5644,8 +5661,8 @@ void mdns_debug_packet(const uint8_t * data, size_t len) uint8_t qs = header.questions; while (qs--) { - content = _mdns_parse_fqdn(data, content, name); - if (!content) { + content = _mdns_parse_fqdn(data, content, name, len); + if (!content || content + MDNS_CLASS_OFFSET + 1 >= data + len) { header.answers = 0; header.additional = 0; header.servers = 0; @@ -5695,7 +5712,7 @@ void mdns_debug_packet(const uint8_t * data, size_t len) while (content < (data + len)) { - content = _mdns_parse_fqdn(data, content, name); + content = _mdns_parse_fqdn(data, content, name, len); if (!content) { _mdns_dbg_printf("ERROR: parse mdns records\n"); break; @@ -5763,13 +5780,13 @@ void mdns_debug_packet(const uint8_t * data, size_t len) _mdns_dbg_printf("%u ", ttl); _mdns_dbg_printf("[%u] ", data_len); if (type == MDNS_TYPE_PTR) { - if (!_mdns_parse_fqdn(data, data_ptr, name)) { + if (!_mdns_parse_fqdn(data, data_ptr, name, len)) { _mdns_dbg_printf("ERROR: parse PTR\n"); continue; } _mdns_dbg_printf("%s.%s.%s.%s.\n", name->host, name->service, name->proto, name->domain); } else if (type == MDNS_TYPE_SRV) { - if (!_mdns_parse_fqdn(data, data_ptr + MDNS_SRV_FQDN_OFFSET, name)) { + if (!_mdns_parse_fqdn(data, data_ptr + MDNS_SRV_FQDN_OFFSET, name, len)) { _mdns_dbg_printf("ERROR: parse SRV\n"); continue; } @@ -5807,7 +5824,7 @@ void mdns_debug_packet(const uint8_t * data, size_t len) _mdns_dbg_printf(IPSTR "\n", IP2STR(&ip)); } else if (type == MDNS_TYPE_NSEC) { const uint8_t * old_ptr = data_ptr; - const uint8_t * new_ptr = _mdns_parse_fqdn(data, data_ptr, name); + const uint8_t * new_ptr = _mdns_parse_fqdn(data, data_ptr, name, len); if (new_ptr) { _mdns_dbg_printf("%s.%s.%s.%s. ", name->host, name->service, name->proto, name->domain); size_t diff = new_ptr - old_ptr; diff --git a/examples/protocols/mdns/mdns_example_test.py b/examples/protocols/mdns/mdns_example_test.py index 20e6bad040..fbd062a435 100644 --- a/examples/protocols/mdns/mdns_example_test.py +++ b/examples/protocols/mdns/mdns_example_test.py @@ -91,6 +91,7 @@ def mdns_server(esp_host): console_log('Received query: {} '.format(dns.__repr__())) sock.sendto(get_dns_answer_to_mdns_lwip(TESTER_NAME_LWIP, dns.id), addr) if len(dns.an) > 0 and dns.an[0].type == dpkt.dns.DNS_A: + console_log('Received answer from {}'.format(dns.an[0].name)) if dns.an[0].name == esp_host + u'.local': console_log('Received answer to esp32-mdns query: {}'.format(dns.__repr__())) esp_answered.set()