From 2cbebc019f52ca82e5fb8c4ec5d5ed5614c8fef5 Mon Sep 17 00:00:00 2001 From: bluhm Date: Tue, 19 Jan 2021 22:22:23 +0000 Subject: [PATCH] pflog(4) tried to log the translated packet with rdr-to, nat-to, and af-to addresses and ports applied. Therefore it created a mbuf chain on the stack with a partial copy. This is too complicated for IP options, extension header, NAT46 af-to, and fragmented mbuf chains. It even caused a crash in syzkaller. Usually the length checks in pf_setup_pdesc() rejected the faked mbuf and the goto copy logged the packet unmodified. Remove the pflog_mtap() function and call bpf_mtap_hdr() directly. As the old buggy code was bypassed in most cases, tcpdump(8) output of pflog does not change. Uncondionally log the unmodified packet. Reported-by: syzbot+947e89e06ac3fec187d0@syzkaller.appspotmail.com OK sashan@ --- sys/net/if_pflog.c | 141 +-------------------------------------------- sys/net/pf.c | 13 +---- 2 files changed, 5 insertions(+), 149 deletions(-) diff --git a/sys/net/if_pflog.c b/sys/net/if_pflog.c index 27ea9bde3b3..b574900593e 100644 --- a/sys/net/if_pflog.c +++ b/sys/net/if_pflog.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if_pflog.c,v 1.94 2021/01/13 09:13:30 mvs Exp $ */ +/* $OpenBSD: if_pflog.c,v 1.95 2021/01/19 22:22:23 bluhm Exp $ */ /* * The authors of this code are John Ioannidis (ji@tla.org), * Angelos D. Keromytis (kermit@csd.uch.gr) and @@ -81,7 +81,6 @@ int pflogoutput(struct ifnet *, struct mbuf *, struct sockaddr *, int pflogioctl(struct ifnet *, u_long, caddr_t); int pflog_clone_create(struct if_clone *, int); int pflog_clone_destroy(struct ifnet *); -void pflog_mtap(caddr_t, struct pfloghdr *, struct mbuf *); struct pflog_softc *pflog_getif(int); struct if_clone pflog_cloner = @@ -242,144 +241,8 @@ pflog_packet(struct pf_pdesc *pd, u_int8_t reason, struct pf_rule *rm, ifn->if_opackets++; ifn->if_obytes += pd->m->m_pkthdr.len; - pflog_mtap(if_bpf, &hdr, pd->m); + bpf_mtap_hdr(if_bpf, &hdr, sizeof(hdr), pd->m, BPF_DIRECTION_OUT); #endif return (0); } - -void -pflog_mtap(caddr_t if_bpf, struct pfloghdr *pfloghdr, struct mbuf *m) -{ - struct mbuf mhdr; - struct m_hdr mptr; - struct mbuf *mp; - u_char *mdst; - int afto, hlen, off; - - struct pf_pdesc pd; - struct pf_addr osaddr, odaddr; - u_int16_t osport = 0, odport = 0; - u_int8_t proto = 0; - - afto = pfloghdr->af != pfloghdr->naf; - - /* - * temporary mbuf will hold an ip/ip6 header and 8 bytes - * of the protocol header - */ - m_inithdr(&mhdr); - mhdr.m_len = 0; /* XXX not done in m_inithdr() */ - -#ifdef INET6 - /* offset for a new header */ - if (afto && pfloghdr->af == AF_INET) - mhdr.m_data += sizeof(struct ip6_hdr) - - sizeof(struct ip); -#endif /* INET6 */ - - mdst = mtod(&mhdr, char *); - switch (pfloghdr->af) { - case AF_INET: { - struct ip *h; - - if (m->m_pkthdr.len < sizeof(*h)) - goto copy; - m_copydata(m, 0, sizeof(*h), mdst); - h = (struct ip *)mdst; - hlen = h->ip_hl << 2; - if (hlen > sizeof(*h) && (m->m_pkthdr.len >= hlen)) - m_copydata(m, sizeof(*h), hlen - sizeof(*h), - mdst + sizeof(*h)); - break; - } -#ifdef INET6 - case AF_INET6: { - struct ip6_hdr *h; - - if (m->m_pkthdr.len < sizeof(*h)) - goto copy; - hlen = sizeof(struct ip6_hdr); - m_copydata(m, 0, hlen, mdst); - h = (struct ip6_hdr *)mdst; - proto = h->ip6_nxt; - break; - } -#endif /* INET6 */ - default: - /* shouldn't happen ever :-) */ - goto copy; - } - - if (m->m_pkthdr.len < hlen + 8 && proto != IPPROTO_NONE) - goto copy; - else if (proto != IPPROTO_NONE) { - /* copy 8 bytes of the protocol header */ - m_copydata(m, hlen, 8, mdst + hlen); - hlen += 8; - } - - mhdr.m_len = hlen; - mhdr.m_pkthdr.len = hlen; - - /* create a chain mhdr -> mptr, mptr->m_data = (m->m_data+hlen) */ - mp = m_getptr(m, hlen, &off); - if (mp != NULL) { - mptr.mh_flags = 0; - mptr.mh_data = mp->m_data + off; - mptr.mh_len = mp->m_len - off; - mptr.mh_next = mp->m_next; - - mhdr.m_next = (struct mbuf *)&mptr; - } - - /* - * Rewrite addresses if needed. Reason pointer must be NULL to avoid - * counting the packet here again. - */ - if (pf_setup_pdesc(&pd, pfloghdr->af, pfloghdr->dir, NULL, - &mhdr, NULL) != PF_PASS) - goto copy; - pd.naf = pfloghdr->naf; - - pf_addrcpy(&osaddr, pd.src, pd.af); - pf_addrcpy(&odaddr, pd.dst, pd.af); - if (pd.sport) - osport = *pd.sport; - if (pd.dport) - odport = *pd.dport; - - if (pd.virtual_proto != PF_VPROTO_FRAGMENT && - (pfloghdr->rewritten = pf_translate(&pd, &pfloghdr->saddr, - pfloghdr->sport, &pfloghdr->daddr, pfloghdr->dport, 0, - pfloghdr->dir))) { - m_copyback(pd.m, pd.off, min(pd.m->m_len - pd.off, pd.hdrlen), - &pd.hdr, M_NOWAIT); -#ifdef INET6 - if (afto) { - pf_addrcpy(&pd.nsaddr, &pfloghdr->saddr, pd.naf); - pf_addrcpy(&pd.ndaddr, &pfloghdr->daddr, pd.naf); - } -#endif /* INET6 */ - pf_addrcpy(&pfloghdr->saddr, &osaddr, pd.af); - pf_addrcpy(&pfloghdr->daddr, &odaddr, pd.af); - pfloghdr->sport = osport; - pfloghdr->dport = odport; - } - - pd.tot_len -= pd.m->m_data - pd.m->m_pktdat; - -#ifdef INET6 - if (afto && pfloghdr->rewritten) - pf_translate_af(&pd); -#endif /* INET6 */ - - m = pd.m; - KASSERT(m == &mhdr); - copy: -#if NBPFILTER > 0 - bpf_mtap_hdr(if_bpf, pfloghdr, sizeof(*pfloghdr), m, - BPF_DIRECTION_OUT); -#endif - return; -} diff --git a/sys/net/pf.c b/sys/net/pf.c index e5e03458045..5d6b49356be 100644 --- a/sys/net/pf.c +++ b/sys/net/pf.c @@ -1,4 +1,4 @@ -/* $OpenBSD: pf.c,v 1.1100 2021/01/16 13:09:46 bluhm Exp $ */ +/* $OpenBSD: pf.c,v 1.1101 2021/01/19 22:22:23 bluhm Exp $ */ /* * Copyright (c) 2001 Daniel Hartmeier @@ -4184,11 +4184,6 @@ pf_translate(struct pf_pdesc *pd, struct pf_addr *saddr, u_int16_t sport, struct pf_addr *daddr, u_int16_t dport, u_int16_t virtual_type, int icmp_dir) { - /* - * when called from bpf_mtap_pflog, there are extra constraints: - * -mbuf is faked, m_data is the bpf buffer - * -pd is not fully set up - */ int rewrite = 0; int afto = pd->af != pd->naf; @@ -4203,18 +4198,17 @@ pf_translate(struct pf_pdesc *pd, struct pf_addr *saddr, u_int16_t sport, break; case IPPROTO_ICMP: - /* pf_translate() is also used when logging invalid packets */ if (pd->af != AF_INET) return (0); - if (afto) { #ifdef INET6 + if (afto) { if (pf_translate_icmp_af(pd, AF_INET6, &pd->hdr.icmp)) return (0); pd->proto = IPPROTO_ICMPV6; rewrite = 1; -#endif /* INET6 */ } +#endif /* INET6 */ if (virtual_type == htons(ICMP_ECHO)) { u_int16_t icmpid = (icmp_dir == PF_IN) ? sport : dport; rewrite += pf_patch_16(pd, @@ -4224,7 +4218,6 @@ pf_translate(struct pf_pdesc *pd, struct pf_addr *saddr, u_int16_t sport, #ifdef INET6 case IPPROTO_ICMPV6: - /* pf_translate() is also used when logging invalid packets */ if (pd->af != AF_INET6) return (0); -- 2.20.1