Re: ipfilter causing kernel panic in FreeBSD 6.1

Steve Clark wrote:
> Darren Reed wrote:
> 
>>Steve Clark wrote:
>>
>>
>>>Darren Reed wrote:
>>>
>>>
>>>>Steve Clark wrote:
>>>>
>>>>
>>>>
>>>>>...
>>>>>Hi Darren,
>>>>>
>>>>>Since I knew it was night time "down under" I went ahead and just 
>>>>>changed to code
>>>>>to print a message and return -1 in nat_finalise() then the tcp 
>>>>>pointer was null.
>>>>>The system has stayed up now almost 24 hours - where yesterday it 
>>>>>had 20 panics.
>>>>>
>>>>>Is there some reason we wouldn't want to just dump/ignore this 
>>>>>packet, since it seems to
>>>>>me that the initial syn and at least the first packet of the 
>>>>>fragmented series had gotten lost
>>>>>and eventually will be retried.
>>>>
>>>>
>>>>Well, the original packet that caused this problem was not
>>>>part of a SYN - it was a midstream TCP packet.  I suspect
>>>>something else is going on there.  It may be part of a retransmitted
>>>>packet sent after the original NAT session is torn down.
>>>>I don't know.  But obviously your policy allows the packet
>>>>to be sent (and received) so the NAT code should make
>>>>this possible.
>>>>
>>>>It might be more appropriate to not allow fragments that are
>>>>not the first fragment (offset = 0) to form NAT sessions for
>>>>the TCP/UDP protocols.  That I could understand.
>>>>
>>>
>>>Yes that is what I was getting at. It is my understanding that in 
>>>freebsd ip_nat happens
>>>before ip_filter - so we really can't filter these before they hit 
>>>ip_nat. Since they are externally
>>>generated we would have to put another fw in front of this firewall to 
>>>keep them out.
>>
>>
>>Ok, then the patch below might be of use to you then.
>>
>>Darren
>>
>>
>>
>>diff -c -r2.195.2.105 ip_nat.c
>>*** ip_nat.c    21 Dec 2007 23:03:24 -0000      2.195.2.105
>>--- ip_nat.c    8 Feb 2008 02:27:04 -0000
>>***************
>>*** 3804,3809 ****
>>--- 3804,3813 ----
>>                 * If there is no current entry in the nat table for 
>>this IP#,
>>                 * create one for it (if there is a matching rule).
>>                 */
>>+               if ((fin->fin_off != 0) && (fin->fin_flx & FI_TCPUDP)) {
>>+                       natfailed = -1;
>>+                       goto nonatfrag;
>>+               }
>>                RWLOCK_EXIT(&ipf_nat);
>>                msk = 0xffffffff;
>>                nmsk = nat_masks;
>>***************
>>*** 3861,3866 ****
>>--- 3865,3871 ----
>>                MUTEX_DOWNGRADE(&ipf_nat);
>>        }
>> 
>>+ nonatfrag:
>>        if (nat != NULL) {
>>                rval = fr_natout(fin, nat, natadd, nflags);
>>                if (rval == 1) {
>>***************
>>*** 4095,4100 ****
>>--- 4100,4109 ----
>>        } else {
>>                u_32_t hv, msk, rmsk;
>> 
>>+               if ((fin->fin_off != 0) && (fin->fin_flx & FI_TCPUDP)) {
>>+                       natfailed = -1;
>>+                       goto nonatfrag;
>>+               }
>>                RWLOCK_EXIT(&ipf_nat);
>>                rmsk = rdr_masks;
>>                msk = 0xffffffff;
>>***************
>>*** 4155,4160 ****
>>--- 4164,4171 ----
>>                }
>>                MUTEX_DOWNGRADE(&ipf_nat);
>>        }
>>+
>>+ nonatfrag:
>>        if (nat != NULL) {
>>                rval = fr_natin(fin, nat, natadd, nflags);
>>                if (rval == 1) {
>>
>>
> 
> Hi Darren,
> 
> Is it possible to make the above patch more selective in the frags it drops? Meaning it drops
> frags even if there is no matching ipnat rule. This allows the initial fragment to pass then all succeeding
> fragments get dropped.
> 
> The ideal situation would be to drop any fragment after the first if there was not already an entry in the
> nat table and it matched a nat rule.
> 
> Regards,
> Steve
> 
> 
Attached is my patch against 4.1.28 to be resolve the above issue.

--- ip_nat.c.orig	Thu May 22 07:59:30 2008
+++ ip_nat.c	Thu May 22 14:41:56 2008
@@ -3787,10 +3787,12 @@
 		 * If there is no current entry in the nat table for this IP#,
 		 * create one for it (if there is a matching rule).
 		 */
+#if 0
         if ((fin->fin_off != 0) && (fin->fin_flx & FI_TCPUDP)) {
             natfailed = -1;
             goto nonatfrag;
         }
+#endif
 		RWLOCK_EXIT(&ipf_nat);
 		msk = 0xffffffff;
 		nmsk = nat_masks;
@@ -3826,7 +3828,7 @@
 					continue;
 			}
 
-			if ((nat = nat_new(fin, np, NULL, nflags,
+			if ((fin->fin_off == 0) && (nat = nat_new(fin, np, NULL, nflags,
 					   NAT_OUTBOUND))) {
 				np->in_hits++;
 				break;
@@ -3847,7 +3849,7 @@
 		}
 		MUTEX_DOWNGRADE(&ipf_nat);
 	}
- nonatfrag:
+ /*nonatfrag:*/
 	if (nat != NULL) {
 		rval = fr_natout(fin, nat, natadd, nflags);
 		if (rval == 1) {
@@ -4081,11 +4083,12 @@
 		nflags = nat->nat_flags;
 	} else {
 		u_32_t hv, msk, rmsk;
+#if 0
         if ((fin->fin_off != 0) && (fin->fin_flx & FI_TCPUDP)) {
             natfailed = -1;
             goto nonatfrag;
         }
-        
+#endif
 		RWLOCK_EXIT(&ipf_nat);
 		rmsk = rdr_masks;
 		msk = 0xffffffff;
@@ -4124,8 +4127,7 @@
 				}
 			}
 
-			nat = nat_new(fin, np, NULL, nflags, NAT_INBOUND);
-			if (nat != NULL) {
+			if ((fin->fin_off == 0) && (nat = nat_new(fin, np, NULL, nflags, NAT_INBOUND))) {
 				np->in_hits++;
 				break;
 			} else
@@ -4146,7 +4148,7 @@
 		}
 		MUTEX_DOWNGRADE(&ipf_nat);
 	}
- nonatfrag:
+ /*nonatfrag:*/
 	if (nat != NULL) {
 		rval = fr_natin(fin, nat, natadd, nflags);
 		if (rval == 1) {