On Tue, May 03, 2022 at 12:05:06AM +0200, Alexander Bluhm wrote: > On Mon, May 02, 2022 at 11:30:58PM +0200, Alexandr Nedvedicky wrote: > > hello, > > > > bluhm@ has committed a fix [1] which makes pf to accept IGMP/MLD messages. > > If I remember correct pf(4) was dropping those messages because > > of Router Alert IP option being present. The IP option is mandatory > > for IGMP/MLD according to RFCs. > > > > For both protocol versions (IPv4, IPv6) standards say: > > > > TTL/hop-limit in IP header must be set to 1 > > > > Router Alert option/extension header must be present > > > > in case of IPv6 the MLD messages must be sent from link-local address. > > > > > > diff below adds exactly those checks. > > > > OK? > > Checking that the TTL equals 1 is a good thing. We should prevent > that someone is forwarding such packets. > > The router alert is a hint to routers on the way to look at these > packets. If they are missing, no harm is done. Maybe some multicast > does not work. But there is no security argument to filter these. > > I have seen IGMP packets without router alert. Our stack has fixed > that in OpenBSD 5.6. Don't believe what is written in RFCs.
The RFC does not use the usual MUST to enforce any of this. So yes, we should probably not be too strict because there is no way to force accept the packet when pf_walk_header() returns PF_DROP. I agree that the TTL MUST be 1. Also the destination address must be a multicast address (IGMP to a unicast IP makes no sense). > > [1] https://marc.info/?l=openbsd-tech&m=165109904223362&w=2 > > > > [2] https://datatracker.ietf.org/doc/html/rfc2710 > > https://datatracker.ietf.org/doc/html/rfc2236 > > > > --------8<---------------8<---------------8<------------------8<-------- > > diff --git a/sys/net/pf.c b/sys/net/pf.c > > index f15e1ead8c0..9b107751d95 100644 > > --- a/sys/net/pf.c > > +++ b/sys/net/pf.c > > @@ -6456,8 +6456,21 @@ pf_walk_header(struct pf_pdesc *pd, struct ip *h, > > u_short *reason) > > pd->off += hlen; > > pd->proto = h->ip_p; > > /* IGMP packets have router alert options, allow them */ > > - if (pd->proto == IPPROTO_IGMP) > > - CLR(pd->badopts, PF_OPT_ROUTER_ALERT); > > + if (pd->proto == IPPROTO_IGMP) { > > + /* > > + * If router alert option is missing or ttl is not 1, then we > > + * deal invalid IGMP packet. According to RFC 1112 ttl must be > > + * set to 1. Also IP header must carry router alert option > > + * as specified in RFC 2236. > > + */ > > + if (ISSET(pd->badopts, PF_OPT_ROUTER_ALERT) && (h->ip_ttl == 1)) > > + CLR(pd->badopts, PF_OPT_ROUTER_ALERT); > > + else { > > + DPFPRINTF(LOG_NOTICE, "invalid IGMP"); > > + REASON_SET(reason, PFRES_IPOPTIONS); > > + return (PF_DROP); > > + } > > + } > > /* stop walking over non initial fragments */ > > if ((h->ip_off & htons(IP_OFFMASK)) != 0) > > return (PF_PASS); > > @@ -6698,7 +6711,22 @@ pf_walk_header6(struct pf_pdesc *pd, struct ip6_hdr > > *h, u_short *reason) > > case MLD_LISTENER_REPORT: > > case MLD_LISTENER_DONE: > > case MLDV2_LISTENER_REPORT: > > - CLR(pd->badopts, PF_OPT_ROUTER_ALERT); > > + /* > > + * According to RFC 2710 all MLD messages are > > + * sent with with router alert header and hop > > + * limit (ttl) set to 1, and link local source > > + * address. If either one is missing then MLD > > + * message is invalid and should be discarded. > > + */ > > + if (ISSET(pd->badopts, PF_OPT_ROUTER_ALERT) && > > + (h->ip6_hlim == 1) && > > + IN6_IS_ADDR_LINKLOCAL(&h->ip6_src)) > > + CLR(pd->badopts, PF_OPT_ROUTER_ALERT); > > + else { > > + DPFPRINTF(LOG_NOTICE, "invalid MLD"); > > + REASON_SET(reason, PFRES_IPOPTIONS); > > + return (PF_DROP); > > + } > > break; > > } > > return (PF_PASS); > -- :wq Claudio
