Re: pf(4) drops valid IGMP/MLD messages
I've just seen that this has been fixed in 7.3, now I see that prune, graft, graft-ack messages are not blocked by PF Thanks a lot Il giorno gio 16 mar 2023 alle ore 16:45 Luca Di Gregorio ha scritto: > Ok, thanks a lot for the info > > Il giorno gio 16 mar 2023 alle 16:40 Theo de Raadt > ha scritto: > >> Luca Di Gregorio wrote: >> >> > do you think that the correction proposed by Alexandr could be done >> with a >> > syspatch, or in the next release? >> >> It does not meet the treshold for becoming a syspatch. >> >
Re: pf(4) drops valid IGMP/MLD messages
Ok, thanks a lot for the info Il giorno gio 16 mar 2023 alle 16:40 Theo de Raadt ha scritto: > Luca Di Gregorio wrote: > > > do you think that the correction proposed by Alexandr could be done with > a > > syspatch, or in the next release? > > It does not meet the treshold for becoming a syspatch. >
Re: pf(4) drops valid IGMP/MLD messages
Luca Di Gregorio wrote: > do you think that the correction proposed by Alexandr could be done with a > syspatch, or in the next release? It does not meet the treshold for becoming a syspatch.
Re: pf(4) drops valid IGMP/MLD messages
Hi,
do you think that the correction proposed by Alexandr could be done with a
syspatch, or in the next release?
Thanks, regards
Il giorno sab 4 mar 2023 alle ore 06:34 Luca Di Gregorio
ha scritto:
> Hi, my modest opinions:
>
> > Instead of implementing more and more details of RFC, we should
> > discuss what the goal is.
>
> I'd like to have multicast routing working properly.
> Multicast routing with good network architecture allows people
> to save tons of resources on hosts, as well as saving a lot of
> network bandwidth.
>
> > What are the IGMP illegal packets that an attacker might use? We
> > should drop them. This IGMP logic is deep down in pf that a user
> > cannot override.
>
> Good point, and I'm happy that OpenBSD people always have security
> in mind. That is the main reason - not the only one - why OpenBSD
> is always my first choice for my projects.
>
> A couple of ideas here:
>
> 1 - discard IP and IGMP packets with illegal lengths:
> Ref:
> https://support.huawei.com/enterprise/en/doc/EDOC1100112357/11182319/defense-against-malformed-packet-attacks
>
> 2 - discard IGMP packets if multicast=NO (rcctl get multicast)
> Maybe the kernel already does this, but I'm not sure about it.
>
> Thanks again, regards
>
> Il giorno sab 4 mar 2023 alle ore 00:38 Alexander Bluhm <
> alexander.bl...@gmx.net> ha scritto:
>
>> On Sat, Mar 04, 2023 at 12:09:41AM +0100, Alexandr Nedvedicky wrote:
>> > 6847 /* IGMP packets have router alert options, allow them */
>> > 6848 if (pd->proto == IPPROTO_IGMP) {
>> > 6849 /*
>> > 6850 * According to RFC 1112 ttl must be set to 1 in
>> all IGMP
>> > 6851 * packets sent do 224.0.0.1
>> > 6852 */
>> > 6853 if ((h->ip_ttl != 1) &&
>> > 6854 (h->ip_dst.s_addr == INADDR_ALLHOSTS_GROUP)) {
>> > 6855 DPFPRINTF(LOG_NOTICE, "Invalid IGMP");
>> > 6856 REASON_SET(reason, PFRES_IPOPTIONS);
>> > 6857 return (PF_DROP);
>> > 6858 }
>> > 6859 CLR(pd->badopts, PF_OPT_ROUTER_ALERT);
>> >
>> > This change should make pf(4) reasonably paranoid while keeping IGMP
>> working.
>>
>> OK bluhm@
>>
>
Re: pf(4) drops valid IGMP/MLD messages
Hi, my modest opinions:
> Instead of implementing more and more details of RFC, we should
> discuss what the goal is.
I'd like to have multicast routing working properly.
Multicast routing with good network architecture allows people
to save tons of resources on hosts, as well as saving a lot of
network bandwidth.
> What are the IGMP illegal packets that an attacker might use? We
> should drop them. This IGMP logic is deep down in pf that a user
> cannot override.
Good point, and I'm happy that OpenBSD people always have security
in mind. That is the main reason - not the only one - why OpenBSD
is always my first choice for my projects.
A couple of ideas here:
1 - discard IP and IGMP packets with illegal lengths:
Ref:
https://support.huawei.com/enterprise/en/doc/EDOC1100112357/11182319/defense-against-malformed-packet-attacks
2 - discard IGMP packets if multicast=NO (rcctl get multicast)
Maybe the kernel already does this, but I'm not sure about it.
Thanks again, regards
Il giorno sab 4 mar 2023 alle ore 00:38 Alexander Bluhm <
alexander.bl...@gmx.net> ha scritto:
> On Sat, Mar 04, 2023 at 12:09:41AM +0100, Alexandr Nedvedicky wrote:
> > 6847 /* IGMP packets have router alert options, allow them */
> > 6848 if (pd->proto == IPPROTO_IGMP) {
> > 6849 /*
> > 6850 * According to RFC 1112 ttl must be set to 1 in
> all IGMP
> > 6851 * packets sent do 224.0.0.1
> > 6852 */
> > 6853 if ((h->ip_ttl != 1) &&
> > 6854 (h->ip_dst.s_addr == INADDR_ALLHOSTS_GROUP)) {
> > 6855 DPFPRINTF(LOG_NOTICE, "Invalid IGMP");
> > 6856 REASON_SET(reason, PFRES_IPOPTIONS);
> > 6857 return (PF_DROP);
> > 6858 }
> > 6859 CLR(pd->badopts, PF_OPT_ROUTER_ALERT);
> >
> > This change should make pf(4) reasonably paranoid while keeping IGMP
> working.
>
> OK bluhm@
>
Re: pf(4) drops valid IGMP/MLD messages
On Sat, Mar 04, 2023 at 12:09:41AM +0100, Alexandr Nedvedicky wrote:
> 6847 /* IGMP packets have router alert options, allow them */
> 6848 if (pd->proto == IPPROTO_IGMP) {
> 6849 /*
> 6850 * According to RFC 1112 ttl must be set to 1 in all IGMP
> 6851 * packets sent do 224.0.0.1
> 6852 */
> 6853 if ((h->ip_ttl != 1) &&
> 6854 (h->ip_dst.s_addr == INADDR_ALLHOSTS_GROUP)) {
> 6855 DPFPRINTF(LOG_NOTICE, "Invalid IGMP");
> 6856 REASON_SET(reason, PFRES_IPOPTIONS);
> 6857 return (PF_DROP);
> 6858 }
> 6859 CLR(pd->badopts, PF_OPT_ROUTER_ALERT);
>
> This change should make pf(4) reasonably paranoid while keeping IGMP working.
OK bluhm@
Re: pf(4) drops valid IGMP/MLD messages
Hello,
On Fri, Mar 03, 2023 at 09:14:38PM +0100, Alexander Bluhm wrote:
> On Fri, Mar 03, 2023 at 08:54:51AM +0100, Luca Di Gregorio wrote:
> > Hi, just another bit of info about this issue.
>
> Instead of implementing more and more details of RFC, we should
> discuss what the goal is.
>
> We had a pf that dropped valid IGMP packets due to router-alert
> option. So I added code to pass them, but still block other options.
> This is neccessary for working multicast.
>
> Then sashan@ commited a diff that blocks packets with bad TTL or
> destination address. This was too strict for some use cases as we
> see now. But it may improve security.
>
> What are the IGMP illegal packets that an attacker might use? We
> should drop them. This IGMP logic is deep down in pf that a user
> cannot override. So we should be careful not to destroy valid use
> cases. Replacing || with && somehow in the if condition looks
> reasonalbe to me.
>
My commit was wrong. It makes pf(4) to drop all IGMP packets which
either have TTL other than 1
or are not sent to multicast address
this is what we currently have in pf_walk_header()
6849 /* IGMP packets have router alert options, allow them */
6850 if (pd->proto == IPPROTO_IGMP) {
6851 /* According to RFC 1112 ttl must be set to 1. */
6852 if ((h->ip_ttl != 1) || !IN_MULTICAST(h->ip_dst.s_addr)) {
6853 DPFPRINTF(LOG_NOTICE, "Invalid IGMP");
6854 REASON_SET(reason, PFRES_IPOPTIONS);
6855 return (PF_DROP);
6856 }
6857 CLR(pd->badopts, PF_OPT_ROUTER_ALERT);
6858 }
I think Luca is right pf(4) is too paranoid breaking IGMP. The RFC 1112.
This is what 'Informal Protocol Description' section says:
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.
In my commit I confused any multicast address with 'all-hosts' group
(224.0.0.1)
So we either want to revert that commit or fix it. I think the condition at
line 6850 should read as follows:
6847 /* IGMP packets have router alert options, allow them */
6848 if (pd->proto == IPPROTO_IGMP) {
6849 /*
6850 * According to RFC 1112 ttl must be set to 1 in all IGMP
6851 * packets sent do 224.0.0.1
6852 */
6853 if ((h->ip_ttl != 1) &&
6854 (h->ip_dst.s_addr == INADDR_ALLHOSTS_GROUP)) {
6855 DPFPRINTF(LOG_NOTICE, "Invalid IGMP");
6856 REASON_SET(reason, PFRES_IPOPTIONS);
6857 return (PF_DROP);
6858 }
6859 CLR(pd->badopts, PF_OPT_ROUTER_ALERT);
This change should make pf(4) reasonably paranoid while keeping IGMP working.
thanks and
regards
sashan
Re: pf(4) drops valid IGMP/MLD messages
On Fri, Mar 03, 2023 at 08:54:51AM +0100, Luca Di Gregorio wrote: > Hi, just another bit of info about this issue. Instead of implementing more and more details of RFC, we should discuss what the goal is. We had a pf that dropped valid IGMP packets due to router-alert option. So I added code to pass them, but still block other options. This is neccessary for working multicast. Then sashan@ commited a diff that blocks packets with bad TTL or destination address. This was too strict for some use cases as we see now. But it may improve security. What are the IGMP illegal packets that an attacker might use? We should drop them. This IGMP logic is deep down in pf that a user cannot override. So we should be careful not to destroy valid use cases. Replacing || with && somehow in the if condition looks reasonalbe to me. bluhm
Re: pf(4) drops valid IGMP/MLD messages
Hi, just another bit of info about this issue.
I've installed from github the newest version of mrouted on a Linux machine.
Just like the built-in OpenBSD's version of mrouted, this github version
sends DVMRP Prune messages
with IP Destination Address = Unicast Address of the adjacent router, and
TTL=255.
Here you can find a tcpdump:
08:49:19.363829 IP (tos 0xc0, ttl 255, id 52275, offset 0, flags [none],
proto IGMP (2), length 44, options (RA))
10.0.12.101 > 10.0.12.1: igmp dvmrp Prune src 10.254.2.0 grp
239.255.255.42 timer 1h47m2s
Best regards,
Luca
Il giorno mar 28 feb 2023 alle ore 15:39 Alexandr Nedvedicky <
sas...@fastmail.net> ha scritto:
> Hello Matthieu,
>
>
>
> On Tue, Feb 28, 2023 at 02:05:50PM +0100, Matthieu Herrb wrote:
> > > --- 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)
>
> the expression above is true for for Query/Report messages
> which are a sub-set of IGMP protocol. See Luca's emails with
> references to RFCs which further extend IGMP protocol.
>
> here in this particular place (pf_walk_header()) I think we really
> want to simply discard all IGMP datagrams sent to 224.0.0.1 with TTL
> different to 1. anything else should be passed further up and let
> pf(4)
> rules to make a decision on those cases.
>
> thanks and
> regards
> sashan
>
Re: pf(4) drops valid IGMP/MLD messages
Hello Matthieu,
On Tue, Feb 28, 2023 at 02:05:50PM +0100, Matthieu Herrb wrote:
> > --- 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)
the expression above is true for for Query/Report messages
which are a sub-set of IGMP protocol. See Luca's emails with
references to RFCs which further extend IGMP protocol.
here in this particular place (pf_walk_header()) I think we really
want to simply discard all IGMP datagrams sent to 224.0.0.1 with TTL
different to 1. anything else should be passed further up and let pf(4)
rules to make a decision on those cases.
thanks and
regards
sashan
Re: pf(4) drops valid IGMP/MLD messages
Hi Matthieu,
DVMRP messages are sent with IGMP protocol. Some Multicast Control messages
(Query, Report) have an IP Destination Address belonging to 224.0.0.0/4,
with TTL=1.
Other DVMRP multicast control messages (Prune, Graft, Graft-Ack) have an IP
Destination Address = Unicast Address of the adjacent multicast router,
with TTL >1.
After revision 1.1128, PF drops IGMP messages with TTL != 1 or IP
Destination Address ! IN_MULTICAST.
I think that your proposed test to drop invalid IGMP is too restrictive.
In fact, it would block not only Prune, Graft, Graft-Ack messages (the IP
Destination Address is Unicast and TTL > 1), but also:
- DVMRP Probe and Report (TTL = 1 but IP Destination Address = 224.0.0.4 -
DVMRP Routers)
- Multicast Control messages sent to 224.0.0.2 (All Routers on this Subnet)
- Multicast Control messages sent to 224.0.0.22 (IGMP)
For reference:
https://www.iana.org/assignments/multicast-addresses/multicast-addresses.xhtml
Regards
Luca
Il giorno mar 28 feb 2023 alle ore 14:05 Matthieu Herrb
ha scritto:
> On Mon, Feb 27, 2023 at 12:04:54AM +0100, Alexandr Nedvedicky wrote:
> > 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(&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
Re: pf(4) drops valid IGMP/MLD messages
On Mon, Feb 27, 2023 at 12:04:54AM +0100, Alexandr Nedvedicky wrote:
> 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(&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
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(&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);
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&r=1&w=2
>
> [2] https://marc.info/?l=openbsd-bugs&m=167727244015783&w=2
>
> [3] https://marc.info/?l=openbsd-bugs&m=167722460220004&w=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(&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.
--
Matthieu Herrb
pf(4) drops valid IGMP/MLD messages
Hello,
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&r=1&w=2
[2] https://marc.info/?l=openbsd-bugs&m=167727244015783&w=2
[3] https://marc.info/?l=openbsd-bugs&m=167722460220004&w=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(&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);
