Perform additional input validation on options pulled

by client from server.  Fixes --iproute vulnerability.


git-svn-id: http://svn.openvpn.net/projects/openvpn/branches/BETA21/openvpn@3126 e7ae566f-a301-0410-adde-c780ea21d3b5

7 files changed, 230 insertions, 64 deletions

diff --git a/misc.c b/misc.c
index 5b7cf3e..c647fd0 100644
--- a/misc.c
+++ b/misc.c
@@ -40,7 +40,7 @@
 #include "memdbg.h"
 
 #ifdef CONFIG_FEATURE_IPROUTE
-const char *iproute_path = IPROUTE_PATH;
+const char *iproute_path = IPROUTE_PATH; /* GLOBAL */
 #endif
 
 /* contains an SSEC_x value defined in misc.h */
@@ -913,9 +913,13 @@ setenv_str (struct env_set *es, const char *name, const char *value)
 void
 setenv_str_safe (struct env_set *es, const char *name, const char *value)
 {
-  char buf[64];
-  openvpn_snprintf (buf, sizeof(buf), "OPENVPN_%s", name);
-  setenv_str (es, buf, value);
+  uint8_t b[64];
+  struct buffer buf;
+  buf_set_write (&buf, b, sizeof (b));
+  if (buf_printf (&buf, "OPENVPN_%s", name))
+    setenv_str (es, BSTR(&buf), value);
+  else
+    msg (M_WARN, "setenv_str_safe: name overflow");
 }
 
 void
diff --git a/options.c b/options.c
index 3a53c31..68a36c2 100644
--- a/options.c
+++ b/options.c
@@ -890,10 +890,17 @@ dhcp_option_address_parse (const char *name, const char *parm, in_addr_t *array,
     }
   else
     {
-      bool error = false;
-      const in_addr_t addr = get_ip_addr (parm, msglevel, &error);
-      if (!error)
-	array[(*len)++] = addr;
+      if (ip_addr_dotted_quad_safe (parm))
+	{
+	  bool error = false;
+	  const in_addr_t addr = get_ip_addr (parm, msglevel, &error);
+	  if (!error)
+	    array[(*len)++] = addr;
+	}
+      else
+	{
+	  msg (msglevel, "dhcp-option parameter %s '%s' must be an IP address", name, parm);
+	}
     }
 }
 
@@ -2495,20 +2502,24 @@ foreign_option (struct options *o, char *argv[], int len, struct env_set *es)
       struct buffer value = alloc_buf_gc (OPTION_PARM_SIZE, &gc);
       int i;
       bool first = true;
+      bool good = true;
 
-      buf_printf (&name, "foreign_option_%d", o->foreign_option_index + 1);
+      good &= buf_printf (&name, "foreign_option_%d", o->foreign_option_index + 1);
       ++o->foreign_option_index;
       for (i = 0; i < len; ++i)
 	{
 	  if (argv[i])
 	    {
 	      if (!first)
-		buf_printf (&value, " ");
-	      buf_printf (&value, "%s", argv[i]);
+		good &= buf_printf (&value, " ");
+	      good &= buf_printf (&value, "%s", argv[i]);
 	      first = false;
 	    }
 	}
-      setenv_str (es, BSTR(&name), BSTR(&value));
+      if (good)
+	setenv_str (es, BSTR(&name), BSTR(&value));
+      else
+	msg (M_WARN, "foreign_option: name/value overflow");
       gc_free (&gc);
     }
 }
@@ -3238,6 +3249,7 @@ add_option (struct options *options,
 {
   struct gc_arena gc = gc_new ();
   ASSERT (MAX_PARMS >= 5);
+  const bool pull_mode = BOOL_CAST (permission_mask & OPT_P_PULL_MODE);
 
   if (!file)
     {
@@ -3264,11 +3276,18 @@ add_option (struct options *options,
 
       read_config_file (options, p[1], level, file, line, msglevel, permission_mask, option_types_found, es);
     }
+#if 0
+  else if (streq (p[0], "foreign-option") && p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_IPWIN32);
+      foreign_option (options, p, 3, es);
+    }
+#endif
   else if (streq (p[0], "echo") || streq (p[0], "parameter"))
     {
       struct buffer string = alloc_buf_gc (OPTION_PARM_SIZE, &gc);
       int j;
-      const bool pull_mode = BOOL_CAST (permission_mask & OPT_P_PULL_MODE);
+      bool good = true;
 
       VERIFY_PERMISSION (OPT_P_ECHO);
 
@@ -3277,16 +3296,21 @@ add_option (struct options *options,
 	  if (!p[j])
 	    break;
 	  if (j > 1)
-	    buf_printf (&string, " ");
-	  buf_printf (&string, "%s", p[j]);
+	    good &= buf_printf (&string, " ");
+	  good &= buf_printf (&string, "%s", p[j]);
 	}
-      msg (M_INFO, "%s:%s",
-	   pull_mode ? "ECHO-PULL" : "ECHO",
-	   BSTR (&string));
+      if (good)
+	{
+	  msg (M_INFO, "%s:%s",
+	       pull_mode ? "ECHO-PULL" : "ECHO",
+	       BSTR (&string));
 #ifdef ENABLE_MANAGEMENT
-      if (management)
-	management_echo (management, BSTR (&string), pull_mode);
+	  if (management)
+	    management_echo (management, BSTR (&string), pull_mode);
 #endif
+	}
+      else
+	msg (M_WARN, "echo/parameter option overflow");
     }
 #ifdef ENABLE_MANAGEMENT
   else if (streq (p[0], "management") && p[1] && p[2])
@@ -3408,7 +3432,13 @@ add_option (struct options *options,
   else if (streq (p[0], "lladdr") && p[1])
     {
       VERIFY_PERMISSION (OPT_P_UP);
-      options->lladdr = p[1];
+      if (ip_addr_dotted_quad_safe (p[1]))
+	options->lladdr = p[1];
+      else
+	{
+	  msg (msglevel, "lladdr parm '%s' must be an IP address", p[1]);
+	  goto err;
+	}
     }
   else if (streq (p[0], "topology") && p[1])
     {
@@ -3423,15 +3453,23 @@ add_option (struct options *options,
 #ifdef CONFIG_FEATURE_IPROUTE
   else if (streq (p[0], "iproute") && p[1])
     {
-      VERIFY_PERMISSION (OPT_P_UP);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       iproute_path = p[1];
     }
 #endif
   else if (streq (p[0], "ifconfig") && p[1] && p[2])
     {
       VERIFY_PERMISSION (OPT_P_UP);
-      options->ifconfig_local = p[1];
-      options->ifconfig_remote_netmask = p[2];
+      if (ip_addr_dotted_quad_safe (p[1]) && ip_addr_dotted_quad_safe (p[2]))
+	{
+	  options->ifconfig_local = p[1];
+	  options->ifconfig_remote_netmask = p[2];
+	}
+      else
+	{
+	  msg (msglevel, "ifconfig parms '%s' and '%s' must be IP addresses", p[1], p[2]);
+	  goto err;
+	}
     }
   else if (streq (p[0], "ifconfig-noexec"))
     {
@@ -4176,12 +4214,38 @@ add_option (struct options *options,
     {
       VERIFY_PERMISSION (OPT_P_ROUTE);
       rol_check_alloc (options);
+      if (pull_mode)
+	{
+	  if (!ip_addr_dotted_quad_safe (p[1]) && !is_special_addr (p[1]))
+	    {
+	      msg (msglevel, "route parameter network/IP '%s' is not an IP address", p[1]);
+	      goto err;
+	    }
+	  if (p[2] && !ip_addr_dotted_quad_safe (p[2]))
+	    {
+	      msg (msglevel, "route parameter netmask '%s' is not an IP address", p[2]);
+	      goto err;
+	    }
+	  if (p[3] && !ip_addr_dotted_quad_safe (p[3]) && !is_special_addr (p[3]))
+	    {
+	      msg (msglevel, "route parameter gateway '%s' is not an IP address", p[3]);
+	      goto err;
+	    }
+	}
       add_route_to_option_list (options->routes, p[1], p[2], p[3], p[4]);
     }
   else if (streq (p[0], "route-gateway") && p[1])
     {
       VERIFY_PERMISSION (OPT_P_ROUTE_EXTRAS);
-      options->route_default_gateway = p[1];      
+      if (ip_addr_dotted_quad_safe (p[1]) || is_special_addr (p[1]))
+	{
+	  options->route_default_gateway = p[1];
+	}
+      else
+	{
+	  msg (msglevel, "route-gateway parm '%s' must be an IP address", p[1]);
+	  goto err;
+	}
     }
   else if (streq (p[0], "route-metric") && p[1])
     {
diff --git a/route.c b/route.c
index 5b7b036..bc312e8 100644
--- a/route.c
+++ b/route.c
@@ -139,43 +139,65 @@ get_special_addr (const struct route_special_addr *spec,
 		  in_addr_t *out,
 		  bool *status)
 {
-  *status = true;
+  if (status)
+    *status = true;
   if (!strcmp (string, "vpn_gateway"))
     {
-      if (spec->remote_endpoint_defined)
-	*out = spec->remote_endpoint;
-      else
+      if (spec)
 	{
-	  msg (M_INFO, PACKAGE_NAME " ROUTE: vpn_gateway undefined");
-	  *status = false;
+	  if (spec->remote_endpoint_defined)
+	    *out = spec->remote_endpoint;
+	  else
+	    {
+	      msg (M_INFO, PACKAGE_NAME " ROUTE: vpn_gateway undefined");
+	      if (status)
+		*status = false;
+	    }
 	}
       return true;
     }
   else if (!strcmp (string, "net_gateway"))
     {
-      if (spec->net_gateway_defined)
-	*out = spec->net_gateway;
-      else
+      if (spec)
 	{
-	  msg (M_INFO, PACKAGE_NAME " ROUTE: net_gateway undefined -- unable to get default gateway from system");
-	  *status = false;
+	  if (spec->net_gateway_defined)
+	    *out = spec->net_gateway;
+	  else
+	    {
+	      msg (M_INFO, PACKAGE_NAME " ROUTE: net_gateway undefined -- unable to get default gateway from system");
+	      if (status)
+		*status = false;
+	    }
 	}
       return true;
     }
   else if (!strcmp (string, "remote_host"))
     {
-      if (spec->remote_host_defined)
-	*out = spec->remote_host;
-      else
+      if (spec)
 	{
-	  msg (M_INFO, PACKAGE_NAME " ROUTE: remote_host undefined");
-	  *status = false;
+	  if (spec->remote_host_defined)
+	    *out = spec->remote_host;
+	  else
+	    {
+	      msg (M_INFO, PACKAGE_NAME " ROUTE: remote_host undefined");
+	      if (status)
+		*status = false;
+	    }
 	}
       return true;
     }
   return false;
 }
 
+bool
+is_special_addr (const char *addr_str)
+{
+  if (addr_str)
+    return get_special_addr (NULL, addr_str, NULL, NULL);
+  else
+    return false;
+}
+
 static bool
 init_route (struct route *r,
 	    const struct route_option *ro,
diff --git a/route.h b/route.h
index e57db75..674d200 100644
--- a/route.h
+++ b/route.h
@@ -150,6 +150,8 @@ void delete_routes (struct route_list *rl,
 
 void setenv_routes (struct env_set *es, const struct route_list *rl);
 
+bool is_special_addr (const char *addr_str);
+
 #if AUTO_USERID
 bool get_default_gateway_mac_addr (unsigned char *macaddr);
 #endif
diff --git a/socket.c b/socket.c
index c1b16ad..a7ed55f 100644
--- a/socket.c
+++ b/socket.c
@@ -252,6 +252,48 @@ openvpn_inet_aton (const char *dotted_quad, struct in_addr *addr)
     return OIA_HOSTNAME; /* probably a hostname */
 }
 
+bool
+ip_addr_dotted_quad_safe (const char *dotted_quad)
+{
+  /* verify non-NULL */
+  if (!dotted_quad)
+    return false;
+
+  /* verify length is within limits */
+  if (strlen (dotted_quad) > 15)
+    return false;
+
+  /* verify that all chars are either numeric or '.' and that no numeric
+     substring is greater than 3 chars */
+  {
+    int nnum = 0;
+    const char *p = dotted_quad;
+    int c;
+
+    while ((c = *p++))
+      {
+	if (c >= '0' && c <= '9')
+	  {
+	    ++nnum;
+	    if (nnum > 3)
+	      return false;
+	  }
+	else if (c == '.')
+	  {
+	    nnum = 0;
+	  }
+	else
+	  return false;
+      }
+  }
+
+  /* verify that string will convert to IP address */
+  {
+    struct in_addr a;
+    return openvpn_inet_aton (dotted_quad, &a) == OIA_IP;
+  }
+}
+
 static void
 update_remote (const char* host,
 	       struct openvpn_sockaddr *addr,
diff --git a/socket.h b/socket.h
index 9084fa0..8eb768d 100644
--- a/socket.h
+++ b/socket.h
@@ -396,6 +396,7 @@ void link_socket_update_buffer_sizes (struct link_socket *ls, int rcvbuf, int sn
 #define OIA_IP         1
 #define OIA_ERROR     -1
 int openvpn_inet_aton (const char *dotted_quad, struct in_addr *addr);
+bool ip_addr_dotted_quad_safe (const char *dotted_quad);
 
 socket_descriptor_t create_socket_tcp (void);
 
diff --git a/tun.c b/tun.c
index c6916ef..22debbe 100644
--- a/tun.c
+++ b/tun.c
@@ -3643,17 +3643,21 @@ tun_standby (struct tuntap *tt)
  */
 
 static void
-write_dhcp_u8 (struct buffer *buf, const int type, const int data)
+write_dhcp_u8 (struct buffer *buf, const int type, const int data, bool *error)
 {
   if (!buf_safe (buf, 3))
-    msg (M_FATAL, "write_dhcp_u8: buffer overflow building DHCP options");
+    {
+      *error = true;
+      msg (M_WARN, "write_dhcp_u8: buffer overflow building DHCP options");
+      return;
+    }
   buf_write_u8 (buf, type);
   buf_write_u8 (buf, 1);
   buf_write_u8 (buf, data);
 }
 
 static void
-write_dhcp_u32_array (struct buffer *buf, const int type, const uint32_t *data, const unsigned int len)
+write_dhcp_u32_array (struct buffer *buf, const int type, const uint32_t *data, const unsigned int len, bool *error)
 {
   if (len > 0)
     {
@@ -3661,9 +3665,17 @@ write_dhcp_u32_array (struct buffer *buf, const int type, const uint32_t *data,
       const int size = len * sizeof (uint32_t);
 
       if (!buf_safe (buf, 2 + size))
-	msg (M_FATAL, "write_dhcp_u32_array: buffer overflow building DHCP options");
+	{
+	  *error = true;
+	  msg (M_WARN, "write_dhcp_u32_array: buffer overflow building DHCP options");
+	  return;
+	}
       if (size < 1 || size > 255)
-	msg (M_FATAL, "write_dhcp_u32_array: size (%d) must be > 0 and <= 255", size);
+	{
+	  *error = true;
+	  msg (M_WARN, "write_dhcp_u32_array: size (%d) must be > 0 and <= 255", size);
+	  return;
+	}
       buf_write_u8 (buf, type);
       buf_write_u8 (buf, size);
       for (i = 0; i < len; ++i)
@@ -3672,46 +3684,61 @@ write_dhcp_u32_array (struct buffer *buf, const int type, const uint32_t *data,
 }
 
 static void
-write_dhcp_str (struct buffer *buf, const int type, const char *str)
+write_dhcp_str (struct buffer *buf, const int type, const char *str, bool *error)
 {
   const int len = strlen (str);
   if (!buf_safe (buf, 2 + len))
-    msg (M_FATAL, "write_dhcp_str: buffer overflow building DHCP options");
+    {
+      *error = true;
+      msg (M_WARN, "write_dhcp_str: buffer overflow building DHCP options");
+      return;
+    }
   if (len < 1 || len > 255)
-    msg (M_FATAL, "write_dhcp_str: string '%s' must be > 0 bytes and <= 255 bytes", str);
+    {
+      *error = true;
+      msg (M_WARN, "write_dhcp_str: string '%s' must be > 0 bytes and <= 255 bytes", str);
+      return;
+    }
   buf_write_u8 (buf, type);
   buf_write_u8 (buf, len);
   buf_write (buf, str, len);
 }
 
-static void
+static bool
 build_dhcp_options_string (struct buffer *buf, const struct tuntap_options *o)
 {
+  bool error = false;
   if (o->domain)
-    write_dhcp_str (buf, 15, o->domain);
+    write_dhcp_str (buf, 15, o->domain, &error);
 
   if (o->netbios_scope)
-    write_dhcp_str (buf, 47, o->netbios_scope);
+    write_dhcp_str (buf, 47, o->netbios_scope, &error);
 
   if (o->netbios_node_type)
-    write_dhcp_u8 (buf, 46, o->netbios_node_type);
+    write_dhcp_u8 (buf, 46, o->netbios_node_type, &error);
 
-  write_dhcp_u32_array (buf, 6, (uint32_t*)o->dns, o->dns_len);
-  write_dhcp_u32_array (buf, 44, (uint32_t*)o->wins, o->wins_len);
-  write_dhcp_u32_array (buf, 42, (uint32_t*)o->ntp, o->ntp_len);
-  write_dhcp_u32_array (buf, 45, (uint32_t*)o->nbdd, o->nbdd_len);
+  write_dhcp_u32_array (buf, 6, (uint32_t*)o->dns, o->dns_len, &error);
+  write_dhcp_u32_array (buf, 44, (uint32_t*)o->wins, o->wins_len, &error);
+  write_dhcp_u32_array (buf, 42, (uint32_t*)o->ntp, o->ntp_len, &error);
+  write_dhcp_u32_array (buf, 45, (uint32_t*)o->nbdd, o->nbdd_len, &error);
 
   /* the MS DHCP server option 'Disable Netbios-over-TCP/IP
      is implemented as vendor option 001, value 002.
      A value of 001 means 'leave NBT alone' which is the default */
   if (o->disable_nbt)
   {
-    buf_write_u8 (buf, 43);
+    if (!buf_safe (buf, 8))
+      {
+	msg (M_WARN, "build_dhcp_options_string: buffer overflow building DHCP options");
+	return false;
+      }
+    buf_write_u8 (buf,  43);
     buf_write_u8 (buf,  6);  /* total length field */
     buf_write_u8 (buf,  0x001);
     buf_write_u8 (buf,  4);  /* length of the vendor specified field */
     buf_write_u32 (buf, 0x002);
   }
+  return !error;
 }
 
 void
@@ -4006,12 +4033,16 @@ open_tun (const char *dev, const char *dev_type, const char *dev_node, bool ipv6
       if (tt->options.dhcp_options)
 	{
 	  struct buffer buf = alloc_buf (256);
-	  build_dhcp_options_string (&buf, &tt->options);
-	  msg (D_DHCP_OPT, "DHCP option string: %s", format_hex (BPTR (&buf), BLEN (&buf), 0, &gc));
-	  if (!DeviceIoControl (tt->hand, TAP_IOCTL_CONFIG_DHCP_SET_OPT,
-				BPTR (&buf), BLEN (&buf),
-				BPTR (&buf), BLEN (&buf), &len, NULL))
-	    msg (M_FATAL, "ERROR: The TAP-Win32 driver rejected a TAP_IOCTL_CONFIG_DHCP_SET_OPT DeviceIoControl call");
+	  if (build_dhcp_options_string (&buf, &tt->options))
+	    {
+	      msg (D_DHCP_OPT, "DHCP option string: %s", format_hex (BPTR (&buf), BLEN (&buf), 0, &gc));
+	      if (!DeviceIoControl (tt->hand, TAP_IOCTL_CONFIG_DHCP_SET_OPT,
+				    BPTR (&buf), BLEN (&buf),
+				    BPTR (&buf), BLEN (&buf), &len, NULL))
+		msg (M_FATAL, "ERROR: The TAP-Win32 driver rejected a TAP_IOCTL_CONFIG_DHCP_SET_OPT DeviceIoControl call");
+	    }
+	  else
+	    msg (M_WARN, "DHCP option string not set due to error");
 	  free_buf (&buf);
 	}
 #endif