| Message ID | 1428652454-1224-1-git-send-email-johannes@sipsolutions.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
Hello, On Fri, 10 Apr 2015, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > In order to solve a problem with 802.11, the so-called hole-196 attack, > add an option (sysctl) called "drop_unicast_in_l2_multicast" which, if > enabled, causes the stack to drop IPv4 unicast packets encapsulated in > link-layer multi- or broadcast frames. Such frames can (as an attack) > be created by any member of the same wireless network and transmitted > as valid encrypted frames since the symmetric key for broadcast frames > is shared between all stations. > > Additionally, enabling this option provides compliance with a SHOULD > clause of RFC 1122. > > +++ b/net/ipv4/route.c > @@ -1727,6 +1727,26 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr, > if (res.type == RTN_BROADCAST) > goto brd_input; > > + /* 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. > + * > + * This doesn't explicitly say L2 *broadcast*, but broadcast is in a > + * way a form of multicast and the most common use case for this is > + * 802.11 protecting against cross-station spoofing (the so-called > + * "hole-196" attack) so do it for both. > + */ > + if (IN_DEV_CONF_GET(in_dev, DROP_UNICAST_IN_L2_MULTICAST) && For this flag IN_DEV_ORCONF can be used, by this way all/drop_unicast_in_l2_multicast=1 can enable it for all interfaces. > + (skb->pkt_type == PACKET_BROADCAST || > + skb->pkt_type == PACKET_MULTICAST)) > + goto e_inval; > + So, this is the same patch as the 2014-Aug version but this time with flag? But how the previous problems were addressed? May be something is changed in kernel afterwards? So, if your are back at step 1 can you check again the problems with this implementation?: http://marc.info/?l=linux-netdev&m=140865079120355&w=2 Thread: http://marc.info/?t=140864197300004&r=1&w=2 In short: - no way to select correct skb->pkt_type in inet_rtm_getroute before ip_route_input, this is a chiken-egg problem, of course, skb->pkt_type = PACKET_HOST can work for now - ip_route_input is called also for ARP, so incoming ARP broadcasts are not replied anymore - CLUSTERIP Regards -- Julian Anastasov <ja@ssi.bg> -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi > > > + if (IN_DEV_CONF_GET(in_dev, DROP_UNICAST_IN_L2_MULTICAST) && > > For this flag IN_DEV_ORCONF can be used, by this way > all/drop_unicast_in_l2_multicast=1 can enable it for all > interfaces. Good point. I need to look into that for the other patches in this series as well. > So, this is the same patch as the 2014-Aug version > but this time with flag? But how the previous problems were > addressed? May be something is changed in kernel afterwards? > > So, if your are back at step 1 can you check again > the problems with this implementation?: I think I am, sorry about that. I had focused on the CLUSTERIP problem in your email, and decided that was solved by simply having the admin for CLUSTERIP networks never set this flag. > - no way to select correct skb->pkt_type in inet_rtm_getroute > before ip_route_input, this is a chiken-egg problem, of course, > skb->pkt_type = PACKET_HOST can work for now This is interesting. Why does that even build an skb and route it, if it really just cares about the struct rtable? Anyway, you're right, using PACKET_HOST would work. I'm not really sure I see the chicken-and-egg problem though, it's not like the result of this is used to route the skb again or something, afaict? > - ip_route_input is called also for ARP, so incoming ARP > broadcasts are not replied anymore Hm. I didn't see this in my testing so far, but I only enable this temporarily, so perhaps by that time it was already done. This looks like another case of using this function to not really route anything but to just look up the route, similar to the inet_rtm_getroute() function you pointed out. Perhaps that would warrant splitting out somewhat. On the other hand, there's not all that much reason not to put this into ip_rcv_finish(), we already touch the rt->rt_type there for MIB counters. I'll look into that. > - CLUSTERIP See above. Thanks, johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Fri, 10 Apr 2015, Johannes Berg wrote: > > - no way to select correct skb->pkt_type in inet_rtm_getroute > > before ip_route_input, this is a chiken-egg problem, of course, > > skb->pkt_type = PACKET_HOST can work for now > > This is interesting. Why does that even build an skb and route it, if it > really just cares about the struct rtable? Anyway, you're right, using > PACKET_HOST would work. I'm not really sure I see the chicken-and-egg > problem though, it's not like the result of this is used to route the > skb again or something, afaict? If we want to set correct pkt_type here we should know the type of daddr (multicast => PACKET_MULTICAST, broadcast => PACKET_BROADCAST, etc) and we can know it only after routing (rt_type). I hope we will not do other checks with pkt_type, so we can put some comment that skb->pkt_type = PACKET_HOST is just to make DROP_UNICAST_IN_L2_MULTICAST check happy. > > - ip_route_input is called also for ARP, so incoming ARP > > broadcasts are not replied anymore > > Hm. I didn't see this in my testing so far, but I only enable this > temporarily, so perhaps by that time it was already done. I didn't tested it myself, may be you can try: other_host_on_LAN# arp -d patched_box other_host_on_LAN# arping -c 1 -I eth0 patched_box There must be reply for broadcast requests. > This looks like another case of using this function to not really route > anything but to just look up the route, similar to the > inet_rtm_getroute() function you pointed out. Perhaps that would warrant > splitting out somewhat. Yes, ARP essentially performs rp_filter check (note that arp_filter check is limited only to local targets, not to proxy_arp) and target address type check. The solution would be to add skb->protocol == htons(ETH_P_IP) check. > On the other hand, there's not all that much reason not to put this into > ip_rcv_finish(), we already touch the rt->rt_type there for MIB > counters. I'll look into that. May be in ip_local_deliver_finish because: - ip_forward() already has PACKET_HOST check. - for broadcast/multicast dests we do not care - CLUSTERIP works in LOCAL_IN (after ip_rcv_finish), LOCAL_IN is here: ip_rcv_finish->dst_input->ip_local_deliver-> ip_local_deliver_finish Regards -- Julian Anastasov <ja@ssi.bg> -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Julian, > > Hm. I didn't see this in my testing so far, but I only enable this > > temporarily, so perhaps by that time it was already done. > > I didn't tested it myself, may be you can try: > > other_host_on_LAN# arp -d patched_box > other_host_on_LAN# arping -c 1 -I eth0 patched_box > > There must be reply for broadcast requests. Sure; I would have expected this to have broken my network but as I said, it was only enabled temporarily. > > On the other hand, there's not all that much reason not to put this into > > ip_rcv_finish(), we already touch the rt->rt_type there for MIB > > counters. I'll look into that. > > May be in ip_local_deliver_finish because: > > - ip_forward() already has PACKET_HOST check. > > - for broadcast/multicast dests we do not care > > - CLUSTERIP works in LOCAL_IN (after ip_rcv_finish), LOCAL_IN > is here: ip_rcv_finish->dst_input->ip_local_deliver-> > ip_local_deliver_finish It's just that the later this is, the more nervous I get about it being really effective. :) I'm willing to discount the CLUSTERIP case, it seems insane to want to run CLUSTERIP over wifi on a network that explicitly limits multicast. Other than that, do you see any reason for not putting it in ip_rcv_finish()? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Mon, 13 Apr 2015, Johannes Berg wrote: > > - CLUSTERIP works in LOCAL_IN (after ip_rcv_finish), LOCAL_IN > > is here: ip_rcv_finish->dst_input->ip_local_deliver-> > > ip_local_deliver_finish > > It's just that the later this is, the more nervous I get about it being > really effective. :) > > I'm willing to discount the CLUSTERIP case, it seems insane to want to > run CLUSTERIP over wifi on a network that explicitly limits multicast. > > Other than that, do you see any reason for not putting it in > ip_rcv_finish()? ok, I don't remember for other cases. Regards -- Julian Anastasov <ja@ssi.bg> -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt index 071fb18dc57c..f97ad76e6f82 100644 --- a/Documentation/networking/ip-sysctl.txt +++ b/Documentation/networking/ip-sysctl.txt @@ -1165,6 +1165,13 @@ promote_secondaries - BOOLEAN promote a corresponding secondary IP address instead of removing all the corresponding secondary IP addresses. +drop_unicast_in_l2_multicast - BOOLEAN + Drop any unicast IP packets that are received in link-layer + multicast (or broadcast) frames. + This behavior (for multicast) is actually a SHOULD in RFC + 1122, but is disabled by default for compatibility reasons. + Default: off (0) + tag - INTEGER Allows you to write a number, which can be used as required. diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h index 411959405ab6..c0e594b209ff 100644 --- a/include/uapi/linux/ip.h +++ b/include/uapi/linux/ip.h @@ -164,6 +164,7 @@ enum IPV4_DEVCONF_ROUTE_LOCALNET, IPV4_DEVCONF_IGMPV2_UNSOLICITED_REPORT_INTERVAL, IPV4_DEVCONF_IGMPV3_UNSOLICITED_REPORT_INTERVAL, + IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST, __IPV4_DEVCONF_MAX }; diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c index c6473f365ad1..b608407f96e7 100644 --- a/net/ipv4/devinet.c +++ b/net/ipv4/devinet.c @@ -2176,6 +2176,8 @@ static struct devinet_sysctl_table { "promote_secondaries"), DEVINET_SYSCTL_FLUSHING_ENTRY(ROUTE_LOCALNET, "route_localnet"), + DEVINET_SYSCTL_FLUSHING_ENTRY(DROP_UNICAST_IN_L2_MULTICAST, + "drop_unicast_in_l2_multicast"), }, }; diff --git a/net/ipv4/route.c b/net/ipv4/route.c index 652b92ebd7ba..5d5e03f9697b 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -1727,6 +1727,26 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr, if (res.type == RTN_BROADCAST) goto brd_input; + /* 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. + * + * This doesn't explicitly say L2 *broadcast*, but broadcast is in a + * way a form of multicast and the most common use case for this is + * 802.11 protecting against cross-station spoofing (the so-called + * "hole-196" attack) so do it for both. + */ + if (IN_DEV_CONF_GET(in_dev, DROP_UNICAST_IN_L2_MULTICAST) && + (skb->pkt_type == PACKET_BROADCAST || + skb->pkt_type == PACKET_MULTICAST)) + goto e_inval; + if (res.type == RTN_LOCAL) { err = fib_validate_source(skb, saddr, daddr, tos, 0, dev, in_dev, &itag);