On Fri, Apr 22, 2022 at 09:03:45PM +0200, Otto Moerbeek wrote:
> On Fri, Apr 22, 2022 at 05:59:18PM +0200, Alexander Bluhm wrote:
>
> > On Thu, Apr 21, 2022 at 09:10:02PM +0200, Alexander Bluhm wrote:
> > > The option I have ever seen in the wild is router alert. So it may
> > > be better to allow IGMP and ICMP6 multicast if router alert is the
> > > only option in the packet.
> >
> > This diff implements exactly that. I have only compile tested it.
> > If we decide that is the way to go, I will adapt my pf regress.
>
> A quick test shows that this still blocks these:
>
> 21:00:35.009640 fe80::aef8:ccff:feca:428c > ff02::1: HBH icmp6: multicast
> listener query v2 [|icmp6] [hlim 1]
This is v2
New diff:
- make off and end relative to opts array
- check length of IPv4 options
- fix call to pf_walk_option
- add case IP6OPT_PADN
- add case MLDV2_LISTENER_REPORT
I have written some regression tests that deal with pf IP options.
ok?
bluhm
Index: net/pf.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.1126
diff -u -p -r1.1126 pf.c
--- net/pf.c 17 Mar 2022 18:27:55 -0000 1.1126
+++ net/pf.c 27 Apr 2022 21:33:36 -0000
@@ -227,6 +227,8 @@ u_int16_t pf_calc_mss(struct pf_addr *
static __inline int pf_set_rt_ifp(struct pf_state *, struct pf_addr *,
sa_family_t, struct pf_src_node **);
struct pf_divert *pf_get_divert(struct mbuf *);
+int pf_walk_option(struct pf_pdesc *, struct ip *,
+ int, int, u_short *);
int pf_walk_header(struct pf_pdesc *, struct ip *,
u_short *);
int pf_walk_option6(struct pf_pdesc *, struct ip6_hdr *,
@@ -3956,7 +3958,7 @@ pf_test_rule(struct pf_pdesc *pd, struct
rtable_l2(ctx.act.rtableid) != pd->rdomain)
pd->destchg = 1;
- if (r->action == PF_PASS && pd->badopts && ! r->allow_opts) {
+ if (r->action == PF_PASS && pd->badopts != 0 && ! r->allow_opts) {
REASON_SET(&ctx.reason, PFRES_IPOPTIONS);
#if NPFLOG > 0
pd->pflog |= PF_LOG_FORCE;
@@ -6382,6 +6384,55 @@ pf_get_divert(struct mbuf *m)
}
int
+pf_walk_option(struct pf_pdesc *pd, struct ip *h, int off, int end,
+ u_short *reason)
+{
+ uint8_t type, length, opts[15 * 4 - sizeof(struct ip)];
+
+ KASSERT(end - off <= sizeof(opts));
+ m_copydata(pd->m, off, end - off, opts);
+ end -= off;
+ off = 0;
+
+ while (off < end) {
+ type = opts[off];
+ if (type == IPOPT_EOL)
+ break;
+ if (type == IPOPT_NOP) {
+ off++;
+ continue;
+ }
+ if (off + 2 > end) {
+ DPFPRINTF(LOG_NOTICE, "IP length opt");
+ REASON_SET(reason, PFRES_IPOPTIONS);
+ return (PF_DROP);
+ }
+ length = opts[off + 1];
+ if (length < 2) {
+ DPFPRINTF(LOG_NOTICE, "IP short opt");
+ REASON_SET(reason, PFRES_IPOPTIONS);
+ return (PF_DROP);
+ }
+ if (off + length > end) {
+ DPFPRINTF(LOG_NOTICE, "IP long opt");
+ REASON_SET(reason, PFRES_IPOPTIONS);
+ return (PF_DROP);
+ }
+ switch (type) {
+ case IPOPT_RA:
+ SET(pd->badopts, PF_OPT_ROUTER_ALERT);
+ break;
+ default:
+ SET(pd->badopts, PF_OPT_OTHER);
+ break;
+ }
+ off += length;
+ }
+
+ return (PF_PASS);
+}
+
+int
pf_walk_header(struct pf_pdesc *pd, struct ip *h, u_short *reason)
{
struct ip6_ext ext;
@@ -6393,11 +6444,20 @@ pf_walk_header(struct pf_pdesc *pd, stru
REASON_SET(reason, PFRES_SHORT);
return (PF_DROP);
}
- if (hlen != sizeof(struct ip))
- pd->badopts++;
+ if (hlen != sizeof(struct ip)) {
+ if (pf_walk_option(pd, h, pd->off + sizeof(struct ip),
+ pd->off + hlen, reason) != PF_PASS)
+ return (PF_DROP);
+ /* header options which contain only padding is fishy */
+ if (pd->badopts == 0)
+ SET(pd->badopts, PF_OPT_OTHER);
+ }
end = pd->off + ntohs(h->ip_len);
pd->off += hlen;
pd->proto = h->ip_p;
+ /* IGMP packets have router alert options, allow them */
+ 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);
@@ -6455,7 +6515,10 @@ pf_walk_option6(struct pf_pdesc *pd, str
return (PF_DROP);
}
switch (opt.ip6o_type) {
+ case IP6OPT_PADN:
+ break;
case IP6OPT_JUMBO:
+ SET(pd->badopts, PF_OPT_JUMBO);
if (pd->jumbolen != 0) {
DPFPRINTF(LOG_NOTICE, "IPv6 multiple jumbo");
REASON_SET(reason, PFRES_IPOPTIONS);
@@ -6480,7 +6543,11 @@ pf_walk_option6(struct pf_pdesc *pd, str
return (PF_DROP);
}
break;
+ case IP6OPT_ROUTER_ALERT:
+ SET(pd->badopts, PF_OPT_ROUTER_ALERT);
+ break;
default:
+ SET(pd->badopts, PF_OPT_OTHER);
break;
}
off += sizeof(opt) + opt.ip6o_len;
@@ -6494,6 +6561,7 @@ pf_walk_header6(struct pf_pdesc *pd, str
{
struct ip6_frag frag;
struct ip6_ext ext;
+ struct icmp6_hdr icmp6;
struct ip6_rthdr rthdr;
u_int32_t end;
int hdr_cnt, fraghdr_cnt = 0, rthdr_cnt = 0;
@@ -6506,9 +6574,17 @@ pf_walk_header6(struct pf_pdesc *pd, str
for (hdr_cnt = 0; hdr_cnt < pf_hdr_limit; hdr_cnt++) {
switch (pd->proto) {
case IPPROTO_ROUTING:
- case IPPROTO_HOPOPTS:
case IPPROTO_DSTOPTS:
- pd->badopts++;
+ SET(pd->badopts, PF_OPT_OTHER);
+ break;
+ case IPPROTO_HOPOPTS:
+ if (pf_walk_option6(pd, h, pd->off + sizeof(ext),
+ pd->off + (ext.ip6e_len + 1) * 8, reason)
+ != PF_PASS)
+ return (PF_DROP);
+ /* option header which contains only padding is fishy */
+ if (pd->badopts == 0)
+ SET(pd->badopts, PF_OPT_OTHER);
break;
}
switch (pd->proto) {
@@ -6587,19 +6663,11 @@ pf_walk_header6(struct pf_pdesc *pd, str
/* reassembly needs the ext header before the frag */
if (pd->fragoff == 0)
pd->extoff = pd->off;
- if (pd->proto == IPPROTO_HOPOPTS && pd->fragoff == 0) {
- if (pf_walk_option6(pd, h,
- pd->off + sizeof(ext),
- pd->off + (ext.ip6e_len + 1) * 8, reason)
- != PF_PASS)
- return (PF_DROP);
- if (ntohs(h->ip6_plen) == 0 &&
- pd->jumbolen != 0) {
- DPFPRINTF(LOG_NOTICE,
- "IPv6 missing jumbo");
- REASON_SET(reason, PFRES_IPOPTIONS);
- return (PF_DROP);
- }
+ if (pd->proto == IPPROTO_HOPOPTS && pd->fragoff == 0 &&
+ ntohs(h->ip6_plen) == 0 && pd->jumbolen != 0) {
+ DPFPRINTF(LOG_NOTICE, "IPv6 missing jumbo");
+ REASON_SET(reason, PFRES_IPOPTIONS);
+ return (PF_DROP);
}
if (pd->proto == IPPROTO_AH)
pd->off += (ext.ip6e_len + 2) * 4;
@@ -6607,9 +6675,24 @@ pf_walk_header6(struct pf_pdesc *pd, str
pd->off += (ext.ip6e_len + 1) * 8;
pd->proto = ext.ip6e_nxt;
break;
+ case IPPROTO_ICMPV6:
+ if (!pf_pull_hdr(pd->m, pd->off, &icmp6, sizeof(icmp6),
+ NULL, reason, AF_INET6)) {
+ DPFPRINTF(LOG_NOTICE, "IPv6 short icmp6hdr");
+ return (PF_DROP);
+ }
+ /* ICMP multicast packets have router alert options */
+ switch (icmp6.icmp6_type) {
+ case MLD_LISTENER_QUERY:
+ case MLD_LISTENER_REPORT:
+ case MLD_LISTENER_DONE:
+ case MLDV2_LISTENER_REPORT:
+ CLR(pd->badopts, PF_OPT_ROUTER_ALERT);
+ break;
+ }
+ /* FALLTHROUGH */
case IPPROTO_TCP:
case IPPROTO_UDP:
- case IPPROTO_ICMPV6:
/* fragments may be short, ignore inner header then */
if (pd->fragoff != 0 && end < pd->off +
(pd->proto == IPPROTO_TCP ? sizeof(struct tcphdr) :
@@ -7167,7 +7250,7 @@ done:
if (action != PF_DROP) {
if (s) {
/* The non-state case is handled in pf_test_rule() */
- if (action == PF_PASS && pd.badopts &&
+ if (action == PF_PASS && pd.badopts != 0 &&
!(s->state_flags & PFSTATE_ALLOWOPTS)) {
action = PF_DROP;
REASON_SET(&reason, PFRES_IPOPTIONS);
Index: net/pfvar_priv.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfvar_priv.h,v
retrieving revision 1.9
diff -u -p -r1.9 pfvar_priv.h
--- net/pfvar_priv.h 8 Apr 2022 18:17:24 -0000 1.9
+++ net/pfvar_priv.h 27 Apr 2022 13:57:57 -0000
@@ -180,6 +180,9 @@ struct pf_pdesc {
u_int32_t fragoff; /* fragment header offset */
u_int32_t jumbolen; /* length from v6 jumbo header */
u_int32_t badopts; /* v4 options or v6 routing headers */
+#define PF_OPT_OTHER 0x0001
+#define PF_OPT_JUMBO 0x0002
+#define PF_OPT_ROUTER_ALERT 0x0004
u_int16_t rdomain; /* original routing domain */
u_int16_t virtual_proto;
