From: Johannes Berg <[email protected]>
RFC 1122 says that unicast packets encapsulated in broadcast
link-layer packets should be dropped. Implement that, but also
extend it to link-layer multicast packets.
Signed-off-by: Johannes Berg <[email protected]>
---
net/ipv4/route.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index eaa4b000c7b4..c374fcc73ee0 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1710,6 +1710,23 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
goto no_route;
}
+ /* RFC 1122 3.3.6:
+ *
+ * When a host sends a datagram to a link-layer broadcast address,
+ * the IP destination address MUST be a legal IP broadcast or IP
+ * multicast address.
+ *
+ * A host SHOULD silently discard a datagram that is received via
+ * a link-layer broadcast (see Section 2.4) but does not specify
+ * an IP multicast or broadcast destination address.
+ *
+ * We also do this for link-layer multicast.
+ */
+ if ((skb->pkt_type == PACKET_BROADCAST ||
+ skb->pkt_type == PACKET_MULTICAST) &&
+ res.type != RTN_BROADCAST)
+ goto e_inval;
+
if (res.type == RTN_BROADCAST)
goto brd_input;
--
2.0.0
From: Julian Anastasov <[email protected]>
Date: Thu, 21 Aug 2014 22:51:02 +0300 (EEST)
>> if (res.type == RTN_BROADCAST)
>> goto brd_input;
>
> Is this place better, after checking for RTN_BROADCAST?
>
> /* ARP link-layer broadcasts are acceptable here */
> if ((skb->pkt_type == PACKET_BROADCAST ||
> skb->pkt_type == PACKET_MULTICAST) &&
> skb->protocol == htons(ETH_P_IP))
> goto e_inval;
Indeed, this would make ARP happier, but that still leaves open the
issue of CLUSTERIP.
On Thu, 2014-08-21 at 19:22 +0200, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> RFC 1122 says that unicast packets encapsulated in broadcast
> link-layer packets should be dropped. Implement that, but also
> extend it to link-layer multicast packets.
I cannot find anything equivalent in the IPv6 RFCs - anyone more
familiar with that who could help check for that?
And if it's *not* in the IPv6 RFCs, how should we implement this?
johannes
On Wed, 2014-08-27 at 09:38 +0200, Hannes Frederic Sowa wrote:
> > And if it's *not* in the IPv6 RFCs, how should we implement this?
>
> I haven't found anything, too. Should I bring this up with IETF?
I don't know if that's really useful? OTOH, there surely must have been
a reason for this to be in the IPv4 RFC, so maybe for that same reason
it should also be in the IPv6 RFC?
However, in our particular case, it's really meant only to close the
so-called "hole-196" vulnerability where rogue clients in your network
can abuse the GTK to do some attacks. Those attacks are also always
possible on non-managed ethernet segments, but those can be segregated
more easily by client than shared medium wireless.
This is only one building block for addressing the vulnerability. The
idea here was that in the wireless stack we already check
frame encrypted with GTK => must have multicast destination address
and in the IPv4 stack we can check
frame has multicast destination address => must have multicast/broadcast
IP addr
This would address this point.
The question now is, in the absence of such a latter required check (and
indeed, in the case of CLUSTERIP), how we implement such a check.
Perhaps a sysctl is needed after all?
johannes
Hello,
On Wed, 27 Aug 2014, Johannes Berg wrote:
> On Fri, 2014-08-22 at 10:54 -0700, David Miller wrote:
>
> > > Is this place better, after checking for RTN_BROADCAST?
> > >
> > > /* ARP link-layer broadcasts are acceptable here */
> > > if ((skb->pkt_type == PACKET_BROADCAST ||
> > > skb->pkt_type == PACKET_MULTICAST) &&
> > > skb->protocol == htons(ETH_P_IP))
> > > goto e_inval;
> >
> > Indeed, this would make ARP happier, but that still leaves open the
> > issue of CLUSTERIP.
>
> Unfortunately, I have no idea how to determine that CLUSTERIP is active
> here? Do we need to tag frames, or would a sysctl work?
>
> Or should we go back to the drawing board and not make this change in
> the IP stack at all? But parsing all the IP layer in the wireless stack
> is really quite ugly as well.
CLUSTERIP works in LOCAL_IN. My preference is to
add checks in every protocol where it is missing but if
you prefer a global check, ip_local_deliver_finish() is
a good place: CLUSTERIP already changed pkt_type to
PACKET_HOST. For example:
if (!(skb_rtable(skb)->rt_flags &
(RTCF_BROADCAST | RTCF_MULTICAST)) &&
(skb->pkt_type == PACKET_BROADCAST ||
skb->pkt_type == PACKET_MULTICAST)) {
kfree_skb(skb);
return;
}
By this way we protect the local stack globally.
BTW, what kind of packets (protocol) we want to drop? UDP?
As for ip_forward(), there is already check for
PACKET_HOST.
Not sure, may be a MIB counter for such drops
would be useful.
Regards
--
Julian Anastasov <[email protected]>
Hi Johannes,
On Do, 2014-08-21 at 19:32 +0200, Johannes Berg wrote:
> On Thu, 2014-08-21 at 19:22 +0200, Johannes Berg wrote:
> > From: Johannes Berg <[email protected]>
> >
> > RFC 1122 says that unicast packets encapsulated in broadcast
> > link-layer packets should be dropped. Implement that, but also
> > extend it to link-layer multicast packets.
>
> I cannot find anything equivalent in the IPv6 RFCs - anyone more
> familiar with that who could help check for that?
>
> And if it's *not* in the IPv6 RFCs, how should we implement this?
I haven't found anything, too. Should I bring this up with IETF?
Bye,
Hannes
On Wed, 2014-08-27 at 13:23 +0300, Julian Anastasov wrote:
> CLUSTERIP works in LOCAL_IN. My preference is to
> add checks in every protocol where it is missing but if
> you prefer a global check, ip_local_deliver_finish() is
> a good place: CLUSTERIP already changed pkt_type to
> PACKET_HOST. For example:
>
> if (!(skb_rtable(skb)->rt_flags &
> (RTCF_BROADCAST | RTCF_MULTICAST)) &&
> (skb->pkt_type == PACKET_BROADCAST ||
> skb->pkt_type == PACKET_MULTICAST)) {
> kfree_skb(skb);
> return;
> }
>
> By this way we protect the local stack globally.
I suppose that'd work then?
> BTW, what kind of packets (protocol) we want to drop? UDP?
All IP protocols, this comes either from the IPv4 RFC (1122) or from the
wireless issue which affects all protocols.
> As for ip_forward(), there is already check for
> PACKET_HOST.
>
> Not sure, may be a MIB counter for such drops
> would be useful.
Yeah, maybe, not sure.
johannes
On Mi, 2014-08-27 at 11:05 +0200, Johannes Berg wrote:
> On Wed, 2014-08-27 at 09:38 +0200, Hannes Frederic Sowa wrote:
>
> > > And if it's *not* in the IPv6 RFCs, how should we implement this?
> >
> > I haven't found anything, too. Should I bring this up with IETF?
>
> I don't know if that's really useful? OTOH, there surely must have been
> a reason for this to be in the IPv4 RFC, so maybe for that same reason
> it should also be in the IPv6 RFC?
Either it is an oversight, but RFC6085 3) tries to at least clarify the
multicast destination with LL unicast address. So there must have been
people trying to enfore a relationship between LL address and IPv6 one.
I think it would be OK to drop it by default in case we don't break any
other assumptions in the stack (e.g. CLUSTERIP).
> However, in our particular case, it's really meant only to close the
> so-called "hole-196" vulnerability where rogue clients in your network
> can abuse the GTK to do some attacks. Those attacks are also always
> possible on non-managed ethernet segments, but those can be segregated
> more easily by client than shared medium wireless.
>
> This is only one building block for addressing the vulnerability. The
> idea here was that in the wireless stack we already check
>
> frame encrypted with GTK => must have multicast destination address
>
> and in the IPv4 stack we can check
>
> frame has multicast destination address => must have multicast/broadcast
> IP addr
>
> This would address this point.
Yes, I was in the room when we discussed this. ;) To me at that time it
seemed like an easy and good solution.
> The question now is, in the absence of such a latter required check (and
> indeed, in the case of CLUSTERIP), how we implement such a check.
> Perhaps a sysctl is needed after all?
Yeah, unfortunate situation.
One could add those IP addresses as broadcast addresses (/32) to the
routing table, so the brd_input jump would be taken.
But this would still break users of CLUSTERIP until they install those
routes. :(
Bye,
Hannes
Hello,
On Thu, 21 Aug 2014, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> RFC 1122 says that unicast packets encapsulated in broadcast
> link-layer packets should be dropped. Implement that, but also
> extend it to link-layer multicast packets.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> net/ipv4/route.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index eaa4b000c7b4..c374fcc73ee0 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1710,6 +1710,23 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
> goto no_route;
> }
>
> + /* RFC 1122 3.3.6:
> + *
> + * When a host sends a datagram to a link-layer broadcast address,
> + * the IP destination address MUST be a legal IP broadcast or IP
> + * multicast address.
> + *
> + * A host SHOULD silently discard a datagram that is received via
> + * a link-layer broadcast (see Section 2.4) but does not specify
> + * an IP multicast or broadcast destination address.
> + *
> + * We also do this for link-layer multicast.
> + */
> + if ((skb->pkt_type == PACKET_BROADCAST ||
> + skb->pkt_type == PACKET_MULTICAST) &&
> + res.type != RTN_BROADCAST)
> + goto e_inval;
This place is ok for IP context but ip_route_input
is also called from ARP context and other places.
You are using pkt_type in route.c for first time.
At least inet_rtm_getroute() does not set it. You
have to audit all call sites, may be skb->protocol check
can be needed too, I guess ARP is broken otherwise.
And I'm not sure if skb->protocol is actual in
ip4ip6_err() after decapsulation. Adding more skb
fields to check is risky due to such places.
OTOH, the receive routines for protocols like
UDP, TCP, SCTP already have pkt_type checks. As result,
this is an extra check for them.
You should also consider that this change breaks
CLUSTERIP which uses multicast link-layer address and
local (shared) IP.
> if (res.type == RTN_BROADCAST)
> goto brd_input;
Is this place better, after checking for RTN_BROADCAST?
/* ARP link-layer broadcasts are acceptable here */
if ((skb->pkt_type == PACKET_BROADCAST ||
skb->pkt_type == PACKET_MULTICAST) &&
skb->protocol == htons(ETH_P_IP))
goto e_inval;
Regards
--
Julian Anastasov <[email protected]>
On Fri, 2014-08-22 at 10:54 -0700, David Miller wrote:
> > Is this place better, after checking for RTN_BROADCAST?
> >
> > /* ARP link-layer broadcasts are acceptable here */
> > if ((skb->pkt_type == PACKET_BROADCAST ||
> > skb->pkt_type == PACKET_MULTICAST) &&
> > skb->protocol == htons(ETH_P_IP))
> > goto e_inval;
>
> Indeed, this would make ARP happier, but that still leaves open the
> issue of CLUSTERIP.
Unfortunately, I have no idea how to determine that CLUSTERIP is active
here? Do we need to tag frames, or would a sysctl work?
Or should we go back to the drawing board and not make this change in
the IP stack at all? But parsing all the IP layer in the wireless stack
is really quite ugly as well.
johannes
Hello,
On Wed, 27 Aug 2014, Johannes Berg wrote:
> On Wed, 2014-08-27 at 13:23 +0300, Julian Anastasov wrote:
>
> > BTW, what kind of packets (protocol) we want to drop? UDP?
>
> All IP protocols, this comes either from the IPv4 RFC (1122) or from the
> wireless issue which affects all protocols.
I did a grep for inet_add_protocol, in case if
we prefer to use per-protocol checks:
Protocols that look ok to me: TCP, SCTP, DCCP
ICMP: missing check in icmp_rcv
UDP, UDPLITE: need check in __udp4_lib_rcv
IGMP: uses only multicast address?
PIM: not sure if __pim_rcv() needs check, before skb_tunnel_rx()
changes pkt_type?
More protocols are also registered with inet_add_protocol(), I don't
see pkt_type checks there, mostly tunnels:
- IPPROTO_GRE
- IPPROTO_L2TP
- IPPROTO_IPIP
- IPPROTO_IPV6 (tunnel64_rcv)
If going to use a global check I hope there are
no protocols that require exception to this rule.
Regards
--
Julian Anastasov <[email protected]>
On Di, 2014-09-02 at 15:03 -0700, David Miller wrote:
> From: YOSHIFUJI Hideaki <[email protected]>
> Date: Wed, 03 Sep 2014 10:59:14 +0900
>
> > Upper-layer needs to cope eith situation of seeing packets with
> > "incorrect" L2 header anyway (e.g., in promiscous mode).
> > I do not see much advantage to drop them here.
>
> It's required to prevent wireless nodes from using the shared wireless
> group keys (used for multicast transmission) to inject unicast frames.
>
> The RFCs really do specify this at least on the ipv4 side.
I have to agree with YOSHIFUJI Hideaki here. I looked at a lot of RFCs
and haven't found anything were it states to use L2 address type for
checks in L3 ipv6 addresses. For IPv4 addresses the situation is clear
though...
There was an RFC update (6085) which specifically allows one to send
ipv6 multicast frames with unicast L2 addresses. In the dicussion that
lead to this RFC it was stated that checking L2 and L3 addresses seems
to be a layering violation, but I can just use this as a hint.
Bye,
Hannes
On Thu, 21 Aug 2014 19:22:27 +0200
Johannes Berg <[email protected]> wrote:
> + /* RFC 1122 3.3.6:
> + *
> + * When a host sends a datagram to a link-layer broadcast address,
> + * the IP destination address MUST be a legal IP broadcast or IP
> + * multicast address.
> + *
> + * A host SHOULD silently discard a datagram that is received via
> + * a link-layer broadcast (see Section 2.4) but does not specify
> + * an IP multicast or broadcast destination address.
> + *
> + * We also do this for link-layer multicast.
> + */
> + if ((skb->pkt_type == PACKET_BROADCAST ||
> + skb->pkt_type == PACKET_MULTICAST) &&
> + res.type != RTN_BROADCAST)
> + goto e_inval;
> +
I think you need to all multicast packet but not broadcast.
The RFC does not specify that you should drop link-layer multicast to a unicast
address. There are several clustering products use that.
Johannes Berg wrote:
> As long as IPv6 doesn't mandate it in the RFCs I'm not really sure we
> should just drop it, even if we think it won't cause any problems?
>
> CLUSTERIP seems like a special configuration, but I'm not sure it can be
> detected and automatically allowed?
Please do not "drop" L2 multicast/broadcast for L3 unicast and
vice versa, unless it is explicitly specified by RFC.
Upper-layer needs to cope eith situation of seeing packets with
"incorrect" L2 header anyway (e.g., in promiscous mode).
I do not see much advantage to drop them here.
--
Hideaki Yoshifuji <[email protected]>
Technical Division, MIRACLE LINUX CORPORATION
On Tue, 2014-09-02 at 14:16 -0700, Stephen Hemminger wrote:
> On Thu, 21 Aug 2014 19:22:27 +0200
> Johannes Berg <[email protected]> wrote:
>
> > + /* RFC 1122 3.3.6:
> > + *
> > + * When a host sends a datagram to a link-layer broadcast address,
> > + * the IP destination address MUST be a legal IP broadcast or IP
> > + * multicast address.
> > + *
> > + * A host SHOULD silently discard a datagram that is received via
> > + * a link-layer broadcast (see Section 2.4) but does not specify
> > + * an IP multicast or broadcast destination address.
> > + *
> > + * We also do this for link-layer multicast.
> > + */
> > + if ((skb->pkt_type == PACKET_BROADCAST ||
> > + skb->pkt_type == PACKET_MULTICAST) &&
> > + res.type != RTN_BROADCAST)
> > + goto e_inval;
> > +
>
> I think you need to all multicast packet but not broadcast.
> The RFC does not specify that you should drop link-layer multicast to a unicast
> address. There are several clustering products use that.
Alright - I feared we'd not be able to do this. Based on a strict
reading of the RFC, you're right. I'm not sure what thinking and
rationale went into this sentence in the RFC, depending on that it would
have made sense to me to also apply it for broadcast (which is really
just a special form of multicast, in a sense, no?)
In any case, for wireless we do need broadcast as well, for obvious
reasons. Therefore, I think with all the discussion here and about IPv6
etc. we're getting to the point where we should think about
disentangling RFC 1122 checks (which may very well be correct and needed
in their own right) and the hole-196 mitigation for wireless.
Our original attempt is documented here:
http://patchwork.ozlabs.org/patch/293133/
and there seemed to be consensus back then that we should do it in the
wireless stack. I can live with that, even if I don't very much like the
idea of dissecting IP protocols there and having to do a route lookup
etc. In any case, if that's what we want to do, I'll probably need some
help on how to do the route lookups etc. Any takers? :)
johannes
From: YOSHIFUJI Hideaki <[email protected]>
Date: Wed, 03 Sep 2014 10:59:14 +0900
> Upper-layer needs to cope eith situation of seeing packets with
> "incorrect" L2 header anyway (e.g., in promiscous mode).
> I do not see much advantage to drop them here.
It's required to prevent wireless nodes from using the shared wireless
group keys (used for multicast transmission) to inject unicast frames.
The RFCs really do specify this at least on the ipv4 side.
On Wed, 2014-08-27 at 11:53 +0200, Hannes Frederic Sowa wrote:
> > I don't know if that's really useful? OTOH, there surely must have been
> > a reason for this to be in the IPv4 RFC, so maybe for that same reason
> > it should also be in the IPv6 RFC?
>
> Either it is an oversight, but RFC6085 3) tries to at least clarify the
> multicast destination with LL unicast address. So there must have been
> people trying to enfore a relationship between LL address and IPv6 one.
That seems to allow a multicast IPv6 frame in a unicast LL address,
which is a different situation but still ...
> I think it would be OK to drop it by default in case we don't break any
> other assumptions in the stack (e.g. CLUSTERIP).
Fair enough.
> > The question now is, in the absence of such a latter required check (and
> > indeed, in the case of CLUSTERIP), how we implement such a check.
> > Perhaps a sysctl is needed after all?
>
> Yeah, unfortunate situation.
>
> One could add those IP addresses as broadcast addresses (/32) to the
> routing table, so the brd_input jump would be taken.
>
> But this would still break users of CLUSTERIP until they install those
> routes. :(
I'm not even sure I understand this part :)
Any suggestions?
As long as IPv6 doesn't mandate it in the RFCs I'm not really sure we
should just drop it, even if we think it won't cause any problems?
CLUSTERIP seems like a special configuration, but I'm not sure it can be
detected and automatically allowed?
johannes
On Wed, 2014-08-27 at 17:31 +0300, Julian Anastasov wrote:
> > All IP protocols, this comes either from the IPv4 RFC (1122) or from the
> > wireless issue which affects all protocols.
>
> I did a grep for inet_add_protocol, in case if
> we prefer to use per-protocol checks:
>
> Protocols that look ok to me: TCP, SCTP, DCCP
>
> ICMP: missing check in icmp_rcv
> UDP, UDPLITE: need check in __udp4_lib_rcv
> IGMP: uses only multicast address?
> PIM: not sure if __pim_rcv() needs check, before skb_tunnel_rx()
> changes pkt_type?
>
> More protocols are also registered with inet_add_protocol(), I don't
> see pkt_type checks there, mostly tunnels:
> - IPPROTO_GRE
> - IPPROTO_L2TP
> - IPPROTO_IPIP
> - IPPROTO_IPV6 (tunnel64_rcv)
>
> If going to use a global check I hope there are
> no protocols that require exception to this rule.
Yeah that's the big question. Are you saying that TCP already implements
this? But I guess for TCP it's least interesting in a sense? Not really
sure.
I'd feel better implementing it at the IP level though, since it's a
fairly low-level requirement and also RFC 1122 is on the IP level
(obviously)
johannes
On Fri, 2014-08-22 at 10:54 -0700, David Miller wrote:
> >> if (res.type == RTN_BROADCAST)
> >> goto brd_input;
> >
> > Is this place better, after checking for RTN_BROADCAST?
> >
> > /* ARP link-layer broadcasts are acceptable here */
> > if ((skb->pkt_type == PACKET_BROADCAST ||
> > skb->pkt_type == PACKET_MULTICAST) &&
> > skb->protocol == htons(ETH_P_IP))
> > goto e_inval;
>
> Indeed, this would make ARP happier, but that still leaves open the
> issue of CLUSTERIP.
I'm back looking at this, but must admit I'm completely confused now :-)
I could add an IPv4 sysctl to control this behaviour:
0 - off
1 - RFC 1122 "SHOULD"
2 - also drop unicast-in-multicast (for wireless)
But I guess due to cluster-IP it would have to default to 0.
However, talk about ip_local_deliver_finish() in this thread has me
wondering if we could just implement it using iptables? I guess
ipt_addrtype and ip6t_addrtype would let me do that?
johannes