CLEANUP: dns: remove 45 "return" statements from dns_validate_dns_response()

The previous leak on do-resolve was particularly tricky to check due
to the important code repetition in dns_validate_dns_response() which
required careful examination of all return statements to check whether
they needed a pool_free() or not. Let's clean all this up using a common
leave point which releases the element itself. This also encourages
to properly set the current response to null right after freeing or
adding it so that it doesn't get added. 45 return and 22 pool_free()
were replaced by one of each.

diff --git a/src/dns.c b/src/dns.c
index b53e3b6fbdef680421bb873fc0f00f8280db4b03..6a8ab831c392c557d4c67f248ce8d22aa71e061a 100644 (file)
--- a/src/dns.c
+++ b/src/dns.c
@@ -692,18 +692,21 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
        struct dns_answer_item *dns_answer_record, *tmp_record;
        struct dns_response_packet *dns_p;
        int i, found = 0;
+       int cause = DNS_RESP_ERROR;
 
        reader         = resp;
        len            = 0;
        previous_dname = NULL;
        dns_query      = NULL;
+       dns_answer_record = NULL;
 
        /* Initialization of response buffer and structure */
        dns_p = &resolution->response;
 
        /* query id */
        if (reader + 2 >= bufend)
-               return DNS_RESP_INVALID;
+               goto invalid_resp;
+
        dns_p->header.id = reader[0] * 256 + reader[1];
        reader += 2;
 
@@ -716,16 +719,23 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
         *  - recursion desired (1 bit)
         */
        if (reader + 2 >= bufend)
-               return DNS_RESP_INVALID;
+               goto invalid_resp;
 
        flags = reader[0] * 256 + reader[1];
 
        if ((flags & DNS_FLAG_REPLYCODE) != DNS_RCODE_NO_ERROR) {
-               if ((flags & DNS_FLAG_REPLYCODE) == DNS_RCODE_NX_DOMAIN)
-                       return DNS_RESP_NX_DOMAIN;
-               else if ((flags & DNS_FLAG_REPLYCODE) == DNS_RCODE_REFUSED)
-                       return DNS_RESP_REFUSED;
-               return DNS_RESP_ERROR;
+               if ((flags & DNS_FLAG_REPLYCODE) == DNS_RCODE_NX_DOMAIN) {
+                       cause = DNS_RESP_NX_DOMAIN;
+                       goto return_error;
+               }
+               else if ((flags & DNS_FLAG_REPLYCODE) == DNS_RCODE_REFUSED) {
+                       cause = DNS_RESP_REFUSED;
+                       goto return_error;
+               }
+               else {
+                       cause = DNS_RESP_ERROR;
+                       goto return_error;
+               }
        }
 
        /* Move forward 2 bytes for flags */
@@ -733,36 +743,42 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
 
        /* 2 bytes for question count */
        if (reader + 2 >= bufend)
-               return DNS_RESP_INVALID;
+               goto invalid_resp;
        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 != 1) {
+               cause = DNS_RESP_QUERY_COUNT_ERROR;
+               goto return_error;
+       }
+
        if (dns_p->header.qdcount > DNS_MAX_QUERY_RECORDS)
-               return DNS_RESP_INVALID;
+               goto invalid_resp;
        reader += 2;
 
        /* 2 bytes for answer count */
        if (reader + 2 >= bufend)
-               return DNS_RESP_INVALID;
+               goto invalid_resp;
        dns_p->header.ancount = reader[0] * 256 + reader[1];
-       if (dns_p->header.ancount == 0)
-               return DNS_RESP_ANCOUNT_ZERO;
+       if (dns_p->header.ancount == 0) {
+               cause = DNS_RESP_ANCOUNT_ZERO;
+               goto return_error;
+       }
+
        /* Check if too many records are announced */
        if (dns_p->header.ancount > max_answer_records)
-               return DNS_RESP_INVALID;
+               goto invalid_resp;
        reader += 2;
 
        /* 2 bytes authority count */
        if (reader + 2 >= bufend)
-               return DNS_RESP_INVALID;
+               goto invalid_resp;
        dns_p->header.nscount = reader[0] * 256 + reader[1];
        reader += 2;
 
        /* 2 bytes additional count */
        if (reader + 2 >= bufend)
-               return DNS_RESP_INVALID;
+               goto invalid_resp;
        dns_p->header.arcount = reader[0] * 256 + reader[1];
        reader += 2;
 
@@ -773,7 +789,7 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
                 * 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;
+                       goto invalid_resp;
                dns_query = &resolution->response_query_records[dns_query_record_id];
                LIST_ADDQ(&dns_p->query_list, &dns_query->list);
 
@@ -784,61 +800,62 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
                len = dns_read_name(resp, bufend, reader, dns_query->name, DNS_MAX_NAME_SIZE, &offset, 0);
 
                if (len == 0)
-                       return DNS_RESP_INVALID;
+                       goto invalid_resp;
 
                reader += offset;
                previous_dname = dns_query->name;
 
                /* move forward 2 bytes for question type */
                if (reader + 2 >= bufend)
-                       return DNS_RESP_INVALID;
+                       goto invalid_resp;
                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;
+                       goto invalid_resp;
                dns_query->class = reader[0] * 256 + reader[1];
                reader += 2;
        }
 
        /* TRUNCATED flag must be checked after we could read the query type
         * because a TRUNCATED SRV query type response can still be exploited */
-       if (dns_query->type != DNS_RTYPE_SRV && flags & DNS_FLAG_TRUNCATED)
-               return DNS_RESP_TRUNCATED;
+       if (dns_query->type != DNS_RTYPE_SRV && flags & DNS_FLAG_TRUNCATED) {
+               cause = DNS_RESP_TRUNCATED;
+               goto return_error;
+       }
 
        /* now parsing response records */
        nb_saved_records = 0;
        for (i = 0; i < dns_p->header.ancount; i++) {
                if (reader >= bufend)
-                       return DNS_RESP_INVALID;
+                       goto invalid_resp;
 
                dns_answer_record = pool_alloc(dns_answer_item_pool);
                if (dns_answer_record == NULL)
-                       return (DNS_RESP_INVALID);
+                       goto invalid_resp;
 
                offset = 0;
                len = dns_read_name(resp, bufend, reader, tmpname, DNS_MAX_NAME_SIZE, &offset, 0);
 
-               if (len == 0) {
-                       pool_free(dns_answer_item_pool, dns_answer_record);
-                       return DNS_RESP_INVALID;
-               }
+               if (len == 0)
+                       goto invalid_resp;
 
                /* Check if the current record dname is valid.  previous_dname
                 * points either to queried dname or last CNAME target */
                if (dns_query->type != DNS_RTYPE_SRV && dns_hostname_cmp(previous_dname, tmpname, len) != 0) {
-                       pool_free(dns_answer_item_pool, dns_answer_record);
                        if (i == 0) {
                                /* First record, means a mismatch issue between
                                 * queried dname and dname found in the first
                                 * record */
-                               return DNS_RESP_INVALID;
+                               goto invalid_resp;
                        }
                        else {
                                /* If not the first record, this means we have a
-                                * CNAME resolution error */
-                               return DNS_RESP_CNAME_ERROR;
+                                * CNAME resolution error.
+                                */
+                               cause = DNS_RESP_CNAME_ERROR;
+                               goto return_error;
                        }
 
                }
@@ -847,59 +864,50 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
                dns_answer_record->name[len] = 0;
 
                reader += offset;
-               if (reader >= bufend) {
-                       pool_free(dns_answer_item_pool, dns_answer_record);
-                       return DNS_RESP_INVALID;
-               }
+               if (reader >= bufend)
+                       goto invalid_resp;
 
                /* 2 bytes for record type (A, AAAA, CNAME, etc...) */
-               if (reader + 2 > bufend) {
-                       pool_free(dns_answer_item_pool, dns_answer_record);
-                       return DNS_RESP_INVALID;
-               }
+               if (reader + 2 > bufend)
+                       goto invalid_resp;
+
                dns_answer_record->type = reader[0] * 256 + reader[1];
                reader += 2;
 
                /* 2 bytes for class (2) */
-               if (reader + 2 > bufend) {
-                       pool_free(dns_answer_item_pool, dns_answer_record);
-                       return DNS_RESP_INVALID;
-               }
+               if (reader + 2 > bufend)
+                       goto invalid_resp;
+
                dns_answer_record->class = reader[0] * 256 + reader[1];
                reader += 2;
 
                /* 4 bytes for ttl (4) */
-               if (reader + 4 > bufend) {
-                       pool_free(dns_answer_item_pool, dns_answer_record);
-                       return DNS_RESP_INVALID;
-               }
+               if (reader + 4 > bufend)
+                       goto invalid_resp;
+
                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) {
-                       pool_free(dns_answer_item_pool, dns_answer_record);
-                       return DNS_RESP_INVALID;
-               }
+               if (reader + 2 > bufend)
+                       goto invalid_resp;
+
                dns_answer_record->data_len = reader[0] * 256 + reader[1];
 
                /* Move forward 2 bytes for data len */
                reader += 2;
 
-               if (reader + dns_answer_record->data_len > bufend) {
-                       pool_free(dns_answer_item_pool, dns_answer_record);
-                       return DNS_RESP_INVALID;
-               }
+               if (reader + dns_answer_record->data_len > bufend)
+                       goto invalid_resp;
 
                /* Analyzing record content */
                switch (dns_answer_record->type) {
                        case DNS_RTYPE_A:
                                /* ipv4 is stored on 4 bytes */
-                               if (dns_answer_record->data_len != 4) {
-                                       pool_free(dns_answer_item_pool, dns_answer_record);
-                                       return DNS_RESP_INVALID;
-                               }
+                               if (dns_answer_record->data_len != 4)
+                                       goto invalid_resp;
+
                                dns_answer_record->address.sa_family = AF_INET;
                                memcpy(&(((struct sockaddr_in *)&dns_answer_record->address)->sin_addr),
                                                reader, dns_answer_record->data_len);
@@ -915,16 +923,14 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
                                 * starts at 1.
                                 */
                                if (i + 1 == dns_p->header.ancount) {
-                                       pool_free(dns_answer_item_pool, dns_answer_record);
-                                       return DNS_RESP_CNAME_ERROR;
+                                       cause = DNS_RESP_CNAME_ERROR;
+                                       goto return_error;
                                }
 
                                offset = 0;
                                len = dns_read_name(resp, bufend, reader, tmpname, DNS_MAX_NAME_SIZE, &offset, 0);
-                               if (len == 0) {
-                                       pool_free(dns_answer_item_pool, dns_answer_record);
-                                       return DNS_RESP_INVALID;
-                               }
+                               if (len == 0)
+                                       goto invalid_resp;
 
                                memcpy(dns_answer_record->target, tmpname, len);
                                dns_answer_record->target[len] = 0;
@@ -939,10 +945,9 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
                                 * - 2 bytes for the port
                                 * - the target hostname
                                 */
-                               if (dns_answer_record->data_len <= 6) {
-                                       pool_free(dns_answer_item_pool, dns_answer_record);
-                                       return DNS_RESP_INVALID;
-                               }
+                               if (dns_answer_record->data_len <= 6)
+                                       goto invalid_resp;
+
                                dns_answer_record->priority = read_n16(reader);
                                reader += sizeof(uint16_t);
                                dns_answer_record->weight = read_n16(reader);
@@ -951,10 +956,9 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
                                reader += sizeof(uint16_t);
                                offset = 0;
                                len = dns_read_name(resp, bufend, reader, tmpname, DNS_MAX_NAME_SIZE, &offset, 0);
-                               if (len == 0) {
-                                       pool_free(dns_answer_item_pool, dns_answer_record);
-                                       return DNS_RESP_INVALID;
-                               }
+                               if (len == 0)
+                                       goto invalid_resp;
+
                                dns_answer_record->data_len = len;
                                memcpy(dns_answer_record->target, tmpname, len);
                                dns_answer_record->target[len] = 0;
@@ -962,10 +966,9 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
 
                        case DNS_RTYPE_AAAA:
                                /* ipv6 is stored on 16 bytes */
-                               if (dns_answer_record->data_len != 16) {
-                                       pool_free(dns_answer_item_pool, dns_answer_record);
-                                       return DNS_RESP_INVALID;
-                               }
+                               if (dns_answer_record->data_len != 16)
+                                       goto invalid_resp;
+
                                dns_answer_record->address.sa_family = AF_INET6;
                                memcpy(&(((struct sockaddr_in6 *)&dns_answer_record->address)->sin6_addr),
                                                reader, dns_answer_record->data_len);
@@ -1024,11 +1027,13 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
                if (found == 1) {
                        tmp_record->last_seen = now.tv_sec;
                        pool_free(dns_answer_item_pool, dns_answer_record);
+                       dns_answer_record = NULL;
                }
                else {
                        dns_answer_record->last_seen = now.tv_sec;
                        dns_answer_record->ar_item = NULL;
                        LIST_ADDQ(&dns_p->answer_list, &dns_answer_record->list);
+                       dns_answer_record = NULL;
                }
        } /* for i 0 to ancount */
 
@@ -1038,20 +1043,22 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
        /* now parsing additional records for SRV queries only */
        if (dns_query->type != DNS_RTYPE_SRV)
                goto skip_parsing_additional_records;
+
        nb_saved_records = 0;
        for (i = 0; i < dns_p->header.arcount; i++) {
                if (reader >= bufend)
-                       return DNS_RESP_INVALID;
+                       goto invalid_resp;
 
                dns_answer_record = pool_alloc(dns_answer_item_pool);
                if (dns_answer_record == NULL)
-                       return (DNS_RESP_INVALID);
+                       goto invalid_resp;
 
                offset = 0;
                len = dns_read_name(resp, bufend, reader, tmpname, DNS_MAX_NAME_SIZE, &offset, 0);
 
                if (len == 0) {
                        pool_free(dns_answer_item_pool, dns_answer_record);
+                       dns_answer_record = NULL;
                        continue;
                }
 
@@ -1059,59 +1066,50 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
                dns_answer_record->name[len] = 0;
 
                reader += offset;
-               if (reader >= bufend) {
-                       pool_free(dns_answer_item_pool, dns_answer_record);
-                       return DNS_RESP_INVALID;
-               }
+               if (reader >= bufend)
+                       goto invalid_resp;
 
                /* 2 bytes for record type (A, AAAA, CNAME, etc...) */
-               if (reader + 2 > bufend) {
-                       pool_free(dns_answer_item_pool, dns_answer_record);
-                       return DNS_RESP_INVALID;
-               }
+               if (reader + 2 > bufend)
+                       goto invalid_resp;
+
                dns_answer_record->type = reader[0] * 256 + reader[1];
                reader += 2;
 
                /* 2 bytes for class (2) */
-               if (reader + 2 > bufend) {
-                       pool_free(dns_answer_item_pool, dns_answer_record);
-                       return DNS_RESP_INVALID;
-               }
+               if (reader + 2 > bufend)
+                       goto invalid_resp;
+
                dns_answer_record->class = reader[0] * 256 + reader[1];
                reader += 2;
 
                /* 4 bytes for ttl (4) */
-               if (reader + 4 > bufend) {
-                       pool_free(dns_answer_item_pool, dns_answer_record);
-                       return DNS_RESP_INVALID;
-               }
+               if (reader + 4 > bufend)
+                       goto invalid_resp;
+
                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) {
-                       pool_free(dns_answer_item_pool, dns_answer_record);
-                       return DNS_RESP_INVALID;
-               }
+               if (reader + 2 > bufend)
+                       goto invalid_resp;
+
                dns_answer_record->data_len = reader[0] * 256 + reader[1];
 
                /* Move forward 2 bytes for data len */
                reader += 2;
 
-               if (reader + dns_answer_record->data_len > bufend) {
-                       pool_free(dns_answer_item_pool, dns_answer_record);
-                       return DNS_RESP_INVALID;
-               }
+               if (reader + dns_answer_record->data_len > bufend)
+                       goto invalid_resp;
 
                /* Analyzing record content */
                switch (dns_answer_record->type) {
                        case DNS_RTYPE_A:
                                /* ipv4 is stored on 4 bytes */
-                               if (dns_answer_record->data_len != 4) {
-                                       pool_free(dns_answer_item_pool, dns_answer_record);
-                                       return DNS_RESP_INVALID;
-                               }
+                               if (dns_answer_record->data_len != 4)
+                                       goto invalid_resp;
+
                                dns_answer_record->address.sa_family = AF_INET;
                                memcpy(&(((struct sockaddr_in *)&dns_answer_record->address)->sin_addr),
                                                reader, dns_answer_record->data_len);
@@ -1119,10 +1117,9 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
 
                        case DNS_RTYPE_AAAA:
                                /* ipv6 is stored on 16 bytes */
-                               if (dns_answer_record->data_len != 16) {
-                                       pool_free(dns_answer_item_pool, dns_answer_record);
-                                       return DNS_RESP_INVALID;
-                               }
+                               if (dns_answer_record->data_len != 16)
+                                       goto invalid_resp;
+
                                dns_answer_record->address.sa_family = AF_INET6;
                                memcpy(&(((struct sockaddr_in6 *)&dns_answer_record->address)->sin6_addr),
                                                reader, dns_answer_record->data_len);
@@ -1130,6 +1127,7 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
 
                        default:
                                pool_free(dns_answer_item_pool, dns_answer_record);
+                               dns_answer_record = NULL;
                                continue;
 
                } /* switch (record type) */
@@ -1176,6 +1174,7 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
                if (found == 1) {
                        tmp_record->last_seen = now.tv_sec;
                        pool_free(dns_answer_item_pool, dns_answer_record);
+                       dns_answer_record = NULL;
                }
                else {
                        dns_answer_record->last_seen = now.tv_sec;
@@ -1194,6 +1193,7 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
                        }
 
                        LIST_ADDQ(&dns_p->ar_list, &dns_answer_record->list);
+                       dns_answer_record = NULL;
                }
        } /* for i 0 to arcount */
 
@@ -1204,6 +1204,13 @@ static int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend,
 
        dns_check_dns_response(resolution);
        return DNS_RESP_VALID;
+
+ invalid_resp:
+       cause = DNS_RESP_INVALID;
+
+ return_error:
+       pool_free(dns_answer_item_pool, dns_answer_record);
+       return cause;
 }
 
 /* Searches dn_name resolution in resp.