โฌข DragonFlyBSD Kernel Audit
DF-3264 / fix.diff
โ† back to finding โ†“ download raw
Fix DF-3264 (pf_state use-after-free: pf_send_tcp drops pf_token mid-state-operation)

Mechanism: the packet path processes states holding pf_token *shared*
(pf_check_in/out).  pf_send_tcp() releases pf_token around ip_output()
(pf.c:2254-2256, 2274-2276, 2289-2291).  Callers on the state path keep
using the struct pf_state after that call returns โ€” pf_test_state_tcp's
synproxy branches (pf.c:4918-4924, 4948-4954, 4963-4986), the bad-state
RST path (pf.c:4738-4746), the create-state synproxy send
(pf.c:4294-4298 -> pf_test tail pf.c:6712-6812), and pf_unlink_state
(pf.c:1489-1497).  During the token-drop window the pf_purge_thread can
acquire the token exclusively, decide the just-expired state is garbage,
and unlink+free it.  When ip_output returns, the netisr thread continues
on freed memory (lockmgr on freed lk, seq bookkeeping writes,
pfsync_update_state, counter updates) โ€” a remote-triggerable UAF.

Design of the fix: every state-path caller of pf_send_tcp() that touches
the state afterwards already holds (or now holds) the per-state lock
s->lk exclusively.  pf_free_state() (purge thread only) try-locks s->lk
before freeing; if the lock is unavailable the state is left on the
state list in the PFTM_UNLINKED two-phase state and is retried on the
next sweep (the same deferral pattern already used for pfsync bulk
references at pf.c:1516-1519).

Three hunks:
 1. pf_free_state: try-lock & defer.
 2. pf_create_state: hold s->lk across the synproxy pf_send_tcp.
 3. pf_test: hold s->lk across the SYNPROXY_DROP tail processing.

--- a/sys/net/pf/pf.c
+++ b/sys/net/pf/pf.c
@@ -1514,6 +1514,22 @@ pf_free_state(struct pf_state *cur)
 	if (pfsyncif != NULL &&
 	    (pfsyncif->sc_bulk_send_next == cur ||
 	    pfsyncif->sc_bulk_terminator == cur))
 		return;
 	KKASSERT(cur->timeout == PFTM_UNLINKED);
+
+	/*
+	 * A packet thread may still be operating on this state:
+	 * pf_send_tcp() temporarily releases pf_token around ip_output(),
+	 * and its callers keep using the state afterwards while holding
+	 * cur->lk exclusively.  If we cannot get the lock without
+	 * blocking, the state is in flight โ€” leave it on the list
+	 * (already unlinked, PFTM_UNLINKED) and retry next sweep.
+	 */
+	if (lockmgr(&cur->lk, LK_EXCLUSIVE | LK_NOWAIT) != 0) {
+		DPFPRINTF(PF_DEBUG_MISC,
+		    ("pf_free_state: state in flight, deferring free\n"));
+		return;
+	}
+
 	/*
 	 * decrements in rule should be ok, token is
 	 * held exclusively in this code path.
@@ -4283,10 +4299,12 @@ pf_create_state(...)
 		s->src.seqhi = htonl(karc4random());
 		/* Find mss option */
 		mss = pf_get_mss(m, off, th->th_off, pd->af);
 		mss = pf_calc_mss(pd->src, pd->af, mss);
 		mss = pf_calc_mss(pd->dst, pd->af, mss);
 		s->src.mss = mss;
 		s->state_flags &= ~PFSTATE_CREATEINPROG;
+		lockmgr(&s->lk, LK_EXCLUSIVE);
 		pf_send_tcp(r, pd->af, pd->dst, pd->src, th->th_dport,
 			    th->th_sport, s->src.seqhi, ntohl(th->th_seq) + 1,
 			    TH_SYN|TH_ACK, 0, s->src.mss, 0, 1, 0, NULL, NULL);
+		lockmgr(&s->lk, LK_RELEASE);
 		REASON_SET(&reason, PFRES_SYNPROXY);
 		return (PF_SYNPROXY_DROP);
 	}

--- a/sys/net/pf/pf.c
+++ b/sys/net/pf/pf.c
@@ -6710,6 +6710,14 @@ pf_test(...)
 done:
+	/*
+	 * SYNPROXY_DROP: pf_create_state()/pf_test_state_tcp() sent a
+	 * generated packet with pf_send_tcp(), which releases pf_token
+	 * around ip_output(); the state could have been purged in that
+	 * window.  Pin it with its lock while we finish the tail
+	 * processing below.
+	 */
+	int locked = 0;
+	if (action == PF_SYNPROXY_DROP && s != NULL) {
+		lockmgr(&s->lk, LK_EXCLUSIVE);
+		locked = 1;
+	}
 	if (action == PF_PASS && h->ip_hl > 5 &&
 	    !((s && s->state_flags & PFSTATE_ALLOWOPTS) || r->allow_opts)) {
@@ -6833,6 +6841,9 @@
 
 	if (action == PF_SYNPROXY_DROP) {
+		if (locked)
+			lockmgr(&s->lk, LK_RELEASE);
 		m_freem(*m0);
 		*m0 = NULL;
 		action = PF_PASS;
 	} else if (r->rt) {
 		/* pf_route can free the mbuf causing *m0 to become NULL */
 		pf_route(m0, r, dir, kif->pfik_ifp, s, &pd);
+		if (locked)
+			lockmgr(&s->lk, LK_RELEASE);
 	}

Note: hunk 3 keeps pf_free_state's deferral meaningful for the
create-state path; hunk 2/3 must both be applied.  A stricter long-term
remedy is to stop dropping pf_token inside pf_send_tcp() (queue the
generated packet instead), which would make every caller safe without
per-state locking; that is the recommended upstream direction.