aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorjames <james@e7ae566f-a301-0410-adde-c780ea21d3b5>2008-07-26 23:08:29 +0000
committerjames <james@e7ae566f-a301-0410-adde-c780ea21d3b5>2008-07-26 23:08:29 +0000
commitb4073a760205f6c341425fe5dd28313e3a12f567 (patch)
treeed22c69f356d8704f19318ef30124679f5e1f4f8
parentFixed compiler warnings in Windows build (MinGW). (diff)
downloadopenvpn-b4073a760205f6c341425fe5dd28313e3a12f567.tar.xz
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
-rw-r--r--misc.c12
-rw-r--r--options.c106
-rw-r--r--route.c54
-rw-r--r--route.h2
-rw-r--r--socket.c42
-rw-r--r--socket.h1
-rw-r--r--tun.c77
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