On Fri, Jan 13, 2012 at 11:44:20AM -0700, Theo de Raadt wrote:
> > I have to drop them all, "including those not yet received".
>
> That last bit is crazy. You cannot maintain state until the potential
> packets fall out of the fragment cache.
After discussion with deraadt@ it came clear that dropping future
fragments can result in a denial of service. So free the fragment
reassembly state immediately.
Unfortunately this triggered another bug. When a fragment could
not be reassembled, it was logged. During logging the reassmble
code is run again and the freed mbuf ended in the fragment cache.
Quick fix is not to log after pf_setup_pdesc() failure. Long term
solution would be to move the call to pf_normalize() from
pf_setup_pdesc() to pf_test().
bluhm
ok?
Index: net/pf_norm.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf_norm.c,v
retrieving revision 1.149
diff -u -p -r1.149 pf_norm.c
--- net/pf_norm.c 13 Jan 2012 11:24:35 -0000 1.149
+++ net/pf_norm.c 13 Jan 2012 21:02:07 -0000
@@ -326,15 +326,7 @@ pf_fillup_fragment(struct pf_fragment_cm
return (frag);
}
- if (TAILQ_EMPTY(&frag->fr_queue)) {
- /*
- * Overlapping IPv6 fragments have been detected. Do not
- * reassemble packet but also drop future fragments.
- * This will be done for this ident/src/dst combination
- * until fragment queue timeout.
- */
- goto drop_fragment;
- }
+ KASSERT(!TAILQ_EMPTY(&frag->fr_queue));
/* Remember maximum fragment len for refragmentation */
if (frent->fe_len > frag->fr_maxlen)
@@ -372,7 +364,7 @@ pf_fillup_fragment(struct pf_fragment_cm
u_int16_t precut;
if (frag->fr_af == AF_INET6)
- goto flush_fragentries;
+ goto free_fragment;
precut = prev->fe_off + prev->fe_len - frent->fe_off;
if (precut >= frent->fe_len) {
@@ -391,7 +383,7 @@ pf_fillup_fragment(struct pf_fragment_cm
u_int16_t aftercut;
if (frag->fr_af == AF_INET6)
- goto flush_fragentries;
+ goto free_fragment;
aftercut = frent->fe_off + frent->fe_len - after->fe_off;
if (aftercut < after->fe_len) {
@@ -419,22 +411,19 @@ pf_fillup_fragment(struct pf_fragment_cm
return (frag);
- flush_fragentries:
+ free_fragment:
/*
* RFC5722: When reassembling an IPv6 datagram, if one or
* more its constituent fragments is determined to be an
* overlapping fragment, the entire datagram (and any constituent
* fragments, including those not yet received) MUST be
* silently discarded.
+ *
+ * We do not drop future fragments, as this could be used
+ * for a denial of service attack.
*/
DPFPRINTF(LOG_NOTICE, "flush overlapping fragments");
- while ((prev = TAILQ_FIRST(&frag->fr_queue)) != NULL) {
- TAILQ_REMOVE(&frag->fr_queue, prev, fr_next);
-
- m_freem(prev->fe_m);
- pool_put(&pf_frent_pl, prev);
- pf_nfrents--;
- }
+ pf_free_fragment(frag);
bad_fragment:
REASON_SET(reason, PFRES_FRAG);
drop_fragment:
Index: net/pf.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.792
diff -u -p -r1.792 pf.c
--- net/pf.c 21 Dec 2011 23:00:16 -0000 1.792
+++ net/pf.c 13 Jan 2012 21:19:13 -0000
@@ -6650,7 +6650,12 @@ pf_test(sa_family_t af, int fwdir, struc
== -1) {
if (action == PF_PASS)
return (PF_PASS);
- pd.pflog |= PF_LOG_FORCE;
+ /*
+ * We must not log the packet here. pflog_bpfcopy() calls
+ * pf_setup_pdesc() again and then the packet might end up
+ * in the reassembly queue but the mbuf will be freed.
+ */
+ pd.pflog = 0;
goto done;
}
pd.eh = eh;
