Hi,
pf_pull_hdr() allows to pass an action pointer parameter as output
value. This is never used, all callers pass a NULL argument. Remove
ACTION_SET() entirely.
The logic if (fragoff >= len) in pf_pull_hdr() looks odd. One is
the offset in the IP packet, the latter the length of some header
within the fragment. In revision 1.1 the logic was used to drop
short TCP or UDP fragments that contained only part of the header.
This does not work since pf_pull_hdr() supports offsets.
----------------------------
revision 1.4
date: 2001/06/24 20:54:55; author: itojun; state: Exp; lines: +18 -16;
pull_hdr() now takes header offset explicitly, to help header chain parsing
(v6, ipsec)
----------------------------
The code drops the packets anyway, so always set reason PFRES_FRAG.
ok?
bluhm
Index: net/pf.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.1186
diff -u -p -r1.1186 pf.c
--- net/pf.c 8 Sep 2023 13:40:52 -0000 1.1186
+++ net/pf.c 9 Oct 2023 17:01:04 -0000
@@ -3201,7 +3201,7 @@ pf_modulate_sack(struct pf_pdesc *pd, st
optsoff = pd->off + sizeof(struct tcphdr);
#define TCPOLEN_MINSACK (TCPOLEN_SACK + 2)
if (olen < TCPOLEN_MINSACK ||
- !pf_pull_hdr(pd->m, optsoff, opts, olen, NULL, NULL, pd->af))
+ !pf_pull_hdr(pd->m, optsoff, opts, olen, NULL, pd->af))
return (0);
eoh = opts + olen;
@@ -3871,7 +3871,7 @@ pf_get_wscale(struct pf_pdesc *pd)
olen = (pd->hdr.tcp.th_off << 2) - sizeof(struct tcphdr);
if (olen < TCPOLEN_WINDOW || !pf_pull_hdr(pd->m,
- pd->off + sizeof(struct tcphdr), opts, olen, NULL, NULL, pd->af))
+ pd->off + sizeof(struct tcphdr), opts, olen, NULL, pd->af))
return (0);
opt = opts;
@@ -3896,7 +3896,7 @@ pf_get_mss(struct pf_pdesc *pd)
olen = (pd->hdr.tcp.th_off << 2) - sizeof(struct tcphdr);
if (olen < TCPOLEN_MAXSEG || !pf_pull_hdr(pd->m,
- pd->off + sizeof(struct tcphdr), opts, olen, NULL, NULL, pd->af))
+ pd->off + sizeof(struct tcphdr), opts, olen, NULL, pd->af))
return (0);
opt = opts;
@@ -5691,7 +5691,7 @@ pf_test_state_icmp(struct pf_pdesc *pd,
ipoff2 = pd->off + ICMP_MINLEN;
if (!pf_pull_hdr(pd2.m, ipoff2, &h2, sizeof(h2),
- NULL, reason, pd2.af)) {
+ reason, pd2.af)) {
DPFPRINTF(LOG_NOTICE,
"ICMP error message too short (ip)");
return (PF_DROP);
@@ -5719,7 +5719,7 @@ pf_test_state_icmp(struct pf_pdesc *pd,
ipoff2 = pd->off + sizeof(struct icmp6_hdr);
if (!pf_pull_hdr(pd2.m, ipoff2, &h2_6, sizeof(h2_6),
- NULL, reason, pd2.af)) {
+ reason, pd2.af)) {
DPFPRINTF(LOG_NOTICE,
"ICMP error message too short (ip6)");
return (PF_DROP);
@@ -5770,7 +5770,7 @@ pf_test_state_icmp(struct pf_pdesc *pd,
* expected. Don't access any TCP header fields after
* th_seq, an ackskew test is not possible.
*/
- if (!pf_pull_hdr(pd2.m, pd2.off, th, 8, NULL, reason,
+ if (!pf_pull_hdr(pd2.m, pd2.off, th, 8, reason,
pd2.af)) {
DPFPRINTF(LOG_NOTICE,
"ICMP error message too short (tcp)");
@@ -5951,7 +5951,7 @@ pf_test_state_icmp(struct pf_pdesc *pd,
int action;
if (!pf_pull_hdr(pd2.m, pd2.off, uh, sizeof(*uh),
- NULL, reason, pd2.af)) {
+ reason, pd2.af)) {
DPFPRINTF(LOG_NOTICE,
"ICMP error message too short (udp)");
return (PF_DROP);
@@ -6079,7 +6079,7 @@ pf_test_state_icmp(struct pf_pdesc *pd,
}
if (!pf_pull_hdr(pd2.m, pd2.off, iih, ICMP_MINLEN,
- NULL, reason, pd2.af)) {
+ reason, pd2.af)) {
DPFPRINTF(LOG_NOTICE,
"ICMP error message too short (icmp)");
return (PF_DROP);
@@ -6185,7 +6185,7 @@ pf_test_state_icmp(struct pf_pdesc *pd,
}
if (!pf_pull_hdr(pd2.m, pd2.off, iih,
- sizeof(struct icmp6_hdr), NULL, reason, pd2.af)) {
+ sizeof(struct icmp6_hdr), reason, pd2.af)) {
DPFPRINTF(LOG_NOTICE,
"ICMP error message too short (icmp6)");
return (PF_DROP);
@@ -6364,7 +6364,7 @@ pf_test_state_icmp(struct pf_pdesc *pd,
*/
void *
pf_pull_hdr(struct mbuf *m, int off, void *p, int len,
- u_short *actionp, u_short *reasonp, sa_family_t af)
+ u_short *reasonp, sa_family_t af)
{
int iplen = 0;
@@ -6374,12 +6374,7 @@ pf_pull_hdr(struct mbuf *m, int off, voi
u_int16_t fragoff = (ntohs(h->ip_off) & IP_OFFMASK) << 3;
if (fragoff) {
- if (fragoff >= len)
- ACTION_SET(actionp, PF_PASS);
- else {
- ACTION_SET(actionp, PF_DROP);
- REASON_SET(reasonp, PFRES_FRAG);
- }
+ REASON_SET(reasonp, PFRES_FRAG);
return (NULL);
}
iplen = ntohs(h->ip_len);
@@ -6395,7 +6390,6 @@ pf_pull_hdr(struct mbuf *m, int off, voi
#endif /* INET6 */
}
if (m->m_pkthdr.len < off + len || iplen < off + len) {
- ACTION_SET(actionp, PF_DROP);
REASON_SET(reasonp, PFRES_SHORT);
return (NULL);
}
@@ -6949,7 +6943,7 @@ pf_walk_header(struct pf_pdesc *pd, stru
end < pd->off + sizeof(ext))
return (PF_PASS);
if (!pf_pull_hdr(pd->m, pd->off, &ext, sizeof(ext),
- NULL, reason, AF_INET)) {
+ reason, AF_INET)) {
DPFPRINTF(LOG_NOTICE, "IP short exthdr");
return (PF_DROP);
}
@@ -6975,7 +6969,7 @@ pf_walk_option6(struct pf_pdesc *pd, str
while (off < end) {
if (!pf_pull_hdr(pd->m, off, &opt.ip6o_type,
- sizeof(opt.ip6o_type), NULL, reason, AF_INET6)) {
+ sizeof(opt.ip6o_type), reason, AF_INET6)) {
DPFPRINTF(LOG_NOTICE, "IPv6 short opt type");
return (PF_DROP);
}
@@ -6984,7 +6978,7 @@ pf_walk_option6(struct pf_pdesc *pd, str
continue;
}
if (!pf_pull_hdr(pd->m, off, &opt, sizeof(opt),
- NULL, reason, AF_INET6)) {
+ reason, AF_INET6)) {
DPFPRINTF(LOG_NOTICE, "IPv6 short opt");
return (PF_DROP);
}
@@ -7009,7 +7003,7 @@ pf_walk_option6(struct pf_pdesc *pd, str
return (PF_DROP);
}
if (!pf_pull_hdr(pd->m, off, &jumbo, sizeof(jumbo),
- NULL, reason, AF_INET6)) {
+ reason, AF_INET6)) {
DPFPRINTF(LOG_NOTICE, "IPv6 short jumbo");
return (PF_DROP);
}
@@ -7058,7 +7052,7 @@ pf_walk_header6(struct pf_pdesc *pd, str
break;
case IPPROTO_HOPOPTS:
if (!pf_pull_hdr(pd->m, pd->off, &ext, sizeof(ext),
- NULL, reason, AF_INET6)) {
+ reason, AF_INET6)) {
DPFPRINTF(LOG_NOTICE, "IPv6 short exthdr");
return (PF_DROP);
}
@@ -7085,7 +7079,7 @@ pf_walk_header6(struct pf_pdesc *pd, str
return (PF_DROP);
}
if (!pf_pull_hdr(pd->m, pd->off, &frag, sizeof(frag),
- NULL, reason, AF_INET6)) {
+ reason, AF_INET6)) {
DPFPRINTF(LOG_NOTICE, "IPv6 short fragment");
return (PF_DROP);
}
@@ -7113,7 +7107,7 @@ pf_walk_header6(struct pf_pdesc *pd, str
return (PF_PASS);
}
if (!pf_pull_hdr(pd->m, pd->off, &rthdr, sizeof(rthdr),
- NULL, reason, AF_INET6)) {
+ reason, AF_INET6)) {
DPFPRINTF(LOG_NOTICE, "IPv6 short rthdr");
return (PF_DROP);
}
@@ -7140,7 +7134,7 @@ pf_walk_header6(struct pf_pdesc *pd, str
return (PF_PASS);
}
if (!pf_pull_hdr(pd->m, pd->off, &ext, sizeof(ext),
- NULL, reason, AF_INET6)) {
+ reason, AF_INET6)) {
DPFPRINTF(LOG_NOTICE, "IPv6 short exthdr");
return (PF_DROP);
}
@@ -7167,7 +7161,7 @@ pf_walk_header6(struct pf_pdesc *pd, str
return (PF_PASS);
}
if (!pf_pull_hdr(pd->m, pd->off, &icmp6, sizeof(icmp6),
- NULL, reason, AF_INET6)) {
+ reason, AF_INET6)) {
DPFPRINTF(LOG_NOTICE, "IPv6 short icmp6hdr");
return (PF_DROP);
}
@@ -7338,7 +7332,7 @@ pf_setup_pdesc(struct pf_pdesc *pd, sa_f
struct tcphdr *th = &pd->hdr.tcp;
if (!pf_pull_hdr(pd->m, pd->off, th, sizeof(*th),
- NULL, reason, pd->af))
+ reason, pd->af))
return (PF_DROP);
pd->hdrlen = sizeof(*th);
if (th->th_dport == 0 ||
@@ -7357,7 +7351,7 @@ pf_setup_pdesc(struct pf_pdesc *pd, sa_f
struct udphdr *uh = &pd->hdr.udp;
if (!pf_pull_hdr(pd->m, pd->off, uh, sizeof(*uh),
- NULL, reason, pd->af))
+ reason, pd->af))
return (PF_DROP);
pd->hdrlen = sizeof(*uh);
if (uh->uh_dport == 0 ||
@@ -7373,7 +7367,7 @@ pf_setup_pdesc(struct pf_pdesc *pd, sa_f
}
case IPPROTO_ICMP: {
if (!pf_pull_hdr(pd->m, pd->off, &pd->hdr.icmp, ICMP_MINLEN,
- NULL, reason, pd->af))
+ reason, pd->af))
return (PF_DROP);
pd->hdrlen = ICMP_MINLEN;
if (pd->off + pd->hdrlen > pd->tot_len) {
@@ -7388,7 +7382,7 @@ pf_setup_pdesc(struct pf_pdesc *pd, sa_f
size_t icmp_hlen = sizeof(struct icmp6_hdr);
if (!pf_pull_hdr(pd->m, pd->off, &pd->hdr.icmp6, icmp_hlen,
- NULL, reason, pd->af))
+ reason, pd->af))
return (PF_DROP);
/* ICMP headers we look further into to match state */
switch (pd->hdr.icmp6.icmp6_type) {
@@ -7411,7 +7405,7 @@ pf_setup_pdesc(struct pf_pdesc *pd, sa_f
}
if (icmp_hlen > sizeof(struct icmp6_hdr) &&
!pf_pull_hdr(pd->m, pd->off, &pd->hdr.icmp6, icmp_hlen,
- NULL, reason, pd->af))
+ reason, pd->af))
return (PF_DROP);
pd->hdrlen = icmp_hlen;
if (pd->off + pd->hdrlen > pd->tot_len) {
Index: net/pf_norm.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf_norm.c,v
retrieving revision 1.228
diff -u -p -r1.228 pf_norm.c
--- net/pf_norm.c 6 Jul 2023 04:55:05 -0000 1.228
+++ net/pf_norm.c 9 Oct 2023 17:02:01 -0000
@@ -1075,7 +1075,7 @@ pf_normalize_ip6(struct pf_pdesc *pd, u_
if (pd->fragoff == 0)
goto no_fragment;
- if (!pf_pull_hdr(pd->m, pd->fragoff, &frag, sizeof(frag), NULL, reason,
+ if (!pf_pull_hdr(pd->m, pd->fragoff, &frag, sizeof(frag), reason,
AF_INET6))
return (PF_DROP);
@@ -1216,7 +1216,7 @@ pf_normalize_tcp_init(struct pf_pdesc *p
olen = (th->th_off << 2) - sizeof(*th);
if (olen < TCPOLEN_TIMESTAMP || !pf_pull_hdr(pd->m,
- pd->off + sizeof(*th), opts, olen, NULL, NULL, pd->af))
+ pd->off + sizeof(*th), opts, olen, NULL, pd->af))
return (0);
opt = opts;
@@ -1299,7 +1299,7 @@ pf_normalize_tcp_stateful(struct pf_pdes
if (olen >= TCPOLEN_TIMESTAMP &&
((src->scrub && (src->scrub->pfss_flags & PFSS_TIMESTAMP)) ||
(dst->scrub && (dst->scrub->pfss_flags & PFSS_TIMESTAMP))) &&
- pf_pull_hdr(pd->m, pd->off + sizeof(*th), opts, olen, NULL, NULL,
+ pf_pull_hdr(pd->m, pd->off + sizeof(*th), opts, olen, NULL,
pd->af)) {
/* Modulate the timestamps. Can be used for NAT detection, OS
@@ -1633,7 +1633,7 @@ pf_normalize_mss(struct pf_pdesc *pd, u_
olen = (pd->hdr.tcp.th_off << 2) - sizeof(struct tcphdr);
optsoff = pd->off + sizeof(struct tcphdr);
if (olen < TCPOLEN_MAXSEG ||
- !pf_pull_hdr(pd->m, optsoff, opts, olen, NULL, NULL, pd->af))
+ !pf_pull_hdr(pd->m, optsoff, opts, olen, NULL, pd->af))
return (0);
opt = opts;
Index: net/pf_osfp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf_osfp.c,v
retrieving revision 1.46
diff -u -p -r1.46 pf_osfp.c
--- net/pf_osfp.c 7 May 2023 12:45:21 -0000 1.46
+++ net/pf_osfp.c 9 Oct 2023 17:02:15 -0000
@@ -111,8 +111,7 @@ pf_osfp_fingerprint(struct pf_pdesc *pd)
ip6 = mtod(pd->m, struct ip6_hdr *);
break;
}
- if (!pf_pull_hdr(pd->m, pd->off, hdr, th->th_off << 2, NULL, NULL,
- pd->af))
+ if (!pf_pull_hdr(pd->m, pd->off, hdr, th->th_off << 2, NULL, pd->af))
return (NULL);
return (pf_osfp_fingerprint_hdr(ip, ip6, (struct tcphdr *)hdr));
Index: net/pfvar.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfvar.h,v
retrieving revision 1.533
diff -u -p -r1.533 pfvar.h
--- net/pfvar.h 6 Jul 2023 04:55:05 -0000 1.533
+++ net/pfvar.h 9 Oct 2023 17:02:57 -0000
@@ -1192,12 +1192,6 @@ enum pfi_kif_refs {
#define SCNT_SRC_NODE_REMOVALS 2
#define SCNT_MAX 3
-#define ACTION_SET(a, x) \
- do { \
- if ((a) != NULL) \
- *(a) = (x); \
- } while (0)
-
#define REASON_SET(a, x) \
do { \
if ((void *)(a) != NULL) { \
@@ -1649,8 +1643,7 @@ void pf_poolmask(struct pf_addr *, struc
struct pf_addr *, struct pf_addr *, sa_family_t);
void pf_addr_inc(struct pf_addr *, sa_family_t);
-void *pf_pull_hdr(struct mbuf *, int, void *, int, u_short *, u_short *,
- sa_family_t);
+void *pf_pull_hdr(struct mbuf *, int, void *, int, u_short *, sa_family_t);
#define PF_HI (true)
#define PF_LO (!PF_HI)
#define PF_ALGNMNT(off) (((off) % 2) == 0 ? PF_HI : PF_LO)
