From c8e6f3e5fb9cd8dd80b8a7278338d0e9064be97f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 7 Sep 2017 10:22:10 +0200 Subject: vpn: fix ifindex of parent IP config in apply_parent_device_config() When creating the NMIP4Config/NMIP6Config instance, we must always use the right ifindex. That is the ifindex, on which we want to apply the config. It also means, that for device-based VPNs (those with priv->ip_ifindex set, like OpenVPN), the parent's config must have the ip-ifindex of the parent device. Not of the VPN's device. One effect of this bug is that in add_ip4_vpn_gateway_route() we resolve the route to the external gateway and only accept it if it's on the parent device. But since the ifindex of the config was wrong, we would accept route on the wrong interface. https://bugzilla.gnome.org/show_bug.cgi?id=787370 --- src/devices/nm-device.c | 10 ++++++++++ src/vpn/nm-vpn-connection.c | 41 ++++++++++++++++------------------------- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 8001e74b0..55ad6c323 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -10038,6 +10038,11 @@ nm_device_replace_vpn4_config (NMDevice *self, NMIP4Config *old, NMIP4Config *co { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + nm_assert (!old || NM_IS_IP4_CONFIG (old)); + nm_assert (!config || NM_IS_IP4_CONFIG (config)); + nm_assert (!old || nm_ip4_config_get_ifindex (old) == nm_device_get_ip_ifindex (self)); + nm_assert (!config || nm_ip4_config_get_ifindex (config) == nm_device_get_ip_ifindex (self)); + if (!_replace_vpn_config_in_list (&priv->vpn4_configs, (GObject *) old, (GObject *) config)) return; @@ -10167,6 +10172,11 @@ nm_device_replace_vpn6_config (NMDevice *self, NMIP6Config *old, NMIP6Config *co { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + nm_assert (!old || NM_IS_IP6_CONFIG (old)); + nm_assert (!config || NM_IS_IP6_CONFIG (config)); + nm_assert (!old || nm_ip6_config_get_ifindex (old) == nm_device_get_ip_ifindex (self)); + nm_assert (!config || nm_ip6_config_get_ifindex (config) == nm_device_get_ip_ifindex (self)); + if (!_replace_vpn_config_in_list (&priv->vpn6_configs, (GObject *) old, (GObject *) config)) return; diff --git a/src/vpn/nm-vpn-connection.c b/src/vpn/nm-vpn-connection.c index 11eb9a495..1a858060f 100644 --- a/src/vpn/nm-vpn-connection.c +++ b/src/vpn/nm-vpn-connection.c @@ -1075,40 +1075,31 @@ apply_parent_device_config (NMVpnConnection *self) { NMVpnConnectionPrivate *priv = NM_VPN_CONNECTION_GET_PRIVATE (self); NMDevice *parent_dev = nm_active_connection_get_device (NM_ACTIVE_CONNECTION (self)); + int ifindex; NMIP4Config *vpn4_parent_config = NULL; NMIP6Config *vpn6_parent_config = NULL; - if (priv->ip_ifindex > 0) { - if (priv->ip4_config) { - vpn4_parent_config = nm_ip4_config_new (nm_netns_get_multi_idx (priv->netns), - priv->ip_ifindex); - } - if (priv->ip6_config) { - vpn6_parent_config = nm_ip6_config_new (nm_netns_get_multi_idx (priv->netns), - priv->ip_ifindex); - } - } else { - int ifindex; - + ifindex = nm_device_get_ip_ifindex (parent_dev); + if (ifindex > 0) { /* If the VPN didn't return a network interface, it is a route-based * VPN (like kernel IPSec) and all IP addressing and routing should * be done on the parent interface instead. */ - - /* Also clear the gateway. We don't configure the gateway as part of the - * vpn-config. Instead we tell NMDefaultRouteManager directly about the - * default route. */ - ifindex = nm_device_get_ip_ifindex (parent_dev); - if (ifindex > 0) { - if (priv->ip4_config) { - vpn4_parent_config = nm_ip4_config_new (nm_netns_get_multi_idx (priv->netns), - ifindex); + if (priv->ip4_config) { + vpn4_parent_config = nm_ip4_config_new (nm_netns_get_multi_idx (priv->netns), + ifindex); + if (priv->ip_ifindex <= 0) nm_ip4_config_merge (vpn4_parent_config, priv->ip4_config, NM_IP_CONFIG_MERGE_NO_DNS); - } - if (priv->ip6_config) { - vpn6_parent_config = nm_ip6_config_new (nm_netns_get_multi_idx (priv->netns), - ifindex); + } + if (priv->ip6_config) { + vpn6_parent_config = nm_ip6_config_new (nm_netns_get_multi_idx (priv->netns), + ifindex); + if (priv->ip_ifindex <= 0) { nm_ip6_config_merge (vpn6_parent_config, priv->ip6_config, NM_IP_CONFIG_MERGE_NO_DNS); + + /* Also clear the gateway. We don't configure the gateway as part of the + * vpn-config. Instead we tell NMDefaultRouteManager directly about the + * default route. */ nm_ip6_config_set_gateway (vpn6_parent_config, NULL); } } -- cgit v1.2.3 From cac10198f652d216c688c69e2ab8dcfb51435498 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 7 Sep 2017 10:35:12 +0200 Subject: vpn: apply parent config in nm_vpn_connection_apply_config() first In practice, it shouldn't matter much, because NM may frequently reapply the IP config. Hence, it anyway must cope with the fact that IP config from a previous iteration is already applied on the VPN device, before applying it to the parent device. Anyway, it makes a bit more sense to apply it first the the parent device. --- src/vpn/nm-vpn-connection.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vpn/nm-vpn-connection.c b/src/vpn/nm-vpn-connection.c index 1a858060f..284a909aa 100644 --- a/src/vpn/nm-vpn-connection.c +++ b/src/vpn/nm-vpn-connection.c @@ -1135,6 +1135,8 @@ nm_vpn_connection_apply_config (NMVpnConnection *self) { NMVpnConnectionPrivate *priv = NM_VPN_CONNECTION_GET_PRIVATE (self); + apply_parent_device_config (self); + if (priv->ip_ifindex > 0) { nm_platform_link_set_up (nm_netns_get_platform (priv->netns), priv->ip_ifindex, NULL); @@ -1157,8 +1159,6 @@ nm_vpn_connection_apply_config (NMVpnConnection *self) nm_platform_link_set_mtu (nm_netns_get_platform (priv->netns), priv->ip_ifindex, priv->mtu); } - apply_parent_device_config (self); - nm_default_route_manager_ip4_update_default_route (nm_netns_get_default_route_manager (priv->netns), self); nm_default_route_manager_ip6_update_default_route (nm_netns_get_default_route_manager (priv->netns), self); -- cgit v1.2.3 From 7046ce5332f821665eb43ba5ada043b8843537e5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 7 Sep 2017 11:11:32 +0200 Subject: platform: add oif argument to nm_platform_ip_route_get() Analog to `ip route get $DST oif $IFACE`. --- src/platform/nm-linux-platform.c | 8 ++++++++ src/platform/nm-platform.c | 8 ++++++-- src/platform/nm-platform.h | 2 ++ src/platform/tests/test-route.c | 2 ++ src/vpn/nm-vpn-connection.c | 2 ++ 5 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index a018f6b70..5c952a0b2 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -6033,6 +6033,7 @@ static NMPlatformError ip_route_get (NMPlatform *platform, int addr_family, gconstpointer address, + int oif_ifindex, NMPObject **out_route) { const gboolean is_v4 = (addr_family == AF_INET); @@ -6066,6 +6067,13 @@ ip_route_get (NMPlatform *platform, if (!_nl_addattr_l (&req.n, sizeof (req), RTA_DST, address, addr_len)) nm_assert_not_reached (); + if (oif_ifindex > 0) { + gint32 ii = oif_ifindex; + + if (!_nl_addattr_l (&req.n, sizeof (req), RTA_OIF, &ii, sizeof (ii))) + nm_assert_not_reached (); + } + seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; nle = _nl_send_nlmsghdr (platform, &req.n, &seq_result, DELAYED_ACTION_RESPONSE_TYPE_ROUTE_GET, &route); if (nle < 0) { diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 8542c945b..7e91b7547 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -3888,12 +3888,14 @@ NMPlatformError nm_platform_ip_route_get (NMPlatform *self, int addr_family, gconstpointer address /* in_addr_t or struct in6_addr */, + int oif_ifindex, NMPObject **out_route) { nm_auto_nmpobj NMPObject *route = NULL; NMPlatformError result; char buf[NM_UTILS_INET_ADDRSTRLEN]; char buf_err[200]; + char buf_oif[64]; _CHECK_SELF (self, klass, FALSE); @@ -3901,9 +3903,10 @@ nm_platform_ip_route_get (NMPlatform *self, g_return_val_if_fail (NM_IN_SET (addr_family, AF_INET, AF_INET6), NM_PLATFORM_ERROR_BUG); - _LOGT ("route: get IPv%c route for: %s", + _LOGT ("route: get IPv%c route for: %s%s", addr_family == AF_INET ? '4' : '6', - inet_ntop (addr_family, address, buf, sizeof (buf))); + inet_ntop (addr_family, address, buf, sizeof (buf)), + oif_ifindex > 0 ? nm_sprintf_buf (buf_oif, " oif %d", oif_ifindex) : ""); if (!klass->ip_route_get) result = NM_PLATFORM_ERROR_OPNOTSUPP; @@ -3911,6 +3914,7 @@ nm_platform_ip_route_get (NMPlatform *self, result = klass->ip_route_get (self, addr_family, address, + oif_ifindex, &route); } diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index e14c71512..b29e6668d 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -823,6 +823,7 @@ typedef struct { NMPlatformError (*ip_route_get) (NMPlatform *self, int addr_family, gconstpointer address, + int oif_ifindex, NMPObject **out_route); gboolean (*check_support_kernel_extended_ifa_flags) (NMPlatform *); @@ -1186,6 +1187,7 @@ gboolean nm_platform_ip_route_flush (NMPlatform *self, NMPlatformError nm_platform_ip_route_get (NMPlatform *self, int addr_family, gconstpointer address, + int oif_ifindex, NMPObject **out_route); const char *nm_platform_link_to_string (const NMPlatformLink *link, char *buf, gsize len); diff --git a/src/platform/tests/test-route.c b/src/platform/tests/test-route.c index f749d7514..2c00fadaa 100644 --- a/src/platform/tests/test-route.c +++ b/src/platform/tests/test-route.c @@ -409,6 +409,7 @@ test_ip_route_get (void) result = nm_platform_ip_route_get (NM_PLATFORM_GET, AF_INET, &a, + nmtst_get_rand_int () % 2 ? 0 : ifindex, &route); g_assert (result == NM_PLATFORM_ERROR_SUCCESS); @@ -417,6 +418,7 @@ test_ip_route_get (void) g_assert (route->parent._ref_count == 1); r = NMP_OBJECT_CAST_IP4_ROUTE (route); g_assert (r->rt_cloned); + g_assert (r->ifindex == ifindex); g_assert (r->network == a); g_assert (r->plen == 32); diff --git a/src/vpn/nm-vpn-connection.c b/src/vpn/nm-vpn-connection.c index 284a909aa..0cb5cb766 100644 --- a/src/vpn/nm-vpn-connection.c +++ b/src/vpn/nm-vpn-connection.c @@ -741,6 +741,7 @@ add_ip4_vpn_gateway_route (NMIP4Config *config, if (nm_platform_ip_route_get (platform, AF_INET, &vpn_gw, + 0, (NMPObject **) &route_resolved) == NM_PLATFORM_ERROR_SUCCESS) { const NMPlatformIP4Route *r = NMP_OBJECT_CAST_IP4_ROUTE (route_resolved); @@ -811,6 +812,7 @@ add_ip6_vpn_gateway_route (NMIP6Config *config, if (nm_platform_ip_route_get (platform, AF_INET6, vpn_gw, + 0, (NMPObject **) &route_resolved) == NM_PLATFORM_ERROR_SUCCESS) { const NMPlatformIP6Route *r = NMP_OBJECT_CAST_IP6_ROUTE (route_resolved); -- cgit v1.2.3 From f6dabee068e1ad20a6d3f5775b196d944996778c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 7 Sep 2017 11:26:29 +0200 Subject: vpn: improve resolving route to external VPN gateway by restricting oif Previously, we would try to resolve the route in general (unrestricted to a certain ifindex), and reject it the result wasn't on the parent device. Now, use the oif argument, and resolve the route only on the parent device. The problem is that kernel would pretend that the destination is onlink, if there is no route to it. Hence, hack around that by only accepting an onlink route, if the VPN gateway itself is site-local. Yes, there are scenarios where this will still lead to a wrong guess. See related bug rh#1489343 for kernel. --- src/vpn/nm-vpn-connection.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/src/vpn/nm-vpn-connection.c b/src/vpn/nm-vpn-connection.c index 0cb5cb766..e116657e8 100644 --- a/src/vpn/nm-vpn-connection.c +++ b/src/vpn/nm-vpn-connection.c @@ -713,13 +713,18 @@ add_ip4_vpn_gateway_route (NMIP4Config *config, NMIP4Config *parent_config; guint32 parent_gw; NMPlatformIP4Route route; + int ifindex; guint32 route_metric; nm_auto_nmpobj const NMPObject *route_resolved = NULL; g_return_if_fail (NM_IS_IP4_CONFIG (config)); g_return_if_fail (NM_IS_DEVICE (parent_device)); g_return_if_fail (vpn_gw != 0); - nm_assert (nm_ip4_config_get_ifindex (config) > 0); + + ifindex = nm_ip4_config_get_ifindex (config); + + nm_assert (ifindex > 0); + nm_assert (ifindex == nm_device_get_ip_ifindex (parent_device)); /* Set up a route to the VPN gateway's public IP address through the default * network device if the VPN gateway is on a different subnet. @@ -741,17 +746,25 @@ add_ip4_vpn_gateway_route (NMIP4Config *config, if (nm_platform_ip_route_get (platform, AF_INET, &vpn_gw, - 0, + ifindex, (NMPObject **) &route_resolved) == NM_PLATFORM_ERROR_SUCCESS) { const NMPlatformIP4Route *r = NMP_OBJECT_CAST_IP4_ROUTE (route_resolved); - if (r->ifindex == nm_ip4_config_get_ifindex (config)) - parent_gw = r->gateway; + if (r->ifindex == ifindex) { + /* `ip route get` always resolves the route, even if the destination is unreachable. + * In which case, it pretends the destination is directly reachable. + * + * So, only accept direct routes, if @vpn_gw is a private network. */ + if ( r->gateway + || nm_utils_ip_is_site_local (AF_INET, &vpn_gw)) + parent_gw = r->gateway; + } } route_metric = nm_device_get_ip4_route_metric (parent_device); memset (&route, 0, sizeof (route)); + route.ifindex = ifindex; route.network = vpn_gw; route.plen = 32; route.gateway = parent_gw; @@ -783,13 +796,18 @@ add_ip6_vpn_gateway_route (NMIP6Config *config, NMIP6Config *parent_config; const struct in6_addr *parent_gw; NMPlatformIP6Route route; + int ifindex; guint32 route_metric; nm_auto_nmpobj const NMPObject *route_resolved = NULL; g_return_if_fail (NM_IS_IP6_CONFIG (config)); g_return_if_fail (NM_IS_DEVICE (parent_device)); g_return_if_fail (vpn_gw != NULL); - nm_assert (nm_ip6_config_get_ifindex (config) > 0); + + ifindex = nm_ip6_config_get_ifindex (config); + + nm_assert (ifindex > 0); + nm_assert (ifindex == nm_device_get_ip_ifindex (parent_device)); parent_config = nm_device_get_ip6_config (parent_device); g_return_if_fail (parent_config != NULL); @@ -812,17 +830,25 @@ add_ip6_vpn_gateway_route (NMIP6Config *config, if (nm_platform_ip_route_get (platform, AF_INET6, vpn_gw, - 0, + ifindex, (NMPObject **) &route_resolved) == NM_PLATFORM_ERROR_SUCCESS) { const NMPlatformIP6Route *r = NMP_OBJECT_CAST_IP6_ROUTE (route_resolved); - if (r->ifindex == nm_ip6_config_get_ifindex (config)) - parent_gw = &r->gateway; + if (r->ifindex == ifindex) { + /* `ip route get` always resolves the route, even if the destination is unreachable. + * In which case, it pretends the destination is directly reachable. + * + * So, only accept direct routes, if @vpn_gw is a private network. */ + if ( !IN6_IS_ADDR_UNSPECIFIED (&r->gateway) + || nm_utils_ip_is_site_local (AF_INET6, &vpn_gw)) + parent_gw = &r->gateway; + } } route_metric = nm_device_get_ip6_route_metric (parent_device); memset (&route, 0, sizeof (route)); + route.ifindex = ifindex; route.network = *vpn_gw; route.plen = 128; route.gateway = *parent_gw; -- cgit v1.2.3 From 10fc74819c3646ed0c8072adbac803a3e1a8a36b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 7 Sep 2017 11:32:06 +0200 Subject: vpn: only rely on `ip route get` to resolve route to external VPN gateway Until recently, we would only consier the IP config of the parent device to determine the route to the external VPN gateway. We changed that, to additionally improve the guess by letting kernel resolve the route. Now, drop checking the parent's config entirely. The only thing that matters is the here and now runtime configuraion on the parent device. And for that we ask kernel to resolve the route. --- src/vpn/nm-vpn-connection.c | 66 ++++++++++++++++----------------------------- 1 file changed, 23 insertions(+), 43 deletions(-) diff --git a/src/vpn/nm-vpn-connection.c b/src/vpn/nm-vpn-connection.c index e116657e8..bd86007ac 100644 --- a/src/vpn/nm-vpn-connection.c +++ b/src/vpn/nm-vpn-connection.c @@ -710,8 +710,8 @@ add_ip4_vpn_gateway_route (NMIP4Config *config, in_addr_t vpn_gw, NMPlatform *platform) { - NMIP4Config *parent_config; - guint32 parent_gw; + guint32 parent_gw = 0; + gboolean has_parent_gw = FALSE; NMPlatformIP4Route route; int ifindex; guint32 route_metric; @@ -726,23 +726,8 @@ add_ip4_vpn_gateway_route (NMIP4Config *config, nm_assert (ifindex > 0); nm_assert (ifindex == nm_device_get_ip_ifindex (parent_device)); - /* Set up a route to the VPN gateway's public IP address through the default - * network device if the VPN gateway is on a different subnet. - */ - parent_config = nm_device_get_ip4_config (parent_device); - g_return_if_fail (parent_config != NULL); - - parent_gw = nm_ip4_config_get_gateway (parent_config); - - /* If the VPN gateway is in the same subnet as one of the parent device's - * IP addresses, don't add the host route to it, but a route through the - * parent device. - */ - if (nm_ip4_config_destination_is_direct (parent_config, vpn_gw, 32)) - parent_gw = 0; - - /* actually, let's ask kernel how to reach @vpn_gw. If (and only if) - * the destination is on @parent_device, then we take that @parent_gw. */ + /* Ask kernel how to reach @vpn_gw. We can only inject the route in + * @parent_device, so whatever we resolve, it can only be on @ifindex. */ if (nm_platform_ip_route_get (platform, AF_INET, &vpn_gw, @@ -756,11 +741,16 @@ add_ip4_vpn_gateway_route (NMIP4Config *config, * * So, only accept direct routes, if @vpn_gw is a private network. */ if ( r->gateway - || nm_utils_ip_is_site_local (AF_INET, &vpn_gw)) + || nm_utils_ip_is_site_local (AF_INET, &vpn_gw)) { parent_gw = r->gateway; + has_parent_gw = TRUE; + } } } + if (!has_parent_gw) + return; + route_metric = nm_device_get_ip4_route_metric (parent_device); memset (&route, 0, sizeof (route)); @@ -793,8 +783,8 @@ add_ip6_vpn_gateway_route (NMIP6Config *config, const struct in6_addr *vpn_gw, NMPlatform *platform) { - NMIP6Config *parent_config; - const struct in6_addr *parent_gw; + const struct in6_addr *parent_gw = NULL; + gboolean has_parent_gw = FALSE; NMPlatformIP6Route route; int ifindex; guint32 route_metric; @@ -809,24 +799,8 @@ add_ip6_vpn_gateway_route (NMIP6Config *config, nm_assert (ifindex > 0); nm_assert (ifindex == nm_device_get_ip_ifindex (parent_device)); - parent_config = nm_device_get_ip6_config (parent_device); - g_return_if_fail (parent_config != NULL); - - /* we add a direct route to the VPN gateway, but we only do that - * on the @parent_device. That is probably not correct in every case... */ - parent_gw = nm_ip6_config_get_gateway (parent_config); - if (!parent_gw) - return; - - /* If the VPN gateway is in the same subnet as one of the parent device's - * IP addresses, don't add the host route to it, but a route through the - * parent device. - */ - if (nm_ip6_config_destination_is_direct (parent_config, vpn_gw, 128)) - parent_gw = &in6addr_any; - - /* actually, let's ask kernel how to reach @vpn_gw. If (and only if) - * the destination is on @parent_device, then we take that @parent_gw. */ + /* Ask kernel how to reach @vpn_gw. We can only inject the route in + * @parent_device, so whatever we resolve, it can only be on @ifindex. */ if (nm_platform_ip_route_get (platform, AF_INET6, vpn_gw, @@ -840,18 +814,24 @@ add_ip6_vpn_gateway_route (NMIP6Config *config, * * So, only accept direct routes, if @vpn_gw is a private network. */ if ( !IN6_IS_ADDR_UNSPECIFIED (&r->gateway) - || nm_utils_ip_is_site_local (AF_INET6, &vpn_gw)) + || nm_utils_ip_is_site_local (AF_INET6, &vpn_gw)) { parent_gw = &r->gateway; + has_parent_gw = TRUE; + } } } + if (!has_parent_gw) + return; + route_metric = nm_device_get_ip6_route_metric (parent_device); memset (&route, 0, sizeof (route)); route.ifindex = ifindex; route.network = *vpn_gw; route.plen = 128; - route.gateway = *parent_gw; + if (parent_gw) + route.gateway = *parent_gw; route.rt_source = NM_IP_CONFIG_SOURCE_VPN; route.metric = route_metric; nm_ip6_config_add_route (config, &route); @@ -861,7 +841,7 @@ add_ip6_vpn_gateway_route (NMIP6Config *config, * routes include a subnet that matches the parent device's subnet, * the parent device's gateway would get routed through the VPN and fail. */ - if (!IN6_IS_ADDR_UNSPECIFIED (parent_gw)) { + if (parent_gw && !IN6_IS_ADDR_UNSPECIFIED (parent_gw)) { memset (&route, 0, sizeof (route)); route.network = *parent_gw; route.plen = 128; -- cgit v1.2.3