Hi,

Drop overlapping IPv6 fragments in pf reassembly code.  RFC 5722
is demanding that.  Our stack does that already.

The text "constituent fragments, including those not yet received,
MUST be discarded" does not make clear, how long to do that.  The
easiest way for pf is to wait until the packet is complete and drop
it then.

Is that a good idea or should we wait for the reassembly timeout?

ok?

bluhm


Index: net/pf_norm.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf_norm.c,v
retrieving revision 1.148
diff -u -p -r1.148 pf_norm.c
--- net/pf_norm.c       3 Jan 2012 17:06:38 -0000       1.148
+++ net/pf_norm.c       11 Jan 2012 20:58:58 -0000
@@ -95,9 +95,11 @@ struct pf_fragment {
 
        RB_ENTRY(pf_fragment) fr_entry;
        TAILQ_ENTRY(pf_fragment) frag_next;
+       TAILQ_HEAD(pf_fragq, pf_frent) fr_queue;
        u_int32_t       fr_timeout;
        u_int16_t       fr_maxlen;      /* maximum length of single fragment */
-       TAILQ_HEAD(pf_fragq, pf_frent) fr_queue;
+       u_int8_t        fr_flags;
+#define        FR_OVERLAP      0x01
 };
 
 struct pf_fragment_tag {
@@ -313,9 +315,10 @@ pf_fillup_fragment(struct pf_fragment_cm
                }
 
                *(struct pf_fragment_cmp *)frag = *key;
+               TAILQ_INIT(&frag->fr_queue);
                frag->fr_timeout = time_second;
                frag->fr_maxlen = frent->fe_len;
-               TAILQ_INIT(&frag->fr_queue);
+               frag->fr_flags = 0;
 
                RB_INSERT(pf_frag_tree, &pf_frag_tree, frag);
                TAILQ_INSERT_HEAD(&pf_fragqueue, frag, frag_next);
@@ -363,10 +366,13 @@ pf_fillup_fragment(struct pf_fragment_cm
        if (prev != NULL && prev->fe_off + prev->fe_len > frent->fe_off) {
                u_int16_t       precut;
 
+               frag->fr_flags |= FR_OVERLAP;
                precut = prev->fe_off + prev->fe_len - frent->fe_off;
-               if (precut >= frent->fe_len)
-                       goto bad_fragment;
-               DPFPRINTF(LOG_NOTICE, "overlap -%d", precut);
+               if (precut >= frent->fe_len) {
+                       DPFPRINTF(LOG_NOTICE, "new frag overlapped");
+                       goto drop_fragment;
+               }
+               DPFPRINTF(LOG_NOTICE, "frag head overlap %d", precut);
                m_adj(frent->fe_m, precut);
                frent->fe_off += precut;
                frent->fe_len -= precut;
@@ -377,9 +383,10 @@ pf_fillup_fragment(struct pf_fragment_cm
        {
                u_int16_t       aftercut;
 
+               frag->fr_flags |= FR_OVERLAP;
                aftercut = frent->fe_off + frent->fe_len - after->fe_off;
-               DPFPRINTF(LOG_NOTICE, "adjust overlap %d", aftercut);
                if (aftercut < after->fe_len) {
+                       DPFPRINTF(LOG_NOTICE, "frag tail overlap %d", aftercut);
                        m_adj(after->fe_m, aftercut);
                        after->fe_off += aftercut;
                        after->fe_len -= aftercut;
@@ -387,6 +394,7 @@ pf_fillup_fragment(struct pf_fragment_cm
                }
 
                /* This fragment is completely overlapped, lose it */
+               DPFPRINTF(LOG_NOTICE, "old frag overlapped");
                next = TAILQ_NEXT(after, fr_next);
                TAILQ_REMOVE(&frag->fr_queue, after, fr_next);
 
@@ -593,6 +601,23 @@ pf_reassemble6(struct mbuf **m0, struct 
 
        if (!pf_isfull_fragment(frag))
                return (PF_PASS);  /* drop because *m0 is NULL, no error */
+
+       /*
+        * 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.
+        */
+       if (frag->fr_flags & FR_OVERLAP) {
+               pf_free_fragment(frag);
+               REASON_SET(reason, PFRES_FRAG);
+               /*
+                * We cannot return PF_DROP here as this requires a valid
+                * mbuf *m0 in pf_test().  But we can savely return PF_PASS
+                * as with *m0 == NULL the packet will be dropped.
+                */
+               return (PF_PASS);
+       }
 
        /* We have all the data */
        extoff = frent->fe_extoff;

Reply via email to