On Tue, May 03, 2022 at 02:08:33PM +0200, Alexandr Nedvedicky wrote:
> Hello
>
> On Tue, May 03, 2022 at 10:44:48AM +0200, Claudio Jeker wrote:
> </snip>
> >
> > 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).
> >
>
> updated diff below requires destination address to be multicast for IGMP.
>
> thanks and
> regards
> sashan
>
> --------8<----------------8<----------------8<----------------8<--------
> diff --git a/sys/net/pf.c b/sys/net/pf.c
> index f15e1ead8c0..feb7965c427 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -6456,8 +6456,15 @@ 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)
> + 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)) {
> + DPFPRINTF(LOG_NOTICE, "TTL in IGMP must be 1");
Maybe use "Invalid IGMP" here (which is similar to the IPv6 case).
Apart from that OK claudio
> + REASON_SET(reason, PFRES_IPOPTIONS);
> + return (PF_DROP);
> + }
> CLR(pd->badopts, PF_OPT_ROUTER_ALERT);
> + }
> /* stop walking over non initial fragments */
> if ((h->ip_off & htons(IP_OFFMASK)) != 0)
> return (PF_PASS);
> @@ -6698,6 +6705,19 @@ 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:
> + /*
> + * According to RFC 2710 all MLD messages are
> + * sent with 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 ((h->ip6_hlim != 1) ||
> + !IN6_IS_ADDR_LINKLOCAL(&h->ip6_src)) {
> + DPFPRINTF(LOG_NOTICE, "invalid MLD");
> + REASON_SET(reason, PFRES_IPOPTIONS);
> + return (PF_DROP);
> + }
> CLR(pd->badopts, PF_OPT_ROUTER_ALERT);
> break;
> }
>
--
:wq Claudio
