[prev in list] [next in list] [prev in thread] [next in thread] 

List:       ipfilter
Subject:    Re: still confused about exactly how to avoid frag cache hole
From:       Cy Schubert - ITSD Open Systems Group <Cy.Schubert () uumail ! gov ! bc ! ca>
Date:       2001-04-30 14:13:59
[Download RAW message or body]

In message <3AEB3930.8CAE186C@graand-visions.com>, Raan Young writes:
> I've searched the ipf mailing list archive and I can't find an answer
> to this question (I did find a couple variations on this question, but
> no answer)...
> 
> In Darren's recent announcement about the fragment caching bug, he
> said --
> 
> +++++++++++
> How to enable it in new versions
> ================================
> Enable a security hole you say ? You will need to have "keep state
> keep frags" in your rule, not just "keep state".  That is rules with
> just "keep state" will no longer create fragment cache enties (as
> happens now).
> 
> Remaining Issues
> ================
> 1. There is an automatic frgament cache used by NAT which is now
> disabled by default and requires "frag" to be inserted into a NAT rule
> in order for it to function.
> 
> 2. Any and all packets which are fragments and match the required
> tuple (being srcip, dstip, ipid) will be let through so long as the
> frag cache entry remains.
> 
> 3. Use of "keep frags" with "keep state" means fragment cache entries
> can be created by packets going in *either* direction. Nothing will
> get added (now) to the fragment cache without being explicitly allowed
> by a rule (IPF or NAT).
> +++++++++++++
> 
> I'm not clear on what this means with respect to avoiding the security
> hole (specifically on an OpenBSD 2.8 system running ipf 3.3.18 with
> patches, as provided by the OpenBSD group, to address this bug) ...
> 
> In other words --
> 
> If my rules use "keep state keep frags" does the security hole still
> exist, even after applying the patches?
> 
> If I add "frag" to my NAT rules, am I re-introducing the security
> hole, even after applying the patches?
> 
> Do I have to remove the "keep frags" and "frag" options to avoid the
> security hole, in addition to applying the patches?

Rereading Darren's advisory again, I read it the same way do.

The 3.4.17 HISTORY file has the following comment:

fix fragment#0 handling bug where they could get in via cache 
information
created by state table entries

Looking at the code, it appears that the bug has been addressed:

@@ -200,7 +203,10 @@
        /*
         * Compute the offset of the expected start of the next packet.
         */
-       fra->ipfr_off = (ip->ip_off & IP_OFFMASK) + (fin->fin_dlen >> 
3);
+       off = ip->ip_off & IP_OFFMASK;
+       if (!off)
+               fra->ipfr_seen0 = 1;
+       fra->ipfr_off = off + (fin->fin_dlen >> 3);
        ATOMIC_INCL(ipfr_stats.ifs_new);
        ATOMIC_INC32(ipfr_inuse);
        return fra;
@@ -256,6 +262,9 @@
        ipfr_t  *f, frag;
        u_int   idx;
 
+       if (!(fin->fin_fi.fi_fl & FI_FRAG))
+               return NULL;
+
        /*
         * For fragments, we record protocol, packet id, TOS and both 
IP#'s
         * (these should all be the same for all fragments of a packet).
@@ -283,6 +292,19 @@
                          IPFR_CMPSZ)) {
                        u_short atoff, off;
 
+                       /*
+                        * XXX - We really need to be guarding against 
the
+                        * retransmission of (src,dst,id,offset-range) 
here
+                        * because a fragmented packet is never resent 
with
+                        * the same IP ID#.
+                        */
+                       off = ip->ip_off & IP_OFFMASK;
+                       if (f->ipfr_seen0) {
+                               if (!off || (fin->fin_fi.fi_fl & 
FI_SHORT))
+                                       continue;
+                       } else if (!off)
+                               f->ipfr_seen0 = 1;
+
                        if (f != table[idx]) {
                                /*
                                 * move fragment info. to the top of 
the list
@@ -295,7 +317,6 @@
                                f->ipfr_prev = NULL;
                                table[idx] = f;
                        }
-                       off = ip->ip_off & IP_OFFMASK;
                        atoff = off + (fin->fin_dlen >> 3);
                        /*
                         * If we've follwed the fragments, and this is 
the


> 
> Raan
> 
> 

I would think the bug has been fixed.  Line 302 of ip_frag.c is where 
the decision is made to allow fragment#0 through or not.


Regards,                         Phone:  (250)387-8437
Cy Schubert                        Fax:  (250)387-5766
Team Leader, Sun/Alpha Team   Internet:  Cy.Schubert@osg.gov.bc.ca
Open Systems Group, ITSD, ISTA
Province of BC

[prev in list] [next in list] [prev in thread] [next in thread]