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 -0000 1.1126
> +++ net/pf.c 27 Apr 2022 22:28:38 -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,22 @@ 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_pull_hdr(pd->m, pd->off, &ext, sizeof(ext),
> + NULL, reason, AF_INET6)) {
> + DPFPRINTF(LOG_NOTICE, "IPv6 short exthdr");
> + return (PF_DROP);
> + }
> + 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 +6668,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 +6680,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 +7255,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;
>
