Hi Alexandr,
RFC1112 is updated by RFC2236
RFC2236 is updated by RFC3376
In RFC3376, sections 4.1.12 and 4.2.14:
"In addition, a system MUST accept and process ... of Query/Report whose IP
Destination Address field contains *any* of the addresses (unicast or
multicast) assigned to the interface on which the Query/Report arrives"
I understand that there is no restriction on IP Destination Address type,
it can be multicast or unicast.
Regarding TTL, section 4:
"Every IGMP message described in this document is sent with an IP
Time-to-Live of 1"
Messages described in the document are Query and Response, not messages for
intra-routers multicast control (like DVMRP Probe, Report, Prune, Graft,
Graft-Ack).
I think that if the IP Destination Address is multicast (224.0.0.0/4), then
the TTL should be 1. Otherwise, the packet is malformed.
I would implement this logic:
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.
In your code, I would do this modification:
- 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);
- }
+ 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);
+ }
Anyway, I'm ok if you revert your commit from May 3rd. If this will be the
case, I expect (please correct me if I'm wrong) that in /etc/pf.conf there
must be the line:
pass proto igmp allow-opts
Otherwise the packet with Router Alert will be discarded.
Regarding MLD, I can't say anything because I've never tested multicast
routing with IP6.
Thanks a lot for your time, regards.
Il giorno ven 24 feb 2023 alle ore 01:50 Alexandr Nedvedicky <
[email protected]> ha scritto:
> Hello Luca,
>
> On Thu, Feb 23, 2023 at 09:22:07AM +0100, Luca Di Gregorio wrote:
> > Synopsis: PF still blocks IGMP multicast control packets
> > Category: system
> > Environment:
> > System : OpenBSD 7.2
> > Details : OpenBSD 7.2 (GENERIC) #6: Sat Jan 21 01:01:28 MST
> 2023
> > [email protected]:
> > /usr/src/sys/arch/amd64/compile/GENERIC
> >
> > Architecture: OpenBSD.amd64
> > Machine : amd64
> >
> > Description:
> > In https://www.openbsd.org/plus72.html it is stated that:
> > "Changed pf(4) handling of IGMP and ICMP6 MLD packets to allow multicast
> > control
> > packets to work by default."
> > But, with PF enabled, igmp dvmrp Prune messages between two mrouted's are
> > still blocked.
> >
> > Tests can be done with the default lines in /etc/pf.conf:
> >
> > set skip on lo
> > block return
> > pass
> > block return in on ! lo0 proto tcp to port 6000:6010
> > block return out log proto {tcp udp} user _pbuild
> >
> </snip>>
>
> I went through the mail thread here. I'd like to check if
> I got the problem right.
>
> IGMP/mroute is busted when pf(4) is enabled. pf(4) just
> assumes all IGMP packets with TTL other than 1 are illegal
> and should be discarded right away without even checking
> the rules.
>
> the idea in pf_walk_header() is to make sure IGMP packet with
> router alert option has TTL 1 and expected destination address.
>
> if legitimate IGMP packet gets dropped here in pf_walk_header()
> then the code does not work as intended (and I'm very sorry for that)
>
> can you give a try diff below? it just reverts my commit from May 3rd.
> I would like to know if it helps. It should let IGMP packet further up
> in pf(4) to state/rule check.
>
> thanks a lot for your help.
>
> regards
> sashan
>
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sys/net/pf.c b/sys/net/pf.c
> index 8cb1326a160..2f941aaea88 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -6845,15 +6845,8 @@ 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) {
> - /* 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, "Invalid IGMP");
> - REASON_SET(reason, PFRES_IPOPTIONS);
> - return (PF_DROP);
> - }
> + if (pd->proto == IPPROTO_IGMP)
> CLR(pd->badopts, PF_OPT_ROUTER_ALERT);
> - }
> /* stop walking over non initial fragments */
> if ((h->ip_off & htons(IP_OFFMASK)) != 0)
> return (PF_PASS);
> @@ -7094,19 +7087,6 @@ 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;
> }
>
