Hello,
On Fri, Mar 03, 2023 at 09:14:38PM +0100, Alexander Bluhm wrote:
> On Fri, Mar 03, 2023 at 08:54:51AM +0100, Luca Di Gregorio wrote:
> > Hi, just another bit of info about this issue.
>
> Instead of implementing more and more details of RFC, we should
> discuss what the goal is.
>
> We had a pf that dropped valid IGMP packets due to router-alert
> option. So I added code to pass them, but still block other options.
> This is neccessary for working multicast.
>
> Then sashan@ commited a diff that blocks packets with bad TTL or
> destination address. This was too strict for some use cases as we
> see now. But it may improve security.
>
> What are the IGMP illegal packets that an attacker might use? We
> should drop them. This IGMP logic is deep down in pf that a user
> cannot override. So we should be careful not to destroy valid use
> cases. Replacing || with && somehow in the if condition looks
> reasonalbe to me.
>
My commit was wrong. It makes pf(4) to drop all IGMP packets which
either have TTL other than 1
or are not sent to multicast address
this is what we currently have in pf_walk_header()
6849 /* IGMP packets have router alert options, allow them */
6850 if (pd->proto == IPPROTO_IGMP) {
6851 /* According to RFC 1112 ttl must be set to 1. */
6852 if ((h->ip_ttl != 1) || !IN_MULTICAST(h->ip_dst.s_addr)) {
6853 DPFPRINTF(LOG_NOTICE, "Invalid IGMP");
6854 REASON_SET(reason, PFRES_IPOPTIONS);
6855 return (PF_DROP);
6856 }
6857 CLR(pd->badopts, PF_OPT_ROUTER_ALERT);
6858 }
I think Luca is right pf(4) is too paranoid breaking IGMP. The RFC 1112.
This is what 'Informal Protocol Description' section says:
Multicast routers send Host Membership Query messages (hereinafter
called Queries) to discover which host groups have members on their
attached local networks. Queries are addressed to the all-hosts
group (address 224.0.0.1), and carry an IP time-to-live of 1.
In my commit I confused any multicast address with 'all-hosts' group
(224.0.0.1)
So we either want to revert that commit or fix it. I think the condition at
line 6850 should read as follows:
6847 /* IGMP packets have router alert options, allow them */
6848 if (pd->proto == IPPROTO_IGMP) {
6849 /*
6850 * According to RFC 1112 ttl must be set to 1 in all IGMP
6851 * packets sent do 224.0.0.1
6852 */
6853 if ((h->ip_ttl != 1) &&
6854 (h->ip_dst.s_addr == INADDR_ALLHOSTS_GROUP)) {
6855 DPFPRINTF(LOG_NOTICE, "Invalid IGMP");
6856 REASON_SET(reason, PFRES_IPOPTIONS);
6857 return (PF_DROP);
6858 }
6859 CLR(pd->badopts, PF_OPT_ROUTER_ALERT);
This change should make pf(4) reasonably paranoid while keeping IGMP working.
thanks and
regards
sashan
