rtm_getifa: Never ignore RTA_IFP

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

rtm_getifa: Never ignore RTA_IFP

Demi M. Obenour
Resending without quoted-printable encoding, in case that helps.
---
If an RTM_ADD command on a routing socket includes an RTA_IFA sockaddr,
and that sockaddr is an exact match for one of the interfaces in the
relevant routing domain, any RTA_IFP sockaddr in the same message is
ignored.  If there are multiple interfaces with the same IP address,
this can cause packets to be sent out the wrong interface.  Fix this
by skipping the exact-match check on RTA_IFA if an RTA_IFP sockaddr
was sent.

The RTA_IFP sockaddr was also being ignored if there was no interface
with the requested index.  Return ENXIO to userspace instead.

The bug can easily be seen by running the following commands as root
on a machine where vether0 and vether1 do not exist, and on which
the subnet 192.0.2.0/24 (reserved for documentation) is not in use:

# ifconfig vether0 destroy 2>/dev/null
# ifconfig vether1 destroy 2>/dev/null
# dummy_mac=fe:ff:ff:ff:ff:ff dummy_ip=192.0.2.5
# ifconfig vether0 create lladdr "$dummy_mac"
# ifconfig vether1 create lladdr "$dummy_mac"
# ifconfig vether0 inet "$dummy_ip" prefixlen 32
# route -n delete "$dummy_ip/32" "$dummy_ip"
# ifconfig vether1 inet "$dummy_ip" prefixlen 32
# route -n delete "$dummy_ip/32" "$dummy_ip"
# route -n add -inet 192.0.2.6 -static -iface -llinfo -link \
    vether1 -ifp vether1 -inet -ifa "$dummy_ip"
# route -n show -inet

On a system with an unpatched kernel, the final command will show
that the route to 192.0.2.6 is via vether0.  If this patch has been
applied, the route to 192.0.2.6 will be (correctly) via vether1.

---
 sys/net/rtsock.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git sys/net/rtsock.c sys/net/rtsock.c
index fa84ddc25e5..dc8446bd78f 100644
--- sys/net/rtsock.c
+++ sys/net/rtsock.c
@@ -1235,7 +1235,8 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
  struct sockaddr_dl *sdl;
 
  sdl = satosdl(info->rti_info[RTAX_IFP]);
- ifp = if_get(sdl->sdl_index);
+ if ((ifp = if_get(sdl->sdl_index)) == NULL)
+ return (ENXIO);
  }
 
 #ifdef IPSEC
@@ -1246,11 +1247,19 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
  * enc0.
  */
  if (info->rti_info[RTAX_DST] &&
-    info->rti_info[RTAX_DST]->sa_family == PF_KEY)
+    info->rti_info[RTAX_DST]->sa_family == PF_KEY) {
  info->rti_ifa = enc_getifa(rtid, 0);
+
+ if (info->rti_ifa != NULL && ifp != NULL &&
+    ifp != info->rti_ifa->ifa_ifp) {
+ if_put(ifp);
+ return (EADDRNOTAVAIL);
+ }
+ }
 #endif
 
- if (info->rti_ifa == NULL && info->rti_info[RTAX_IFA] != NULL)
+ if (info->rti_ifa == NULL && ifp == NULL &&
+    info->rti_info[RTAX_IFA] != NULL)
  info->rti_ifa = ifa_ifwithaddr(info->rti_info[RTAX_IFA], rtid);
 
  if (info->rti_ifa == NULL) {
@@ -1273,10 +1282,15 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
     sa, sa, rtid);
  }
 
- if_put(ifp);
-
- if (info->rti_ifa == NULL)
+ if (info->rti_ifa == NULL) {
+ if_put(ifp);
  return (ENETUNREACH);
+ }
+
+ if (ifp != NULL && ifp != info->rti_ifa->ifa_ifp)
+ panic("rtm_getifa: returned route to wrong interface");
+
+ if_put(ifp);
 
  return (0);
 }
 


Reply | Threaded
Open this post in threaded view
|

Re: rtm_getifa: Never ignore RTA_IFP

Claudio Jeker
On Sun, Sep 13, 2020 at 11:28:11AM -0400, Demi M. Obenour wrote:

> Resending without quoted-printable encoding, in case that helps.
> ---
> If an RTM_ADD command on a routing socket includes an RTA_IFA sockaddr,
> and that sockaddr is an exact match for one of the interfaces in the
> relevant routing domain, any RTA_IFP sockaddr in the same message is
> ignored.  If there are multiple interfaces with the same IP address,
> this can cause packets to be sent out the wrong interface.  Fix this
> by skipping the exact-match check on RTA_IFA if an RTA_IFP sockaddr
> was sent.
>
> The RTA_IFP sockaddr was also being ignored if there was no interface
> with the requested index.  Return ENXIO to userspace instead.
>
> The bug can easily be seen by running the following commands as root
> on a machine where vether0 and vether1 do not exist, and on which
> the subnet 192.0.2.0/24 (reserved for documentation) is not in use:
>
> # ifconfig vether0 destroy 2>/dev/null
> # ifconfig vether1 destroy 2>/dev/null
> # dummy_mac=fe:ff:ff:ff:ff:ff dummy_ip=192.0.2.5
> # ifconfig vether0 create lladdr "$dummy_mac"
> # ifconfig vether1 create lladdr "$dummy_mac"
> # ifconfig vether0 inet "$dummy_ip" prefixlen 32
> # route -n delete "$dummy_ip/32" "$dummy_ip"
> # ifconfig vether1 inet "$dummy_ip" prefixlen 32
> # route -n delete "$dummy_ip/32" "$dummy_ip"
> # route -n add -inet 192.0.2.6 -static -iface -llinfo -link \
>     vether1 -ifp vether1 -inet -ifa "$dummy_ip"
> # route -n show -inet
>
> On a system with an unpatched kernel, the final command will show
> that the route to 192.0.2.6 is via vether0.  If this patch has been
> applied, the route to 192.0.2.6 will be (correctly) via vether1.
>

Can you please explain why you think this fix is needed. Looking at the
code above my first reaction is don't do that. The configuration with the
same IP on multiple interface you do is something I would generally not
recommend.
It seems like you try to do a poor mans link aggregation and should
instead use trunk(4) or aggr(4).

> ---
>  sys/net/rtsock.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git sys/net/rtsock.c sys/net/rtsock.c
> index fa84ddc25e5..dc8446bd78f 100644
> --- sys/net/rtsock.c
> +++ sys/net/rtsock.c
> @@ -1235,7 +1235,8 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
>   struct sockaddr_dl *sdl;
>  
>   sdl = satosdl(info->rti_info[RTAX_IFP]);
> - ifp = if_get(sdl->sdl_index);
> + if ((ifp = if_get(sdl->sdl_index)) == NULL)
> + return (ENXIO);
>   }
>  
>  #ifdef IPSEC
> @@ -1246,11 +1247,19 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
>   * enc0.
>   */
>   if (info->rti_info[RTAX_DST] &&
> -    info->rti_info[RTAX_DST]->sa_family == PF_KEY)
> +    info->rti_info[RTAX_DST]->sa_family == PF_KEY) {
>   info->rti_ifa = enc_getifa(rtid, 0);
> +
> + if (info->rti_ifa != NULL && ifp != NULL &&
> +    ifp != info->rti_ifa->ifa_ifp) {
> + if_put(ifp);
> + return (EADDRNOTAVAIL);
> + }
> + }
>  #endif
>  
> - if (info->rti_ifa == NULL && info->rti_info[RTAX_IFA] != NULL)
> + if (info->rti_ifa == NULL && ifp == NULL &&
> +    info->rti_info[RTAX_IFA] != NULL)
>   info->rti_ifa = ifa_ifwithaddr(info->rti_info[RTAX_IFA], rtid);
>  
>   if (info->rti_ifa == NULL) {
> @@ -1273,10 +1282,15 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
>      sa, sa, rtid);
>   }
>  
> - if_put(ifp);
> -
> - if (info->rti_ifa == NULL)
> + if (info->rti_ifa == NULL) {
> + if_put(ifp);
>   return (ENETUNREACH);
> + }
> +
> + if (ifp != NULL && ifp != info->rti_ifa->ifa_ifp)
> + panic("rtm_getifa: returned route to wrong interface");
> +
> + if_put(ifp);
>  
>   return (0);
>  }
>  
>
>

--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: rtm_getifa: Never ignore RTA_IFP

Demi M. Obenour
On 2020-09-13 12:40, Claudio Jeker wrote:

> On Sun, Sep 13, 2020 at 11:28:11AM -0400, Demi M. Obenour wrote:
>> Resending without quoted-printable encoding, in case that helps.
>> ---
>> If an RTM_ADD command on a routing socket includes an RTA_IFA sockaddr,
>> and that sockaddr is an exact match for one of the interfaces in the
>> relevant routing domain, any RTA_IFP sockaddr in the same message is
>> ignored.  If there are multiple interfaces with the same IP address,
>> this can cause packets to be sent out the wrong interface.  Fix this
>> by skipping the exact-match check on RTA_IFA if an RTA_IFP sockaddr
>> was sent.
>>
>> The RTA_IFP sockaddr was also being ignored if there was no interface
>> with the requested index.  Return ENXIO to userspace instead.
>>
>> The bug can easily be seen by running the following commands as root
>> on a machine where vether0 and vether1 do not exist, and on which
>> the subnet 192.0.2.0/24 (reserved for documentation) is not in use:
>>
>> # ifconfig vether0 destroy 2>/dev/null
>> # ifconfig vether1 destroy 2>/dev/null
>> # dummy_mac=fe:ff:ff:ff:ff:ff dummy_ip=192.0.2.5
>> # ifconfig vether0 create lladdr "$dummy_mac"
>> # ifconfig vether1 create lladdr "$dummy_mac"
>> # ifconfig vether0 inet "$dummy_ip" prefixlen 32
>> # route -n delete "$dummy_ip/32" "$dummy_ip"
>> # ifconfig vether1 inet "$dummy_ip" prefixlen 32
>> # route -n delete "$dummy_ip/32" "$dummy_ip"
>> # route -n add -inet 192.0.2.6 -static -iface -llinfo -link \
>>     vether1 -ifp vether1 -inet -ifa "$dummy_ip"
>> # route -n show -inet
>>
>> On a system with an unpatched kernel, the final command will show
>> that the route to 192.0.2.6 is via vether0.  If this patch has been
>> applied, the route to 192.0.2.6 will be (correctly) via vether1.
>>
>
> Can you please explain why you think this fix is needed. Looking at the
> code above my first reaction is don't do that. The configuration with the
> same IP on multiple interface you do is something I would generally not
> recommend.
> It seems like you try to do a poor mans link aggregation and should
> instead use trunk(4) or aggr(4).

Thanks for your response.  Indeed, having the same IP address on
multiple interfaces is not (contrary to what I thought at first)
common practice, so I will explain why I want to support it.

The short answer is that when OpenBSD is used as a layer 3 IPv4 router
with a significant number of ports, it is far simpler to assign a
single IP to the OpenBSD machine, rather than having to assign a
separate subnet for each connection.  Since there is technically one
network per interface, using the same IP address for all of the ports
saves a significant number of IP addresses.  With one IP address per
machine, each machine only needs to be assigned a single IP address,
no matter how many machines it is connected to.  Furthermore, this
address does not depend on what machines the router is connected to,
which makes automatic IP address assignment much simpler.

The long answer involves a discussion of QubesOS network topology,
and I have omitted it in the interest of brevity.  However, I would
be more than happy to include it upon request.

On Linux, forcing a route to use a particular interface and source
address is trivial:

```
# ip route replace to "$subnet" dev "$iface" src "$src"
```

However, I was not able to find an OpenBSD equivalent for that command.  
The closest I was able to find was

```
route -n add -iface -inet -static "$subnet" -link "$iface" \
    -ifp "$iface" -ifa "$src"
```

but it doesn’t work if there are multiple interfaces that have
$src as their address.  It is also worth noting that I was actually
not using route(8) directly, but rather using a custom C program
and routing sockets; this allowed me to specify the destination MAC
address at the same time.

As it happens, `route -n add -iface -inet -static "$subnet" -link "$iface"`
works.  However, it would not be sufficient if there were multiple
IP addresses assigned to $iface and I needed to select which one
to use.  It also doesn’t let me specify a MAC address, although I
might be able to do so in C.  Finally, when I pass `-ifp "$iface"`
to route(8), I expect that the route will go through $iface.  If the
kernel cannot ensure this, it should return an error to userspace,
rather than silently ignoring what I told it to do.

Thanks for your time and attention.

Sincerely,

Demi