Re: [External] : Re: pf igmp icmp6 multicast router alert
Hello,
On Thu, Apr 28, 2022 at 10:31:33PM +0200, Alexander Bluhm wrote:
>
> Thanks. regress/sys/netinet6/frag6 found a small issue. If the
> icmp6 header is fragmented, we cannot pull the icmp6 header. I had
> to copy the fragment check to the beginning of case IPPROTO_ICMPV6.
>
> This chunk is new:
> + case IPPROTO_ICMPV6:
> + /* fragments may be short, ignore inner header then */
> + if (pd->fragoff != 0 && end < pd->off + sizeof(icmp6)) {
> + pd->off = pd->fragoff;
> + pd->proto = IPPROTO_FRAGMENT;
> + return (PF_PASS);
> + }
>
> Although it is questionable if we should allow fragmented header
> chains, I don't want to change behavior here. If I recall correctly
> newer RFCs forbid fragmented header chains. But I had implemented
> this code before the IPv6 standards have discovered the security
> implications.
>
> I am currently running a full regress.
>
new diff still reads OK to me.
thanks and
regards
sashan
Re: [External] : Re: pf igmp icmp6 multicast router alert
On Thu, Apr 28, 2022 at 08:15:20AM +0200, Alexandr Nedvedicky wrote:
> Hello,
>
> On Thu, Apr 28, 2022 at 12:36:40AM +0200, Alexander Bluhm wrote:
> > On Wed, Apr 27, 2022 at 11:47:45PM +0200, Alexander Bluhm wrote:
> > > 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
> >
> > - pf_pull_hdr() before pf_walk_option6() was missing
> >
> > ok?
>
> diff reads OK to me as far as I can tell.
>
>
> OK sashan
Thanks. regress/sys/netinet6/frag6 found a small issue. If the
icmp6 header is fragmented, we cannot pull the icmp6 header. I had
to copy the fragment check to the beginning of case IPPROTO_ICMPV6.
This chunk is new:
+ case IPPROTO_ICMPV6:
+ /* fragments may be short, ignore inner header then */
+ if (pd->fragoff != 0 && end < pd->off + sizeof(icmp6)) {
+ pd->off = pd->fragoff;
+ pd->proto = IPPROTO_FRAGMENT;
+ return (PF_PASS);
+ }
Although it is questionable if we should allow fragmented header
chains, I don't want to change behavior here. If I recall correctly
newer RFCs forbid fragmented header chains. But I had implemented
this code before the IPv6 standards have discovered the security
implications.
I am currently running a full regress.
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.c17 Mar 2022 18:27:55 - 1.1126
+++ net/pf.c28 Apr 2022 20:13:00 -
@@ -227,6 +227,8 @@ u_int16_tpf_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(, 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
Re: [External] : Re: pf igmp icmp6 multicast router alert
Hello, On Thu, Apr 28, 2022 at 12:36:40AM +0200, Alexander Bluhm wrote: > On Wed, Apr 27, 2022 at 11:47:45PM +0200, Alexander Bluhm wrote: > > 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 > > - pf_pull_hdr() before pf_walk_option6() was missing > > ok? diff reads OK to me as far as I can tell. OK sashan
Re: pf igmp icmp6 multicast router alert
On Thu, Apr 28, 2022 at 12:36:40AM +0200, Alexander Bluhm wrote:
> On Wed, Apr 27, 2022 at 11:47:45PM +0200, Alexander Bluhm wrote:
> > 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
>
> - pf_pull_hdr() before pf_walk_option6() was missing
>
> ok?
tested this version on my home net that had this issue after
installing a new CPE. It works fine. Can't comment on the
code itself, this is too far out of my comfort zone.
-Otto
>
> 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 - 1.1126
> +++ net/pf.c 27 Apr 2022 22:28:38 -
> @@ -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(, 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:
> +
Re: [External] : Re: pf igmp icmp6 multicast router alert
Hello,
On Thu, Apr 28, 2022 at 12:00:09AM +0200, Alexander Bluhm wrote:
> On Fri, Apr 22, 2022 at 07:40:17PM +0200, Alexandr Nedvedicky wrote:
> > > + case IPPROTO_ICMPV6:
> > > + if (!pf_pull_hdr(pd->m, pd->off, , 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:
> >
> > I wonder if we should have a similar check we have for IPv4 address,
> > where we require a multicast address. for example in case of
> > MLD_LISTENER_QUERY the packet destination address should be fe80::/10.
> > I need to look at RFCs more closely first. Just asking in case someone
> > else
> > knows from top of the head.
>
> Where do we check multicast adddress for IPv4? At this point we
> are just comparing protocol and IP options. I would not make it
> more complex, so I will not add multicast adddress checks here.
>
yes, you are right there is no such check for IPv4.
I was just quickly reading RFC 3810 [1], section 5 Message Formats
reads as follows:
8<---8<-8<
MLDv2 is a sub-protocol of ICMPv6, that is, MLDv2 message types are a
subset of ICMPv6 messages, and MLDv2 messages are identified in IPv6
packets by a preceding Next Header value of 58. All MLDv2 messages
described in this document MUST be sent with a link-local IPv6 Source
Vida & CostaStandards Track[Page 13]
RFC 3810 MLDv2 for IPv6June 2004
Address, an IPv6 Hop Limit of 1, and an IPv6 Router Alert option
[RFC2711] in a Hop-by-Hop Options header. (The Router Alert option
is necessary to cause routers to examine MLDv2 messages sent to IPv6
multicast addresses in which the routers themselves have no
interest.) MLDv2 Reports can be sent with the source address set to
the unspecified address [RFC3513], if a valid link-local IPv6 source
address has not been acquired yet for the sending interface. (See
section 5.2.13. for details.)
8<---8<-8<
I'm not sure if we do check for src/dst addresses and
hop limit if we process MLD message.
I'm going to look at your diff now. We can add those checks
in follow up commit in case they are missing.
regards
sashan
[1] https://www.ietf.org/rfc/rfc3810.txt
Re: pf igmp icmp6 multicast router alert
On Wed, Apr 27, 2022 at 11:47:45PM +0200, Alexander Bluhm wrote:
> 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
- pf_pull_hdr() before pf_walk_option6() was missing
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.c17 Mar 2022 18:27:55 - 1.1126
+++ net/pf.c27 Apr 2022 22:28:38 -
@@ -227,6 +227,8 @@ u_int16_tpf_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(, 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
Re: [External] : Re: pf igmp icmp6 multicast router alert
On Fri, Apr 22, 2022 at 07:40:17PM +0200, Alexandr Nedvedicky wrote:
> > + case IPPROTO_ICMPV6:
> > + if (!pf_pull_hdr(pd->m, pd->off, , 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:
>
> I wonder if we should have a similar check we have for IPv4 address,
> where we require a multicast address. for example in case of
> MLD_LISTENER_QUERY the packet destination address should be fe80::/10.
> I need to look at RFCs more closely first. Just asking in case someone
> else
> knows from top of the head.
Where do we check multicast adddress for IPv4? At this point we
are just comparing protocol and IP options. I would not make it
more complex, so I will not add multicast adddress checks here.
bluhm
Re: pf igmp icmp6 multicast router alert
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.c17 Mar 2022 18:27:55 - 1.1126
+++ net/pf.c27 Apr 2022 21:33:36 -
@@ -227,6 +227,8 @@ u_int16_tpf_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(, 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)
Re: [External] : Re: pf igmp icmp6 multicast router alert
Hello,
Looks like time has come to tweak a tuning knob from paranoia
towards 'get network working'
I have just few nits, see below.
thanks and
regards
sashan
>
> 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);
> +
> + while (off < end) {
> + type = opts[off - sizeof(struct ip)];
would it be possible to do 'off - sizeof (struct ip)' computation
outside of while loop? I think we can adjust both `off` and `end`:
off -= sizeof(struct ip);
len -= sizeof(struct ip);
just before we enter the while () loop.
> pd->off += (ext.ip6e_len + 2) * 4;
> @@ -6607,9 +6659,23 @@ 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, , 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:
I wonder if we should have a similar check we have for IPv4 address,
where we require a multicast address. for example in case of
MLD_LISTENER_QUERY the packet destination address should be fe80::/10.
I need to look at RFCs more closely first. Just asking in case someone else
knows from top of the head.
Re: pf igmp icmp6 multicast router alert
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.
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.c17 Mar 2022 18:27:55 - 1.1126
+++ net/pf.c22 Apr 2022 15:52:36 -
@@ -227,6 +227,8 @@ u_int16_tpf_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(, PFRES_IPOPTIONS);
#if NPFLOG > 0
pd->pflog |= PF_LOG_FORCE;
@@ -6382,6 +6384,43 @@ 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);
+
+ while (off < end) {
+ type = opts[off - sizeof(struct ip)];
+ if (type == IPOPT_EOL)
+ break;
+ if (type == IPOPT_NOP) {
+ off++;
+ continue;
+ }
+ length = opts[off - sizeof(struct ip)];
+ 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 +6432,18 @@ 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)) {
+ pf_walk_option(pd, h, sizeof(*h), hlen, reason);
+ /* 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);
@@ -6456,6 +6502,7 @@ pf_walk_option6(struct pf_pdesc *pd, str
}
switch (opt.ip6o_type) {
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 +6527,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 +6545,7 @@ pf_walk_header6(struct pf_pdesc *pd, str
{
struct ip6_frag frag;
struct ip6_ext ext;
+ struct icmp6_hdr icmp6;
struct
Re: pf igmp icmp6 multicast router alert
Florian Obser wrote: > That's my gut feeling as well. But I don't have any hard evidence that > that is actually correct. However, since we learned that router-alert is > an actual problem it is also a good incremental step. How many years did it take to notice this bug? Surely this was affecting other people, but noone sniffed to discover the reason. If we only make pf forgiving for a few options, but other options cause problems in the future, it may also take a long time to fix them. Can we turn it upside down, and consider blocking only options which are believed to be dangerous, or at least make such a list to think it through? Like maybe; known option, known to be dangerous known option, but good or required known option, but no dangerous impact seen currently undefined options (assume bad) And then use such a list to guide the switch () statement in this icmp case?
Re: pf igmp icmp6 multicast router alert
On 2022-04-21 21:10 +02, Alexander Bluhm wrote: > On Thu, Apr 21, 2022 at 08:56:07PM +0200, Otto Moerbeek wrote: >> > Currently it allows all options. Should I make it specific to >> > router alert with IGMP or ICMP6? >> >> To me it looks like the icmp6 case already is limited to MLD? > > The question is the other way around. My current diff allows any > option with ICMP6 MLD. Do we want to restict the option to router > alert? > > In our ip6.h we have: > #define IP6OPT_JUMBO0xC2/* 11 0 00010 = 194 */ > #define IP6OPT_NSAP_ADDR0xC3/* 11 0 00011 */ > #define IP6OPT_TUNNEL_LIMIT 0x04/* 00 0 00100 */ > #define IP6OPT_ROUTER_ALERT 0x05/* 00 0 00101 (RFC3542, recommended) > */ > > And who knows what other options have been designed. > > In ip.h I see these: > #define IPOPT_RR7 /* record packet route */ > #define IPOPT_TS68 /* timestamp */ > #define IPOPT_SECURITY 130 /* provide s,c,h,tcc */ > #define IPOPT_LSRR 131 /* loose source route */ > #define IPOPT_SATID 136 /* satnet id */ > #define IPOPT_SSRR 137 /* strict source route */ > #define IPOPT_RA148 /* router alert */ > > 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. That's my gut feeling as well. But I don't have any hard evidence that that is actually correct. However, since we learned that router-alert is an actual problem it is also a good incremental step. > > bluhm > -- I'm not entirely sure you are real.
Re: pf igmp icmp6 multicast router alert
On Thu, Apr 21, 2022 at 08:56:07PM +0200, Otto Moerbeek wrote: > > Currently it allows all options. Should I make it specific to > > router alert with IGMP or ICMP6? > > To me it looks like the icmp6 case already is limited to MLD? The question is the other way around. My current diff allows any option with ICMP6 MLD. Do we want to restict the option to router alert? In our ip6.h we have: #define IP6OPT_JUMBO0xC2/* 11 0 00010 = 194 */ #define IP6OPT_NSAP_ADDR0xC3/* 11 0 00011 */ #define IP6OPT_TUNNEL_LIMIT 0x04/* 00 0 00100 */ #define IP6OPT_ROUTER_ALERT 0x05/* 00 0 00101 (RFC3542, recommended) */ And who knows what other options have been designed. In ip.h I see these: #define IPOPT_RR7 /* record packet route */ #define IPOPT_TS68 /* timestamp */ #define IPOPT_SECURITY 130 /* provide s,c,h,tcc */ #define IPOPT_LSRR 131 /* loose source route */ #define IPOPT_SATID 136 /* satnet id */ #define IPOPT_SSRR 137 /* strict source route */ #define IPOPT_RA148 /* router alert */ 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. bluhm
Re: pf igmp icmp6 multicast router alert
On Thu, Apr 21, 2022 at 07:08:38PM +0200, Alexander Bluhm wrote:
> Hi,
>
> IGMP and ICMP6 for multicast packets have router alert options.
> Per default pf drops all IP packets with options. Usually people
> ask what is wrong until someone points out that they have to use a
> pf rule with allow-opts.
>
> As this is normal behavior and our kernel generates such packets,
> the pf default is bad.
>
> Diff is untested, but otto@ and florian@ could try it.
Tested this with success on two hosts that hasd issues because their
outgoing icmp6 messages were blcoked.
> Currently it allows all options. Should I make it specific to
> router alert with IGMP or ICMP6?
To me it looks like the icmp6 case already is limited to MLD?
-Otto
>
> 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 - 1.1126
> +++ net/pf.c 21 Apr 2022 16:30:18 -
> @@ -6398,6 +6398,9 @@ pf_walk_header(struct pf_pdesc *pd, stru
> 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)
> + pd->badopts = 0;
> /* stop walking over non initial fragments */
> if ((h->ip_off & htons(IP_OFFMASK)) != 0)
> return (PF_PASS);
> @@ -6494,6 +6497,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_tend;
> int hdr_cnt, fraghdr_cnt = 0, rthdr_cnt = 0;
> @@ -6607,9 +6611,23 @@ 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, , 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:
> + pd->badopts = 0;
> + 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) :
>
pf igmp icmp6 multicast router alert
Hi,
IGMP and ICMP6 for multicast packets have router alert options.
Per default pf drops all IP packets with options. Usually people
ask what is wrong until someone points out that they have to use a
pf rule with allow-opts.
As this is normal behavior and our kernel generates such packets,
the pf default is bad.
Diff is untested, but otto@ and florian@ could try it.
Currently it allows all options. Should I make it specific to
router alert with IGMP or ICMP6?
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.c17 Mar 2022 18:27:55 - 1.1126
+++ net/pf.c21 Apr 2022 16:30:18 -
@@ -6398,6 +6398,9 @@ pf_walk_header(struct pf_pdesc *pd, stru
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)
+ pd->badopts = 0;
/* stop walking over non initial fragments */
if ((h->ip_off & htons(IP_OFFMASK)) != 0)
return (PF_PASS);
@@ -6494,6 +6497,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_tend;
int hdr_cnt, fraghdr_cnt = 0, rthdr_cnt = 0;
@@ -6607,9 +6611,23 @@ 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, , 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:
+ pd->badopts = 0;
+ 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) :
