netdev - Re: ipv4/udp: Verify multicast group is ours in upd_v4_early

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CA++eYdsHw0-2rs1JB-VXiRwwGGCYpS5viW9EMb9sEK7nDcQnYQ@mail.gmail.com>
Date:	Fri, 31 Jul 2015 01:15:15 +0200
From:	Kenneth Klette Jonassen <kennetkl@....uio.no>
To:	Shawn Bohrer <shawn.bohrer@...il.com>
Cc:	Kenneth Klette Jonassen <kennetkl@....uio.no>,
	sbohrer@...advisors.com, netdev <netdev@...r.kernel.org>,
	Yurij.Plotnikov@...etlabs.ru, David Miller <davem@...emloft.net>,
	Ståle Kristoffersen <stalk@...dgetech.tv>
Subject: Re: ipv4/udp: Verify multicast group is ours in upd_v4_early_demux()

On Thu, Jul 30, 2015 at 11:32 PM, Shawn Bohrer <shawn.bohrer@...il.com> wrote:
> On Thu, Jul 30, 2015 at 10:26:08PM +0200, Kenneth Klette Jonassen wrote:
>> Commit 6e54030 breaks the IP_MULTICAST_ALL socket option. There is
>> already a check in ip_mc_sf_allow() that should do the filtering you
>> claim to fix. Was it considered?
>>
>> Commit message:
>> 421b3885bf6d56391297844f43fb7154a6396e12 "udp: ipv4: Add udp early
>> demux" introduced a regression that allowed sockets bound to INADDR_ANY
>> to receive packets from multicast groups that the socket had not joined...
>>
>> man ip(7):
>> IP_MULTICAST_ALL (since Linux 2.6.31)
>> This option can be used to modify the delivery policy of
>> multicast messages to sockets bound to the wildcard INADDR_ANY
>> address.  The argument is a boolean integer (defaults to 1).
>> If set to 1, the socket will receive messages from all the
>> groups that have been joined globally on the whole system.
>> Otherwise, it will deliver messages only from the groups that
>> have been explicitly joined…
>
> Do you have an application or test case that is now broken after this
> commit?  Or is this just a theoretical breakage based off of my commit
> message?  If you have a simple example test case can you share it?
> Also if this breaks your application but you don't have a test case
> can you describe the breakage?  Is IP_MULTICAST_ALL 0 or 1?  Are you
> not receiving packets or receiving packets you don't expect to
> receive?
>
> I admittedly haven't tried to create a test case yet, but looking over
> the code I'm not sure how this could have broken IP_MULTICAST_ALL.
> Commit 6e54030 added a call to ip_check_mc_rcu() which as far as I can
> tell checks to see that a received multicast packet was a mc address
> joined on that interface.

Ah, ok. Thanks for the clarification. Feedback was based on your
commit message, as we already had the per-socket check in
ip_mc_sf_allow() when mc_all==0. Since ip_check_mc_rcu() is really a
per-device check, it should be ok.

> Without this check it is possible to
> receive multicast packets for an address that hasn't been joined on
> that machine at all.  Which was the bug I was fixing.

Agreed. It is really filtering inadvertently received multicast packets.

Then we should probably move the ip_early_demux code from
ip_rcv_finish() to ip_route_input_noref()? As is, we now call
ip_check_mc_rcu() twice for some workloads, i.e., once for a failed
early_demux(), then again right before ip_route_input_slow().
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html