bridge: multicast: add sanity check for general query destination
Subsystems (3)
PATCH v2 1/2 bridge: multicast: add sanity check for general query destination
General IGMP and MLD queries are supposed to have the multicast link-local all-nodes address as their destination according to RFC2236 section 9, RFC3376 section 4.1.12/9.1, RFC2710 section 8 and RFC3810 section 5.1.15.
Without this check, such malformed IGMP/MLD queries can result in a denial of service: The queries are ignored by most IGMP/MLD listeners therefore they will not respond with an IGMP/MLD report. However, without this patch these malformed MLD queries would enable the snooping part in the bridge code, potentially shutting down the according ports towards these hosts for multicast traffic as the bridge did not learn about these listeners.
---
v2: simplified is_general_query variable assignment (thanks Cong!)
net/bridge/br_multicast.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
net/bridge/br_multicast.c +19 -0
@@ -1181,6 +1181,14 @@ static int br_ip4_multicast_query(struct net_bridge *br, IGMPV3_MRC(ih3->code) * (HZ / IGMP_TIMER_SCALE) : 1; } +/* RFC2236+RFC3376 (IGMPv2+IGMPv3) require the multicast link layer +* all-systems destination addresses (224.0.0.1) for general queries +*/ +if (!group && iph->daddr !=htonl (INADDR_ALLHOSTS_GROUP )) { + err = -EINVAL ; + goto out; + } +br_multicast_query_received (br, port, &br->ip4_querier, !!iph->saddr, max_delay);
@@ -1228,6 +1236,7 @@ static int br_ip6_multicast_query(struct net_bridge *br,unsigned long max_delay;unsigned long now = jiffies;const struct in6_addr *group =NULL ; +bool is_general_query;int err =0 ;spin_lock (&br->multicast_lock);
@@ -1262,6 +1271,16 @@ static int br_ip6_multicast_query(struct net_bridge *br, max_delay =max (msecs_to_jiffies (mldv2_mrc (mld2q)),1UL ); } + is_general_query = group &&ipv6_addr_any (group); + +/* RFC2710+RFC3810 (MLDv1+MLDv2) require the multicast link layer +* all-nodes destination address (ff02::1) for general queries +*/ +if (is_general_query && !ipv6_addr_is_ll_all_nodes (&ip6h->daddr)) { + err = -EINVAL ; + goto out; + } +br_multicast_query_received (br, port, &br->ip6_querier, !ipv6_addr_any (&ip6h->saddr), max_delay);
Signature · 1 line
1.7.10.4
PATCH v2 2/2 bridge: multicast: enable snooping on general queries only
Without this check someone could easily create a denial of service by injecting multicast-specific queries to enable the bridge snooping part if no real querier issuing periodic general queries is present on the link which would result in the bridge wrongly shutting down ports for multicast traffic as the bridge did not learn about these listeners.
With this patch the snooping code is enabled upon receiving valid, general queries only.
---
v2: unchanged
net/bridge/br_multicast.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
net/bridge/br_multicast.c +5 -3
@@ -1127,9 +1127,10 @@ static void br_multicast_query_received(struct net_bridge *br, struct net_bridge_port *port, struct bridge_mcast_querier *querier, int saddr, + bool is_general_query, unsigned long max_delay) { - if (saddr) + if (saddr && is_general_query) br_multicast_update_querier_timer(br, querier, max_delay); else if (timer_pending(&querier->timer)) return;
@@ -1190,7 +1191,7 @@ static int br_ip4_multicast_query(struct net_bridge *br, }br_multicast_query_received (br, port, &br->ip4_querier, !!iph->saddr, - max_delay); + !group, max_delay);if (!group) goto out;
@@ -1282,7 +1283,8 @@ static int br_ip6_multicast_query(struct net_bridge *br, }br_multicast_query_received (br, port, &br->ip6_querier, - !ipv6_addr_any (&ip6h->saddr), max_delay); + !ipv6_addr_any (&ip6h->saddr), + is_general_query, max_delay);if (!group) goto out;
Signature · 1 line
1.7.10.4
Re: [PATCHv2 2/2] bridge: multicast: enable snooping on general queries only
net/bridge/br_multicast.c static int br_ip6_multicast_query(struct net_bridge *br, · line 1287
Quoted 4 lines · Linus Lüssing
net/bridge/br_multicast.c +2
-1br_multicast_query_received (br, port, &br->ip6_querier, - !ipv6_addr_any (&ip6h->saddr), max_delay); + !ipv6_addr_any (&ip6h->saddr), + is_general_query, max_delay);
Just a small nit, maybe for a later patch:
After your change 6565b9eeef194a ("bridge: multicast: add sanity check for query source addresses"), which is still in -net only, we could replace !ipv6_addr_any(&ip6h->saddr) with '1'?
Greetings,
Hannes
Re: [PATCHv2 2/2] bridge: multicast: enable snooping on general queries only
Quoted 11 lines · Hannes Frederic Sowa
net/bridge/br_multicast.c static int br_ip6_multicast_query(struct net_bridge *br, · line 1287
Quoted 4 lines · Linus Lüssing
net/bridge/br_multicast.c +2
-1br_multicast_query_received (br, port, &br->ip6_querier, - !ipv6_addr_any (&ip6h->saddr), max_delay); + !ipv6_addr_any (&ip6h->saddr), + is_general_query, max_delay);Just a small nit, maybe for a later patch:
After your change
6565b9eeef194a("bridge: multicast: add sanity check for query source addresses"), which is still in -net only, we could replace !ipv6_addr_any(&ip6h->saddr) with '1'?
Aiy, good point, that part is obsolete now and br_multicast_query_received() could be simplified, right. Going to do that once we are out of deep-RC territory again and/or the according commit is available in net-next. Thanks for the hint!
Cheers, Linus
Re: [PATCHv2 2/2] bridge: multicast: enable snooping on general queries only
In reply to Linus LüssingFrom: Linus Lüssing <[email protected]>
Date: Mon, 10 Mar 2014 22:25:25 +0100
Quoted 11 lines
Without this check someone could easily create a denial of service by injecting multicast-specific queries to enable the bridge snooping part if no real querier issuing periodic general queries is present on the link which would result in the bridge wrongly shutting down ports for multicast traffic as the bridge did not learn about these listeners.
With this patch the snooping code is enabled upon receiving valid, general queries only.
Signed-off-by Linus Lüssing <[email protected]>
Applied.
From: Linus Lüssing <[email protected]>
Date: Mon, 10 Mar 2014 22:25:24 +0100
Quoted 13 lines
General IGMP and MLD queries are supposed to have the multicast link-local all-nodes address as their destination according to RFC2236 section 9, RFC3376 section 4.1.12/9.1, RFC2710 section 8 and RFC3810 section 5.1.15.
Without this check, such malformed IGMP/MLD queries can result in a denial of service: The queries are ignored by most IGMP/MLD listeners therefore they will not respond with an IGMP/MLD report. However, without this patch these malformed MLD queries would enable the snooping part in the bridge code, potentially shutting down the according ports towards these hosts for multicast traffic as the bridge did not learn about these listeners.
Quoted 2 lines
Reported-by Jan Stancek <[email protected]>Signed-off-by Linus Lüssing <[email protected]>
Applied.
Quoted 4 lines · Linus Lüssing
General IGMP and MLD queries are supposed to have the multicast link-local all-nodes address as their destination according to RFC2236 section 9, RFC3376 section 4.1.12/9.1, RFC2710 section 8 and RFC3810 section 5.1.15.
[...]
That reminds me, shouldn't we also check this in igmp_rcv()?
Ben.
Signature · 2 lines
Ben Hutchings Any sufficiently advanced bug is indistinguishable from a feature.