From 9fb46e120655ac481b2af8f865d5ae56c39b831a Mon Sep 17 00:00:00 2001 From: Simon Goldschmidt Date: Mon, 15 Sep 2014 21:50:41 +0200 Subject: [PATCH] added source port randomization to make the DNS client more robust (see bug #43144) --- CHANGELOG | 4 + src/core/dns.c | 258 ++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 203 insertions(+), 59 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 133723de..2b9e4687 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -6,6 +6,10 @@ HISTORY ++ New features: + 2014-09-15: Simon Goldschmidt + * dns.c: added source port randomization to make the DNS client more robust + (see bug #43144) + 2013-09-02: Simon Goldschmidt * arch.h and many other files: added optional macros PACK_STRUCT_FLD_8() and PACK_STRUCT_FLD_S() to prevent gcc 4 from warning about struct members that diff --git a/src/core/dns.c b/src/core/dns.c index dc20227b..9d545101 100644 --- a/src/core/dns.c +++ b/src/core/dns.c @@ -88,14 +88,15 @@ /* A list of DNS security features follows */ #define LWIP_DNS_SECURE_RAND_XID 1 #define LWIP_DNS_SECURE_NO_MULTIPLE_OUTSTANDING 2 +#define LWIP_DNS_SECURE_RAND_SRC_PORT 4 /** Use all DNS security features by default. * This is overridable but should only be needed by very small targets * or when using against non standard DNS servers. */ #ifndef LWIP_DNS_SECURE -#define LWIP_DNS_SECURE (LWIP_DNS_SECURE_RAND_XID | LWIP_DNS_SECURE_NO_MULTIPLE_OUTSTANDING) +#define LWIP_DNS_SECURE (LWIP_DNS_SECURE_RAND_XID | LWIP_DNS_SECURE_NO_MULTIPLE_OUTSTANDING | LWIP_DNS_SECURE_RAND_SRC_PORT) #endif -/** Random generator function to create random TXIDs for queries */ +/** Random generator function to create random TXIDs and source ports for queries */ #ifndef DNS_RAND_TXID #if ((LWIP_DNS_SECURE & LWIP_DNS_SECURE_RAND_XID) != 0) #define DNS_RAND_TXID LWIP_RAND @@ -105,6 +106,11 @@ static u16_t dns_txid; #endif #endif +/** Limits the source port to be >= 1024 by default */ +#ifndef DNS_PORT_ALLOWED +#define DNS_PORT_ALLOWED(port) ((port) >= 1024) +#endif + /** DNS server IP address */ #ifndef DNS_SERVER_ADDRESS #define DNS_SERVER_ADDRESS(ipaddr) (ip4_addr_set_u32(ipaddr, ipaddr_addr("208.67.222.222"))) /* resolver1.opendns.com */ @@ -139,6 +145,19 @@ static u16_t dns_txid; #define DNS_MAX_REQUESTS DNS_TABLE_SIZE #endif +/* The number of UDP source ports used in parallel */ +#if ((LWIP_DNS_SECURE & LWIP_DNS_SECURE_RAND_SRC_PORT) != 0) +#ifndef DNS_MAX_SOURCE_PORTS +#define DNS_MAX_SOURCE_PORTS DNS_MAX_REQUESTS +#endif +#else +#ifdef DNS_MAX_SOURCE_PORTS +#undef DNS_MAX_SOURCE_PORTS +#endif +#define DNS_MAX_SOURCE_PORTS 1 +#endif + + /* DNS protocol flags */ #define DNS_FLAG1_RESPONSE 0x80 #define DNS_FLAG1_OPCODE_STATUS 0x10 @@ -204,16 +223,18 @@ struct dns_answer { /** DNS table entry */ struct dns_table_entry { + u32_t ttl; + ip_addr_t ipaddr; + u16_t txid; u8_t state; - u8_t numdns; + u8_t server_idx; u8_t tmr; u8_t retries; u8_t seqno; - u8_t err; - u16_t txid; - u32_t ttl; +#if ((LWIP_DNS_SECURE & LWIP_DNS_SECURE_RAND_SRC_PORT) != 0) + u8_t pcb_idx; +#endif char name[DNS_MAX_NAME_LENGTH]; - ip_addr_t ipaddr; }; /** DNS request table entry: used when dns_gehostbyname cannot answer the @@ -264,7 +285,10 @@ static void dns_check_entries(void); *----------------------------------------------------------------------------*/ /* DNS variables */ -static struct udp_pcb *dns_pcb; +static struct udp_pcb *dns_pcbs[DNS_MAX_SOURCE_PORTS]; +#if ((LWIP_DNS_SECURE & LWIP_DNS_SECURE_RAND_SRC_PORT) != 0) +static u8_t dns_last_pcb_idx; +#endif static u8_t dns_seqno; static struct dns_table_entry dns_table[DNS_TABLE_SIZE]; static struct dns_req_entry dns_requests[DNS_MAX_REQUESTS]; @@ -329,23 +353,23 @@ dns_init() LWIP_DEBUGF(DNS_DEBUG, ("dns_init: initializing\n")); /* if dns client not yet initialized... */ - if (dns_pcb == NULL) { - dns_pcb = udp_new(); +#if ((LWIP_DNS_SECURE & LWIP_DNS_SECURE_RAND_SRC_PORT) == 0) + if (dns_pcbs[0] == NULL) { + dns_pcbs[0] = udp_new(); + LWIP_ASSERT("dns_pcbs[0] != NULL", dns_pcbs[0] != NULL); - if (dns_pcb != NULL) { - /* initialize DNS table not needed (initialized to zero since it is a - * global variable) */ - LWIP_ASSERT("For implicit initialization to work, DNS_STATE_UNUSED needs to be 0", - DNS_STATE_UNUSED == 0); + /* initialize DNS table not needed (initialized to zero since it is a + * global variable) */ + LWIP_ASSERT("For implicit initialization to work, DNS_STATE_UNUSED needs to be 0", + DNS_STATE_UNUSED == 0); - /* initialize DNS client */ - udp_bind(dns_pcb, IP_ADDR_ANY, 0); - udp_recv(dns_pcb, dns_recv, NULL); - - /* initialize default DNS primary server */ - dns_setserver(0, &dnsserver); - } + /* initialize DNS client */ + udp_bind(dns_pcbs[0], IP_ADDR_ANY, 0); + udp_recv(dns_pcbs[0], dns_recv, NULL); } +#endif + /* initialize default DNS primary server */ + dns_setserver(0, &dnsserver); #if DNS_LOCAL_HOSTLIST dns_init_local(); #endif @@ -360,9 +384,12 @@ dns_init() void dns_setserver(u8_t numdns, ip_addr_t *dnsserver) { - if ((numdns < DNS_MAX_SERVERS) && (dns_pcb != NULL) && - (dnsserver != NULL) && !ip_addr_isany(dnsserver)) { - dns_servers[numdns] = (*dnsserver); + if (numdns < DNS_MAX_SERVERS) { + if (dnsserver != NULL) { + dns_servers[numdns] = (*dnsserver); + } else { + dns_servers[numdns] = *IP_ADDR_ANY; + } } } @@ -390,10 +417,8 @@ dns_getserver(u8_t numdns) void dns_tmr(void) { - if (dns_pcb != NULL) { - LWIP_DEBUGF(DNS_DEBUG, ("dns_tmr: dns_check_entries\n")); - dns_check_entries(); - } + LWIP_DEBUGF(DNS_DEBUG, ("dns_tmr: dns_check_entries\n")); + dns_check_entries(); } #if DNS_LOCAL_HOSTLIST @@ -470,7 +495,7 @@ dns_local_removehost(const char *hostname, const ip_addr_t *addr) struct local_hostlist_entry *entry = local_hostlist_dynamic; struct local_hostlist_entry *last_entry = NULL; while (entry != NULL) { - if (((hostname == NULL) || !strcmp(entry->name, hostname)) && + if (((hostname == NULL) || !LWIP_DNS_STRICMP(entry->name, hostname)) && ((addr == NULL) || ip_addr_cmp(&entry->addr, addr))) { struct local_hostlist_entry *free_entry; if (last_entry != NULL) { @@ -636,13 +661,11 @@ dns_parse_name(char *query) /** * Send a DNS query packet. * - * @param numdns index of the DNS server in the dns_servers table - * @param name hostname to query - * @param txid transmission id for the query + * @param entry the DNS table entry for which to send a request * @return ERR_OK if packet is sent; an err_t indicating the problem otherwise */ static err_t -dns_send(u8_t numdns, const char* name, u16_t txid) +dns_send(struct dns_table_entry* entry) { err_t err; struct dns_hdr *hdr; @@ -651,11 +674,12 @@ dns_send(u8_t numdns, const char* name, u16_t txid) char *query, *nptr; const char *pHostname; u8_t n; + u8_t pcb_idx; LWIP_DEBUGF(DNS_DEBUG, ("dns_send: dns_servers[%"U16_F"] \"%s\": request\n", - (u16_t)(numdns), name)); - LWIP_ASSERT("dns server out of array", numdns < DNS_MAX_SERVERS); - LWIP_ASSERT("dns server has no IP address set", !ip_addr_isany(&dns_servers[numdns])); + (u16_t)(entry->server_idx), entry->name)); + LWIP_ASSERT("dns server out of array", entry->server_idx < DNS_MAX_SERVERS); + LWIP_ASSERT("dns server has no IP address set", !ip_addr_isany(&dns_servers[entry->server_idx])); /* if here, we have either a new query or a retry on a previous query to process */ p = pbuf_alloc(PBUF_TRANSPORT, SIZEOF_DNS_HDR + DNS_MAX_NAME_LENGTH + 1 + @@ -666,11 +690,11 @@ dns_send(u8_t numdns, const char* name, u16_t txid) /* fill dns header */ hdr = (struct dns_hdr*)p->payload; memset(hdr, 0, SIZEOF_DNS_HDR); - hdr->id = htons(txid); + hdr->id = htons(entry->txid); hdr->flags1 = DNS_FLAG1_RD; hdr->numquestions = PP_HTONS(1); query = (char*)hdr + SIZEOF_DNS_HDR; - pHostname = name; + pHostname = entry->name; --pHostname; /* convert hostname into suitable query format. */ @@ -697,11 +721,15 @@ dns_send(u8_t numdns, const char* name, u16_t txid) LWIP_ASSERT("p->tot_len >= realloc_size", p->tot_len >= realloc_size); pbuf_realloc(p, realloc_size); - /* connect to the server for faster receiving */ - udp_connect(dns_pcb, &dns_servers[numdns], DNS_SERVER_PORT); +#if ((LWIP_DNS_SECURE & LWIP_DNS_SECURE_RAND_SRC_PORT) != 0) + pcb_idx = entry->pcb_idx; +#else + pcb_idx = 0; +#endif /* send dns packet */ - LWIP_DEBUGF(DNS_DEBUG, ("sending DNS request ID %d for name \"%s\" to server %d\r\n", txid, name, numdns)); - err = udp_sendto(dns_pcb, p, &dns_servers[numdns], DNS_SERVER_PORT); + LWIP_DEBUGF(DNS_DEBUG, ("sending DNS request ID %d for name \"%s\" to server %d\r\n", + entry->txid, entry->name, entry->server_idx)); + err = udp_sendto(dns_pcbs[pcb_idx], p, &dns_servers[entry->server_idx], DNS_SERVER_PORT); /* free pbuf */ pbuf_free(p); @@ -712,6 +740,76 @@ dns_send(u8_t numdns, const char* name, u16_t txid) return err; } +#if ((LWIP_DNS_SECURE & LWIP_DNS_SECURE_RAND_SRC_PORT) != 0) +static struct udp_pcb* +dns_alloc_random_port(void) +{ + err_t err; + struct udp_pcb* ret; + + ret = udp_new(); + if (ret == NULL) { + /* out of memory, have to reuse an existing pcb */ + return NULL; + } + do { + u16_t port = DNS_RAND_TXID(); + if (!DNS_PORT_ALLOWED(port)) { + /* this port is not allowed, try again */ + err = ERR_USE; + continue; + } + err = udp_bind(ret, IP_ADDR_ANY, port); + } while(err == ERR_USE); + if ((err != ERR_OK) && (err != ERR_USE)) { + udp_remove(ret); + return NULL; + } + udp_recv(ret, dns_recv, NULL); + return ret; +} + +/** + * dns_alloc_pcb() - allocates a new pcb (or reuses an existing one) to be used + * for sending a request + * + * @return an index into dns_pcbs + */ +static u8_t +dns_alloc_pcb(void) +{ + u8_t i; + u8_t idx; + + for (i = 0; i < DNS_MAX_SOURCE_PORTS; i++) { + if (dns_pcbs[i] == NULL) { + break; + } + } + if (i < DNS_MAX_SOURCE_PORTS) { + dns_pcbs[i] = dns_alloc_random_port(); + if (dns_pcbs[i] != NULL) { + /* succeeded */ + dns_last_pcb_idx = i; + return i; + } + } + /* if we come here, creating a new UDP pcb failed, so we have to use + an already existing one */ + idx = dns_last_pcb_idx + 1; + for (i = 0; i < DNS_MAX_SOURCE_PORTS; i++) { + if (idx >= DNS_MAX_SOURCE_PORTS) { + idx = 0; + } + if (dns_pcbs[idx] != NULL) { + dns_last_pcb_idx = idx; + return idx; + } + } + return DNS_MAX_SOURCE_PORTS; +} +#endif /* ((LWIP_DNS_SECURE & LWIP_DNS_SECURE_RAND_SRC_PORT) != 0) */ + /** * dns_call_found() - call the found callback and check if there are duplicate * entries for the given hostname. If there are any, their found callback will @@ -723,8 +821,10 @@ dns_send(u8_t numdns, const char* name, u16_t txid) static void dns_call_found(u8_t idx, ip_addr_t* addr) { -#if ((LWIP_DNS_SECURE & LWIP_DNS_SECURE_NO_MULTIPLE_OUTSTANDING) != 0) u8_t i; + LWIP_UNUSED_ARG(i); + +#if ((LWIP_DNS_SECURE & LWIP_DNS_SECURE_NO_MULTIPLE_OUTSTANDING) != 0) for (i = 0; i < DNS_MAX_REQUESTS; i++) { if (dns_requests[i].found && (dns_requests[i].dns_table_idx == idx)) { (*dns_requests[i].found)(dns_table[idx].name, addr, dns_requests[i].arg); @@ -738,6 +838,27 @@ dns_call_found(u8_t idx, ip_addr_t* addr) } dns_requests[idx].found = NULL; #endif +#if ((LWIP_DNS_SECURE & LWIP_DNS_SECURE_RAND_SRC_PORT) != 0) + /* close the pcb used unless other request are using it */ + for (i = 0; i < DNS_MAX_REQUESTS; i++) { + if (i == idx) { + continue; /* only check other requests */ + } + if (dns_table[i].state == DNS_STATE_ASKING) { + if (dns_table[i].pcb_idx == dns_table[idx].pcb_idx) { + /* another request is still using the same pcb */ + dns_table[idx].pcb_idx = DNS_MAX_SOURCE_PORTS; + break; + } + } + } + if (dns_table[idx].pcb_idx < DNS_MAX_SOURCE_PORTS) { + /* if we come here, the pcb is not used any more and can be removed */ + udp_remove(dns_pcbs[dns_table[idx].pcb_idx]); + dns_pcbs[dns_table[idx].pcb_idx] = NULL; + dns_table[idx].pcb_idx = DNS_MAX_SOURCE_PORTS; + } +#endif } /* Create a query transmission ID that is unique for all outstanding queries */ @@ -786,13 +907,13 @@ dns_check_entry(u8_t i) /* initialize new entry */ txid = dns_create_txid(); entry->txid = txid; - entry->state = DNS_STATE_ASKING; - entry->numdns = 0; - entry->tmr = 1; + entry->state = DNS_STATE_ASKING; + entry->server_idx = 0; + entry->tmr = 1; entry->retries = 0; /* send DNS packet for this entry */ - err = dns_send(entry->numdns, entry->name, txid); + err = dns_send(entry); if (err != ERR_OK) { LWIP_DEBUGF(DNS_DEBUG | LWIP_DBG_LEVEL_WARNING, ("dns_send returned error: %s\n", lwip_strerr(err))); @@ -803,10 +924,10 @@ dns_check_entry(u8_t i) case DNS_STATE_ASKING: if (--entry->tmr == 0) { if (++entry->retries == DNS_MAX_RETRIES) { - if ((entry->numdns+1numdns+1])) { + if ((entry->server_idx + 1 < DNS_MAX_SERVERS) && !ip_addr_isany(&dns_servers[entry->server_idx + 1])) { /* change of server */ - entry->numdns++; - entry->tmr = 1; + entry->server_idx++; + entry->tmr = 1; entry->retries = 0; break; } else { @@ -814,7 +935,7 @@ dns_check_entry(u8_t i) /* call specified callback function if provided */ dns_call_found(i, NULL); /* flush this entry */ - entry->state = DNS_STATE_UNUSED; + entry->state = DNS_STATE_UNUSED; break; } } @@ -823,7 +944,7 @@ dns_check_entry(u8_t i) entry->tmr = entry->retries; /* send DNS packet for this entry */ - err = dns_send(entry->numdns, entry->name, entry->txid); + err = dns_send(entry); if (err != ERR_OK) { LWIP_DEBUGF(DNS_DEBUG | LWIP_DBG_LEVEL_WARNING, ("dns_send returned error: %s\n", lwip_strerr(err))); @@ -904,9 +1025,10 @@ dns_recv(void *arg, struct udp_pcb *pcb, struct pbuf *p, ip_addr_t *addr, u16_t entry_idx = i; if ((entry->state == DNS_STATE_ASKING) && (entry->txid == txid)) { + u8_t dns_err; /* This entry is now completed. */ entry->state = DNS_STATE_DONE; - entry->err = hdr->flags2 & DNS_FLAG2_ERR_MASK; + dns_err = hdr->flags2 & DNS_FLAG2_ERR_MASK; /* We only care about the question(s) and the answers. The authrr and the extrarr are simply discarded. */ @@ -914,7 +1036,7 @@ dns_recv(void *arg, struct udp_pcb *pcb, struct pbuf *p, ip_addr_t *addr, u16_t nanswers = htons(hdr->numanswers); /* Check for error. If so, call callback to inform. */ - if (((hdr->flags1 & DNS_FLAG1_RESPONSE) == 0) || (entry->err != 0) || (nquestions != 1)) { + if (((hdr->flags1 & DNS_FLAG1_RESPONSE) == 0) || (dns_err != 0) || (nquestions != 1)) { LWIP_DEBUGF(DNS_DEBUG, ("dns_recv: \"%s\": error in flags\n", entry->name)); /* call callback to indicate error, clean up memory and return */ goto responseerr; @@ -922,7 +1044,7 @@ dns_recv(void *arg, struct udp_pcb *pcb, struct pbuf *p, ip_addr_t *addr, u16_t /* Check whether response comes from the same network address to which the question was sent. (RFC 5452) */ - if (!ip_addr_cmp(addr, &dns_servers[entry->numdns])) { + if (!ip_addr_cmp(addr, &dns_servers[entry->server_idx])) { /* call callback to indicate error, clean up memory and return */ goto responseerr; } @@ -1078,7 +1200,7 @@ dns_enqueue(const char *name, size_t hostnamelen, dns_found_callback found, /* find a free request entry */ req = NULL; for (r = 0; r < DNS_MAX_REQUESTS; r++) { - if (dns_requests[r].found == 0) { + if (dns_requests[r].found == NULL) { req = &dns_requests[r]; break; } @@ -1088,12 +1210,12 @@ dns_enqueue(const char *name, size_t hostnamelen, dns_found_callback found, LWIP_DEBUGF(DNS_DEBUG, ("dns_enqueue: \"%s\": DNS request entries table is full\n", name)); return ERR_MEM; } + req->dns_table_idx = i; #else /* in this configuration, the entry index is the same as the request index */ req = &dns_requests[i]; #endif - /* use this entry */ LWIP_DEBUGF(DNS_DEBUG, ("dns_enqueue: \"%s\": use DNS entry %"U16_F"\n", name, (u16_t)(i))); @@ -1106,6 +1228,18 @@ dns_enqueue(const char *name, size_t hostnamelen, dns_found_callback found, MEMCPY(entry->name, name, namelen); entry->name[namelen] = 0; +#if ((LWIP_DNS_SECURE & LWIP_DNS_SECURE_RAND_SRC_PORT) != 0) + entry->pcb_idx = dns_alloc_pcb(); + if (entry->pcb_idx >= DNS_MAX_SOURCE_PORTS) { + /* failed to get a UDP pcb */ + LWIP_DEBUGF(DNS_DEBUG, ("dns_enqueue: \"%s\": failed to allocate a pcb\n", name)); + entry->state = DNS_STATE_UNUSED; + req->found = NULL; + return ERR_MEM; + } + LWIP_DEBUGF(DNS_DEBUG, ("dns_enqueue: \"%s\": use DNS pcb %"U16_F"\n", name, (u16_t)(entry->pcb_idx))); +#endif + dns_seqno++; /* force to send query without waiting timer */ @@ -1142,12 +1276,18 @@ dns_gethostbyname(const char *hostname, ip_addr_t *addr, dns_found_callback foun size_t hostnamelen; /* not initialized or no valid server yet, or invalid addr pointer * or invalid hostname or invalid hostname length */ - if ((dns_pcb == NULL) || (addr == NULL) || + if ((addr == NULL) || (!hostname) || (!hostname[0])) { return ERR_ARG; } +#if ((LWIP_DNS_SECURE & LWIP_DNS_SECURE_RAND_SRC_PORT) == 0) + if (dns_pcbs[0] == NULL) { + return ERR_ARG; + } +#endif hostnamelen = strlen(hostname); if (hostnamelen >= DNS_MAX_NAME_LENGTH) { + LWIP_DEBUGF(DNS_DEBUG, ("dns_gethostbyname: name too long to resolve")); return ERR_ARG; }