On Sun, Feb 26, 2023 at 12:09:13AM +0100, Alexandr Nedvedicky wrote: > Hello,
Hi, one remark at the end. I usually add explicit pass rules for multicast IPv6 traffic on my boxes where IPv6 matters, because otherwise I've seen issues in the past, but I don't have details anymore. So I'm happy if pf starts behaving better by default with IPv6, but see my comment below. > > the issue has been discovered and analyzed by Luca di Gregorio > on bugs@ [1]. The thread [1] covers all details. People who > wish to read brief summary can skip to Luca's email [2]. > > To wrap it up the current handling IGMP/MLD messages in pf(4) > is exact opposite. I failed to translate English words from > RFC standards into correct C code. Patch below are two one-liners > which make multicast for IPv4/IPv6 to work again with pf(4) enabled. > > Let me quote Luca's summary for IPv6 here: > > If the IP Destination Address is multicast, then the TTL > should be 1. > > If the IP Destination Address is not multicast, then > no restrictions on TTL. > > and Luca's summary for IPv4: > > If the IP Destination Address is 224.0.0.0/4, then the TTL > should be 1. > > If the IP Destination Address is not 224.0.0.0/4, then no > restrictions on TTL. > > As I've said all details and references to RFCs can be found > in Luca's emails on bugs@ [1]. > > Diff below to fix IGMP/MLD handling has been essentially proposed > by Luca [3]. OK to commit? > > thanks and > regards > sashan > > [1] https://marc.info/?t=167714059600002&r=1&w=2 > > [2] https://marc.info/?l=openbsd-bugs&m=167727244015783&w=2 > > [3] https://marc.info/?l=openbsd-bugs&m=167722460220004&w=2 > > --------8<---------------8<---------------8<------------------8<-------- > diff --git a/sys/net/pf.c b/sys/net/pf.c > index 8cb1326a160..c328109026c 100644 > --- a/sys/net/pf.c > +++ b/sys/net/pf.c > @@ -6847,7 +6847,7 @@ pf_walk_header(struct pf_pdesc *pd, struct ip *h, > u_short *reason) > /* IGMP packets have router alert options, allow them */ > if (pd->proto == IPPROTO_IGMP) { > /* According to RFC 1112 ttl must be set to 1. */ > - if ((h->ip_ttl != 1) || !IN_MULTICAST(h->ip_dst.s_addr)) { > + if ((h->ip_ttl != 1) && IN_MULTICAST(h->ip_dst.s_addr)) { > DPFPRINTF(LOG_NOTICE, "Invalid IGMP"); > REASON_SET(reason, PFRES_IPOPTIONS); > return (PF_DROP); > @@ -7101,8 +7101,8 @@ pf_walk_header6(struct pf_pdesc *pd, struct ip6_hdr *h, > u_short *reason) > * missing then MLD message is invalid and > * should be discarded. > */ > - if ((h->ip6_hlim != 1) || > - !IN6_IS_ADDR_LINKLOCAL(&h->ip6_src)) { > + if ((h->ip6_hlim != 1) && > + IN6_IS_ADDR_LINKLOCAL(&h->ip6_src)) { > DPFPRINTF(LOG_NOTICE, "Invalid MLD"); > REASON_SET(reason, PFRES_IPOPTIONS); > return (PF_DROP); > Unless I'm missing more context, this hunk looks wrong to me. Valid MLD packets must have a ttl of 1 *and* come from a LL address. The initial logic seems ok to me. -- Matthieu Herrb
