Darren Reed wrote:
Oren K. wrote:
Hi,
The 'current version' source downloadable from
http://coombs.anu.edu.au/~avalon/ gives a version of ipfilter in which
by default any packet fragments beyond the first are dropped with BAD-IN
status. I understand this is a result of a fix made due to kernel panic
that was reported here:
http://marc.info/?l=ipfilter&m=121267676118062&w=2
The kernel panic is patched into 4.1.31. However the patch that was to
solve dropping subsequent fragments was only posted in the mailing list
(same post as above, as 'mypatch.txt') but not patched into the trunk. I
applied this patch manually and it solved the problem. My question is,
shouldn't this patch be in the main trunk? It seems to me that having
ipfilter drop packet fragments by default is an undesirable behavior.
So I think you're arguing for something like this...which I'll add now.
Darren
------------------------------------------------------------------------
Index: ip_nat.c
===================================================================
RCS file: /devel/CVS/IP-Filter/ip_nat.c,v
retrieving revision 2.195.2.121
diff -c -r2.195.2.121 ip_nat.c
*** ip_nat.c 6 Nov 2008 21:18:34 -0000 2.195.2.121
--- ip_nat.c 28 Dec 2008 15:06:41 -0000
***************
*** 3811,3826 ****
} else {
u_32_t hv, msk, nmsk;
/*
* 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;
- }
- msk = 0xffffffff;
- nmsk = nat_masks;
maskloop:
iph = ipa & htonl(msk);
hv = NAT_HASH_FN(iph, 0, ipf_natrules_sz);
--- 3811,3825 ----
} else {
u_32_t hv, msk, nmsk;
+ if ((fin->fin_off != 0) && (fin->fin_flx & FI_TCPUDP))
+ goto nonatfrag;
+
+ msk = 0xffffffff;
+ nmsk = nat_masks;
/*
* If there is no current entry in the nat table for this IP#,
* create one for it (if there is a matching rule).
*/
maskloop:
iph = ipa & htonl(msk);
hv = NAT_HASH_FN(iph, 0, ipf_natrules_sz);
***************
*** 4107,4116 ****
} else {
u_32_t hv, msk, rmsk;
! if ((fin->fin_off != 0) && (fin->fin_flx & FI_TCPUDP)) {
! natfailed = -1;
goto nonatfrag;
! }
rmsk = rdr_masks;
msk = 0xffffffff;
/*
--- 4106,4114 ----
} else {
u_32_t hv, msk, rmsk;
! if ((fin->fin_off != 0) && (fin->fin_flx & FI_TCPUDP))
goto nonatfrag;
!
rmsk = rdr_masks;
msk = 0xffffffff;
/*
Hello Darren,
What I found as the best way to implement the patch was in fr_checknatin() and
in fr_checknatout()
was to make the following change after removing your original patch of:
! if ((fin->fin_off != 0) && (fin->fin_flx & FI_TCPUDP))
goto nonatfrag;
!
etc.
Was to only do the nat_new if this was fin_off was zero.
if ((fin->fin_off == 0) && (nat = nat_new(fin, np, NULL, nflags,
NAT_INBOUND))) {
np->in_hits++;
break;
} else
natfailed = -1;
}
attached is my patch against 4.1.28 which has been working flawlessly at over 400 sites for
the past 6 months.
Steve
*** ip_nat.c.orig Thu Feb 7 11:46:47 2008
--- ip_nat.c Thu May 22 11:55:27 2008
***************
*** 2571,2587 ****
nat->nat_p = fin->fin_p;
nat->nat_mssclamp = np->in_mssclamp;
if (nat->nat_p == IPPROTO_TCP)
! {
! if(tcp != NULL)
! {
! nat->nat_seqnext[0] = ntohl(tcp->th_seq);
! }
! else
! {
! printf("IP Filter: nat_finalise() - bad fragment\n");
! return -1;
! }
! }
if ((np->in_apr != NULL) && ((ni->nai_flags & NAT_SLAVE) == 0))
if (appr_new(fin, nat) == -1)
--- 2571,2577 ----
nat->nat_p = fin->fin_p;
nat->nat_mssclamp = np->in_mssclamp;
if (nat->nat_p == IPPROTO_TCP)
! nat->nat_seqnext[0] = ntohl(tcp->th_seq);
if ((np->in_apr != NULL) && ((ni->nai_flags & NAT_SLAVE) == 0))
if (appr_new(fin, nat) == -1)
***************
*** 3797,3802 ****
--- 3787,3798 ----
* 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;
***************
*** 3804,3813 ****
maskloop:
iph = ipa & htonl(msk);
hv = NAT_HASH_FN(iph, 0, ipf_natrules_sz);
- if ((fin->fin_off != 0) && (fin->fin_flx & FI_TCPUDP)) {
- natfailed = -1;
- goto nonatfrag;
- }
for (np = nat_rules[hv]; np; np = np->in_mnext)
{
if ((np->in_ifps[1] && (np->in_ifps[1] != ifp)))
--- 3800,3805 ----
***************
*** 3836,3842 ****
continue;
}
! if ((nat = nat_new(fin, np, NULL, nflags,
NAT_OUTBOUND))) {
np->in_hits++;
break;
--- 3828,3834 ----
continue;
}
! if ((fin->fin_off == 0) && (nat = nat_new(fin, np,
NULL, nflags,
NAT_OUTBOUND))) {
np->in_hits++;
break;
***************
*** 3857,3863 ****
}
MUTEX_DOWNGRADE(&ipf_nat);
}
!
if (nat != NULL) {
rval = fr_natout(fin, nat, natadd, nflags);
if (rval == 1) {
--- 3849,3855 ----
}
MUTEX_DOWNGRADE(&ipf_nat);
}
! /*nonatfrag:*/
if (nat != NULL) {
rval = fr_natout(fin, nat, natadd, nflags);
if (rval == 1) {
***************
*** 3865,3871 ****
nat->nat_ref++;
MUTEX_EXIT(&nat->nat_lock);
nat->nat_touched = fr_ticks;
- nonatfrag:
fin->fin_nat = nat;
}
} else
--- 3857,3862 ----
***************
*** 4092,4098 ****
nflags = nat->nat_flags;
} else {
u_32_t hv, msk, rmsk;
!
RWLOCK_EXIT(&ipf_nat);
rmsk = rdr_masks;
msk = 0xffffffff;
--- 4083,4094 ----
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;
***************
*** 4100,4109 ****
/*
* 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;
- }
*/
maskloop:
iph = in.s_addr & htonl(msk);
--- 4096,4101 ----
***************
*** 4135,4142 ****
}
}
! nat = nat_new(fin, np, NULL, nflags, NAT_INBOUND);
! if (nat != NULL) {
np->in_hits++;
break;
} else
--- 4127,4133 ----
}
}
! if ((fin->fin_off == 0) && (nat = nat_new(fin, np,
NULL, nflags, NAT_INBOUND))) {
np->in_hits++;
break;
} else
***************
*** 4157,4162 ****
--- 4148,4154 ----
}
MUTEX_DOWNGRADE(&ipf_nat);
}
+ /*nonatfrag:*/
if (nat != NULL) {
rval = fr_natin(fin, nat, natadd, nflags);
if (rval == 1) {
***************
*** 4164,4170 ****
nat->nat_ref++;
MUTEX_EXIT(&nat->nat_lock);
nat->nat_touched = fr_ticks;
- nonatfrag:
fin->fin_nat = nat;
}
} else
--- 4156,4161 ----
