Shmulik Regev brought cryptographically secure transaction IDs

ares/CHANGES

@@ -2,6 +2,32 @@
 
 * May 30 2007
 
+- Shmulik Regev brought cryptographically secure transaction IDs:
+
+  The c-ares library implementation uses a DNS "Transaction ID" field that is
+  seeded with a pseudo random number (based on gettimeofday) which is
+  incremented (++) between consecutive calls and is therefore rather
+  predictable. In general, predictability of DNS Transaction ID is a well
+  known security problem (e.g.
+  http://bak.spc.org/dms/archive/dns_id_attack.txt) and makes a c-ares based
+  implementation vulnerable to DNS poisoning. Credit goes to Amit Klein
+  (Trusteer) for identifying this problem.
+
+  The patch I wrote changes the implementation to use a more secure way of
+  generating unique IDs. It starts by obtaining a key with reasonable entropy
+  which is used with an RC4 stream to generate the cryptographically secure
+  transaction IDs.
+
+  Note that the key generation code (in ares_init:randomize_key) has two
+  versions, the Windows specific one uses a cryptographically safe function
+  provided (but undocumented :) by the operating system (described at
+  http://blogs.msdn.com/michael_howard/archive/2005/01/14/353379.aspx).  The
+  default implementation is a bit naive and uses the standard 'rand'
+  function. Surely a better way to generate random keys exists for other
+  platforms.
+
+  The patch can be tested by using the adig utility and using the '-s' option.
+
 - Brad House added ares_save_options() and ares_destroy_options() that can be
   used to keep options for later re-usal when ares_init_options() is used.
   

ares/ares_init.c

@@ -72,6 +72,8 @@ static int config_nameserver(struct server_state **servers, int *nservers,
 static int set_search(ares_channel channel, const char *str);
 static int set_options(ares_channel channel, const char *str);
 static const char *try_option(const char *p, const char *q, const char *opt);
+static void init_id_key(rc4_key* key,int key_data_len);
+
 #ifndef WIN32
 static int sortlist_alloc(struct apattern **sortlist, int *nsort, struct apattern *pat);
 static int ip_addr(const char *s, int len, struct in_addr *addr);
@@ -102,7 +104,6 @@ int ares_init_options(ares_channel *channelptr, struct ares_options *options,
   int i;
   int status = ARES_SUCCESS;
   struct server_state *server;
-  struct timeval tv;
 
 #ifdef CURLDEBUG
   const char *env = getenv("CARES_MEMDEBUG");
@@ -203,15 +204,9 @@ int ares_init_options(ares_channel *channelptr, struct ares_options *options,
       server->qtail = NULL;
     }
 
-  /* Choose a somewhat random query ID.  The main point is to avoid
+  init_id_key(&channel->id_key, ARES_ID_KEY_LEN);
-   * collisions with stale queries.  An attacker trying to spoof a DNS
-   * answer also has to guess the query ID, but it's only a 16-bit
-   * field, so there's not much to be done about that.
-   */
-  gettimeofday(&tv, NULL);
-  channel->next_id = (unsigned short)
-    ((tv.tv_sec ^ tv.tv_usec ^ getpid()) & 0xffff);
 
+  channel->next_id = ares__generate_new_id(&channel->id_key);
   channel->queries = NULL;
 
   *channelptr = channel;
@@ -1271,3 +1266,67 @@ static void natural_mask(struct apattern *pat)
     pat->mask.addr.addr4.s_addr = htonl(IN_CLASSC_NET);
 }
 #endif
+/* initialize an rc4 key. If possible a cryptographically secure random key
+   is generated using a suitable function (for example win32's RtlGenRandom as
+   described in
+   http://blogs.msdn.com/michael_howard/archive/2005/01/14/353379.aspx
+   otherwise the code defaults to cross-platform albeit less secure mechanism
+   using rand
+*/
+static void randomize_key(unsigned char* key,int key_data_len)
+{
+  int randomized = 0;
+#ifdef WIN32
+  HMODULE lib=LoadLibrary("ADVAPI32.DLL");
+  if (lib) {
+    BOOLEAN (APIENTRY *pfn)(void*, ULONG) =
+      (BOOLEAN (APIENTRY *)(void*,ULONG))GetProcAddress(lib,"SystemFunction036");
+    if (pfn && pfn(key,key_data_len) )
+      randomized = 1;
+
+    FreeLibrary(lib);
+  }
+#endif
+
+  if ( !randomized ) {
+    int counter;
+    for (counter=0;counter<key_data_len;counter++)
+      key[counter]=rand() % 256;
+  }
+}
+
+static void init_id_key(rc4_key* key,int key_data_len)
+{
+  unsigned char index1;
+  unsigned char index2;
+  unsigned char* state;
+  short counter;
+  unsigned char *key_data_ptr = 0;
+
+  key_data_ptr = calloc(1,key_data_len);
+  randomize_key(key->state,key_data_len);
+  state = &key->state[0];
+  for(counter = 0; counter < 256; counter++)
+        state[counter] = counter;
+  key->x = 0;
+  key->y = 0;
+  index1 = 0;
+  index2 = 0;
+  for(counter = 0; counter < 256; counter++)
+  {
+    index2 = (key_data_ptr[index1] + state[counter] +
+              index2) % 256;
+    ARES_SWAP_BYTE(&state[counter], &state[index2]);
+
+    index1 = (index1 + 1) % key_data_len;
+  }
+  free(key_data_ptr);
+
+}
+
+short ares__generate_new_id(rc4_key* key)
+{
+  short r;
+  ares__rc4(key, (unsigned char *)&r, sizeof(r));
+  return r;
+}

ares/ares_private.h

@@ -80,6 +80,8 @@
 
 #endif
 
+#define ARES_ID_KEY_LEN 31
+
 #include "ares_ipv6.h"
 
 struct send_request {
@@ -156,6 +158,13 @@ struct apattern {
   unsigned short type;
 };
 
+typedef struct rc4_key
+{
+  unsigned char state[256];
+  unsigned char x;
+  unsigned char y;
+} rc4_key;
+
 struct ares_channeldata {
   /* Configuration data */
   int flags;
@@ -176,6 +185,8 @@ struct ares_channeldata {
 
   /* ID to use for next query */
   unsigned short next_id;
+  /* key to use when generating new ids */
+  rc4_key id_key;
 
   /* Active queries */
   struct query *queries;
@@ -184,10 +195,15 @@ struct ares_channeldata {
   void *sock_state_cb_data;
 };
 
+void ares__rc4(rc4_key* key,unsigned char *buffer_ptr, int buffer_len);
 void ares__send_query(ares_channel channel, struct query *query, time_t now);
 void ares__close_sockets(ares_channel channel, struct server_state *server);
 int ares__get_hostent(FILE *fp, int family, struct hostent **host);
 int ares__read_line(FILE *fp, char **buf, int *bufsize);
+short ares__generate_new_id(rc4_key* key);
+
+#define ARES_SWAP_BYTE(a,b) \
+  { unsigned char swapByte = *(a);  *(a) = *(b);  *(b) = swapByte; }
 
 #define SOCK_STATE_CALLBACK(c, s, r, w)                                 \
   do {                                                                  \

ares/ares_query.c

@@ -39,6 +39,64 @@ struct qquery {
 
 static void qcallback(void *arg, int status, unsigned char *abuf, int alen);
 
+void ares__rc4(rc4_key* key, unsigned char *buffer_ptr, int buffer_len)
+{
+  unsigned char x;
+  unsigned char y;
+  unsigned char* state;
+  unsigned char xorIndex;
+  short counter;
+
+  x = key->x;
+  y = key->y;
+
+  state = &key->state[0];
+  for(counter = 0; counter < buffer_len; counter ++)
+  {
+	x = (x + 1) % 256;
+	y = (state[x] + y) % 256;
+	ARES_SWAP_BYTE(&state[x], &state[y]);
+
+	xorIndex = (state[x] + state[y]) % 256;
+
+	buffer_ptr[counter] ^= state[xorIndex];
+  }
+  key->x = x;
+  key->y = y;
+}
+
+static struct query* find_query_by_id(ares_channel channel, int id)
+{
+  int qid;
+  struct query* q;
+  DNS_HEADER_SET_QID(((unsigned char*)&qid), id);
+
+  /* Find the query corresponding to this packet. */
+  for (q = channel->queries; q; q = q->next)
+  {
+	if (q->qid == qid)
+	  return q;
+  }
+  return NULL;
+}
+
+
+/* a unique query id is generated using an rc4 key. Since the id may already
+   be used by a running query (as infrequent as it may be), a lookup is
+   performed per id generation. In practice this search should happen only
+   once per newly generated id
+*/
+static int generate_unique_id(ares_channel channel)
+{
+  int id;
+
+  do {
+	id = ares__generate_new_id(&channel->id_key);
+  } while (find_query_by_id(channel,id));
+
+  return id;
+}
+
 void ares_query(ares_channel channel, const char *name, int dnsclass,
                 int type, ares_callback callback, void *arg)
 {
@@ -50,7 +108,8 @@ void ares_query(ares_channel channel, const char *name, int dnsclass,
   rd = !(channel->flags & ARES_FLAG_NORECURSE);
   status = ares_mkquery(name, dnsclass, type, channel->next_id, rd, &qbuf,
                         &qlen);
-  channel->next_id++;
+  channel->next_id = generate_unique_id(channel);
+
   if (status != ARES_SUCCESS)
     {
       callback(arg, status, NULL, 0);