Hi Matthieu, DVMRP messages are sent with IGMP protocol. Some Multicast Control messages (Query, Report) have an IP Destination Address belonging to 224.0.0.0/4, with TTL=1. Other DVMRP multicast control messages (Prune, Graft, Graft-Ack) have an IP Destination Address = Unicast Address of the adjacent multicast router, with TTL >1.
After revision 1.1128, PF drops IGMP messages with TTL != 1 or IP Destination Address ! IN_MULTICAST. I think that your proposed test to drop invalid IGMP is too restrictive. In fact, it would block not only Prune, Graft, Graft-Ack messages (the IP Destination Address is Unicast and TTL > 1), but also: - DVMRP Probe and Report (TTL = 1 but IP Destination Address = 224.0.0.4 - DVMRP Routers) - Multicast Control messages sent to 224.0.0.2 (All Routers on this Subnet) - Multicast Control messages sent to 224.0.0.22 (IGMP) For reference: https://www.iana.org/assignments/multicast-addresses/multicast-addresses.xhtml Regards Luca Il giorno mar 28 feb 2023 alle ore 14:05 Matthieu Herrb <matth...@herrb.eu> ha scritto: > On Mon, Feb 27, 2023 at 12:04:54AM +0100, Alexandr Nedvedicky wrote: > > Hello, > > > > </snip> > > > > > --------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. > > > > > > > yes you are right. Your comment made me to take better look > > at RFC 1112 [1]. Section 'Informal Protocol Description' > > reads as follows: > > > > 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. > > > > I think I've confused all-hosts group (224.0.0.1) with any multicast > > address (any class-D 224.0.0.0). I think the diff below is what we > > actually need to get IPv4 IGMP working again: > > > > [1] https://www.ietf.org/rfc/rfc1112.txt > > > > --------8<---------------8<---------------8<------------------8<-------- > > diff --git a/sys/net/pf.c b/sys/net/pf.c > > index 8cb1326a160..c50173186da 100644 > > --- a/sys/net/pf.c > > +++ b/sys/net/pf.c > > @@ -6846,8 +6846,12 @@ pf_walk_header(struct pf_pdesc *pd, struct ip *h, > u_short *reason) > > pd->proto = h->ip_p; > > /* 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)) { > > + /* > > + * According to RFC 1112 ttl must be set to 1 in all IGMP > > + * packets sent do 224.0.0.1 > > + */ > > + if ((h->ip_ttl != 1) && > > + (h->ip_dst.s_addr == INADDR_ALLHOSTS_GROUP)) { > > DPFPRINTF(LOG_NOTICE, "Invalid IGMP"); > > REASON_SET(reason, PFRES_IPOPTIONS); > > return (PF_DROP); > > > > > Hi, > > The expression look wrong to me again. > I read in RFC1112 that correct packets should have: > > (h->ip_ttl == 1 && h->ip.ip_dst.s_addr == INADDR_ALLHOST_GROUP) > > so the test to drop invalid IGMP should be: > > if (h->ip_ttl != 1 || h->ip.ip_dst.s_addr != INADDR_ALLHOST_GROUP) { > ... > return (PF_DROP > } > > Again I may be missing something > -- > Matthieu Herrb >
