[PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"

[PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"

[Sabrina Dubroca]

This reverts commit 8013d1d7eafb0589ca766db6b74026f76b7f5cb4.

There are several issues with this patch.
It completely cancels the security changes introduced by 6fd99094de2b
("ipv6: Don't reduce hop limit for an interface").
The current default value (min hop limit = 1) can result in the same
denial of service that 6fd99094de2b prevents, but it is hard to define
a correct and sane default value.
More generally, it is yet another IPv6 sysctl, and we already have too
many.

This was introduced to satisfy a TAHI test case which, in my opinion, is
too strict, turning the RFC's "SHOULD" into a "MUST":

    If the received Cur Hop Limit value is non-zero, the host
    SHOULD set its CurHopLimit variable to the received value.

The behavior of this sysctl is wrong in multiple ways.  Some are
fixable, but let's not rush this commit into mainline, and revert this
while we still can, then we can come up with a better solution.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 Documentation/networking/ip-sysctl.txt |  8 --------
 include/linux/ipv6.h                   |  1 -
 include/uapi/linux/ipv6.h              |  1 -
 net/ipv6/addrconf.c                    | 10 ----------
 net/ipv6/ndisc.c                       | 16 +++++++++-------
 5 files changed, 9 insertions(+), 27 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index ebe94f2cab98..c845aca413c8 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -1371,14 +1371,6 @@ accept_ra_from_local - BOOLEAN
 	   disabled if accept_ra_from_local is disabled
                on a specific interface.
 
-accept_ra_min_hop_limit - INTEGER
-	Minimum hop limit Information in Router Advertisement.
-
-	Hop limit Information in Router Advertisement less than this
-	variable shall be ignored.
-
-	Default: 1
-
 accept_ra_pinfo - BOOLEAN
 	Learn Prefix Information in Router Advertisement.
 
diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index f1f32af6d9b9..5e6d3b8f5a42 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -29,7 +29,6 @@ struct ipv6_devconf {
 	__s32		max_desync_factor;
 	__s32		max_addresses;
 	__s32		accept_ra_defrtr;
-	__s32		accept_ra_min_hop_limit;
 	__s32		accept_ra_pinfo;
 	__s32		ignore_routes_with_linkdown;
 #ifdef CONFIG_IPV6_ROUTER_PREF
diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h
index 38b4fef20219..44d13121de52 100644
--- a/include/uapi/linux/ipv6.h
+++ b/include/uapi/linux/ipv6.h
@@ -172,7 +172,6 @@ enum {
 	DEVCONF_ACCEPT_RA_MTU,
 	DEVCONF_STABLE_SECRET,
 	DEVCONF_USE_OIF_ADDRS_ONLY,
-	DEVCONF_ACCEPT_RA_MIN_HOP_LIMIT,
 	DEVCONF_IGNORE_ROUTES_WITH_LINKDOWN,
 	DEVCONF_MAX
 };
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 99c0f2b843f0..c8829ce0bbe7 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -195,7 +195,6 @@ static struct ipv6_devconf ipv6_devconf __read_mostly = {
 	.max_addresses		= IPV6_MAX_ADDRESSES,
 	.accept_ra_defrtr	= 1,
 	.accept_ra_from_local	= 0,
-	.accept_ra_min_hop_limit= 1,
 	.accept_ra_pinfo	= 1,
 #ifdef CONFIG_IPV6_ROUTER_PREF
 	.accept_ra_rtr_pref	= 1,
@@ -239,7 +238,6 @@ static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
 	.max_addresses		= IPV6_MAX_ADDRESSES,
 	.accept_ra_defrtr	= 1,
 	.accept_ra_from_local	= 0,
-	.accept_ra_min_hop_limit= 1,
 	.accept_ra_pinfo	= 1,
 #ifdef CONFIG_IPV6_ROUTER_PREF
 	.accept_ra_rtr_pref	= 1,
@@ -4658,7 +4656,6 @@ static inline void ipv6_store_devconf(struct ipv6_devconf *cnf,
 	array[DEVCONF_MAX_DESYNC_FACTOR] = cnf->max_desync_factor;
 	array[DEVCONF_MAX_ADDRESSES] = cnf->max_addresses;
 	array[DEVCONF_ACCEPT_RA_DEFRTR] = cnf->accept_ra_defrtr;
-	array[DEVCONF_ACCEPT_RA_MIN_HOP_LIMIT] = cnf->accept_ra_min_hop_limit;
 	array[DEVCONF_ACCEPT_RA_PINFO] = cnf->accept_ra_pinfo;
 #ifdef CONFIG_IPV6_ROUTER_PREF
 	array[DEVCONF_ACCEPT_RA_RTR_PREF] = cnf->accept_ra_rtr_pref;
@@ -5594,13 +5591,6 @@ static struct addrconf_sysctl_table
 			.proc_handler	= proc_dointvec,
 		},
 		{
-			.procname	= "accept_ra_min_hop_limit",
-			.data		= &ipv6_devconf.accept_ra_min_hop_limit,
-			.maxlen		= sizeof(int),
-			.mode		= 0644,
-			.proc_handler	= proc_dointvec,
-		},
-		{
 			.procname	= "accept_ra_pinfo",
 			.data		= &ipv6_devconf.accept_ra_pinfo,
 			.maxlen		= sizeof(int),
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 64a71354b069..fac72658bf59 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1235,16 +1235,18 @@ static void ndisc_router_discovery(struct sk_buff *skb)
 
 	if (rt)
 		rt6_set_expires(rt, jiffies + (HZ * lifetime));
-	if (in6_dev->cnf.accept_ra_min_hop_limit < 256 &&
-	    ra_msg->icmph.icmp6_hop_limit) {
-		if (in6_dev->cnf.accept_ra_min_hop_limit <= ra_msg->icmph.icmp6_hop_limit) {
+	if (ra_msg->icmph.icmp6_hop_limit) {
+		/* Only set hop_limit on the interface if it is higher than
+		 * the current hop_limit.
+		 */
+		if (in6_dev->cnf.hop_limit < ra_msg->icmph.icmp6_hop_limit) {
 			in6_dev->cnf.hop_limit = ra_msg->icmph.icmp6_hop_limit;
-			if (rt)
-				dst_metric_set(&rt->dst, RTAX_HOPLIMIT,
-					       ra_msg->icmph.icmp6_hop_limit);
 		} else {
-			ND_PRINTK(2, warn, "RA: Got route advertisement with lower hop_limit than minimum\n");
+			ND_PRINTK(2, warn, "RA: Got route advertisement with lower hop_limit than current\n");
 		}
+		if (rt)
+			dst_metric_set(&rt->dst, RTAX_HOPLIMIT,
+				       ra_msg->icmph.icmp6_hop_limit);
 	}
 
 skip_defrtr:
-- 
2.5.1

Re: [PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"

[David Miller]

From: Sabrina Dubroca <sd@queasysnail.net>
Date: Wed, 2 Sep 2015 11:43:01 +0200

> This reverts commit 8013d1d7eafb0589ca766db6b74026f76b7f5cb4.
> 
> There are several issues with this patch.
> It completely cancels the security changes introduced by 6fd99094de2b
> ("ipv6: Don't reduce hop limit for an interface").
> The current default value (min hop limit = 1) can result in the same
> denial of service that 6fd99094de2b prevents, but it is hard to define
> a correct and sane default value.
> More generally, it is yet another IPv6 sysctl, and we already have too
> many.
> 
> This was introduced to satisfy a TAHI test case which, in my opinion, is
> too strict, turning the RFC's "SHOULD" into a "MUST":
> 
>     If the received Cur Hop Limit value is non-zero, the host
>     SHOULD set its CurHopLimit variable to the received value.
> 
> The behavior of this sysctl is wrong in multiple ways.  Some are
> fixable, but let's not rush this commit into mainline, and revert this
> while we still can, then we can come up with a better solution.
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

I don't agree with this revert.

If you look at the original commit, the quoted RFC recommends adding
a configurable method to protect against this.

And that's exactly what the commit you are trying to revert is doing.

The only thing I would entertain is potentially an adjustment of the
default, working in concert with the TAHI folks to make sure their
tests still pass with any new default.

Thanks.

Re: [PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"

[Florian Westphal]

David Miller <davem@davemloft.net> wrote:
> From: Sabrina Dubroca <sd@queasysnail.net>
> Date: Wed, 2 Sep 2015 11:43:01 +0200
> 
> > This reverts commit 8013d1d7eafb0589ca766db6b74026f76b7f5cb4.
> > 
> > There are several issues with this patch.
> > It completely cancels the security changes introduced by 6fd99094de2b
> > ("ipv6: Don't reduce hop limit for an interface").
> > The current default value (min hop limit = 1) can result in the same
> > denial of service that 6fd99094de2b prevents, but it is hard to define
> > a correct and sane default value.
> > More generally, it is yet another IPv6 sysctl, and we already have too
> > many.
> > 
> > This was introduced to satisfy a TAHI test case which, in my opinion, is
> > too strict, turning the RFC's "SHOULD" into a "MUST":
> > 
> >     If the received Cur Hop Limit value is non-zero, the host
> >     SHOULD set its CurHopLimit variable to the received value.
> > 
> > The behavior of this sysctl is wrong in multiple ways.  Some are
> > fixable, but let's not rush this commit into mainline, and revert this
> > while we still can, then we can come up with a better solution.
> > 
> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> 
> I don't agree with this revert.
> 
> If you look at the original commit, the quoted RFC recommends adding
> a configurable method to protect against this.

Which also means it recommends a configurable method to NOT protect
against this.

Which begs the question in which scenario you would want to configure
end hosts in such a way that an RA can shrink hoplimit to values
where machines can't talk to internet hosts anymore.

> The only thing I would entertain is potentially an adjustment of the
> default, working in concert with the TAHI folks to make sure their
> tests still pass with any new default.

So, assuming we would change the default to 64 (the hoplimit default).
Where would it make sense to reconfigure this to a lower value?

Moreover, if we would (hypothetically) assume that an administrator wants
a smaller hop limit value and has to change knob to allow e.g. min
hoplimit of 10 they might as well just change the default hoplimit value
rather than altering min hoplimit and then set it via RA...?

Re: [PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"

[Sabrina Dubroca]

2015-09-02, 16:11:10 -0700, David Miller wrote:
> From: Sabrina Dubroca <sd@queasysnail.net>
> Date: Wed, 2 Sep 2015 11:43:01 +0200
> 
> > This reverts commit 8013d1d7eafb0589ca766db6b74026f76b7f5cb4.
> > 
> > There are several issues with this patch.
> > It completely cancels the security changes introduced by 6fd99094de2b
> > ("ipv6: Don't reduce hop limit for an interface").
> > The current default value (min hop limit = 1) can result in the same
> > denial of service that 6fd99094de2b prevents, but it is hard to define
> > a correct and sane default value.
> > More generally, it is yet another IPv6 sysctl, and we already have too
> > many.
> > 
> > This was introduced to satisfy a TAHI test case which, in my opinion, is
> > too strict, turning the RFC's "SHOULD" into a "MUST":
> > 
> >     If the received Cur Hop Limit value is non-zero, the host
> >     SHOULD set its CurHopLimit variable to the received value.
> > 
> > The behavior of this sysctl is wrong in multiple ways.  Some are
> > fixable, but let's not rush this commit into mainline, and revert this
> > while we still can, then we can come up with a better solution.
> > 
> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> 
> I don't agree with this revert.
> 
> If you look at the original commit, the quoted RFC recommends adding
> a configurable method to protect against this.
> 
> And that's exactly what the commit you are trying to revert is doing.
> 
> The only thing I would entertain is potentially an adjustment of the
> default, working in concert with the TAHI folks to make sure their
> tests still pass with any new default.

Would you agree with a default of 64, as Florian suggested?


Can we still modify the behavior of this sysctl? It's already been in
Linus's tree for a while, but if we can, I would rather restrict the
values we let the user write to accept_ra_min_hop_limit, as anything
outside [0..255] does not really make sense.

Allowing an RA to update the hop limit if

   current hop limit < RA.hop_limit < accept_ra_min_hop_limit

might also be desirable, but I'm not so sure about this case.


-- 
Sabrina

Re: [PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"

[Hangbin Liu]

Hi Sabrina,

2015-09-09 18:10 GMT+08:00 Sabrina Dubroca <sd@queasysnail.net>:
> 2015-09-02, 16:11:10 -0700, David Miller wrote:
>> From: Sabrina Dubroca <sd@queasysnail.net>
>> Date: Wed, 2 Sep 2015 11:43:01 +0200
>>
>> > This reverts commit 8013d1d7eafb0589ca766db6b74026f76b7f5cb4.
>> >
>> > There are several issues with this patch.
>> > It completely cancels the security changes introduced by 6fd99094de2b
>> > ("ipv6: Don't reduce hop limit for an interface").
>> > The current default value (min hop limit = 1) can result in the same
>> > denial of service that 6fd99094de2b prevents, but it is hard to define
>> > a correct and sane default value.
>> > More generally, it is yet another IPv6 sysctl, and we already have too
>> > many.
>> >
>> > This was introduced to satisfy a TAHI test case which, in my opinion, is
>> > too strict, turning the RFC's "SHOULD" into a "MUST":
>> >
>> >     If the received Cur Hop Limit value is non-zero, the host
>> >     SHOULD set its CurHopLimit variable to the received value.
>> >
>> > The behavior of this sysctl is wrong in multiple ways.  Some are
>> > fixable, but let's not rush this commit into mainline, and revert this
>> > while we still can, then we can come up with a better solution.
>> >
>> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
>>
>> I don't agree with this revert.
>>
>> If you look at the original commit, the quoted RFC recommends adding
>> a configurable method to protect against this.
>>
>> And that's exactly what the commit you are trying to revert is doing.
>>
>> The only thing I would entertain is potentially an adjustment of the
>> default, working in concert with the TAHI folks to make sure their
>> tests still pass with any new default.
>
> Would you agree with a default of 64, as Florian suggested?

Set default to 64 make sense to me. It can make the system more security
by default.

>
>
> Can we still modify the behavior of this sysctl? It's already been in
> Linus's tree for a while, but if we can, I would rather restrict the
> values we let the user write to accept_ra_min_hop_limit, as anything
> outside [0..255] does not really make sense.

Yes, so the checked if (in6_dev->cnf.accept_ra_min_hop_limit < 256 &&
ra_msg->icmph.icmp6_hop_limit)  make sure we only update the value between
[1..255].

>
> Allowing an RA to update the hop limit if
>
>    current hop limit < RA.hop_limit < accept_ra_min_hop_limit
>
> might also be desirable, but I'm not so sure about this case.

Yes, and we also should allow an RA to update the hop limit if

accept_ra_min_hop_limit <= RA.hop_limit < current hop limit

e.g accept_ra_min_hop_limit = RA.hop_limit =64, current hop limit = 128

Thanks
Hangbin Liu

Re: [PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"

[Sabrina Dubroca]

Hello,

2015-09-10, 10:54:38 +0800, Hangbin Liu wrote:
> > Can we still modify the behavior of this sysctl? It's already been in
> > Linus's tree for a while, but if we can, I would rather restrict the
> > values we let the user write to accept_ra_min_hop_limit, as anything
> > outside [0..255] does not really make sense.
> 
> Yes, so the checked if (in6_dev->cnf.accept_ra_min_hop_limit < 256 &&
> ra_msg->icmph.icmp6_hop_limit)  make sure we only update the value between
> [1..255].

I was thinking of returning -EINVAL when the user tries to set it to
300, using proc_dointvec_minmax.


> > Allowing an RA to update the hop limit if
> >
> >    current hop limit < RA.hop_limit < accept_ra_min_hop_limit
> >
> > might also be desirable, but I'm not so sure about this case.
> 
> Yes, and we also should allow an RA to update the hop limit if
> 
> accept_ra_min_hop_limit <= RA.hop_limit < current hop limit
> 
> e.g accept_ra_min_hop_limit = RA.hop_limit =64, current hop limit = 128

Yes, that's what we're doing at the moment, and I would leave it as is.


Thanks,

-- 
Sabrina

Re: [PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"

[Hangbin Liu]

Hi Sabrina,

2015-09-10 17:19 GMT+08:00 Sabrina Dubroca <sd@queasysnail.net>:
> Hello,
>
> 2015-09-10, 10:54:38 +0800, Hangbin Liu wrote:
>> > Can we still modify the behavior of this sysctl? It's already been in
>> > Linus's tree for a while, but if we can, I would rather restrict the
>> > values we let the user write to accept_ra_min_hop_limit, as anything
>> > outside [0..255] does not really make sense.
>>
>> Yes, so the checked if (in6_dev->cnf.accept_ra_min_hop_limit < 256 &&
>> ra_msg->icmph.icmp6_hop_limit)  make sure we only update the value between
>> [1..255].
>
> I was thinking of returning -EINVAL when the user tries to set it to
> 300, using proc_dointvec_minmax.

I'm OK with this. But we can also set cnf.hop_limit to 300,  if add check for
accept_ra_min_hop_limit, we'd better check all hop_limit values.

Thanks
Hangbin

Re: [PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"

[YOSHIFUJI Hideaki]

Sabrina Dubroca wrote:
> 2015-09-02, 16:11:10 -0700, David Miller wrote:
>> From: Sabrina Dubroca <sd@queasysnail.net>
>> Date: Wed, 2 Sep 2015 11:43:01 +0200
>>
>>> This reverts commit 8013d1d7eafb0589ca766db6b74026f76b7f5cb4.
>>>
>>> There are several issues with this patch.
>>> It completely cancels the security changes introduced by 6fd99094de2b
>>> ("ipv6: Don't reduce hop limit for an interface").
>>> The current default value (min hop limit = 1) can result in the same
>>> denial of service that 6fd99094de2b prevents, but it is hard to define
>>> a correct and sane default value.
>>> More generally, it is yet another IPv6 sysctl, and we already have too
>>> many.
>>>
>>> This was introduced to satisfy a TAHI test case which, in my opinion, is
>>> too strict, turning the RFC's "SHOULD" into a "MUST":
>>>
>>>     If the received Cur Hop Limit value is non-zero, the host
>>>     SHOULD set its CurHopLimit variable to the received value.
>>>
>>> The behavior of this sysctl is wrong in multiple ways.  Some are
>>> fixable, but let's not rush this commit into mainline, and revert this
>>> while we still can, then we can come up with a better solution.
>>>
>>> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
>>
>> I don't agree with this revert.
>>
>> If you look at the original commit, the quoted RFC recommends adding
>> a configurable method to protect against this.
>>
>> And that's exactly what the commit you are trying to revert is doing.
>>
>> The only thing I would entertain is potentially an adjustment of the
>> default, working in concert with the TAHI folks to make sure their
>> tests still pass with any new default.
> 
> Would you agree with a default of 64, as Florian suggested?

1 was chosen to restore our behavior before introduction of current
hoplimit check.  I am not in favor of changing that value.

Plus, 64 is too restrictive and 32 would be enough for global
internet, IMHO.

> 
> 
> Can we still modify the behavior of this sysctl? It's already been in
> Linus's tree for a while, but if we can, I would rather restrict the
> values we let the user write to accept_ra_min_hop_limit, as anything
> outside [0..255] does not really make sense.

[1..256], maybe, but it is not harmful to set outside the range.
0 is always ignored. If it is set to 256 or more, the option is
completely ignored.

> 
> Allowing an RA to update the hop limit if
> 
>    current hop limit < RA.hop_limit < accept_ra_min_hop_limit
> 
> might also be desirable, but I'm not so sure about this case.
> 
> 

It might be... byt I don't think it is a good idea since it becomes
more complex.

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

Re: [PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"

[Sabrina Dubroca]

2015-09-10, 14:52:45 +0900, YOSHIFUJI Hideaki wrote:
> Sabrina Dubroca wrote:
> > 2015-09-02, 16:11:10 -0700, David Miller wrote:
> >> The only thing I would entertain is potentially an adjustment of the
> >> default, working in concert with the TAHI folks to make sure their
> >> tests still pass with any new default.
> > 
> > Would you agree with a default of 64, as Florian suggested?
> 
> 1 was chosen to restore our behavior before introduction of current
> hoplimit check.  I am not in favor of changing that value.

But our old behavior had a security issue, which is why the >= current
check was introduced.


> Plus, 64 is too restrictive and 32 would be enough for global
> internet, IMHO.

I guess I could live with that, if 32 is indeed enough for everybody.


> > Can we still modify the behavior of this sysctl? It's already been in
> > Linus's tree for a while, but if we can, I would rather restrict the
> > values we let the user write to accept_ra_min_hop_limit, as anything
> > outside [0..255] does not really make sense.
> 
> [1..256], maybe, but it is not harmful to set outside the range.
> 0 is always ignored. If it is set to 256 or more, the option is
> completely ignored.

Not harmful, but maybe slightly misleading, and requires the "< 256"
check when processing a RA.


> > Allowing an RA to update the hop limit if
> > 
> >    current hop limit < RA.hop_limit < accept_ra_min_hop_limit
> > 
> > might also be desirable, but I'm not so sure about this case.
> > 
> > 
> 
> It might be... byt I don't think it is a good idea since it becomes
> more complex.

A bit more complex, yes.  But I don't think this should hold us back
if it results in better behavior.


Thanks,

-- 
Sabrina

Re: [PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"

[YOSHIFUJI Hideaki]

Sabrina Dubroca wrote:
> 2015-09-10, 14:52:45 +0900, YOSHIFUJI Hideaki wrote:
>> Sabrina Dubroca wrote:
>>> 2015-09-02, 16:11:10 -0700, David Miller wrote:
>>>> The only thing I would entertain is potentially an adjustment of the
>>>> default, working in concert with the TAHI folks to make sure their
>>>> tests still pass with any new default.
>>>
>>> Would you agree with a default of 64, as Florian suggested?
>>
>> 1 was chosen to restore our behavior before introduction of current
>> hoplimit check.  I am not in favor of changing that value.
> 
> But our old behavior had a security issue, which is why the >= current
> check was introduced.

We have the knob to "protect" ourselves now but it has drawbacks no to
accept lower values than specified.  We can never have ultimate default
for everybody.  The knob might "mitigate" the issue but once we have
any rouge routers on our L2, we lose anyway.  So, I do want to keep it
as-is not to change our traditional behavior.

> 
> 
>> Plus, 64 is too restrictive and 32 would be enough for global
>> internet, IMHO.
> 
> I guess I could live with that, if 32 is indeed enough for everybody.
> 
> 
>>> Can we still modify the behavior of this sysctl? It's already been in
>>> Linus's tree for a while, but if we can, I would rather restrict the
>>> values we let the user write to accept_ra_min_hop_limit, as anything
>>> outside [0..255] does not really make sense.
>>
>> [1..256], maybe, but it is not harmful to set outside the range.
>> 0 is always ignored. If it is set to 256 or more, the option is
>> completely ignored.
> 
> Not harmful, but maybe slightly misleading, and requires the "< 256"
> check when processing a RA.

The "Cur Hop Limit" field is 8bit long...

> 
> 
>>> Allowing an RA to update the hop limit if
>>>
>>>    current hop limit < RA.hop_limit < accept_ra_min_hop_limit
>>>
>>> might also be desirable, but I'm not so sure about this case.
>>>
>>>
>>
>> It might be... byt I don't think it is a good idea since it becomes
>> more complex.
> 
> A bit more complex, yes.  But I don't think this should hold us back
> if it results in better behavior.
> 
> 
> Thanks,
> 

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

Re: [PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"

[Florian Westphal]

YOSHIFUJI Hideaki <hideaki.yoshifuji@miraclelinux.com> wrote:
> Sabrina Dubroca wrote:
> > 2015-09-10, 14:52:45 +0900, YOSHIFUJI Hideaki wrote:
> >> Sabrina Dubroca wrote:
> >>> Would you agree with a default of 64, as Florian suggested?
> >>
> >> 1 was chosen to restore our behavior before introduction of current
> >> hoplimit check.  I am not in favor of changing that value.
> > 
> > But our old behavior had a security issue, which is why the >= current
> > check was introduced.
> 
> We have the knob to "protect" ourselves now but it has drawbacks no to
> accept lower values than specified.  We can never have ultimate default
> for everybody.  The knob might "mitigate" the issue but once we have
> any rouge routers on our L2, we lose anyway.  So, I do want to keep it
> as-is not to change our traditional behavior.

If that argument is brough forward (and it's a good point!), then the
entire case for rejecting 'low' hoplimit values in first place becomes moot.

If this is an important security issue, then either the sysctl has to be
removed or the default raised to some 'safe' value (32, for example).

If its not a security issue -- and it isn't if we think "1" is a good
default choice -- then we should seriously consider reverting both
the added sysctl and the 'original' commit (6fd99094de2b; "ipv6: Don't
reduce hop limit for an interface").

Cheers,
Florian

Re: [PATCH net-next] Revert "net/ipv6: add sysctl option accept_ra_min_hop_limit"

[D.S. Ljungmark]

[-- Attachment #1: Type: text/plain, Size: 2234 bytes --]

On 11/09/15 12:53, Florian Westphal wrote:
> YOSHIFUJI Hideaki <hideaki.yoshifuji@miraclelinux.com> wrote:
>> Sabrina Dubroca wrote:
>>> 2015-09-10, 14:52:45 +0900, YOSHIFUJI Hideaki wrote:
>>>> Sabrina Dubroca wrote:
>>>>> Would you agree with a default of 64, as Florian suggested?
>>>>
>>>> 1 was chosen to restore our behavior before introduction of current
>>>> hoplimit check.  I am not in favor of changing that value.
>>>
>>> But our old behavior had a security issue, which is why the >= current
>>> check was introduced.
>>
>> We have the knob to "protect" ourselves now but it has drawbacks no to
>> accept lower values than specified.  We can never have ultimate default
>> for everybody.  The knob might "mitigate" the issue but once we have
>> any rouge routers on our L2, we lose anyway.  So, I do want to keep it
>> as-is not to change our traditional behavior.
> 
> If that argument is brough forward (and it's a good point!), then the
> entire case for rejecting 'low' hoplimit values in first place becomes moot.
> 
> If this is an important security issue, then either the sysctl has to be
> removed or the default raised to some 'safe' value (32, for example).
> 
> If its not a security issue -- and it isn't if we think "1" is a good
> default choice -- then we should seriously consider reverting both
> the added sysctl and the 'original' commit (6fd99094de2b; "ipv6: Don't
> reduce hop limit for an interface").
> 


The most common use-case for this is public WiFi.  So far, a negible
amount of access points have even remote ability to filter "unwanted" L2
traffic.

The fact that a single, empty RA packet with a hop limit of 2 will take
down your entire ipv6, even if your infrastructure uses DHCPv6 for
addressing is problematic.

There are scenarios where an L2 agent can push a link-local or
Peer-to-peer routes with a low hoplimit. These routes would then lower
the interface-level hop limit to something that breaks your other routing.

Personally, I think the concept of hop-limit being per interface in IPv6
is disasterously stupid, but I'm not arguing against the RFC there.

//D.S.



-- 
8362 CB14 98AD 11EF CEB6  FA81 FCC3 7674 449E 3CFC


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]