diff mbox

[net] ipv6: stop sending PTB packets for MTU < 1280

Message ID 1421357665-2804-1-git-send-email-hagen@jauu.net
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Hagen Paul Pfeifer Jan. 15, 2015, 9:34 p.m. UTC 
Reduce the attack vector and stop generating IPv6 Fragment Header for
paths with an MTU smaller than the minimum required IPv6 MTU
size (1280 byte) - called atomic fragments.

See IETF I-D "Deprecating the Generation of IPv6 Atomic Fragments" [1]
for more information and how this "feature" can be misused.

[1] https://tools.ietf.org/html/draft-ietf-6man-deprecate-atomfrag-generation-00

Cc: stable@vger.kernel.org
Signed-off-by: Fernando Gont <fgont@si6networks.com>
Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>
---
 net/ipv6/route.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Hannes Frederic Sowa Jan. 19, 2015, 1:55 p.m. UTC | #1 
On Do, 2015-01-15 at 22:34 +0100, Hagen Paul Pfeifer wrote:
> Reduce the attack vector and stop generating IPv6 Fragment Header for
> paths with an MTU smaller than the minimum required IPv6 MTU
> size (1280 byte) - called atomic fragments.
> 
> See IETF I-D "Deprecating the Generation of IPv6 Atomic Fragments" [1]
> for more information and how this "feature" can be misused.
> 
> [1] https://tools.ietf.org/html/draft-ietf-6man-deprecate-atomfrag-generation-00
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Fernando Gont <fgont@si6networks.com>
> Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

I think this is the correct way forward on how to deal with atomic
fragments.

Hagen, do you submit patches to remove dst_allfrag/RTAX_FEATURE_ALLFRAG,
IPCORK_ALLFRAG, etc. for net-next, too?

Thanks,
Hannes


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hagen Paul Pfeifer Jan. 19, 2015, 2 p.m. UTC | #2 
On 19 January 2015 at 14:55, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>
> I think this is the correct way forward on how to deal with atomic
> fragments.
>
> Hagen, do you submit patches to remove dst_allfrag/RTAX_FEATURE_ALLFRAG,
> IPCORK_ALLFRAG, etc. for net-next, too?

Yes, patch sits already in the pipe. I wanted to wait for davem's pull.

Hagen
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Jan. 19, 2015, 7:50 p.m. UTC | #3 
From: Hagen Paul Pfeifer <hagen@jauu.net>
Date: Mon, 19 Jan 2015 15:00:21 +0100

> On 19 January 2015 at 14:55, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>
>> I think this is the correct way forward on how to deal with atomic
>> fragments.
>>
>> Hagen, do you submit patches to remove dst_allfrag/RTAX_FEATURE_ALLFRAG,
>> IPCORK_ALLFRAG, etc. for net-next, too?
> 
> Yes, patch sits already in the pipe. I wanted to wait for davem's pull.

It's one thing to change policy about how we might or might not
automatically set this bit in the kernel, but at a minimum you cannot
just remove RTAX_FEATURE_ALLFRAG, it's in a userspace header and
you'll break application builds.

Second of all, there is absolutely no reason to prevent the user from
setting this bit.  If someone wants to set RTAX_FEATURE_ALLFRAG on a
route on their own system, that is their business.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Jan. 19, 2015, 7:51 p.m. UTC | #4 
From: Hagen Paul Pfeifer <hagen@jauu.net>
Date: Thu, 15 Jan 2015 22:34:25 +0100

> Reduce the attack vector and stop generating IPv6 Fragment Header for
> paths with an MTU smaller than the minimum required IPv6 MTU
> size (1280 byte) - called atomic fragments.
> 
> See IETF I-D "Deprecating the Generation of IPv6 Atomic Fragments" [1]
> for more information and how this "feature" can be misused.
> 
> [1] https://tools.ietf.org/html/draft-ietf-6man-deprecate-atomfrag-generation-00
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Fernando Gont <fgont@si6networks.com>
> Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>

Applied, thanks.

But any follow-ups to get rid of RTAX_FEATURE_ALLFRAG altogether are
not to be seriously considered in my opinion.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Frederic Sowa Jan. 19, 2015, 8:05 p.m. UTC | #5 
On Mo, 2015-01-19 at 14:50 -0500, David Miller wrote:
> From: Hagen Paul Pfeifer <hagen@jauu.net>
> Date: Mon, 19 Jan 2015 15:00:21 +0100
> 
> > On 19 January 2015 at 14:55, Hannes Frederic Sowa
> > <hannes@stressinduktion.org> wrote:
> >> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> >>
> >> I think this is the correct way forward on how to deal with atomic
> >> fragments.
> >>
> >> Hagen, do you submit patches to remove dst_allfrag/RTAX_FEATURE_ALLFRAG,
> >> IPCORK_ALLFRAG, etc. for net-next, too?
> > 
> > Yes, patch sits already in the pipe. I wanted to wait for davem's pull.
> 
> It's one thing to change policy about how we might or might not
> automatically set this bit in the kernel, but at a minimum you cannot
> just remove RTAX_FEATURE_ALLFRAG, it's in a userspace header and
> you'll break application builds.

Sure, the define would need to be left alone.

> Second of all, there is absolutely no reason to prevent the user from
> setting this bit.  If someone wants to set RTAX_FEATURE_ALLFRAG on a
> route on their own system, that is their business.

Oh yes, although we never exposed an ip route knob for that, it is still
possible users did set manually, so we cannot get rid of that, agreed.

Bye,
Hannes


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hagen Paul Pfeifer Jan. 19, 2015, 10:05 p.m. UTC | #6 
On 19 January 2015 at 21:05, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:

> Oh yes, although we never exposed an ip route knob for that, it is still
> possible users did set manually, so we cannot get rid of that, agreed.

I thought about that, sure but come to the conclusion that the code
cleanup outweigh an potential user somewhere in space (well, the use
case is broken and should be fixed). Additionally, I left the
RTAX_FEATURE_ALLFRAG (as mentioned) and removed all related
functionality - no visible API change (except no-op behavior).

Davem, I will sent the patch anyway. Feel free to accept/ignore.

Hagen
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Loganaden Velvindron Jan. 20, 2015, 4:02 a.m. UTC | #7 
On Mon, Jan 19, 2015 at 10:05 PM, Hagen Paul Pfeifer <hagen@jauu.net> wrote:
> On 19 January 2015 at 21:05, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>
>> Oh yes, although we never exposed an ip route knob for that, it is still
>> possible users did set manually, so we cannot get rid of that, agreed.
>
> I thought about that, sure but come to the conclusion that the code
> cleanup outweigh an potential user somewhere in space (well, the use
> case is broken and should be fixed). Additionally, I left the
> RTAX_FEATURE_ALLFRAG (as mentioned) and removed all related
> functionality - no visible API change (except no-op behavior).
>
> Davem, I will sent the patch anyway. Feel free to accept/ignore.
>
> Hagen
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi.

Last time I was inquiring about depracated atomic fragments, people
were concerned that there wasn't enough practical data to decide
whether to go forward or not.

Would a sysctl with it turned on by default be a good option, until we
are 100% sure ?

Kind regards,
//Logan
C-x-C-c
Fernando Gont Jan. 20, 2015, 4:52 a.m. UTC | #8 
Hi, Loganaden,

On 01/20/2015 01:02 AM, Loganaden Velvindron wrote:
> 
> Last time I was inquiring about depracated atomic fragments, people
> were concerned that there wasn't enough practical data to decide
> whether to go forward or not.

What kind of practical data?

FWIW,
<https://tools.ietf.org/id/draft-ietf-6man-deprecate-atomfrag-generation-00.txt>
seems to be good enough when it comes to reasons for deprecating them.


Besides, please check Section 5.2 of
<http://www.ietf.org/id/draft-gont-v6ops-ipv6-ehs-in-real-world-01.txt>
-- my "connection" to kernel.org was vulnerable to such attack.



> Would a sysctl with it turned on by default be a good option, until we
> are 100% sure ?

<https://tools.ietf.org/id/draft-ietf-6man-deprecate-atomfrag-generation-00.txt>
 was adopted by the 6man wg last November. While the I-D is not ready an
RFC, and there might be minor modifications, it seems that there's
agreement in not generating atomic fragments.

If you do want to have a sysctl for this, please make it default to "off".

Thanks!

Best regards,
diff mbox

Patch

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 34dcbb5..d4603fb 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1160,12 +1160,9 @@  static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
 		struct net *net = dev_net(dst->dev);
 
 		rt6->rt6i_flags |= RTF_MODIFIED;
-		if (mtu < IPV6_MIN_MTU) {
-			u32 features = dst_metric(dst, RTAX_FEATURES);
+		if (mtu < IPV6_MIN_MTU)
 			mtu = IPV6_MIN_MTU;
-			features |= RTAX_FEATURE_ALLFRAG;
-			dst_metric_set(dst, RTAX_FEATURES, features);
-		}
+
 		dst_metric_set(dst, RTAX_MTU, mtu);
 		rt6_update_expires(rt6, net->ipv6.sysctl.ip6_rt_mtu_expires);
 	}