[PATCH] IPv4 Multicast: prevent reception of mcast frames from unjoined groups

[PATCH] IPv4 Multicast: prevent reception of mcast frames from unjoined groups

[Neil Horman]

The current ipv4 multicast code incorrectly allows frames destined to any
multicast group to be received on any socket that is bound to INADDR_ANY,
regardless of weather or not that socket is subscribed to that group.  The
problem is in the ip_mc_sf_allow function.  If a multicast frame enters that
function, and no maching address is found on the passed in interfaces mc_list,
it returns 1, which causes the frame to be delivered.  It should instead return
0, whcih causes the calling function to skip delivery to the passed in socket.
Tested successfully by myself and the reporter.

Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>



igmp.c |   13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index 2769dc4..bf54d39 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -2215,6 +2215,9 @@ int ip_mc_sf_allow(struct sock *sk, __be32 loc_addr, __be32 rmt_addr, int dif)
 	struct ip_sf_socklist *psl;
 	int i;
 
+	if (ipv4_is_lbcast(loc_addr))
+		return 1;
+
 	if (!ipv4_is_multicast(loc_addr))
 		return 1;
 
@@ -2223,8 +2226,14 @@ int ip_mc_sf_allow(struct sock *sk, __be32 loc_addr, __be32 rmt_addr, int dif)
 		    pmc->multi.imr_ifindex == dif)
 			break;
 	}
-	if (!pmc)
-		return 1;
+	if (!pmc) {
+		/*
+		 * We aren't explicitly added to any mc group
+		 * so don't deliver
+		 */
+		return 0;
+	}
+
 	psl = pmc->sflist;
 	if (!psl)
 		return pmc->sfmode == MCAST_EXCLUDE;
-- 
/****************************************************
 * Neil Horman <nhorman@tuxdriver.com>
 * Software Engineer, Red Hat
 ****************************************************/

Re: [PATCH] IPv4 Multicast: prevent reception of mcast frames from unjoined groups

[Alexey Kuznetsov]

Hello!

> The current ipv4 multicast code incorrectly allows frames destined to any
> multicast group to be received on any socket that is bound to INADDR_ANY,

Yes, it used to behave this way.

And this is more fresh (than reference to practice in days of yore :-))
rationale given by David Stevens:


> Subject: Re: [PATCH] make ipv4 multicast packets only get delivered to sockets	that
>  are joined to group
> From: David Stevens <dlstevens@us.ibm.com>
> Date: Thu, 14 Sep 2006 08:39:39 -0700
> 
> Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> wrote on 09/14/2006 03:30:37 AM:
> 
> > Hello!
> > 
> > >         No, it returns 1 (allow) if there are no filters to explicitly
> > > filter it. I wrote that code. :-)
> > 
> > I see. It did not behave this way old times.
> > 
> > From your mails I understood that current behaviour matches another
> > implementations (BSD whatever), is it true?
> 
> Hi, Alexey,
> 
> If you mean IPv6 code in current BSD derivatives, I don't know.
> 
> The IPv6 behaviour was different from IPv4 on Linux and was changed for
> compatibility with IPv4 (discussion on netdev agreed that binding
> should determine socket delivery, not group membership, or simply
> that there was no reason to be different from long-standing IPv4 
> practice).
> 
> The IPv4 code is that way for compatibility with everything else since
> about ~4.3BSD (with the possible exception of Solaris 8, apparently).
> 
> FWIW, I think Deering's original interpretation is correct. Adding
> a multicast address to an interface by joining a group is little
> different from adding a unicast address via SIOCSIFADDR, which
> certainly does affect packets delivered to the machine and to any
> INADDR_ANY-bound socket. Binding to the multicast address and not
> INADDR_ANY will give you only packets for that group, if that's
> what you want, just as in the unicast address-specific bind.
> 
>                                                         +-DLS

Re: [PATCH] IPv4 Multicast: prevent reception of mcast frames from unjoined groups

[David Stevens]

netdev-owner@vger.kernel.org wrote on 07/11/2008 08:21:13 AM:

> The current ipv4 multicast code incorrectly allows frames destined to 
any
> multicast group to be received on any socket that is bound to 
INADDR_ANY,
> regardless of weather or not that socket is subscribed to that group.

NACK. As Alexey already quoted from me, multicasting just doesn't work
that way, since the beginning of time.

                                                                +-DLS

Re: [PATCH] IPv4 Multicast: prevent reception of mcast frames from unjoined groups

[Neil Horman]

On Fri, Jul 11, 2008 at 08:48:01PM +0400, Alexey Kuznetsov wrote:
> Hello!
> 
> > The current ipv4 multicast code incorrectly allows frames destined to any
> > multicast group to be received on any socket that is bound to INADDR_ANY,
> 
> Yes, it used to behave this way.
> 
Not sure what you mean here, it still does behave this way.

> And this is more fresh (than reference to practice in days of yore :-))
> rationale given by David Stevens:
> 
> 
> > Subject: Re: [PATCH] make ipv4 multicast packets only get delivered to sockets	that
> >  are joined to group
> > From: David Stevens <dlstevens@us.ibm.com>
> > Date: Thu, 14 Sep 2006 08:39:39 -0700
> > 
> > Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> wrote on 09/14/2006 03:30:37 AM:
> > 
> > > Hello!
> > > 
> > > >         No, it returns 1 (allow) if there are no filters to explicitly
> > > > filter it. I wrote that code. :-)
> > > 
> > > I see. It did not behave this way old times.
> > > 
> > > From your mails I understood that current behaviour matches another
> > > implementations (BSD whatever), is it true?
> > 
> > Hi, Alexey,
> > 
> > If you mean IPv6 code in current BSD derivatives, I don't know.
> > 
> > The IPv6 behaviour was different from IPv4 on Linux and was changed for
> > compatibility with IPv4 (discussion on netdev agreed that binding
> > should determine socket delivery, not group membership, or simply
> > that there was no reason to be different from long-standing IPv4 
> > practice).
> > 
> > The IPv4 code is that way for compatibility with everything else since
> > about ~4.3BSD (with the possible exception of Solaris 8, apparently).
> > 
> > FWIW, I think Deering's original interpretation is correct. Adding
> > a multicast address to an interface by joining a group is little
> > different from adding a unicast address via SIOCSIFADDR, which
> > certainly does affect packets delivered to the machine and to any
> > INADDR_ANY-bound socket. Binding to the multicast address and not
> > INADDR_ANY will give you only packets for that group, if that's
> > what you want, just as in the unicast address-specific bind.
> > 
> >                                                         +-DLS

I see what David is trying to say, but
there is still a bug here.  in ip_mc_sf_allow, we scan the mc_list for the
passed in inet_dev looking for a match, if we find a match we check it to see if
the sfmode matches the mode of the source filter list, and we return accept or
deny based on that information.  The problem arises when there is no match on
the mc_list.  The mc_list represents mcast groups that the interface has joined.
currently in ip_mc_sf_allow we assume that we need to allow the frame if no
match is found, but that has the affect of causing frames to be received for
groups a socket hasn't joined.  I asked david aobut this specifically.

I can see how semantically for a source filter we might want to return allow for
no match in the mc_list, but the code just doesn't seem to allow for that.  In
udp_v4_mcast_next, we return a socket if (among other tests) ip_mc_sf_allow
returns 1, and we're universally returning 1 if the received packet has a
multicast destination address that doesn't appear on the source interfaces
mc_list.

David, can you provide insight here?

Regards
Neil

-- 
/****************************************************
 * Neil Horman <nhorman@tuxdriver.com>
 * Software Engineer, Red Hat
 ****************************************************/

Re: [PATCH] IPv4 Multicast: prevent reception of mcast frames from unjoined groups

[Neil Horman]

On Fri, Jul 11, 2008 at 10:16:41AM -0700, David Stevens wrote:
> netdev-owner@vger.kernel.org wrote on 07/11/2008 08:21:13 AM:
> 
> > The current ipv4 multicast code incorrectly allows frames destined to 
> any
> > multicast group to be received on any socket that is bound to 
> INADDR_ANY,
> > regardless of weather or not that socket is subscribed to that group.
> 
> NACK. As Alexey already quoted from me, multicasting just doesn't work
> that way, since the beginning of time.
> 
>                                                                 +-DLS

So you're saying that if I take a process, call bind, specifying INADDR_ANY, and
then call setsockopt(...,IP_ADD_MEMBERSHIP,...) specifying a multicast group X,
that I can expect to recieve messages from other multicast addresses that other
processes in the system have joined to?  That doesn't seem at all correct, and
it contradicts what you and I discussed privately.  As the code currently
stands, if you do the above in process A, joining mcast group X, and then start
a second process on the same system, joining mcast group Y, process A will
recieve mcast group Y frames.

Neil

-- 
/****************************************************
 * Neil Horman <nhorman@tuxdriver.com>
 * Software Engineer, Red Hat
 ****************************************************/

Re: [PATCH] IPv4 Multicast: prevent reception of mcast frames from unjoined groups

[David Stevens]

> currently in ip_mc_sf_allow we assume that we need to allow the frame if 
no
> match is found, but that has the affect of causing frames to be received 
for
> groups a socket hasn't joined.  I asked david aobut this specifically.

        You asked me about this? Or the other David? :-) I don't remember
seeing anything on this-- at least not recently.

        But in the sentence above, I think you missed the point of the
mail I sent before. Joining a group or not on a particular socket has
nothing at all to do with delivery of multicasts to the socket.

        Multicast addresses, like unicast addresses, are for the entire
machine, not just the socket that does the join. If anyone on the
machine has joined the group and your binding matches the packet, you
will receive a copy. That's intentional. If you don't join any groups
at all, but bind to INADDR_ANY, you will receive packets for the port
and protocol and any local unicast or multicast address (including
groups joined by any other process on the machine).

 +-DLS

Re: [PATCH] IPv4 Multicast: prevent reception of mcast frames from unjoined groups

[Neil Horman]

On Fri, Jul 11, 2008 at 10:53:01AM -0700, David Stevens wrote:
> > currently in ip_mc_sf_allow we assume that we need to allow the frame if 
> no
> > match is found, but that has the affect of causing frames to be received 
> for
> > groups a socket hasn't joined.  I asked david aobut this specifically.
> 
>         You asked me about this? Or the other David? :-) I don't remember
> seeing anything on this-- at least not recently.
> 
It was you (I wrote from my rh address :)), the question of note that I asked
you about:

>>>
>>> 4) Finally, what if process B bound itself to INADDR_ANY rather than to
>>>the
>>> specific multicast group.  Should it see process A's sent frames then?
>>
>>        Not if the group membership is on lo and the sends are on eth0.
>>The
>>reason it isn't seeing the packets is not the binding, but the group
>>membership. To hear packets you're sending out an interface, you must
>>join that group on that interface *and* the sender must allow loopback by
>>not clearing IP_MULTICAST_LOOP. Joining the group on a different interface
>>really is joining a different group, as far as multicasting is concerned.

>         But in the sentence above, I think you missed the point of the
> mail I sent before. Joining a group or not on a particular socket has
> nothing at all to do with delivery of multicasts to the socket.
> 
>         Multicast addresses, like unicast addresses, are for the entire
> machine, not just the socket that does the join. If anyone on the
> machine has joined the group and your binding matches the packet, you
> will receive a copy. That's intentional. If you don't join any groups
> at all, but bind to INADDR_ANY, you will receive packets for the port
> and protocol and any local unicast or multicast address (including
> groups joined by any other process on the machine).
> 
>  +-DLS

If thats the final word, then I'll believe you, but it seems to me that
receiving multicast traffic on a socket that didn't specifically join a
multicast group is asking for trouble, as every application needs to be prepared
to handle data payloads it was not expected to recieve.

Can you clarify your statement above with the one that I copied in from you
earlier?

Regards
Neil


-- 
/****************************************************
 * Neil Horman <nhorman@tuxdriver.com>
 * Software Engineer, Red Hat
 ****************************************************/

Re: [PATCH] IPv4 Multicast: prevent reception of mcast frames from unjoined groups

[David Stevens]

netdev-owner@vger.kernel.org wrote on 07/11/2008 10:35:49 AM:

> So you're saying that if I take a process, call bind, specifying 
INADDR_ANY, and
> then call setsockopt(...,IP_ADD_MEMBERSHIP,...) specifying a multicast 
group X,
> that I can expect to recieve messages from other multicast addresses 
that other
> processes in the system have joined to? 

Yes.

> That doesn't seem at all correct, and
> it contradicts what you and I discussed privately. 

        I don't remember discussing this with you privately, but I think
either I wasn't clear, or there's a misunderstanding. I think it doesn't
seem correct to you because you have a different model in mind. You're 
thinking
(as almost everyone seems to) that the join has something to do with the 
socket.
The model, from the original implementation, is the same as for unicast
addresses-- it is adding an address _to_the_machine_. It isn't a private
address for that socket only.
        The binding is what determines delivery to the socket. If you have
other traffic on the same port that you don't want, then you don't want
INADDR_ANY.

> As the code currently
> stands, if you do the above in process A, joining mcast group X, and 
then start
> a second process on the same system, joining mcast group Y, process A 
will
> recieve mcast group Y frames.

        Yes. In fact, there are programs that rely on this. Some versions
of JVM join all the groups it needs in one process, but other processes
do all the I/O for those groups.

That's how it has always worked in the original socket implementation 
(BSD)
and derivatives. The control for socket reception is the binding, so if 
you
want only mcast X packets, you need to bind to X. If you bind to 
INADDR_ANY,
that *means* you don't care what the destination address is and you'll 
receive
all unicast and multicast packets with a matching port and protocol.

                                                                +-DLS

Re: [PATCH] IPv4 Multicast: prevent reception of mcast frames from unjoined groups

[Rémi Denis-Courmont]

Le vendredi 11 juillet 2008 20:35:49 Neil Horman, vous avez écrit :
> So you're saying that if I take a process, call bind, specifying
> INADDR_ANY, and then call setsockopt(...,IP_ADD_MEMBERSHIP,...) specifying
> a multicast group X, that I can expect to recieve messages from other
> multicast addresses that other processes in the system have joined to? 

Yes, he says so. I am not aware of a real standard for the IPv4 socket API: 
POSIX and IETF only defined the IPv6 multicast API, it seems. That being 
noted, the Linux behavior is in accordance with RFC3493:

|     IPV6_JOIN_GROUP
|
|        Join a multicast group on a specified local interface.
|        If the interface index is specified as 0,
|        the kernel chooses the local interface.
|        For example, some kernels look up the multicast group
|        in the normal IPv6 routing table and use the resulting
|        interface.

Note the use of *interface* rather than *socket* here. And then:

|  Note that to receive multicast datagrams a process must join the
|  multicast group to which datagrams will be sent.  UDP applications
|  must also bind the UDP port to which datagrams will be sent.  Some
|  processes also bind the multicast group address to the socket, in
|  addition to the port, to prevent other datagrams destined to that
|  same port from being delivered to the socket.

-- 
Rémi Denis-Courmont
http://www.remlab.net/

Re: [PATCH] IPv4 Multicast: prevent reception of mcast frames from unjoined groups

[David Stevens]

Yes, that's me, and that's also true. It wasn't the address, it's the time 
delay. I think that
was months ago. But you left out a distinguishing piece in your example 
here, which is
all the difference.

Joins are also per-interface. So, joining a group on "lo" does not join 
the group on
"eth0". In your example below (4), the reason it won't receive the packets 
is because
the machine is not a group member on eth0. If any process joined the group 
on eth0
then an INADDR_ANY-bound socket would receive them (whether it joined or 
not).
I guess I wasn't clear -- the reason you have to join below is to 
guarantee someone
has joined. If some other process already joined on that interface, you 
would
receive them also.

                                                                +-DLS

Neil Horman <nhorman@tuxdriver.com> wrote on 07/11/2008 11:23:04 AM:

> 
> >>>
> >>> 4) Finally, what if process B bound itself to INADDR_ANY rather than 
to
> >>>the
> >>> specific multicast group.  Should it see process A's sent frames 
then?
> >>
> >>        Not if the group membership is on lo and the sends are on 
eth0.
> >>The
> >>reason it isn't seeing the packets is not the binding, but the group
> >>membership. To hear packets you're sending out an interface, you must
> >>join that group on that interface *and* the sender must allow loopback 
by
> >>not clearing IP_MULTICAST_LOOP. Joining the group on a different 
interface
> >>really is joining a different group, as far as multicasting is 
concerned.
> 
> >         But in the sentence above, I think you missed the point of the
> > mail I sent before. Joining a group or not on a particular socket has
> > nothing at all to do with delivery of multicasts to the socket.
> > 
> >         Multicast addresses, like unicast addresses, are for the 
entire
> > machine, not just the socket that does the join. If anyone on the
> > machine has joined the group and your binding matches the packet, you
> > will receive a copy. That's intentional. If you don't join any groups
> > at all, but bind to INADDR_ANY, you will receive packets for the port
> > and protocol and any local unicast or multicast address (including
> > groups joined by any other process on the machine).
> > 
> >  +-DLS
> 
> If thats the final word, then I'll believe you, but it seems to me that
> receiving multicast traffic on a socket that didn't specifically join a
> multicast group is asking for trouble, as every application needs to be 
prepared
> to handle data payloads it was not expected to recieve.
> 
> Can you clarify your statement above with the one that I copied in from 
you
> earlier?
> 
> Regards
> Neil
> 
> 
> -- 
> /****************************************************
>  * Neil Horman <nhorman@tuxdriver.com>
>  * Software Engineer, Red Hat
>  ****************************************************/

Re: [PATCH] IPv4 Multicast: prevent reception of mcast frames from unjoined groups

[Neil Horman]

On Fri, Jul 11, 2008 at 12:00:57PM -0700, David Stevens wrote:
> Yes, that's me, and that's also true. It wasn't the address, it's the time 
> delay. I think that
> was months ago. But you left out a distinguishing piece in your example 
> here, which is
> all the difference.
> 
> Joins are also per-interface. So, joining a group on "lo" does not join 
> the group on
> "eth0". In your example below (4), the reason it won't receive the packets 
> is because
> the machine is not a group member on eth0. If any process joined the group 
> on eth0
> then an INADDR_ANY-bound socket would receive them (whether it joined or 
> not).
> I guess I wasn't clear -- the reason you have to join below is to 
> guarantee someone
> has joined. If some other process already joined on that interface, you 
> would
> receive them also.
> 
>                                                                 +-DLS
> 
> Neil Horman <nhorman@tuxdriver.com> wrote on 07/11/2008 11:23:04 AM:
> 
> > 
> > >>>
> > >>> 4) Finally, what if process B bound itself to INADDR_ANY rather than 
> to
> > >>>the
> > >>> specific multicast group.  Should it see process A's sent frames 
> then?
> > >>
> > >>        Not if the group membership is on lo and the sends are on 
> eth0.
> > >>The
> > >>reason it isn't seeing the packets is not the binding, but the group
> > >>membership. To hear packets you're sending out an interface, you must
> > >>join that group on that interface *and* the sender must allow loopback 
> by
> > >>not clearing IP_MULTICAST_LOOP. Joining the group on a different 
> interface
> > >>really is joining a different group, as far as multicasting is 
> concerned.
> > 
> > >         But in the sentence above, I think you missed the point of the
> > > mail I sent before. Joining a group or not on a particular socket has
> > > nothing at all to do with delivery of multicasts to the socket.
> > > 
> > >         Multicast addresses, like unicast addresses, are for the 
> entire
> > > machine, not just the socket that does the join. If anyone on the
> > > machine has joined the group and your binding matches the packet, you
> > > will receive a copy. That's intentional. If you don't join any groups
> > > at all, but bind to INADDR_ANY, you will receive packets for the port
> > > and protocol and any local unicast or multicast address (including
> > > groups joined by any other process on the machine).
> > > 
> > >  +-DLS
> > 
> > If thats the final word, then I'll believe you, but it seems to me that
> > receiving multicast traffic on a socket that didn't specifically join a
> > multicast group is asking for trouble, as every application needs to be 
> prepared
> > to handle data payloads it was not expected to recieve.
> > 
> > Can you clarify your statement above with the one that I copied in from 
> you
> > earlier?
> > 
> > Regards
> > Neil
> > 
> > 
> > -- 
> > /****************************************************
> >  * Neil Horman <nhorman@tuxdriver.com>
> >  * Software Engineer, Red Hat
> >  ****************************************************/


Alright, we'll it sounds like this has been well thought out, and my
understanding is in the wrong.  Patch rescinded. Sorry for the noise.

Best
Neil

-- 
/****************************************************
 * Neil Horman <nhorman@tuxdriver.com>
 * Software Engineer, Red Hat
 ****************************************************/