[PATCH net-next v2] ipv6: sysctl to restrict candidate source addresses

[PATCH net-next v2] ipv6: sysctl to restrict candidate source addresses

[Erik Kline]

Per RFC 6724, section 4, "Candidate Source Addresses":

    It is RECOMMENDED that the candidate source addresses be the set
    of unicast addresses assigned to the interface that will be used
    to send to the destination (the "outgoing" interface).

Add a sysctl to enable this behaviour.

Signed-off-by: Erik Kline <ek@google.com>
---
 Documentation/networking/ip-sysctl.txt |  7 +++++++
 include/linux/ipv6.h                   |  1 +
 include/uapi/linux/ipv6.h              |  1 +
 net/ipv6/addrconf.c                    | 30 +++++++++++++++++++++++++-----
 4 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index 5fae770..c3bf04d 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -1455,6 +1455,13 @@ router_solicitations - INTEGER
 	routers are present.
 	Default: 3
 
+use_oif_addr - BOOLEAN
+	When enabled, the candidate source addresses for destinations
+	routed via this interface are restricted to the set of addresses
+	configured on this interface (vis. RFC 6724, section 4).
+
+	Default: false
+
 use_tempaddr - INTEGER
 	Preference for Privacy Extensions (RFC3041).
 	  <= 0 : disable Privacy Extensions
diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index 82806c6..4633c88 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -57,6 +57,7 @@ struct ipv6_devconf {
 		bool initialized;
 		struct in6_addr secret;
 	} stable_secret;
+	__s32		use_oif_addr;
 	void		*sysctl;
 };
 
diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h
index 5efa54a..cf9d65a 100644
--- a/include/uapi/linux/ipv6.h
+++ b/include/uapi/linux/ipv6.h
@@ -171,6 +171,7 @@ enum {
 	DEVCONF_USE_OPTIMISTIC,
 	DEVCONF_ACCEPT_RA_MTU,
 	DEVCONF_STABLE_SECRET,
+	DEVCONF_USE_OIF_ADDR,
 	DEVCONF_MAX
 };
 
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 21c2c81..a43687d 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -211,7 +211,8 @@ static struct ipv6_devconf ipv6_devconf __read_mostly = {
 	.accept_ra_mtu		= 1,
 	.stable_secret		= {
 		.initialized = false,
-	}
+	},
+	.use_oif_addr		= 0,
 };
 
 static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
@@ -253,6 +254,7 @@ static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
 	.stable_secret		= {
 		.initialized = false,
 	},
+	.use_oif_addr		= 0,
 };
 
 /* Check if a valid qdisc is available */
@@ -1366,7 +1368,8 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev,
 				*score = &scores[0], *hiscore = &scores[1];
 	struct ipv6_saddr_dst dst;
 	struct net_device *dev;
-	int dst_type;
+	struct inet6_dev *idev;
+	int dst_type, use_oif_addr = 0;
 
 	dst_type = __ipv6_addr_type(daddr);
 	dst.addr = daddr;
@@ -1380,9 +1383,12 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev,
 
 	rcu_read_lock();
 
-	for_each_netdev_rcu(net, dev) {
-		struct inet6_dev *idev;
+	if (dst_dev) {
+		idev = __in6_dev_get(dst_dev);
+		use_oif_addr = (idev) ? idev->cnf.use_oif_addr : 0;
+	}
 
+	for_each_netdev_rcu(net, dev) {
 		/* Candidate Source Address (section 4)
 		 *  - multicast and link-local destination address,
 		 *    the set of candidate source address MUST only
@@ -1394,9 +1400,14 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev,
 		 *    include addresses assigned to interfaces
 		 *    belonging to the same site as the outgoing
 		 *    interface.)
+		 *  - "It is RECOMMENDED that the candidate source addresses
+		 *    be the set of unicast addresses assigned to the
+		 *    interface that will be used to send to the destination
+		 *    (the 'outgoing' interface)." (RFC 6724)
 		 */
 		if (((dst_type & IPV6_ADDR_MULTICAST) ||
-		     dst.scope <= IPV6_ADDR_SCOPE_LINKLOCAL) &&
+		     dst.scope <= IPV6_ADDR_SCOPE_LINKLOCAL ||
+		     use_oif_addr) &&
 		    dst.ifindex && dev->ifindex != dst.ifindex)
 			continue;
 
@@ -4586,6 +4597,7 @@ static inline void ipv6_store_devconf(struct ipv6_devconf *cnf,
 	array[DEVCONF_ACCEPT_RA_FROM_LOCAL] = cnf->accept_ra_from_local;
 	array[DEVCONF_ACCEPT_RA_MTU] = cnf->accept_ra_mtu;
 	/* we omit DEVCONF_STABLE_SECRET for now */
+	array[DEVCONF_USE_OIF_ADDR] = cnf->use_oif_addr;
 }
 
 static inline size_t inet6_ifla6_size(void)
@@ -5585,6 +5597,14 @@ static struct addrconf_sysctl_table
 			.proc_handler	= addrconf_sysctl_stable_secret,
 		},
 		{
+			.procname       = "use_oif_addr",
+			.data           = &ipv6_devconf.use_oif_addr,
+			.maxlen         = sizeof(int),
+			.mode           = 0644,
+			.proc_handler   = proc_dointvec,
+
+		},
+		{
 			/* sentinel */
 		}
 	},
-- 
2.4.3.573.g4eafbef

Re: [PATCH net-next v2] ipv6: sysctl to restrict candidate source addresses

[Lorenzo Colitti]

On Mon, Jul 6, 2015 at 12:05 PM, Erik Kline <ek@google.com> wrote:
> Per RFC 6724, section 4, "Candidate Source Addresses":
>
>     It is RECOMMENDED that the candidate source addresses be the set
>     of unicast addresses assigned to the interface that will be used
>     to send to the destination (the "outgoing" interface).
>
> Add a sysctl to enable this behaviour.
>
> Signed-off-by: Erik Kline <ek@google.com>

I think this is useful, because it ensures that devices with a working
IPv6 configuration on interface A, and a partial IPv6 configuration on
interface B do not attempt to send packets on interface B using
interface A's source address.

Example: there are home routers in the wild that send out an IPv6
router advertisement that configures a default route but no IPv6
address. This change makes it so that the host does not attempt to use
an IPv6 address from another network (e.g., a cellular data
connection) on the home network.

It is also what the RFC recommends.

Acked-by: Lorenzo Colitti <lorenzo@google.com>

Re: [PATCH net-next v2] ipv6: sysctl to restrict candidate source addresses

[Hannes Frederic Sowa]

On Wed, 2015-07-08 at 10:29 +0900, Lorenzo Colitti wrote:
> On Mon, Jul 6, 2015 at 12:05 PM, Erik Kline <ek@google.com> wrote:
> > Per RFC 6724, section 4, "Candidate Source Addresses":
> > 
> >     It is RECOMMENDED that the candidate source addresses be the set
> >     of unicast addresses assigned to the interface that will be used
> >     to send to the destination (the "outgoing" interface).
> > 
> > Add a sysctl to enable this behaviour.
> > 
> > Signed-off-by: Erik Kline <ek@google.com>
> 
> I think this is useful, because it ensures that devices with a working
> IPv6 configuration on interface A, and a partial IPv6 configuration on
> interface B do not attempt to send packets on interface B using
> interface A's source address.
> 
> Example: there are home routers in the wild that send out an IPv6
> router advertisement that configures a default route but no IPv6
> address. This change makes it so that the host does not attempt to use
> an IPv6 address from another network (e.g., a cellular data
> connection) on the home network.
> 
> It is also what the RFC recommends.
> 
> Acked-by: Lorenzo Colitti <lorenzo@google.com>

I wonder a little bit, because addresses which match the outgoing
interface should get a higher score in saddr_eval, thus be automatically
preferred. Is this check not strong enough?

I really wonder if we can improve source address selection and make this
behavior a default without introducing a new sysctl.

Bye,
Hannes

Re: [PATCH net-next v2] ipv6: sysctl to restrict candidate source addresses

[Lorenzo Colitti]

On Wed, Jul 8, 2015 at 5:09 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> I wonder a little bit, because addresses which match the outgoing
> interface should get a higher score in saddr_eval, thus be automatically
> preferred. Is this check not strong enough?

It isn't strong enough because the "prefer outgoing interface" rule is
lower priority than other rules such as "prefer deprecated addresses"
and "prefer matching scope".

For example, consider the case where you have an IPv6 default route
but not an IPv6 address on one interface (e.g., wifi), and a working
configuration (IPv6 default route and IPv6 address) on another
interface (e.g., cellular data). In this situation, the host will send
the packet out on the wifi interface using the source address from the
the cellular interface to try to talk to wifi. This will not work
because the wifi network will almost certainly drop the packet due to
BCP-38 ingress filtering.

> I really wonder if we can improve source address selection and make this
> behavior a default without introducing a new sysctl.

Another option I see is to change the behaviour to follow the RFC
recommendation, and thus always use the matching interface. Is that a
better option?

At the moment, we're going against the explicit recommendation of the
RFC, and I don't see a good reason for that.

Re: [PATCH net-next v2] ipv6: sysctl to restrict candidate source addresses

[Hannes Frederic Sowa]

On Wed, 2015-07-08 at 17:19 +0900, Lorenzo Colitti wrote:
> On Wed, Jul 8, 2015 at 5:09 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > I wonder a little bit, because addresses which match the outgoing
> > interface should get a higher score in saddr_eval, thus be 
> > automatically
> > preferred. Is this check not strong enough?
> 
> It isn't strong enough because the "prefer outgoing interface" rule is
> lower priority than other rules such as "prefer deprecated addresses"
> and "prefer matching scope".
> 
> For example, consider the case where you have an IPv6 default route
> but not an IPv6 address on one interface (e.g., wifi), and a working
> configuration (IPv6 default route and IPv6 address) on another
> interface (e.g., cellular data). In this situation, the host will send
> the packet out on the wifi interface using the source address from the
> the cellular interface to try to talk to wifi. This will not work
> because the wifi network will almost certainly drop the packet due to
> BCP-38 ingress filtering.

So in this case we report errors to user space instead of picking an
address from another interface. If we turn on this knob unconditionally
we cannot leave the set of possible source addresses empty, this will
change application behavior and admin habits too much, I fear.

> > I really wonder if we can improve source address selection and make 
> > this
> > behavior a default without introducing a new sysctl.
> 
> Another option I see is to change the behaviour to follow the RFC
> recommendation, and thus always use the matching interface. Is that a
> better option?
> 
> At the moment, we're going against the explicit recommendation of the
> RFC, and I don't see a good reason for that.

I really would like to come up with a sane works-always behavior for
this, but besides doing a retry on the complete source address selection
algorithm I currently cannot come up with an idea.

Maybe we can tweak saddr_eval a bit.

Thanks,
Hannes

Re: [PATCH net-next v2] ipv6: sysctl to restrict candidate source addresses

[Lorenzo Colitti]

On Wed, Jul 8, 2015 at 5:43 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
>> For example, consider the case where you have an IPv6 default route
>> but not an IPv6 address on one interface (e.g., wifi), and a working
>> configuration (IPv6 default route and IPv6 address) on another
>> interface (e.g., cellular data). In this situation, the host will send
>> the packet out on the wifi interface using the source address from the
>> the cellular interface to try to talk to wifi. This will not work
>> because the wifi network will almost certainly drop the packet due to
>> BCP-38 ingress filtering.
>
> So in this case we report errors to user space instead of picking an
> address from another interface.

So you're proposing that when the sysctl is turned on, if we've picked
an outgoing interface, but that interface only has a link-local
address, then we return EADDRNOTAVAIL instead of picking the
link-local address?

That would preclude the ability to use a link-local address to talk to
an on-link (directly-connected) global address, which is possible
today. But it's at least better than the current behaviour. I'm not
sure how to implement this though - I think it's equivalent to
ordering rule 5 above rule 2, but that's a pretty big change and hard
to make in the current code because the rules are an enum.

> If we turn on this knob unconditionally
> we cannot leave the set of possible source addresses empty, this will
> change application behavior and admin habits too much, I fear.

In general, even if the user turns on this setting, there will always
be at least one address - the link-local address on the interface.
That's what RFC 6724 says the host should use. In this case, what will
happen is that once the source address lookup is complete, when
getaddrinfo() chooses the destination address, it will sort this
combination last, because of rule 2 (prefer matching scope), which is
the highest priority rule apart from rule 1 (avoid unreachable). So
the application will try other combinations, which will include
falling back to IPv4 in the likely case that it is available.

What happens today is that getaddrinfo sorts the (global address, ipv6
address on wrong interface) combination first, and the app gets stuck.

>> At the moment, we're going against the explicit recommendation of the
>> RFC, and I don't see a good reason for that.
>
> I really would like to come up with a sane works-always behavior for
> this, but besides doing a retry on the complete source address selection
> algorithm I currently cannot come up with an idea.

Nothing is works-always in the presence of broken configurations. I
think we should have a mode where we follow the RFC, because in that
case when we get stuck we can convincingly argue that the network is
broken and not our implementation.

In IETF language, "RECOMMENDED" means that "there may exist valid
reasons in particular circumstances to ignore a particular item, but
the full implications must be understood and carefully weighed before
choosing a different course". What I don't understand is - what's the
harm here if we provide a mode that follows the RFC? What are do
gaining by not following it?

Re: [PATCH net-next v2] ipv6: sysctl to restrict candidate source addresses

[Erik Kline]

> I really would like to come up with a sane works-always behavior for
> this, but besides doing a retry on the complete source address selection
> algorithm I currently cannot come up with an idea.
>
> Maybe we can tweak saddr_eval a bit.

I think it all comes down to this: source address selection really
doesn't know anything about routing that could help it make a better
evaluation.

Reading the RFCs that seems to be by design, or at the very least
there is a kind of "implied flat-ish routing table" at work, which the
algorithm works around by having various "prefer same interface" type
of rules. So, after the routing lookup to determine outgoing interface
it's just looking at all the addresses on all the interfaces.  There
is no checking of any of the multiple possible routing tables, in part
because there just isn't all the right information available.

So, I figured the safe thing to do would be to not change existing
default behaviour but just introduce a knob to at least make it
possible to get the RFC recommended behaviour.

---

Re: not having a source address or returning a link-local source for a
global destination:  I think that's perfectly ok, if the knob is set.
Frequently the source address will just be tossed into a salad bowl of
(src, dst) pairs returned from DNS and 3484/6724 sorting will then
help pick a more globally optimum (src, dst) to work with.

Re: [PATCH net-next v2] ipv6: sysctl to restrict candidate source addresses

[David Miller]

From: Erik Kline <ek@google.com>
Date: Wed, 8 Jul 2015 21:41:58 +0900

>> I really would like to come up with a sane works-always behavior for
>> this, but besides doing a retry on the complete source address selection
>> algorithm I currently cannot come up with an idea.
>>
>> Maybe we can tweak saddr_eval a bit.
> 
> I think it all comes down to this: source address selection really
> doesn't know anything about routing that could help it make a better
> evaluation.
> 
> Reading the RFCs that seems to be by design, or at the very least
> there is a kind of "implied flat-ish routing table" at work, which the
> algorithm works around by having various "prefer same interface" type
> of rules. So, after the routing lookup to determine outgoing interface
> it's just looking at all the addresses on all the interfaces.  There
> is no checking of any of the multiple possible routing tables, in part
> because there just isn't all the right information available.
> 
> So, I figured the safe thing to do would be to not change existing
> default behaviour but just introduce a knob to at least make it
> possible to get the RFC recommended behaviour.
> 
> ---
> 
> Re: not having a source address or returning a link-local source for a
> global destination:  I think that's perfectly ok, if the knob is set.
> Frequently the source address will just be tossed into a salad bowl of
> (src, dst) pairs returned from DNS and 3484/6724 sorting will then
> help pick a more globally optimum (src, dst) to work with.

The fact is, if wireless APs are going to reject packets sent out in
the very circumstances this is claiming to help, our default behavior
is broken because communication is not possible.

So we should look at a way at making the new behavior the default, and
in fact that makes sense and we can even optimize this piece of saddr
selection code to not do an iteration over all devices in the system
for no reason at all.  It can just do a quick dev_get_by_index() and
work with that.

Re: [PATCH net-next v2] ipv6: sysctl to restrict candidate source addresses

[YOSHIFUJI Hideaki]

David Miller wrote:

> So we should look at a way at making the new behavior the default, and
> in fact that makes sense and we can even optimize this piece of saddr
> selection code to not do an iteration over all devices in the system
> for no reason at all.  It can just do a quick dev_get_by_index() and
> work with that.
> 

I agree.  I've cooked up a patch for that optimization, so that Eric
can rebase his patch.

Regards,

-- 
Hideaki Yoshifuji <hideaki.yoshifuji@miraclelinux.com>
Technical Division, MIRACLE LINUX CORPORATION