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
