trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From zw...@apache.org
Subject svn commit: r989974 - in /trafficserver/traffic/branches/2.0.x: CHANGES iocore/dns/DNS.cc iocore/dns/DNSConnection.cc iocore/dns/P_DNSConnection.h iocore/dns/P_DNSProcessor.h proxy/config/records.config.in proxy/mgmt2/RecordsConfig.cc
Date Thu, 26 Aug 2010 23:27:43 GMT
Author: zwoop
Date: Thu Aug 26 23:27:43 2010
New Revision: 989974

URL: http://svn.apache.org/viewvc?rev=989974&view=rev
Log:
TS-425 Backport of fixes for CVE-2010-2952

Modified:
    trafficserver/traffic/branches/2.0.x/CHANGES
    trafficserver/traffic/branches/2.0.x/iocore/dns/DNS.cc
    trafficserver/traffic/branches/2.0.x/iocore/dns/DNSConnection.cc
    trafficserver/traffic/branches/2.0.x/iocore/dns/P_DNSConnection.h
    trafficserver/traffic/branches/2.0.x/iocore/dns/P_DNSProcessor.h
    trafficserver/traffic/branches/2.0.x/proxy/config/records.config.in
    trafficserver/traffic/branches/2.0.x/proxy/mgmt2/RecordsConfig.cc

Modified: trafficserver/traffic/branches/2.0.x/CHANGES
URL: http://svn.apache.org/viewvc/trafficserver/traffic/branches/2.0.x/CHANGES?rev=989974&r1=989973&r2=989974&view=diff
==============================================================================
--- trafficserver/traffic/branches/2.0.x/CHANGES (original)
+++ trafficserver/traffic/branches/2.0.x/CHANGES Thu Aug 26 23:27:43 2010
@@ -2,6 +2,8 @@
 
 Changes with Apache Traffic Server 2.0.1
 
+  *) Port of CVE-2010-2952 for 2.0.x [TS-TS-425].
+
   *) Backport part of TS-322 that deals with indexing arrays with char
    (author: Marcus Ruckert) [TS-334].
 

Modified: trafficserver/traffic/branches/2.0.x/iocore/dns/DNS.cc
URL: http://svn.apache.org/viewvc/trafficserver/traffic/branches/2.0.x/iocore/dns/DNS.cc?rev=989974&r1=989973&r2=989974&view=diff
==============================================================================
--- trafficserver/traffic/branches/2.0.x/iocore/dns/DNS.cc (original)
+++ trafficserver/traffic/branches/2.0.x/iocore/dns/DNS.cc Thu Aug 26 23:27:43 2010
@@ -44,7 +44,7 @@ int dns_failover_number = DEFAULT_FAILOV
 int dns_failover_period = DEFAULT_FAILOVER_PERIOD;
 int dns_failover_try_period = DEFAULT_FAILOVER_TRY_PERIOD;
 int dns_max_dns_in_flight = MAX_DNS_IN_FLIGHT;
-unsigned int dns_sequence_number = 0;
+int dns_validate_qname = 0;
 unsigned int dns_handler_initialized = 0;
 int dns_ns_rr = 0;
 int dns_ns_rr_init_down = 1;
@@ -61,7 +61,7 @@ ClassAllocator<HostEnt> dnsBufAllocator(
 // Function Prototypes
 //
 static bool dns_process(DNSHandler * h, HostEnt * ent, int len);
-static DNSEntry *get_dns(DNSHandler * h, u_short id);
+static DNSEntry *get_dns(DNSHandler * h, uint16 id);
 // returns true when e is done
 static void dns_result(DNSHandler * h, DNSEntry * e, HostEnt * ent, bool retry);
 static void write_dns(DNSHandler * h);
@@ -117,6 +117,7 @@ DNSProcessor::start(int)
   IOCORE_EstablishStaticConfigInt32(dns_failover_number, "proxy.config.dns.failover_number");
   IOCORE_EstablishStaticConfigInt32(dns_failover_period, "proxy.config.dns.failover_period");
   IOCORE_EstablishStaticConfigInt32(dns_max_dns_in_flight, "proxy.config.dns.max_dns_in_flight");
+  IOCORE_EstablishStaticConfigInt32(dns_validate_qname, "proxy.config.dns.validate_query_name");
   IOCORE_EstablishStaticConfigInt32(dns_ns_rr, "proxy.config.dns.round_robin_nameservers");
   IOCORE_ReadConfigStringAlloc(dns_ns_list, "proxy.config.dns.nameservers");
 
@@ -152,17 +153,6 @@ DNSProcessor::open(unsigned int aip, int
 void
 DNSProcessor::dns_init()
 {
-  ink64 sval, cval = 0;
-
-  DNS_READ_DYN_STAT(dns_sequence_number_stat, sval, cval);
-
-  if (cval > 0) {
-    dns_sequence_number = (unsigned int)
-      (cval + DNS_SEQUENCE_NUMBER_RESTART_OFFSET);
-  } else {                      // select a sequence number at random
-    dns_sequence_number = (unsigned int) (ink_get_hrtime() / HRTIME_MSECOND);
-  }
-  Debug("dns", "initial dns_sequence_number = %d\n", (u_short) dns_sequence_number);
   gethostname(try_server_names[0], 255);
   Debug("dns", "localhost=%s\n", try_server_names[0]);
   Debug("dns", "Round-robin nameservers = %d\n", dns_ns_rr);
@@ -385,7 +375,7 @@ DNSHandler::open_con(unsigned int aip, i
 #error port me
 #endif
     if (r < 0) {        // Add the FD to epoll fd
-      Error("iocore_dns", "open_con: Failed to add %d server to epoll list\n", icon);
+      Error("[iocore_dns] open_con: Failed to add %d server to epoll list\n", icon);
     } else {
       con[icon].num = icon;
       Debug("dns", "opening connection %d.%d.%d.%d:%d SUCCEEDED for %d", DOT_SEPARATED(aip),
aport, icon);
@@ -784,7 +774,7 @@ DNSHandler::mainEvent(int event, Event *
 
 /** Find a DNSEntry by id. */
 inline static DNSEntry *
-get_dns(DNSHandler * h, u_short id)
+get_dns(DNSHandler * h, uint16 id)
 {
   for (DNSEntry * e = h->entries.head; e; e = (DNSEntry *) e->link.next) {
     if (e->once_written_flag)
@@ -798,6 +788,7 @@ get_dns(DNSHandler * h, u_short id)
   return NULL;
 }
 
+/** Find a DNSEntry by query name and type. */
 inline static DNSEntry *
 get_entry(DNSHandler * h, char *qname, int qtype)
 {
@@ -849,6 +840,39 @@ write_dns(DNSHandler * h)
   h->in_write_dns = false;
 }
 
+uint16
+DNSHandler::get_query_id()
+{
+  uint16 q1, q2;
+  q2 = q1 = (uint16)(generator.random() & 0xFFFF);
+  if (query_id_in_use(q2)) {
+    uint16 i = q2>>6;
+    while (qid_in_flight[i] == INTU64_MAX) {
+      if (++i ==  sizeof(qid_in_flight)/sizeof(uint64)) {
+        i = 0;
+      }
+      if (i == q1>>6) {
+        Error("[iocore_dns] get_query_id: Exhausted all DNS query ids");
+        return q1;
+      }
+    }
+    i <<= 6;
+    q2 &= 0x3F;
+    while (query_id_in_use(i+q2)) {
+      ++q2;
+      q2 &= 0x3F;
+      if (q2 == (q1 & 0x3F)) {
+        Error("[iocore_dns] get_query_id: Exhausted all DNS query ids");
+        return q1;
+      }
+    }
+    q2 += i;
+  }
+
+  set_query_id_in_use(q2);
+  return q2;
+}
+
 /**
   Construct and Write the request for a single entry (using send(3N)).
 
@@ -874,19 +898,15 @@ write_dns_event(DNSHandler * h, DNSEntry
   }
 #endif
 
-  int id = dns_retries - e->retries;
-  if (id >= MAX_DNS_RETRIES)
-    id = MAX_DNS_RETRIES - 1;   // limit id history
-
-  ++dns_sequence_number;
-
-  DNS_SET_DYN_COUNT(dns_sequence_number_stat, dns_sequence_number);
-
 #ifdef DNS_PROXY
   if (!e->proxy) {
 #endif
-    u_short i = (u_short) dns_sequence_number;
+    uint16 i = h->get_query_id();
     ((HEADER *) (buffer))->id = htons(i);
+    if(e->id[dns_retries - e->retries] >= 0) {
+      //clear previous id in case named was switched or domain was expanded
+      h->release_query_id(e->id[dns_retries - e->retries]);
+    }
     e->id[dns_retries - e->retries] = i;
 #ifdef DNS_PROXY
   } else {
@@ -1186,8 +1206,20 @@ DNSEntry::post(DNSHandler * h, HostEnt *
   //
   if (timeout)
     timeout->cancel(this);
-  mutex = NULL;
+
+#ifdef DNS_PROXY
+  if (!proxy) {
+#endif
+    for (int i = 0; i < MAX_DNS_RETRIES; i++) {
+      if (id[i] < 0)
+        break;
+      h->release_query_id(id[i]);      
+    }
+#ifdef DNS_PROXY
+  }
+#endif
   action.mutex = NULL;
+  mutex = NULL;
   dnsEntryAllocator.free(this);
   return 0;
 }
@@ -1202,6 +1234,20 @@ DNSEntry::postEvent(int event, Event * e
   if (result_ent)
     if (ink_atomic_increment(&result_ent->ref_count, -1) == 1)
       dnsProcessor.free_hostent(result_ent);
+
+#ifdef DNS_PROXY
+  if (!proxy) {
+#endif
+    for (int i = 0; i < MAX_DNS_RETRIES; i++) {
+      if (id[i] < 0)
+        break;
+      dnsH->release_query_id(id[i]);      
+    }
+#ifdef DNS_PROXY
+  }
+#endif
+  action.mutex = NULL;
+  mutex = NULL;
   dnsEntryAllocator.free(this);
   return EVENT_DONE;
 }
@@ -1212,7 +1258,7 @@ dns_process(DNSHandler * handler, HostEn
 {
   ProxyMutex *mutex = handler->mutex;
   HEADER *h = (HEADER *) (buf->buf);
-  DNSEntry *e = get_dns(handler, (u_short) ntohs(h->id));
+  DNSEntry *e = get_dns(handler, (uint16) ntohs(h->id));
   bool retry = false;
   bool server_ok = true;
   inku32 temp_ttl = 0;
@@ -1221,7 +1267,7 @@ dns_process(DNSHandler * handler, HostEn
   // Do we have an entry for this id?
   //
   if (!e || !e->written_flag) {
-    Debug("dns", "unknown DNS id = %u", (u_short) ntohs(h->id));
+    Debug("dns", "unknown DNS id = %u", (uint16) ntohs(h->id));
     if (!handler->hostent_cache)
       handler->hostent_cache = buf;
     else
@@ -1286,15 +1332,42 @@ dns_process(DNSHandler * handler, HostEn
     int n;
     unsigned char *srv[50];
     int num_srv = 0;
+    int rname_len = -1;
 
     //
     // Expand name
     //
     if ((n = ink_dn_expand((u_char *) h, eom, cp, bp, buflen)) < 0)
       goto Lerror;
+
+    // Should we validate the query name?
+    if (dns_validate_qname) {
+      int qlen = e->qname_len;
+      int rlen = strlen((char *)bp);
+
+      rname_len = rlen; // Save for later use
+      if ((qlen > 0) && ('.' == e->qname[qlen-1]))
+        --qlen;
+      if ((rlen > 0) && ('.' == bp[rlen-1]))
+        --rlen;
+      // TODO: At some point, we might want to care about the case here, and use an algorithm
+      // to randomly pick upper case characters in the query, and validate the response with
+      // case sensitivity.
+      if ((qlen != rlen) || (strncasecmp(e->qname, (const char*)bp, qlen) != 0)) {
+        // Bad mojo, forged?
+        Warning("received DNS response with query name of %s, but response query name is
%s", e->qname, bp);
+        goto Lerror;
+      } else {
+        Debug("dns", "query name validated properly for %s", e->qname);
+      }
+    }
+
     cp += n + QFIXEDSZ;
     if (e->qtype == T_A) {
-      n = strlen((char *) bp) + 1;
+      if (-1 == rname_len)
+        n = strlen((char *)bp) + 1;
+      else
+        n = rname_len + 1;
       buf->ent.h_name = (char *) bp;
       bp += n;
       buflen -= n;
@@ -1315,6 +1388,8 @@ dns_process(DNSHandler * handler, HostEn
     // Once it's full, a new entry get inputted into try_server_names round-
     // robin style every 50 success dns response.
 
+    // TODO: Why do we do strlen(e->qname) ? That should be available in 
+    // e->qname_len, no ?
     if (local_num_entries >= DEFAULT_NUM_TRY_SERVER) {
       if ((attempt_num_entries % 50) == 0) {
         try_servers = (try_servers + 1) % SIZE(try_server_names);
@@ -1522,24 +1597,6 @@ ink_dns_init(ModuleVersion v)
   // create a stat block for HostDBStats
   dns_rsb = RecAllocateRawStatBlock((int) DNS_Stat_Count);
 
-  IOCORE_RegisterConfigInteger(RECT_CONFIG, "proxy.config.dns.retries", 3, RECU_DYNAMIC,
RECC_NULL, NULL);
-
-  IOCORE_RegisterConfigInteger(RECT_CONFIG, "proxy.config.dns.lookup_timeout", 30, RECU_DYNAMIC,
RECC_NULL, NULL);
-
-  IOCORE_RegisterConfigInteger(RECT_CONFIG,
-                               "proxy.config.dns.search_default_domains", 1, RECU_DYNAMIC,
RECC_NULL, NULL);
-
-  IOCORE_RegisterConfigInteger(RECT_CONFIG, "proxy.config.dns.failover_number", 4, RECU_DYNAMIC,
RECC_NULL, NULL);
-
-  IOCORE_RegisterConfigInteger(RECT_CONFIG, "proxy.config.dns.failover_period", 60, RECU_DYNAMIC,
RECC_NULL, NULL);
-
-  IOCORE_RegisterConfigInteger(RECT_CONFIG, "proxy.config.dns.max_dns_in_flight", 60, RECU_DYNAMIC,
RECC_NULL, NULL);
-
-  IOCORE_RegisterConfigInteger(RECT_CONFIG,
-                               "proxy.config.dns.round_robin_nameservers", 0, RECU_DYNAMIC,
RECC_NULL, NULL);
-
-  IOCORE_RegisterConfigString(RECT_CONFIG, "proxy.config.dns.nameservers", NULL, RECU_DYNAMIC,
RECC_NULL, NULL);
-
   //
   // Register statistics callbacks
   //
@@ -1578,9 +1635,6 @@ ink_dns_init(ModuleVersion v)
                      "proxy.process.dns.in_flight",
                      RECD_INT, RECP_NON_PERSISTENT, (int) dns_in_flight_stat, RecRawStatSyncSum);
 
-  RecRegisterRawStat(dns_rsb, RECT_PROCESS,
-                     "proxy.process.dns.sequence_number",
-                     RECD_INT, RECP_NULL, (int) dns_sequence_number_stat, RecRawStatSyncCount);
 }
 
 

Modified: trafficserver/traffic/branches/2.0.x/iocore/dns/DNSConnection.cc
URL: http://svn.apache.org/viewvc/trafficserver/traffic/branches/2.0.x/iocore/dns/DNSConnection.cc?rev=989974&r1=989973&r2=989974&view=diff
==============================================================================
--- trafficserver/traffic/branches/2.0.x/iocore/dns/DNSConnection.cc (original)
+++ trafficserver/traffic/branches/2.0.x/iocore/dns/DNSConnection.cc Thu Aug 26 23:27:43 2010
@@ -39,8 +39,8 @@
 // set in the OS
 // #define RECV_BUF_SIZE            (1024*64)
 // #define SEND_BUF_SIZE            (1024*64)
-#define FIRST_RANDOM_PORT        16000
-#define LAST_RANDOM_PORT         32000
+#define FIRST_RANDOM_PORT        (16000)
+#define LAST_RANDOM_PORT         (60000)
 
 #define ROUNDUP(x, y) ((((x)+((y)-1))/(y))*(y))
 
@@ -49,7 +49,7 @@
 //
 
 DNSConnection::DNSConnection():
-fd(NO_FD), num(0), epoll_ptr(NULL)
+  fd(NO_FD), generator(time(NULL) ^ (long) this), num(0), epoll_ptr(NULL)
 {
   memset(&sa, 0, sizeof(struct sockaddr_in));
 }
@@ -97,18 +97,16 @@ DNSConnection::connect(unsigned int ip, 
 
   if (bind_random_port) {
     int retries = 0;
-    int offset = 0;
     while (retries++ < 10000) {
       struct sockaddr_in bind_sa;
       memset(&sa, 0, sizeof(bind_sa));
       bind_sa.sin_family = AF_INET;
       bind_sa.sin_addr.s_addr = INADDR_ANY;
-      int p = time(NULL) + offset;
-      p = (p % (LAST_RANDOM_PORT - FIRST_RANDOM_PORT)) + FIRST_RANDOM_PORT;
+      uint32 p = generator.random();
+      p = (uint16)((p % (LAST_RANDOM_PORT - FIRST_RANDOM_PORT)) + FIRST_RANDOM_PORT);
       bind_sa.sin_port = htons(p);
-      Debug("dns", "random port = %d\n", p);
+      Debug("dns", "random port = %u\n", p);
       if ((res = socketManager.ink_bind(fd, (struct sockaddr *) &bind_sa, sizeof(bind_sa),
Proto)) < 0) {
-        offset += 101;
         continue;
       }
       goto Lok;

Modified: trafficserver/traffic/branches/2.0.x/iocore/dns/P_DNSConnection.h
URL: http://svn.apache.org/viewvc/trafficserver/traffic/branches/2.0.x/iocore/dns/P_DNSConnection.h?rev=989974&r1=989973&r2=989974&view=diff
==============================================================================
--- trafficserver/traffic/branches/2.0.x/iocore/dns/P_DNSConnection.h (original)
+++ trafficserver/traffic/branches/2.0.x/iocore/dns/P_DNSConnection.h Thu Aug 26 23:27:43
2010
@@ -61,7 +61,7 @@ struct DNSConnection
 {
   int fd;
   struct sockaddr_in sa;
-
+  InkRand generator;
   int num;                      //added by YTS Team, yamsat
 
     Link<DNSConnection> link;        //added by YTS Team, yamsat

Modified: trafficserver/traffic/branches/2.0.x/iocore/dns/P_DNSProcessor.h
URL: http://svn.apache.org/viewvc/trafficserver/traffic/branches/2.0.x/iocore/dns/P_DNSProcessor.h?rev=989974&r1=989973&r2=989974&view=diff
==============================================================================
--- trafficserver/traffic/branches/2.0.x/iocore/dns/P_DNSProcessor.h (original)
+++ trafficserver/traffic/branches/2.0.x/iocore/dns/P_DNSProcessor.h Thu Aug 26 23:27:43 2010
@@ -27,13 +27,18 @@
 #define DNS_PROXY 1
 /*
   #include "I_DNS.h"
-  #include "inktomi++.h"
   #include <arpa/nameser.h>
   #include "I_Cache.h"
   #include "P_Net.h"
 */
 #include "I_EventSystem.h"
 
+// From new ink_port.h on trunk
+typedef unsigned short uint16;
+typedef unsigned int uint32;
+typedef unsigned long long uint64;
+#define INTU64_MAX (18446744073709551615ULL)
+
 #define MAX_NAMED                           32
 #define DEFAULT_DNS_RETRIES                 3
 #define MAX_DNS_RETRIES                     5
@@ -243,6 +248,11 @@ struct DNSHandler:Continuation
   ink_res_state m_res;
   int txn_lookup_timeout;
 
+  InkRand generator;
+  // bitmap of query ids in use
+  uint64 qid_in_flight[(USHRT_MAX+1)/64];
+
+
   void received_one(int i)
   {
     failover_number[i] = failover_soon_number[i] = crossed_failover_number[i] = 0;
@@ -285,6 +295,19 @@ struct DNSHandler:Continuation
   void retry_named(int ndx, ink_hrtime t, bool reopen = true);
   void try_primary_named(bool reopen = true);
   void switch_named(int ndx);
+  uint16 get_query_id();
+
+  void release_query_id(uint16 qid) {
+    qid_in_flight[qid >> 6] &= (uint64)~(0x1ULL << (qid & 0x3F));
+  };
+
+  void set_query_id_in_use(uint16 qid) {
+    qid_in_flight[qid >> 6] |= (uint64)(0x1ULL << (qid & 0x3F));
+  };
+
+  bool query_id_in_use(uint16 qid) {
+    return (qid_in_flight[qid >> 6] & (uint64)(0x1ULL << (qid & 0x3F)))
!= 0;
+  };
 
   DNSHandler();
 };
@@ -297,7 +320,7 @@ inline DNSHandler::DNSHandler()
   n_con(0),
   options(0),
   in_flight(0), name_server(0), in_write_dns(0), hostent_cache(0), last_primary_retry(0),
last_primary_reopen(0),
-  m_res(0), txn_lookup_timeout(0)
+  m_res(0), txn_lookup_timeout(0), generator(time(NULL) ^ (long) this)
 {
   for (int i = 0; i < MAX_NAMED; i++) {
     ifd[i] = -1;
@@ -306,6 +329,7 @@ inline DNSHandler::DNSHandler()
     crossed_failover_number[i] = 0;
     ns_down[i] = 1;
   }
+  memset(&qid_in_flight, 0, sizeof(qid_in_flight));  
   SET_HANDLER(&DNSHandler::startEvent);
   Debug("net_epoll", "inline DNSHandler::DNSHandler()");
 }

Modified: trafficserver/traffic/branches/2.0.x/proxy/config/records.config.in
URL: http://svn.apache.org/viewvc/trafficserver/traffic/branches/2.0.x/proxy/config/records.config.in?rev=989974&r1=989973&r2=989974&view=diff
==============================================================================
--- trafficserver/traffic/branches/2.0.x/proxy/config/records.config.in (original)
+++ trafficserver/traffic/branches/2.0.x/proxy/config/records.config.in Thu Aug 26 23:27:43
2010
@@ -338,6 +338,10 @@ CONFIG proxy.config.dns.splitdns.def_dom
 CONFIG proxy.config.dns.url_expansions STRING NULL
 CONFIG proxy.config.dns.round_robin_nameservers INT 0
 CONFIG proxy.config.dns.nameservers STRING NULL
+   # This provides additional resilience against DNS forgery, particularly in
+   # forward or transparent proxies, but requires that the resolver populates
+   # the queries section of the response properly.
+CONFIG proxy.config.dns.validate_query_name INT 0
 ##############################################################################
 #
 # DNS Proxy

Modified: trafficserver/traffic/branches/2.0.x/proxy/mgmt2/RecordsConfig.cc
URL: http://svn.apache.org/viewvc/trafficserver/traffic/branches/2.0.x/proxy/mgmt2/RecordsConfig.cc?rev=989974&r1=989973&r2=989974&view=diff
==============================================================================
--- trafficserver/traffic/branches/2.0.x/proxy/mgmt2/RecordsConfig.cc (original)
+++ trafficserver/traffic/branches/2.0.x/proxy/mgmt2/RecordsConfig.cc Thu Aug 26 23:27:43
2010
@@ -2587,6 +2587,8 @@ RecordElement RecordsConfig[] = {
   ,
   {CONFIG, "proxy.config.dns.max_dns_in_flight", "", INK_INT, "60", RU_REREAD, RR_NULL, RC_NULL,
NULL, RA_NULL}
   ,
+  {CONFIG, "proxy.config.dns.validate_query_name", "", INK_INT, "0", RU_REREAD, RR_NULL,
RC_NULL, "[0-1]", RA_NULL}
+  ,
   {CONFIG, "proxy.config.dns.splitDNS.enabled", "", INK_INT, "0", RU_REREAD, RR_NULL, RC_INT,
"[0-1]", RA_NULL}
   ,
   {CONFIG, "proxy.config.dns.splitdns.filename", "", INK_STRING, "splitdns.config", RU_NULL,
RR_NULL, RC_NULL, NULL,



Mime
View raw message