From a750683d2f3d35716a072dd7b0ba6c1d79a2119a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 17 Dec 2008 12:51:36 +0000 Subject: [PATCH] Partial backport of DNS address/name checking (r16621), and backport of 0x20 hack (r17171). svn:r17636 --- ChangeLog | 10 ++++- doc/TODO.020 | 4 +- src/or/config.c | 1 + src/or/dns.c | 4 ++ src/or/eventdns.c | 96 ++++++++++++++++++++++++++++++++++++++++++++--- src/or/or.h | 2 + 6 files changed, 109 insertions(+), 8 deletions(-) diff --git a/ChangeLog b/ChangeLog index 250053f12..5d4574492 100644 --- a/ChangeLog +++ b/ChangeLog @@ -32,7 +32,6 @@ Changes in version 0.2.0.33 - 200?-??-?? - Use 64 bits instead of 32 bits for connection identifiers used with the controller protocol, to greatly reduce risk of identifier reuse. - o Minor features: - Report the case where all signatures in a detached set are rejected differently than the case where there is an error handling the detached @@ -40,6 +39,15 @@ Changes in version 0.2.0.33 - 200?-??-?? - When we realize that another process has modified our cached descriptors, print out a more useful error message rather than triggering an assertion. Fixes bug 885. Patch from Karsten. + - Implement the 0x20 hack to better resist DNS poisoning: set the + case on outgoing DNS requests randomly, and reject responses that do + not match the case correctly. This logic can be disabled with the + ServerDNSRamdomizeCase setting, if you are using one of the 0.3% + of servers that do not reliably preserve case in replies. See + "Increased DNS Forgery Resistance through 0x20-Bit Encoding" + for more info. + - Check DNS replies for more matching fields to better resist DNS + poisoning. Changes in version 0.2.0.32 - 2008-11-20 diff --git a/doc/TODO.020 b/doc/TODO.020 index abda5d74b..bc0855f22 100644 --- a/doc/TODO.020 +++ b/doc/TODO.020 @@ -8,8 +8,10 @@ Backport for 0.2.0: Backport for 0.2.0 once better tested: o r16136: prevent circid collision. [Also backport to 0.1.2.x??] o r16558: Avoid mis-routing CREATED cells. - - r16621: Make some DNS code more robust (partial; see also libevent + Xo r16621: Make some DNS code more robust (partial; see also libevent approach). (Also maybe r16674) + [Partially backported. Instead of the basic name checking, I backported + r17171 instead, to be even more resistant to poisoning.] - r17091: distinguish "no routers support pending circuits" from "no circuits are pending." See also r17181 and r17184. - r17137: send END cell in response to connect to nonexistent hidserv port. diff --git a/src/or/config.c b/src/or/config.c index 9e9bccc88..d69254b30 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -274,6 +274,7 @@ static config_var_t _option_vars[] = { V(ServerDNSAllowBrokenResolvConf, BOOL, "1"), V(ServerDNSAllowNonRFC953Hostnames, BOOL,"0"), V(ServerDNSDetectHijacking, BOOL, "1"), + V(ServerDNSRandomizeCase, BOOL, "1"), V(ServerDNSResolvConfFile, STRING, NULL), V(ServerDNSSearchDomains, BOOL, "0"), V(ServerDNSTestAddresses, CSV, diff --git a/src/or/dns.c b/src/or/dns.c index db52651ce..c8da9969d 100644 --- a/src/or/dns.c +++ b/src/or/dns.c @@ -198,6 +198,10 @@ dns_init(void) { init_cache_map(); evdns_set_transaction_id_fn(dns_get_transaction_id); + if (get_options()->ServerDNSRandomizeCase) + evdns_set_option("randomize-case", "1", DNS_OPTIONS_ALL); + else + evdns_set_option("randomize-case", "0", DNS_OPTIONS_ALL); if (server_mode(get_options())) return configure_nameservers(1); return 0; diff --git a/src/or/eventdns.c b/src/or/eventdns.c index 34e622b6e..b442b5cbc 100644 --- a/src/or/eventdns.c +++ b/src/or/eventdns.c @@ -307,6 +307,9 @@ static int global_max_retransmits = 3; /* number of times we'll retransmit a req /* number of timeouts in a row before we consider this server to be down */ static int global_max_nameserver_timeout = 3; +/* DOCDOC */ +static int global_randomize_case = 1; + /* These are the timeout values for nameservers. If we find a nameserver is down */ /* we try to probe it at intervals as given below. Values are in seconds. */ static const struct timeval global_nameserver_timeouts[] = {{10, 0}, {60, 0}, {300, 0}, {900, 0}, {3600, 0}}; @@ -377,6 +380,9 @@ inet_aton(const char *c, struct in_addr *addr) #define ISSPACE(c) isspace((int)(unsigned char)(c)) #define ISDIGIT(c) isdigit((int)(unsigned char)(c)) +#define ISALPHA(c) isalpha((int)(unsigned char)(c)) +#define TOLOWER(c) (char)tolower((int)(unsigned char)(c)) +#define TOUPPER(c) (char)toupper((int)(unsigned char)(c)) #ifndef NDEBUG static const char * @@ -813,9 +819,10 @@ name_parse(u8 *packet, int length, int *idx, char *name_out, size_t name_out_len static int reply_parse(u8 *packet, int length) { int j = 0; /* index into packet */ + int k; u16 _t; /* used by the macros */ u32 _t32; /* used by the macros */ - char tmp_name[256]; /* used by the macros */ + char tmp_name[256], cmp_name[256]; /* used by the macros */ u16 trans_id, questions, answers, authority, additional, datalength; u16 flags = 0; @@ -823,6 +830,7 @@ reply_parse(u8 *packet, int length) { struct reply reply; struct request *req = NULL; unsigned int i; + int name_matches = 0; GET16(trans_id); GET16(flags); @@ -848,11 +856,28 @@ reply_parse(u8 *packet, int length) { /* if (!answers) return; */ /* must have an answer of some form */ /* This macro skips a name in the DNS reply. */ -#define SKIP_NAME \ +#define GET_NAME \ do { tmp_name[0] = '\0'; \ if (name_parse(packet, length, &j, tmp_name, sizeof(tmp_name))<0) \ goto err; \ } while(0); +#define TEST_NAME \ + do { tmp_name[0] = '\0'; \ + cmp_name[0] = '\0'; \ + k = j; \ + if (name_parse(packet, length, &j, tmp_name, sizeof(tmp_name))<0) \ + goto err; \ + if (name_parse(req->request, req->request_len, &k, cmp_name, sizeof(cmp_name))<0) \ + goto err; \ + if (global_randomize_case) { \ + if (strcmp(tmp_name, cmp_name) == 0) \ + name_matches = 1; /* we ignore mismatching names */ \ + } else { \ + if (strcasecmp(tmp_name, cmp_name) == 0) \ + name_matches = 1; \ + } \ + } while(0) + reply.type = req->request_type; @@ -861,11 +886,14 @@ reply_parse(u8 *packet, int length) { /* the question looks like * */ - SKIP_NAME; + TEST_NAME; j += 4; if (j >= length) goto err; } + if (!name_matches) + goto err; + /* now we have the answer section which looks like * */ @@ -875,7 +903,7 @@ reply_parse(u8 *packet, int length) { /* XXX I'd be more comfortable if we actually checked the name */ /* here. -NM */ - SKIP_NAME; + GET_NAME; GET16(type); GET16(class); GET32(ttl); @@ -1082,6 +1110,22 @@ evdns_set_transaction_id_fn(uint16_t (*fn)(void)) trans_id_function = default_transaction_id_fn; } + +static void +get_random_bytes(char *buf, size_t n) +{ + unsigned i; + for (i = 0; i < n-1; i += 2) { + u16 tid = trans_id_function(); + buf[i] = (tid >> 8) & 0xff; + buf[i+1] = tid & 0xff; + } + if (i < n) { + u16 tid = trans_id_function(); + buf[i] = tid & 0xff; + } +} + /* Try to choose a strong transaction id which isn't already in flight */ static u16 transaction_id_pick(void) { @@ -1143,17 +1187,34 @@ nameserver_pick(void) { /* this is called when a namesever socket is ready for reading */ static void nameserver_read(struct nameserver *ns) { + struct sockaddr_storage ss; + struct sockaddr *sa = (struct sockaddr *)&ss; + struct sockaddr_in *sin; + socklen_t addrlen = sizeof(ss); u8 packet[1500]; for (;;) { const int r = - (int)recv(ns->socket, packet,(socklen_t)sizeof(packet), 0); + (int)recvfrom(ns->socket, packet,(socklen_t)sizeof(packet), 0, + sa, &addrlen); if (r < 0) { int err = last_error(ns->socket); if (error_is_eagain(err)) return; nameserver_failed(ns, strerror(err)); return; } + if (sa->sa_family != AF_INET) { + log(EVDNS_LOG_WARN, + "Address family mismatch on received DNS packet."); + return; + } + sin = (struct sockaddr_in *)sa; + if (sin->sin_addr.s_addr != ns->address) { + log(EVDNS_LOG_WARN, + "Address mismatch on received DNS packet. Address was %s.", + debug_ntoa(sin->sin_addr.s_addr)); + return; + } ns->timedout = 0; reply_parse(packet, r); } @@ -2243,12 +2304,29 @@ request_new(int type, const char *name, int flags, /* the request data is alloced in a single block with the header */ struct request *const req = (struct request *) malloc(sizeof(struct request) + request_max_len); + char namebuf[256]; int rlen; (void) flags; if (!req) return NULL; memset(req, 0, sizeof(struct request)); + if (global_randomize_case) { + unsigned i; + char randbits[32]; + strlcpy(namebuf, name, sizeof(namebuf)); + get_random_bytes(randbits, (name_len+7)/8); + for (i = 0; i < name_len; ++i) { + if (ISALPHA(namebuf[i])) { + if ((randbits[i >> 3] & (1<<(i%7)))) + namebuf[i] = TOLOWER(namebuf[i]); + else + namebuf[i] = TOUPPER(namebuf[i]); + } + } + name = namebuf; + } + /* request data lives just after the header */ req->request = ((u8 *) req) + sizeof(struct request); /* denotes that the request data shouldn't be free()ed */ @@ -2690,7 +2768,13 @@ evdns_set_option(const char *option, const char *val, int flags) if (!(flags & DNS_OPTION_MISC)) return 0; log(EVDNS_LOG_DEBUG, "Setting retries to %d", retries); global_max_retransmits = retries; + } else if (!strncmp(option, "randomize-case:", 15)) { + int randcase = strtoint(val); + if (!(flags & DNS_OPTION_MISC)) return 0; + log(EVDNS_LOG_DEBUG, "Setting randomize_case to %d", randcase); + global_randomize_case = randcase; } + return 0; } @@ -3127,7 +3211,7 @@ evdns_server_callback(struct evdns_server_request *req, void *data) } } - r = evdns_request_respond(req, 0); + r = evdns_server_request_respond(req, 0); if (r<0) printf("eeek, couldn't send reply.\n"); } diff --git a/src/or/or.h b/src/or/or.h index 48bc3375d..a0a6a5e37 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2297,6 +2297,8 @@ typedef struct { * the local domains. */ int ServerDNSDetectHijacking; /**< Boolean: If true, check for DNS failure * hijacking. */ + int ServerDNSRandomizeCase; /**< Boolean: Use the 0x20-hack to prevent + * DNS poisoning attacks. */ char *ServerDNSResolvConfFile; /**< If provided, we configure our internal * resolver from the file here rather than from * /etc/resolv.conf (Unix) or the registry (Windows). */