| Message ID | 20250206015711.2627417-1-hawkinsw@obs.cr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] icmp: MUST silently discard certain extended echo requests | expand |
I was attempting to minimize unnecessary CCs on the initial email but Patchwork chided me for not including required people. I am doing that with this follow-up message. Sorry! Will On Wed, Feb 5, 2025 at 8:57 PM Will Hawkins <hawkinsw@obs.cr> wrote: > > Per RFC 8335 Section 4, > """ > When a node receives an ICMP Extended Echo Request message and any of > the following conditions apply, the node MUST silently discard the > incoming message: > > ... > - The Source Address of the incoming message is not a unicast address. > - The Destination Address of the incoming message is a multicast address. > """ > > Packets meeting the former criteria do not pass martian detection, but > packets meeting the latter criteria must be explicitly dropped. > > Signed-off-by: Will Hawkins <hawkinsw@obs.cr> > --- > > I hope that this small change is helpful. There is code that already > prevents the kernel from transmitting packets that violate the latter > criteria, but I read the RFC as saying that these rogue packets must > be dropped before even being handled. > > I have a history of Kernel contribution but I do so infrequently. I > have tried very hard to follow all the proper protocol. Forgive me > if I messed up. Thank you for all the work that you do maintaining > _the_ most important Kernel subsystem! > > net/ipv4/icmp.c | 16 ++++++++++++++++ > net/ipv6/icmp.c | 15 +++++++++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c > index 963a89ae9c26..081264b6e9eb 100644 > --- a/net/ipv4/icmp.c > +++ b/net/ipv4/icmp.c > @@ -1241,6 +1241,22 @@ int icmp_rcv(struct sk_buff *skb) > > /* Check for ICMP Extended Echo (PROBE) messages */ > if (icmph->type == ICMP_EXT_ECHO) { > + /* > + * RFC 8335: 4 When a node receives [a message] and any of > + * the following conditions apply, the node MUST silently > + * discard the incoming message: > + * ... > + * - The Source Address of the incoming message is not > + * a unicast address. > + * - The Destination Address of the incoming message is a > + * multicast address. > + * (Former constraint is handled by martian detection.) > + */ > + if (rt->rt_flags & RTCF_MULTICAST) { > + reason = SKB_DROP_REASON_INVALID_PROTO; > + goto error; > + } > + > /* We can't use icmp_pointers[].handler() because it is an array of > * size NR_ICMP_TYPES + 1 (19 elements) and PROBE has code 42. > */ > diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c > index 071b0bc1179d..76498a37e465 100644 > --- a/net/ipv6/icmp.c > +++ b/net/ipv6/icmp.c > @@ -738,6 +738,21 @@ static enum skb_drop_reason icmpv6_echo_reply(struct sk_buff *skb) > net->ipv6.sysctl.icmpv6_echo_ignore_multicast) > return reason; > > + /* > + * RFC 8335: 4 When a node receives [a message] and any of > + * the following conditions apply, the node MUST silently > + * discard the incoming message: > + * ... > + * - The Source Address of the incoming message is not > + * a unicast address. > + * - The Destination Address of the incoming message is a > + * multicast address. > + * (Former constraint is handled by martian detection.) > + */ > + if (icmph->icmp6_type == ICMPV6_EXT_ECHO_REQUEST && > + ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr)) > + return reason; > + > saddr = &ipv6_hdr(skb)->daddr; > > acast = ipv6_anycast_destination(skb_dst(skb), saddr); > -- > 2.47.1 >
On 2/6/25 2:57 AM, Will Hawkins wrote: > Per RFC 8335 Section 4, > """ > When a node receives an ICMP Extended Echo Request message and any of > the following conditions apply, the node MUST silently discard the > incoming message: > > ... > - The Source Address of the incoming message is not a unicast address. > - The Destination Address of the incoming message is a multicast address. > """ I think it would be helpful mentioning this is for ICMP PROBE extension. > Packets meeting the former criteria do not pass martian detection, but > packets meeting the latter criteria must be explicitly dropped. > > Signed-off-by: Will Hawkins <hawkinsw@obs.cr> The patch should target the net-next tree, and you should add a related self-test (i.e. extending icmp.sh). Also even if the new behavior will respect the RFC, changing the established behavior could break existing setups, I *think* we would need at least a sysctl to revert to the old one. /P
On Tue, Feb 11, 2025 at 4:12 AM Paolo Abeni <pabeni@redhat.com> wrote: > > On 2/6/25 2:57 AM, Will Hawkins wrote: > > Per RFC 8335 Section 4, > > """ > > When a node receives an ICMP Extended Echo Request message and any of > > the following conditions apply, the node MUST silently discard the > > incoming message: > > > > ... > > - The Source Address of the incoming message is not a unicast address. > > - The Destination Address of the incoming message is a multicast address. > > """ > > I think it would be helpful mentioning this is for ICMP PROBE extension. Absolutely. > > > Packets meeting the former criteria do not pass martian detection, but > > packets meeting the latter criteria must be explicitly dropped. > > > > Signed-off-by: Will Hawkins <hawkinsw@obs.cr> > > The patch should target the net-next tree, and you should add a related I am terribly sorry for targeting the wrong tree. I interpreted "As you can probably guess from the names, the net tree is for fixes to existing code already in the mainline tree from Linus ..." incorrectly. I will adjust in the next revision. Sorry again! > self-test (i.e. extending icmp.sh). Also even if the new behavior will I will absolutely add a test! > respect the RFC, changing the established behavior could break existing > setups, I *think* we would need at least a sysctl to revert to the old one. I agree and will add such a sysctl! Thank you for the review! Will > > /P >
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c index 963a89ae9c26..081264b6e9eb 100644 --- a/net/ipv4/icmp.c +++ b/net/ipv4/icmp.c @@ -1241,6 +1241,22 @@ int icmp_rcv(struct sk_buff *skb) /* Check for ICMP Extended Echo (PROBE) messages */ if (icmph->type == ICMP_EXT_ECHO) { + /* + * RFC 8335: 4 When a node receives [a message] and any of + * the following conditions apply, the node MUST silently + * discard the incoming message: + * ... + * - The Source Address of the incoming message is not + * a unicast address. + * - The Destination Address of the incoming message is a + * multicast address. + * (Former constraint is handled by martian detection.) + */ + if (rt->rt_flags & RTCF_MULTICAST) { + reason = SKB_DROP_REASON_INVALID_PROTO; + goto error; + } + /* We can't use icmp_pointers[].handler() because it is an array of * size NR_ICMP_TYPES + 1 (19 elements) and PROBE has code 42. */ diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c index 071b0bc1179d..76498a37e465 100644 --- a/net/ipv6/icmp.c +++ b/net/ipv6/icmp.c @@ -738,6 +738,21 @@ static enum skb_drop_reason icmpv6_echo_reply(struct sk_buff *skb) net->ipv6.sysctl.icmpv6_echo_ignore_multicast) return reason; + /* + * RFC 8335: 4 When a node receives [a message] and any of + * the following conditions apply, the node MUST silently + * discard the incoming message: + * ... + * - The Source Address of the incoming message is not + * a unicast address. + * - The Destination Address of the incoming message is a + * multicast address. + * (Former constraint is handled by martian detection.) + */ + if (icmph->icmp6_type == ICMPV6_EXT_ECHO_REQUEST && + ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr)) + return reason; + saddr = &ipv6_hdr(skb)->daddr; acast = ipv6_anycast_destination(skb_dst(skb), saddr);
Per RFC 8335 Section 4, """ When a node receives an ICMP Extended Echo Request message and any of the following conditions apply, the node MUST silently discard the incoming message: ... - The Source Address of the incoming message is not a unicast address. - The Destination Address of the incoming message is a multicast address. """ Packets meeting the former criteria do not pass martian detection, but packets meeting the latter criteria must be explicitly dropped. Signed-off-by: Will Hawkins <hawkinsw@obs.cr> --- I hope that this small change is helpful. There is code that already prevents the kernel from transmitting packets that violate the latter criteria, but I read the RFC as saying that these rogue packets must be dropped before even being handled. I have a history of Kernel contribution but I do so infrequently. I have tried very hard to follow all the proper protocol. Forgive me if I messed up. Thank you for all the work that you do maintaining _the_ most important Kernel subsystem! net/ipv4/icmp.c | 16 ++++++++++++++++ net/ipv6/icmp.c | 15 +++++++++++++++ 2 files changed, 31 insertions(+)