From b4073a760205f6c341425fe5dd28313e3a12f567 Mon Sep 17 00:00:00 2001 From: james Date: Sat, 26 Jul 2008 23:08:29 +0000 Subject: 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 --- misc.c | 12 ++++--- options.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++------------- route.c | 54 ++++++++++++++++++++++---------- route.h | 2 ++ socket.c | 42 +++++++++++++++++++++++++ socket.h | 1 + tun.c | 77 +++++++++++++++++++++++++++++++-------------- 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 -- cgit