Hi all, Please find in attachment 10 patches to cover the following new topic in HAProxy:
1. a new DNS parser, which stores the DNS response into a DNS structure, instead of manipulating a buffer. => it doesn't add any feature by itself, but it will make DNS consumer tasks much easier when using DNS responses 2. when the DNS response finishes with a CNAME, now HAProxy sends a new query, changing the query type (from AAAA to A or A to AAAA) I heavily tested the code, but I'd like more people to test it in their own environment. We can now move forward on the next big development: filling servers in a backend based on records read in a DNS responses. Conrad: I have a quick and dirty and not finished patch to read and store SRV records. If you want to use it for your own dev, please let me know. Baptiste
From 2d196c70952be351508e3ee154d6c57d5cefee2e Mon Sep 17 00:00:00 2001 From: Baptiste Assmann <[email protected]> Date: Mon, 18 Apr 2016 19:42:57 +0200 Subject: [PATCH 01/11] CLEANUP/MINOR dns: comment do not follow up code update The loop comment is not appropriate anymore and needed to be updated according to the code. backport: no --- src/dns.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dns.c b/src/dns.c index c76637f..b9dce6b 100644 --- a/src/dns.c +++ b/src/dns.c @@ -725,7 +725,7 @@ int dns_get_ip_from_response(unsigned char *resp, unsigned char *resp_end, /* move forward data_len for analyzing next record in the response */ reader += data_len; } /* switch (record type) */ - } /* for i 0 to ancount */ + } /* list for each record entries */ /* Select an IP regarding configuration preference. * Top priority is the prefered network ip version, -- 1.9.1
From 83e6c3f60ade30a175b40b17f312fbf1e5b5aae2 Mon Sep 17 00:00:00 2001 From: Baptiste Assmann <[email protected]> Date: Sat, 26 Mar 2016 14:12:50 +0100 Subject: [PATCH 02/11] MINOR: chunk: new strncat function Purpose of this function is to append data to the end of a chunk when we know only the pointer to the beginning of the string and the string length. --- include/common/chunk.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/include/common/chunk.h b/include/common/chunk.h index aac5282..205523c 100644 --- a/include/common/chunk.h +++ b/include/common/chunk.h @@ -120,6 +120,19 @@ static inline int chunk_strcat(struct chunk *chk, const char *str) return 1; } +/* appends <nb> characters from str after <chk>. + * Returns 0 in case of failure. + */ +static inline int chunk_strncat(struct chunk *chk, const char *str, int nb) +{ + if (unlikely(chk->len < 0 || chk->len + nb >= chk->size)) + return 0; + + memcpy(chk->str + chk->len, str, nb); + chk->len += nb; + return 1; +} + /* Adds a trailing zero to the current chunk and returns the pointer to the * following part. The purpose is to be able to use a chunk as a series of * short independant strings with chunk_* functions, which do not need to be -- 1.9.1
From 1a8a094fcdc40bc10200ed4d036a5e553904b4f3 Mon Sep 17 00:00:00 2001 From: Baptiste Assmann <[email protected]> Date: Sat, 26 Mar 2016 15:09:48 +0100 Subject: [PATCH 03/11] MINOR: dns: wrong DNS_MAX_UDP_MESSAGE value Current implementation of HAProxy's DNS resolution expect only 512 bytes of data in the response. Update DNS_MAX_UDP_MESSAGE to match this. Backport: can be backported to 1.6 --- include/types/dns.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/types/dns.h b/include/types/dns.h index 1b240fa..19c6edc 100644 --- a/include/types/dns.h +++ b/include/types/dns.h @@ -30,7 +30,7 @@ */ #define DNS_MAX_LABEL_SIZE 63 #define DNS_MAX_NAME_SIZE 255 -#define DNS_MAX_UDP_MESSAGE 4096 +#define DNS_MAX_UDP_MESSAGE 512 /* DNS error messages */ #define DNS_TOO_LONG_FQDN "hostname too long" -- 1.9.1
From adac8d19d888cf63315290f71ccfe0dcfd05b80d Mon Sep 17 00:00:00 2001 From: Baptiste Assmann <[email protected]> Date: Wed, 9 Dec 2015 14:02:01 +0100 Subject: [PATCH 04/11] MINOR: dns: new MAX values DNS_MIN_RECORD_SIZE: minimal size of a DNS record DNS_MAX_QUERY_RECORDS: maximum number of query records we allow. For now, we send one DNS query per request. DNS_MAX_ANSWER_RECORDS: maximum number of records we may found in a response WIP dns: new MAX values --- include/types/dns.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/include/types/dns.h b/include/types/dns.h index 19c6edc..01d42f9 100644 --- a/include/types/dns.h +++ b/include/types/dns.h @@ -32,6 +32,16 @@ #define DNS_MAX_NAME_SIZE 255 #define DNS_MAX_UDP_MESSAGE 512 +/* DNS minimun record size: 1 char + 1 NULL + type + class */ +#define DNS_MIN_RECORD_SIZE ( 1 + 1 + 2 + 2 ) + +/* maximum number of query records in a DNS response + * For now, we allow only one */ +#define DNS_MAX_QUERY_RECORDS 1 + +/* maximum number of answer record in a DNS response */ +#define DNS_MAX_ANSWER_RECORDS ((DNS_MAX_UDP_MESSAGE - DNS_HEADER_SIZE) / DNS_MIN_RECORD_SIZE) + /* DNS error messages */ #define DNS_TOO_LONG_FQDN "hostname too long" #define DNS_LABEL_TOO_LONG "one label too long" -- 1.9.1
From 5c060839107daeb55c061c721ba9924159e12ee1 Mon Sep 17 00:00:00 2001 From: Baptiste Assmann <[email protected]> Date: Tue, 21 Jul 2015 15:34:51 +0200 Subject: [PATCH 05/11] MINOR: dns: new macro to compute DNS header size macro to compute in a simple way the size of the dns_header structure. Make the code more readable were used. --- include/types/dns.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/types/dns.h b/include/types/dns.h index 01d42f9..994dec3 100644 --- a/include/types/dns.h +++ b/include/types/dns.h @@ -70,6 +70,9 @@ */ #define SRV_MAX_PREF_NET 5 +/* DNS header size */ +#define DNS_HEADER_SIZE sizeof(struct dns_header) + /* DNS request or response header structure */ struct dns_header { uint16_t id; -- 1.9.1
From 99e2fa5764946bd4d5346ebce1d0db35f3e65b80 Mon Sep 17 00:00:00 2001 From: Baptiste Assmann <[email protected]> Date: Tue, 21 Jul 2015 15:36:24 +0200 Subject: [PATCH 06/11] MINOR: dns: new DNS structures to store received packets struct dns_query_item: describes a DNS query record struct dns_answer_item: describes a DNS answer record struct dns_response_packet: describes a DNS response packet --- include/types/dns.h | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/include/types/dns.h b/include/types/dns.h index 994dec3..03ffdc1 100644 --- a/include/types/dns.h +++ b/include/types/dns.h @@ -89,6 +89,36 @@ struct dns_question { unsigned short qclass; /* query class */ }; +struct dns_query_item { + struct list list; + char name[DNS_MAX_NAME_SIZE]; /* query name */ + unsigned short type; /* question type */ + unsigned short class; /* query class */ +}; + +struct dns_answer_item { + struct list list; + char *name; /* answer name + * For SRV type, name also includes service + * and protocol value */ + int16_t type; /* question type */ + int16_t class; /* query class */ + int32_t ttl; /* response TTL */ + int16_t priority; /* SRV type priority */ + int16_t weight; /* SRV type weight */ + int16_t port; /* SRV type port */ + int16_t data_len; /* number of bytes in target below */ + struct sockaddr address; /* IPv4 or IPv6, network format */ + char *target; /* Response data: SRV or CNAME type target */ +}; + +struct dns_response_packet { + struct dns_header header; + struct list query_list; + struct list answer_list; + /* authority and additional_information ignored for now */ +}; + /* * resolvers section and parameters. It is linked to the name servers * servers points to it. -- 1.9.1
From cbc781e5815b6e9f9a8af6e59b68f00b42ba471f Mon Sep 17 00:00:00 2001 From: Baptiste Assmann <[email protected]> Date: Sat, 14 May 2016 11:26:22 +0200 Subject: [PATCH 07/11] MEDIUM: dns: new DNS response parser New DNS response parser function which turn the DNS response from a network buffer into a DNS structure, much easier for later analysis by upper layer. Memory is pre-allocated at start-up in a chunk dedicated to DNS response store. New error code to report a wrong number of queries in a DNS response. --- include/proto/dns.h | 4 +- include/proto/server.h | 2 +- include/types/dns.h | 9 +- src/dns.c | 524 ++++++++++++++++++++++++++++--------------------- src/server.c | 6 +- 5 files changed, 311 insertions(+), 234 deletions(-) diff --git a/include/proto/dns.h b/include/proto/dns.h index 170eefa..c62834f 100644 --- a/include/proto/dns.h +++ b/include/proto/dns.h @@ -32,8 +32,8 @@ int dns_build_query(int query_id, int query_type, char *hostname_dn, int hostnam struct task *dns_process_resolve(struct task *t); int dns_init_resolvers(void); uint16_t dns_rnd16(void); -int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, char *dn_name, int dn_name_len); -int dns_get_ip_from_response(unsigned char *resp, unsigned char *resp_end, +int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, struct dns_response_packet *dns_p); +int dns_get_ip_from_response(struct dns_response_packet *dns_p, struct dns_resolution *resol, void *currentip, short currentip_sin_family, void **newip, short *newip_sin_family); diff --git a/include/proto/server.h b/include/proto/server.h index ee45f63..d79ee2f 100644 --- a/include/proto/server.h +++ b/include/proto/server.h @@ -47,7 +47,7 @@ void apply_server_state(void); /* functions related to server name resolution */ int snr_update_srv_status(struct server *s); -int snr_resolution_cb(struct dns_resolution *resolution, struct dns_nameserver *nameserver, unsigned char *response, int response_len); +int snr_resolution_cb(struct dns_resolution *resolution, struct dns_nameserver *nameserver, struct dns_response_packet *dns_p); int snr_resolution_error_cb(struct dns_resolution *resolution, int error_code); /* increase the number of cumulated connections on the designated server */ diff --git a/include/types/dns.h b/include/types/dns.h index 03ffdc1..5d6b5a1 100644 --- a/include/types/dns.h +++ b/include/types/dns.h @@ -42,6 +42,12 @@ /* maximum number of answer record in a DNS response */ #define DNS_MAX_ANSWER_RECORDS ((DNS_MAX_UDP_MESSAGE - DNS_HEADER_SIZE) / DNS_MIN_RECORD_SIZE) +/* size of dns_buffer used to store responses from the buffer + * dns_buffer is used to store data collected from records found in a response. + * Before using it, caller will always check that there is at least DNS_MAX_NAME_SIZE bytes + * available */ +#define DNS_ANALYZE_BUFFER_SIZE DNS_MAX_UDP_MESSAGE + DNS_MAX_NAME_SIZE + /* DNS error messages */ #define DNS_TOO_LONG_FQDN "hostname too long" #define DNS_LABEL_TOO_LONG "one label too long" @@ -204,7 +210,7 @@ struct dns_resolution { struct list list; /* resolution list */ struct dns_resolvers *resolvers; /* resolvers section associated to this resolution */ void *requester; /* owner of this name resolution */ - int (*requester_cb)(struct dns_resolution *, struct dns_nameserver *, unsigned char *, int); + int (*requester_cb)(struct dns_resolution *, struct dns_nameserver *, struct dns_response_packet *); /* requester callback for valid response */ int (*requester_error_cb)(struct dns_resolution *, int); /* requester callback, for error management */ @@ -256,6 +262,7 @@ enum { DNS_RESP_TIMEOUT, /* DNS server has not answered in time */ DNS_RESP_TRUNCATED, /* DNS response is truncated */ DNS_RESP_NO_EXPECTED_RECORD, /* No expected records were found in the response */ + DNS_RESP_QUERY_COUNT_ERROR, /* we did not get the expected number of queries in the response */ }; /* return codes after searching an IP in a DNS response buffer, using a family preference */ diff --git a/src/dns.c b/src/dns.c index b9dce6b..7d5ab00 100644 --- a/src/dns.c +++ b/src/dns.c @@ -37,6 +37,17 @@ struct list dns_resolvers = LIST_HEAD_INIT(dns_resolvers); struct dns_resolution *resolution = NULL; +/* + * pre-allocated memory for maximum record names in a DNS response + * Each name is DNS_MAX_NAME_SIZE, we add 1 for the NULL character + * + * WARNING: this is not thread safe... + */ +struct dns_response_packet dns_response; +struct chunk dns_trash = { }; +struct dns_query_item dns_query_records[DNS_MAX_QUERY_RECORDS]; +struct dns_answer_item dns_answer_records[DNS_MAX_ANSWER_RECORDS]; + static int64_t dns_query_id_seed; /* random seed */ /* proto_udp callback functions for a DNS resolution */ @@ -124,11 +135,13 @@ void dns_resolve_recv(struct dgram_conn *dgram) struct dns_nameserver *nameserver; struct dns_resolvers *resolvers; struct dns_resolution *resolution; + struct dns_query_item *query; unsigned char buf[DNS_MAX_UDP_MESSAGE + 1]; unsigned char *bufend; int fd, buflen, ret; unsigned short query_id; struct eb32_node *eb; + struct dns_response_packet *dns_p = &dns_response; fd = dgram->t.sock.fd; @@ -187,12 +200,12 @@ void dns_resolve_recv(struct dgram_conn *dgram) /* number of responses received */ resolution->nb_responses += 1; - ret = dns_validate_dns_response(buf, bufend, resolution->hostname_dn, resolution->hostname_dn_len); + ret = dns_validate_dns_response(buf, bufend, dns_p); /* treat only errors */ switch (ret) { + case DNS_RESP_QUERY_COUNT_ERROR: case DNS_RESP_INVALID: - case DNS_RESP_WRONG_NAME: nameserver->counters.invalid += 1; resolution->requester_error_cb(resolution, DNS_RESP_INVALID); continue; @@ -233,8 +246,18 @@ void dns_resolve_recv(struct dgram_conn *dgram) continue; } + /* Now let's check the query's dname corresponds to the one we sent. + * We can check only the first query of the list. We send one query at a time + * so we get one query in the response */ + query = LIST_NEXT(&dns_p->query_list, struct dns_query_item *, list); + if (query && memcmp(query->name, resolution->hostname_dn, resolution->hostname_dn_len) != 0) { + nameserver->counters.other += 1; + resolution->requester_error_cb(resolution, DNS_RESP_WRONG_NAME); + continue; + } + nameserver->counters.valid += 1; - resolution->requester_cb(resolution, nameserver, buf, buflen); + resolution->requester_cb(resolution, nameserver, dns_p); } } @@ -332,35 +355,117 @@ void dns_update_resolvers_timeout(struct dns_resolvers *resolvers) } /* + * Analyse, re-build and copy the name <name> from the DNS response packet <buffer>. + * <name> must point to the 'data_len' information or pointer 'c0' for compressed data. + * The result is copied into <dest>, ensuring we don't overflow using <dest_len> + * Returns the number of bytes the caller can move forward. If 0 it means an error occured + * while parsing the name. + * <offset> is the number of bytes the caller could move forward. + */ +int dns_read_name(unsigned char *buffer, unsigned char *bufend, unsigned char *name, char *destination, int dest_len, int *offset) +{ + int nb_bytes = 0, n = 0; + int label_len; + unsigned char *reader = name; + char *dest = destination; + + while (1) { + /* name compression is in use */ + if ((*reader & 0xc0) == 0xc0) { + /* a pointer must point BEFORE current position */ + if ((buffer + reader[1]) > reader) { + goto out_error; + } + + n = dns_read_name(buffer, bufend, buffer + reader[1], dest, dest_len - nb_bytes, offset); + if (n == 0) + goto out_error; + + dest += n; + nb_bytes += n; + goto out; + } + + label_len = *reader; + if (label_len == 0) + goto out; + /* Check if: + * - we won't read outside the buffer + * - there is enough place in the destination + */ + if ((reader + label_len >= bufend) || (nb_bytes + label_len >= dest_len)) + goto out_error; + + /* +1 to take label len + label string */ + label_len += 1; + + memcpy(dest, reader, label_len); + + dest += label_len; + nb_bytes += label_len; + reader += label_len; + } + + out: + /* offset computation: + * parse from <name> until finding either NULL or a pointer "c0xx" + */ + reader = name; + *offset = 0; + while (reader < bufend) { + if ((reader[0] & 0xc0) == 0xc0) { + *offset += 2; + break; + } + else if (*reader == 0) { + *offset += 1; + break; + } + *offset += 1; + ++reader; + } + + return nb_bytes; + + out_error: + return 0; +} + +/* * Function to validate that the buffer DNS response provided in <resp> and * finishing before <bufend> is valid from a DNS protocol point of view. - * The caller can also ask the function to check if the response contains data - * for a domain name <dn_name> whose length is <dn_name_len> returns one of the - * DNS_RESP_* code. + * + * The result is stored in the structured pointed by <dns_p>. + * It's up to the caller to allocate memory for <dns_p>. + * + * This function returns one of the DNS_RESP_* code to indicate the type of + * error found. */ -int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, char *dn_name, int dn_name_len) +int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, struct dns_response_packet *dns_p) { - unsigned char *reader, *cname, *ptr; - int i, len, flags, type, ancount, cnamelen, expected_record; + unsigned char *reader; + char *previous_dname, tmpname[DNS_MAX_NAME_SIZE]; + int len, flags, offset, ret; + int dns_query_record_id, dns_answer_record_id; + struct dns_query_item *dns_query; + struct dns_answer_item *dns_answer_record; reader = resp; - cname = NULL; - cnamelen = 0; len = 0; - expected_record = 0; /* flag to report if at least one expected record type is found in the response. - * For now, only records containing an IP address (A and AAAA) are - * considered as expected. - * Later, this function may be updated to let the caller decide what type - * of record is expected to consider the response as valid. (SRV or TXT types) - */ - - /* move forward 2 bytes for the query id */ - reader += 2; - if (reader >= bufend) + previous_dname = NULL; + + /* initialization of local buffer */ + memset(dns_p, '\0', sizeof(struct dns_response_packet)); + chunk_reset(&dns_trash); + + /* query id */ + if (reader + 2 >= bufend) return DNS_RESP_INVALID; + dns_p->header.id = reader[0] * 256 + reader[1]; + reader += 2; /* - * flags are stored over 2 bytes + * flags and rcode are stored over 2 bytes * First byte contains: * - response flag (1 bit) * - opcode (4 bits) @@ -387,196 +492,215 @@ int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, char * /* move forward 2 bytes for flags */ reader += 2; - if (reader >= bufend) - return DNS_RESP_INVALID; - /* move forward 2 bytes for question count */ - reader += 2; - if (reader >= bufend) + /* 2 bytes for question count */ + if (reader + 2 >= bufend) return DNS_RESP_INVALID; - - /* analyzing answer count */ - if (reader + 2 > bufend) + dns_p->header.qdcount = reader[0] * 256 + reader[1]; + /* (for now) we send one query only, so we expect only one in the response too */ + if (dns_p->header.qdcount != 1) + return DNS_RESP_QUERY_COUNT_ERROR; + if (dns_p->header.qdcount > DNS_MAX_QUERY_RECORDS) return DNS_RESP_INVALID; - ancount = reader[0] * 256 + reader[1]; + reader += 2; - if (ancount == 0) + /* 2 bytes for answer count */ + if (reader + 2 >= bufend) + return DNS_RESP_INVALID; + dns_p->header.ancount = reader[0] * 256 + reader[1]; + if (dns_p->header.ancount == 0) return DNS_RESP_ANCOUNT_ZERO; - - /* move forward 2 bytes for answer count */ - reader += 2; - if (reader >= bufend) + /* check if too many records are announced */ + if (dns_p->header.ancount > DNS_MAX_ANSWER_RECORDS) return DNS_RESP_INVALID; + reader += 2; - /* move forward 4 bytes authority and additional count */ - reader += 4; - if (reader >= bufend) + /* 2 bytes authority count */ + if (reader + 2 >= bufend) return DNS_RESP_INVALID; + dns_p->header.nscount = reader[0] * 256 + reader[1]; + reader += 2; - /* check if the name can stand in response */ - if (dn_name && ((reader + dn_name_len + 1) > bufend)) + /* 2 bytes additional count */ + if (reader + 2 >= bufend) return DNS_RESP_INVALID; + dns_p->header.arcount = reader[0] * 256 + reader[1]; + reader += 2; - /* check hostname */ - if (dn_name && (memcmp(reader, dn_name, dn_name_len) != 0)) - return DNS_RESP_WRONG_NAME; + /* parsing dns queries */ + LIST_INIT(&dns_p->query_list); + for (dns_query_record_id = 0; dns_query_record_id < dns_p->header.qdcount; dns_query_record_id++) { + /* use next pre-allocated dns_query_item after ensuring there is + * still one available. + * It's then added to our packet query list. + */ + if (dns_query_record_id > DNS_MAX_QUERY_RECORDS) + return DNS_RESP_INVALID; + dns_query = &dns_query_records[dns_query_record_id]; + LIST_ADDQ(&dns_p->query_list, &dns_query->list); - /* move forward hostname len bytes + 1 for NULL byte */ - if (dn_name) { - reader = reader + dn_name_len + 1; - } - else { - ptr = reader; - while (*ptr) { - ptr++; - if (ptr >= bufend) - return DNS_RESP_INVALID; - } - reader = ptr + 1; - } + /* name is a NULL terminated string in our case, since we have + * one query per response and the first one can't be compressed + * (using the 0x0c format) + */ + offset = 0; + len = dns_read_name(resp, bufend, reader, dns_query->name, DNS_MAX_NAME_SIZE, &offset); - /* move forward 4 bytes for question type and question class */ - reader += 4; - if (reader >= bufend) - return DNS_RESP_INVALID; + if (len == 0) + return DNS_RESP_INVALID; + + reader += offset; + previous_dname = dns_query->name; + + /* move forward 2 bytes for question type */ + if (reader + 2 >= bufend) + return DNS_RESP_INVALID; + dns_query->type = reader[0] * 256 + reader[1]; + reader += 2; + + /* move forward 2 bytes for question class */ + if (reader + 2 >= bufend) + return DNS_RESP_INVALID; + dns_query->class = reader[0] * 256 + reader[1]; + reader += 2; + } /* now parsing response records */ - for (i = 1; i <= ancount; i++) { + LIST_INIT(&dns_p->answer_list); + for (dns_answer_record_id = 0; dns_answer_record_id < dns_p->header.ancount; dns_answer_record_id++) { if (reader >= bufend) return DNS_RESP_INVALID; - /* - * name can be a pointer, so move forward reader cursor accordingly - * if 1st byte is '11XXXXXX', it means name is a pointer - * and 2nd byte gives the offset from resp where the hostname can - * be found - */ - if ((*reader & 0xc0) == 0xc0) { - /* - * pointer, hostname can be found at resp + *(reader + 1) - */ - if (reader + 1 > bufend) - return DNS_RESP_INVALID; + /* pull next response record from the list, if still one available, then add it + * to the record list */ + if (dns_answer_record_id > DNS_MAX_ANSWER_RECORDS) + return DNS_RESP_INVALID; + dns_answer_record = &dns_answer_records[dns_answer_record_id]; + LIST_ADDQ(&dns_p->answer_list, &dns_answer_record->list); - ptr = resp + *(reader + 1); + offset = 0; + len = dns_read_name(resp, bufend, reader, tmpname, DNS_MAX_NAME_SIZE, &offset); - /* check if the pointer points inside the buffer */ - if (ptr >= bufend) + if (len == 0) + return DNS_RESP_INVALID; + + /* check if the current record dname is valid. + * previous_dname points either to queried dname or last CNAME target + */ + if (memcmp(previous_dname, tmpname, len) != 0) { + if (dns_answer_record_id == 0) { + /* first record, means a mismatch issue between queried dname + * and dname found in the first record */ return DNS_RESP_INVALID; - } - else { - /* - * name is a string which starts at first byte - * checking against last cname when recursing through the response - */ - /* look for the end of the string and ensure it's in the buffer */ - ptr = reader; - len = 0; - while (*ptr) { - ++len; - ++ptr; - if (ptr >= bufend) - return DNS_RESP_INVALID; + } else { + /* if not the first record, this means we have a CNAME resolution + * error */ + return DNS_RESP_CNAME_ERROR; } - /* if cname is set, it means a CNAME recursion is in progress */ - ptr = reader; } - /* ptr now points to the name */ - if ((*reader & 0xc0) != 0xc0) { - /* if cname is set, it means a CNAME recursion is in progress */ - if (cname) { - /* check if the name can stand in response */ - if ((reader + cnamelen) > bufend) - return DNS_RESP_INVALID; - /* compare cname and current name */ - if (memcmp(ptr, cname, cnamelen) != 0) - return DNS_RESP_CNAME_ERROR; - - cname = reader; - cnamelen = dns_str_to_dn_label_len((const char *)cname); - - /* move forward cnamelen bytes + NULL byte */ - reader += (cnamelen + 1); - } - /* compare server hostname to current name */ - else if (dn_name) { - /* check if the name can stand in response */ - if ((reader + dn_name_len) > bufend) - return DNS_RESP_INVALID; - if (memcmp(ptr, dn_name, dn_name_len) != 0) - return DNS_RESP_WRONG_NAME; + dns_answer_record->name = chunk_newstr(&dns_trash); + if (dns_answer_record->name == NULL) + return DNS_RESP_INVALID; - reader += (dn_name_len + 1); - } - else { - reader += (len + 1); - } - } - else { - /* shortname in progress */ - /* move forward 2 bytes for information pointer and address pointer */ - reader += 2; - } + ret = chunk_strncat(&dns_trash, tmpname, len); + if (ret == 0) + return DNS_RESP_INVALID; + reader += offset; if (reader >= bufend) return DNS_RESP_INVALID; - /* - * we know the record is either for our server hostname - * or a valid CNAME in a crecursion - */ + if (reader >= bufend) + return DNS_RESP_INVALID; - /* now reading record type (A, AAAA, CNAME, etc...) */ + /* 2 bytes for record type (A, AAAA, CNAME, etc...) */ if (reader + 2 > bufend) return DNS_RESP_INVALID; - type = reader[0] * 256 + reader[1]; + dns_answer_record->type = reader[0] * 256 + reader[1]; + reader += 2; - /* move forward 2 bytes for type (2) */ + /* 2 bytes for class (2) */ + if (reader + 2 > bufend) + return DNS_RESP_INVALID; + dns_answer_record->class = reader[0] * 256 + reader[1]; reader += 2; - /* move forward 6 bytes for class (2) and ttl (4) */ - reader += 6; - if (reader >= bufend) + /* 4 bytes for ttl (4) */ + if (reader + 4 > bufend) return DNS_RESP_INVALID; + dns_answer_record->ttl = reader[0] * 16777216 + reader[1] * 65536 + + reader[2] * 256 + reader[3]; + reader += 4; /* now reading data len */ if (reader + 2 > bufend) return DNS_RESP_INVALID; - len = reader[0] * 256 + reader[1]; + dns_answer_record->data_len = reader[0] * 256 + reader[1]; /* move forward 2 bytes for data len */ reader += 2; /* analyzing record content */ - switch (type) { + switch (dns_answer_record->type) { case DNS_RTYPE_A: /* ipv4 is stored on 4 bytes */ - if (len != 4) + if (dns_answer_record->data_len != 4) return DNS_RESP_INVALID; - expected_record = 1; + dns_answer_record->address.sa_family = AF_INET; + memcpy(&(((struct sockaddr_in *)&dns_answer_record->address)->sin_addr), + reader, dns_answer_record->data_len); break; case DNS_RTYPE_CNAME: - cname = reader; - cnamelen = len; + /* check if this is the last record and update the caller about the status: + * no IP could be found and last record was a CNAME. Could be triggered + * by a wrong query type + * + * + 1 because dns_answer_record_id starts at 0 while number of answers + * is an integer and starts at 1. + */ + if (dns_answer_record_id + 1 == dns_p->header.ancount) + return DNS_RESP_CNAME_ERROR; + + offset = 0; + len = dns_read_name(resp, bufend, reader, tmpname, DNS_MAX_NAME_SIZE, &offset); + + if (len == 0) + return DNS_RESP_INVALID; + + dns_answer_record->target = chunk_newstr(&dns_trash); + if (dns_answer_record->target == NULL) + return DNS_RESP_INVALID; + + ret = chunk_strncat(&dns_trash, tmpname, len); + if (ret == 0) + return DNS_RESP_INVALID; + + previous_dname = dns_answer_record->target; + break; case DNS_RTYPE_AAAA: /* ipv6 is stored on 16 bytes */ - if (len != 16) + if (dns_answer_record->data_len != 16) return DNS_RESP_INVALID; - expected_record = 1; + dns_answer_record->address.sa_family = AF_INET6; + memcpy(&(((struct sockaddr_in6 *)&dns_answer_record->address)->sin6_addr), + reader, dns_answer_record->data_len); break; + } /* switch (record type) */ - /* move forward len for analyzing next record in the response */ - reader += len; + /* move forward dns_answer_record->data_len for analyzing next record in the response */ + reader += dns_answer_record->data_len; } /* for i 0 to ancount */ - if (expected_record == 0) - return DNS_RESP_NO_EXPECTED_RECORD; + /* let's add a last \0 to close our last string */ + ret = chunk_strncat(&dns_trash, "\0", 1); + if (ret == 0) + return DNS_RESP_INVALID; return DNS_RESP_VALID; } @@ -592,18 +716,19 @@ int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, char * * returns one of the DNS_UPD_* code */ #define DNS_MAX_IP_REC 20 -int dns_get_ip_from_response(unsigned char *resp, unsigned char *resp_end, +int dns_get_ip_from_response(struct dns_response_packet *dns_p, struct dns_resolution *resol, void *currentip, short currentip_sin_family, void **newip, short *newip_sin_family) { + struct dns_answer_item *record; int family_priority; char *dn_name; int dn_name_len; - int i, ancount, cnamelen, type, data_len, currentip_found; - unsigned char *reader, *cname, *ptr, *newip4, *newip6; + int i, cnamelen, currentip_found; + unsigned char *cname, *newip4, *newip6; struct { - unsigned char *ip; + void *ip; unsigned char type; } rec[DNS_MAX_IP_REC]; int currentip_sel; @@ -614,59 +739,19 @@ int dns_get_ip_from_response(unsigned char *resp, unsigned char *resp_end, family_priority = resol->opts->family_prio; dn_name = resol->hostname_dn; dn_name_len = resol->hostname_dn_len; - cname = *newip = newip4 = newip6 = NULL; cnamelen = currentip_found = 0; *newip_sin_family = AF_UNSPEC; - ancount = *(resp + 7); /* Assume no more than 256 answers */ - - /* bypass DNS response header */ - reader = resp + sizeof(struct dns_header); - - /* bypass DNS query section */ - /* move forward hostname len bytes + 1 for NULL byte */ - reader = reader + dn_name_len + 1; - - /* move forward 4 bytes for question type and question class */ - reader += 4; /* now parsing response records */ - for (i = 1; i <= ancount; i++) { - /* - * name can be a pointer, so move forward reader cursor accordingly - * if 1st byte is '11XXXXXX', it means name is a pointer - * and 2nd byte gives the offset from buf where the hostname can - * be found - */ - if ((*reader & 0xc0) == 0xc0) - ptr = resp + *(reader + 1); - else - ptr = reader; - + list_for_each_entry(record, &dns_response.answer_list, list) { if (cname) { - if (memcmp(ptr, cname, cnamelen)) { + if (memcmp(record->name, cname, cnamelen) != 0) { return DNS_UPD_NAME_ERROR; } } - else if (memcmp(ptr, dn_name, dn_name_len)) + else if (memcmp(record->name, dn_name, dn_name_len) != 0) { return DNS_UPD_NAME_ERROR; - - if ((*reader & 0xc0) == 0xc0) { - /* move forward 2 bytes for information pointer and address pointer */ - reader += 2; - } - else { - if (cname) { - cname = reader; - cnamelen = dns_str_to_dn_label_len((char *)cname); - - /* move forward cnamelen bytes + NULL byte */ - reader += (cnamelen + 1); - } - else { - /* move forward dn_name_len bytes + NULL byte */ - reader += (dn_name_len + 1); - } } /* @@ -674,56 +759,32 @@ int dns_get_ip_from_response(unsigned char *resp, unsigned char *resp_end, * or a valid CNAME in a crecursion */ - /* now reading record type (A, AAAA, CNAME, etc...) */ - type = reader[0] * 256 + reader[1]; - - /* move forward 2 bytes for type (2) */ - reader += 2; - - /* move forward 6 bytes for class (2) and ttl (4) */ - reader += 6; - - /* now reading data len */ - data_len = reader[0] * 256 + reader[1]; - - /* move forward 2 bytes for data len */ - reader += 2; - /* analyzing record content */ - switch (type) { + switch (record->type) { case DNS_RTYPE_A: /* Store IPv4, only if some room is avalaible. */ if (rec_nb < DNS_MAX_IP_REC) { - rec[rec_nb].ip = reader; + rec[rec_nb].ip = &(((struct sockaddr_in *)&record->address)->sin_addr); rec[rec_nb].type = AF_INET; rec_nb++; } - /* move forward data_len for analyzing next record in the response */ - reader += data_len; break; case DNS_RTYPE_CNAME: - cname = reader; - cnamelen = data_len; + cname = record->target; + cnamelen = record->data_len; - reader += data_len; break; case DNS_RTYPE_AAAA: /* Store IPv6, only if some room is avalaible. */ if (rec_nb < DNS_MAX_IP_REC) { - rec[rec_nb].ip = reader; + rec[rec_nb].ip = &(((struct sockaddr_in6 *)&record->address)->sin6_addr); rec[rec_nb].type = AF_INET6; rec_nb++; } - /* move forward data_len for analyzing next record in the response */ - reader += data_len; break; - default: - /* not supported record type */ - /* move forward data_len for analyzing next record in the response */ - reader += data_len; } /* switch (record type) */ } /* list for each record entries */ @@ -886,8 +947,19 @@ int dns_init_resolvers(void) struct dns_nameserver *curnameserver; struct dgram_conn *dgram; struct task *t; + char *dns_trash_str; int fd; + dns_trash_str = malloc(global.tune.bufsize); + if (dns_trash_str == NULL) { + Alert("Starting [%s] resolvers: out of memory.\n", curr_resolvers->id); + return 0; + } + + /* allocate memory for the dns_trash buffer used to temporarily store + * the records of the received response */ + chunk_init(&dns_trash, dns_trash_str, global.tune.bufsize); + /* give a first random value to our dns query_id seed */ dns_query_id_seed = random(); diff --git a/src/server.c b/src/server.c index 62c08b0..17ec074 100644 --- a/src/server.c +++ b/src/server.c @@ -2664,17 +2664,15 @@ int snr_update_srv_status(struct server *s) * 0 on error * 1 when no error or safe ignore */ -int snr_resolution_cb(struct dns_resolution *resolution, struct dns_nameserver *nameserver, unsigned char *response, int response_len) +int snr_resolution_cb(struct dns_resolution *resolution, struct dns_nameserver *nameserver, struct dns_response_packet *dns_p) { struct server *s; void *serverip, *firstip; short server_sin_family, firstip_sin_family; - unsigned char *response_end; int ret; struct chunk *chk = get_trash_chunk(); /* initializing variables */ - response_end = response + response_len; /* pointer to mark the end of the response */ firstip = NULL; /* pointer to the first valid response found */ /* it will be used as the new IP if a change is required */ firstip_sin_family = AF_UNSPEC; @@ -2698,7 +2696,7 @@ int snr_resolution_cb(struct dns_resolution *resolution, struct dns_nameserver * goto invalid; } - ret = dns_get_ip_from_response(response, response_end, resolution, + ret = dns_get_ip_from_response(dns_p, resolution, serverip, server_sin_family, &firstip, &firstip_sin_family); -- 1.9.1
From e52fa51cbbd706c6d3df6e31dc420fb3786c997f Mon Sep 17 00:00:00 2001 From: Baptiste Assmann <[email protected]> Date: Mon, 5 Sep 2016 08:38:57 +0200 Subject: [PATCH 08/11] MINOR: dns: query type change when last record is a CNAME DNS servers don't return A or AAAA record if the query points to a CNAME not resolving to the right type. We know it because the last record of the response is a CNAME. We can trigger a new query, switching to a new query type, handled by the layer above. --- src/server.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/server.c b/src/server.c index 17ec074..af7a314 100644 --- a/src/server.c +++ b/src/server.c @@ -2808,6 +2808,7 @@ int snr_resolution_error_cb(struct dns_resolution *resolution, int error_code) case DNS_RESP_TRUNCATED: case DNS_RESP_ERROR: case DNS_RESP_NO_EXPECTED_RECORD: + case DNS_RESP_CNAME_ERROR: res_preferred_afinet = resolution->opts->family_prio == AF_INET && resolution->query_type == DNS_RTYPE_A; res_preferred_afinet6 = resolution->opts->family_prio == AF_INET6 && resolution->query_type == DNS_RTYPE_AAAA; @@ -2868,9 +2869,6 @@ int snr_resolution_error_cb(struct dns_resolution *resolution, int error_code) } break; - case DNS_RESP_CNAME_ERROR: - break; - case DNS_RESP_TIMEOUT: if (resolution->status != RSLV_STATUS_TIMEOUT) { resolution->status = RSLV_STATUS_TIMEOUT; -- 1.9.1
From 5330380ea5e7bee3d2e141a3855b65db6655ecf3 Mon Sep 17 00:00:00 2001 From: Baptiste Assmann <[email protected]> Date: Sun, 17 Apr 2016 22:43:26 +0200 Subject: [PATCH 09/11] MINOR: dns: proper domain name validation when receiving DNS response The analyse of CNAME resolution and request's domain name was performed twice: - when validating the response buffer - when loading the right IP address from the response Now DNS response are properly loaded into a DNS response structure, we do the domain name validation when loading/validating the response in the DNS strcucture and later processing of this task is now useless. backport: no --- src/dns.c | 40 ++++++++-------------------------------- 1 file changed, 8 insertions(+), 32 deletions(-) diff --git a/src/dns.c b/src/dns.c index 7d5ab00..fc9354e 100644 --- a/src/dns.c +++ b/src/dns.c @@ -129,6 +129,7 @@ void dns_reset_resolution(struct dns_resolution *resolution) * - check if the packet requires processing (not outdated resolution) * - ensure the DNS packet received is valid and call requester's callback * - call requester's error callback if invalid response + * - check the dn_name in the packet against the one sent */ void dns_resolve_recv(struct dgram_conn *dgram) { @@ -710,8 +711,7 @@ int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, struct * If existing IP not found, return the first IP matching family_priority, * otherwise, first ip found * The following tasks are the responsibility of the caller: - * - resp contains an error free DNS response - * - the response matches the dn_name + * - <dns_p> contains an error free DNS response * For both cases above, dns_validate_dns_response is required * returns one of the DNS_UPD_* code */ @@ -723,10 +723,8 @@ int dns_get_ip_from_response(struct dns_response_packet *dns_p, { struct dns_answer_item *record; int family_priority; - char *dn_name; - int dn_name_len; - int i, cnamelen, currentip_found; - unsigned char *cname, *newip4, *newip6; + int i, currentip_found; + unsigned char *newip4, *newip6; struct { void *ip; unsigned char type; @@ -737,28 +735,12 @@ int dns_get_ip_from_response(struct dns_response_packet *dns_p, int score, max_score; family_priority = resol->opts->family_prio; - dn_name = resol->hostname_dn; - dn_name_len = resol->hostname_dn_len; - cname = *newip = newip4 = newip6 = NULL; - cnamelen = currentip_found = 0; + *newip = newip4 = newip6 = NULL; + currentip_found = 0; *newip_sin_family = AF_UNSPEC; /* now parsing response records */ list_for_each_entry(record, &dns_response.answer_list, list) { - if (cname) { - if (memcmp(record->name, cname, cnamelen) != 0) { - return DNS_UPD_NAME_ERROR; - } - } - else if (memcmp(record->name, dn_name, dn_name_len) != 0) { - return DNS_UPD_NAME_ERROR; - } - - /* - * we know the record is either for our server hostname - * or a valid CNAME in a crecursion - */ - /* analyzing record content */ switch (record->type) { case DNS_RTYPE_A: @@ -770,10 +752,9 @@ int dns_get_ip_from_response(struct dns_response_packet *dns_p, } break; + /* we're looking for IPs only. CNAME validation is done when + * parsing the response buffer for the first time */ case DNS_RTYPE_CNAME: - cname = record->target; - cnamelen = record->data_len; - break; case DNS_RTYPE_AAAA: @@ -857,11 +838,6 @@ int dns_get_ip_from_response(struct dns_response_packet *dns_p, } } - /* only CNAMEs in the response, no IP found */ - if (cname && !newip4 && !newip6) { - return DNS_UPD_CNAME; - } - /* no IP found in the response */ if (!newip4 && !newip6) { return DNS_UPD_NO_IP_FOUND; -- 1.9.1
From 6a8dd6a4f1941acc3cf6960574134c4a1fbe5ae7 Mon Sep 17 00:00:00 2001 From: Baptiste Assmann <[email protected]> Date: Mon, 5 Sep 2016 19:09:49 +0200 Subject: [PATCH 10/11] MINOR: dns: comments in types/dns.h about structures endianness To avoid issues when porting code to some architecture, we need to know the endianess the structures are currently used. This patch simply had a short notice before those structures to report endianess and ease contributor's job. --- include/types/dns.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/types/dns.h b/include/types/dns.h index 5d6b5a1..b339249 100644 --- a/include/types/dns.h +++ b/include/types/dns.h @@ -90,11 +90,13 @@ struct dns_header { } __attribute__ ((packed)); /* short structure to describe a DNS question */ +/* NOTE: big endian structure */ struct dns_question { unsigned short qtype; /* question type */ unsigned short qclass; /* query class */ }; +/* NOTE: big endian structure */ struct dns_query_item { struct list list; char name[DNS_MAX_NAME_SIZE]; /* query name */ @@ -102,6 +104,7 @@ struct dns_query_item { unsigned short class; /* query class */ }; +/* NOTE: big endian structure */ struct dns_answer_item { struct list list; char *name; /* answer name -- 1.9.1
