Re: pf(4) drops valid IGMP/MLD messages
Hello,
> > 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(>ip6_src)) {
> > + if ((h->ip6_hlim != 1) &&
> > + IN6_IS_ADDR_LINKLOCAL(>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);
OpenBSD Errata: February 26, 2023 (wscons)
Errata patches for wscons(4) have been released for OpenBSD 7.1 and 7.2. Binary updates for the amd64, i386 and arm64 platform are available via the syspatch utility. Source code patches can be found on the respective errata page: https://www.openbsd.org/errata71.html https://www.openbsd.org/errata72.html
Re: pf(4) drops valid IGMP/MLD messages
On Sun, Feb 26, 2023 at 12:09:13AM +0100, Alexandr Nedvedicky wrote:
> Hello,
Hi,
one remark at the end.
I usually add explicit pass rules for multicast IPv6 traffic on my
boxes where IPv6 matters, because otherwise I've seen issues in the
past, but I don't have details anymore.
So I'm happy if pf starts behaving better by default with IPv6, but
see my comment below.
>
> the issue has been discovered and analyzed by Luca di Gregorio
> on bugs@ [1]. The thread [1] covers all details. People who
> wish to read brief summary can skip to Luca's email [2].
>
> To wrap it up the current handling IGMP/MLD messages in pf(4)
> is exact opposite. I failed to translate English words from
> RFC standards into correct C code. Patch below are two one-liners
> which make multicast for IPv4/IPv6 to work again with pf(4) enabled.
>
> Let me quote Luca's summary for IPv6 here:
>
> If the IP Destination Address is multicast, then the TTL
> should be 1.
>
> If the IP Destination Address is not multicast, then
> no restrictions on TTL.
>
> and Luca's summary for IPv4:
>
> 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.
>
> As I've said all details and references to RFCs can be found
> in Luca's emails on bugs@ [1].
>
> Diff below to fix IGMP/MLD handling has been essentially proposed
> by Luca [3]. OK to commit?
>
> thanks and
> regards
> sashan
>
> [1] https://marc.info/?t=16771405962=1=2
>
> [2] https://marc.info/?l=openbsd-bugs=167727244015783=2
>
> [3] https://marc.info/?l=openbsd-bugs=167722460220004=2
>
> 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(>ip6_src)) {
> + if ((h->ip6_hlim != 1) &&
> + IN6_IS_ADDR_LINKLOCAL(>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.
--
Matthieu Herrb
