Hello,
On Tue, May 03, 2022 at 09:19:44AM +0200, Alexander Bluhm wrote:
> On Tue, May 03, 2022 at 12:26:52AM +0200, Alexandr Nedvedicky wrote:
> > OK ? or should I also drop a check for link-local source address
> > in IPv6?
>
> The link-local check makes sense.
>
</snip>
> > + CLR(pd->badopts, PF_OPT_ROUTER_ALERT);
>
> You return in the if block. No need for else.
fixed
> > + }
>
> Can you turn around the logic?
>
> if (something bad)
> return
> clear badopts
>
sure
updated diff is below.
thanks for taking a look at it.
regards
sashan
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/pf.c b/sys/net/pf.c
index f15e1ead8c0..bf9593952ec 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) {
+ DPFPRINTF(LOG_NOTICE, "TTL in IGMP must be 1");
+ 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;
}
