Hi, while poking around in the DNS response parsing, I thought this might be a good idea, but not 100% sure about considerations that may have went into the existing code.
I was about to add more variables like `ancount` (e.g. `arcount`), then realized it's all already there, in the struct dns_header. Downsides would be having to remember to call ntohX() on integer members and heap access vs. stack access. But I believe that's negligable. One could even keep the `ancount` variable and just assign it once by reading the struct member, I think the code simplification is still nice (especially the `flags` checks). Just a thought, comments welcome. Cheers, Conrad -- Conrad Hoffmann Traffic Engineer SoundCloud Ltd. | Rheinsberger Str. 76/77, 10115 Berlin, Germany Managing Director: Alexander Ljung | Incorporated in England & Wales with Company No. 6343600 | Local Branch Office | AG Charlottenburg | HRB 110657B
From 842223e59a219f204848993b3613847b6019eaf1 Mon Sep 17 00:00:00 2001 From: Conrad Hoffmann <[email protected]> Date: Sat, 25 Jun 2016 16:12:01 +0200 Subject: [PATCH] CLEANUP: dns: use struct dns_header for parsing Use the already present struct dns_header as wire representation and validate the response by checking the members of this struct. Mainly useful to not keep adding additional variables as more parts of the response become interesting, e.g. the additional record section for SRV records. --- src/dns.c | 53 ++++++++++++++--------------------------------------- 1 file changed, 14 insertions(+), 39 deletions(-) diff --git a/src/dns.c b/src/dns.c index 3b3dfc5..345edfb 100644 --- a/src/dns.c +++ b/src/dns.c @@ -342,7 +342,8 @@ void dns_update_resolvers_timeout(struct dns_resolvers *resolvers) int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, char *dn_name, int dn_name_len) { unsigned char *reader, *cname, *ptr; - int i, len, flags, type, ancount, cnamelen, expected_record; + int i, len, type, cnamelen, expected_record; + struct dns_header *header; reader = resp; cname = NULL; @@ -355,11 +356,12 @@ int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, char * * 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) + /* Assert response includes at least a valid header. */ + if (reader + sizeof(struct dns_header) >= bufend) return DNS_RESP_INVALID; + header = (struct dns_header*)reader; + /* * flags are stored over 2 bytes * First byte contains: @@ -369,50 +371,23 @@ int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, char * * - truncated (1 bit) * - recursion desired (1 bit) */ - if (reader + 2 >= bufend) - return DNS_RESP_INVALID; - - flags = reader[0] * 256 + reader[1]; - - if (flags & DNS_FLAG_TRUNCATED) + if (header->tc) return DNS_RESP_TRUNCATED; - if ((flags & DNS_FLAG_REPLYCODE) != DNS_RCODE_NO_ERROR) { - if ((flags & DNS_FLAG_REPLYCODE) == DNS_RCODE_NX_DOMAIN) + if (header->rcode != DNS_RCODE_NO_ERROR) { + if (header->rcode == DNS_RCODE_NX_DOMAIN) return DNS_RESP_NX_DOMAIN; - else if ((flags & DNS_FLAG_REPLYCODE) == DNS_RCODE_REFUSED) + else if (header->rcode == DNS_RCODE_REFUSED) return DNS_RESP_REFUSED; return DNS_RESP_ERROR; } - /* 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) - return DNS_RESP_INVALID; - - /* analyzing answer count */ - if (reader + 2 > bufend) - return DNS_RESP_INVALID; - ancount = reader[0] * 256 + reader[1]; - - if (ancount == 0) + if (ntohs(header->ancount) == 0) return DNS_RESP_ANCOUNT_ZERO; - /* move forward 2 bytes for answer count */ - reader += 2; - if (reader >= bufend) - return DNS_RESP_INVALID; - - /* move forward 4 bytes authority and additional count */ - reader += 4; - if (reader >= bufend) - return DNS_RESP_INVALID; + /* move forward by size of header */ + reader += sizeof(struct dns_header); /* check if the name can stand in response */ if (dn_name && ((reader + dn_name_len + 1) > bufend)) @@ -442,7 +417,7 @@ int dns_validate_dns_response(unsigned char *resp, unsigned char *bufend, char * return DNS_RESP_INVALID; /* now parsing response records */ - for (i = 1; i <= ancount; i++) { + for (i = 1; i <= ntohs(header->ancount); i++) { if (reader >= bufend) return DNS_RESP_INVALID; -- 2.9.0
