ipfilter / Bugs / #1 ipfilter accepts packets with invalid TCP flag combinations

Rhonda - 2007-04-02

ethereal trace: saved as libpcap (tcpdump, ethereal, etc.)

Invalid-TCP-flag-trace

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Darren - 2007-04-02

Logged In: YES
user_id=1448875
Originator: NO

Try adding this rule at the very front:
block in quick log all with bad

The recommended manner for dealing with this would be:
block in quick proto tcp all flags SP/SP
block in quick proto tcp all flags SU/SU

The normal way people do filtering here is with:
pass in quick on <if> proto tcp from any to any port = 22 flags S keep state

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Darren - 2007-04-02

Logged In: YES
user_id=1448875
Originator: NO

A sequence number of "0" is not an invalid TCP sequence number and the sequence number can be 0 if TH_SYN is set. TH_SYN is an indication of the connection being opened, not whether or not the sequence number field is populated - it is always populated, even if it is "0".

This patch will flag SYN+PSH or SYN+URG as being "bad".

Index: fil.c

RCS file: /devel/CVS/IP-Filter/fil.c,v
retrieving revision 2.243.2.99
diff -c -r2.243.2.99 fil.c
*** fil.c 17 Feb 2007 12:41:41 -0000 2.243.2.99
--- fil.c 2 Apr 2007 00:58:44 -0000
***************
*** 1136,1141 ****
--- 1136,1145 ----
} else if ((flags & (TH_URG|TH_PUSH|TH_FIN)) != 0) {
fin->fin_flx |= FI_BAD;
}
+ } else if ((flags & TH_SYN) != 0) {
+ if ((flags & (TH_URG|TH_PSH)) != 0) {
+ fin->fin_flx |= FI_BAD;
+ }
}

/*

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Rhonda - 2007-04-03

Logged In: YES
user_id=1759249
Originator: YES

i'll try the patch this week - thanks!!!

i copied in most of the relevant info from the ethereal trace - not all of the info typed in for the description is "wrong", just there for a quick glance without having to open the ethereal trace. the Seq and Ack values are fine - it's the flag combinations that are a problem.

and i realize that 'keep state' could solve the problem, however i spent about a month in 2005 trying to get it work, and not only did no packet ever match an open state but it also used *far* more system resources than i have available for the filtering stage of packet processing. thus it's not an option at this point.

i expected that there is a way to write flag-based rules to block invalid flag combinations, but as i said in the original description and email: it seems to me that any *invalid* flag combination should already be blocked in the code, and that i should only need to write rules to deal with valid flag combinations...

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Rhonda - 2007-04-03

Logged In: YES
user_id=1759249
Originator: YES

looking at the code, there are two issues with this patch:

1) the second clause of the if stmt this is appended to will preclude detecting bad flag combinations if the TH_URG is not set and the urg pointer is non-zero. any further validation of TCP flags is skipped on such packets. (line 1109 in fil.c, ipfilter version 4.1.19)
in my version of the file, i have made the following change:
} else if ((flags & TH_URG) == 0 && (tcp->th_urp != 0)) {
/* Ignore this case, it shows up in "real" traffic with */
/* bogus values in the urgent pointer field. */
;
- } else if (((flags & (TH_SYN|TH_FIN)) != 0) &&
+ }
+
+ /* LU fix:
+ * Separate the rest of the flag validation from the URG check.
+ * otherwise, the 2nd URG check case above can cause us to
+ * accept packets that are bad in other ways */
+ if (((flags & (TH_SYN|TH_FIN)) != 0) &&
((flags & (TH_RST|TH_ACK)) == TH_RST)) {

2) the two specific problematic packets have TCP flags SP and SU. the if stmt in the code that the patch is appended will always take the preceding clause for these two kinds of packets, so the patch will never execute.
here's the code that the patch is appended to:
} else if (!(flags & TH_ACK)) {
/*
* If the ack bit isn't set, then either the SYN or
* RST bit must be set. If the SYN bit is set, then
* we expect the ACK field to be 0. If the ACK is
* not set and if URG, PSH or FIN are set, consdier
* that to indicate a bad TCP packet.
*/
if ((flags == TH_SYN) && (tcp->th_ack != 0)) {
/*
* Cisco PIX sets the ACK field to a random value.
* In light of this, do not set FI_BAD until a patch
* is available from Cisco to ensure that
* interoperability between existing systems is
* achieved.
*/
/*fin->fin_flx |= FI_BAD*/;
} else if (!(flags & (TH_RST|TH_SYN))) {
fin->fin_flx |= FI_BAD;
} else if ((flags & (TH_URG|TH_PUSH|TH_FIN)) != 0) {
fin->fin_flx |= FI_BAD;
}
}
since the ACK flag is not set on these packets, this clause should fire instead of the patch clause.
on the other hand, this clause *ought* to be preventing these packets already, and it isn't. so i'm going to try putting the patch in before this clause instead of after it and see if that works...

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Darren - 2007-04-05

Logged In: YES
user_id=1448875
Originator: NO

After reading your comments, I had a look at the code again and
redid the patch (see below.)

Index: fil.c

RCS file: /devel/CVS/IP-Filter/fil.c,v
retrieving revision 2.359
diff -c -r2.359 fil.c
*** fil.c 2 Apr 2007 14:15:56 -0000 2.359
--- fil.c 5 Apr 2007 01:22:24 -0000
***************
*** 1172,1185 ****
*/
if ((flags & TH_URG) != 0 && (tcp->th_urp == 0)) {
fin->fin_flx |= FI_BAD;
} else if ((flags & TH_URG) == 0 && (tcp->th_urp != 0)) {
! /* Ignore this case, it shows up in "real" traffic with */
! /* bogus values in the urgent pointer field. */
! ;
} else if (((flags & (TH_SYN|TH_FIN)) != 0) &&
((flags & (TH_RST|TH_ACK)) == TH_RST)) {
/* TH_FIN|TH_RST|TH_ACK seems to appear "naturally" */
fin->fin_flx |= FI_BAD;
} else if (!(flags & TH_ACK)) {
/*
* If the ack bit isn't set, then either the SYN or
*/
if ((flags & TH_URG) != 0 && (tcp->th_urp == 0)) {
fin->fin_flx |= FI_BAD;
+ #if 0
} else if ((flags & TH_URG) == 0 && (tcp->th_urp != 0)) {
! /*
! * Ignore this case (#if 0) as it shows up in "real"
! * traffic with bogus values in the urgent pointer field.
! */
! fin->fin_flx |= FI_BAD;
! #endif
} else if (((flags & (TH_SYN|TH_FIN)) != 0) &&
((flags & (TH_RST|TH_ACK)) == TH_RST)) {
/* TH_FIN|TH_RST|TH_ACK seems to appear "naturally" */
fin->fin_flx |= FI_BAD;
+ #if 1
+ } else if (((flags & TH_SYN) != 0) &&
+ ((flags & (TH_URG|TH_PUSH)) != 0)) {
+ /*
+ * SYN with URG and PUSH set is not for normal TCP but it is
+ * possible(?) with T/TCP...but who uses T/TCP?
+ */
+ fin->fin_flx |= FI_BAD;
+ #endif
} else if (!(flags & TH_ACK)) {
/*
* If the ack bit isn't set, then either the SYN or
***************
*** 1202,1211 ****
} else if ((flags & (TH_URG|TH_PUSH|TH_FIN)) != 0) {
fin->fin_flx |= FI_BAD;
}
- } else if ((flags & TH_SYN) != 0) {
- if ((flags & (TH_URG|TH_PSH)) != 0) {
- fin->fin_flx |= FI_BAD;
- }
}

/*
--- 1215,1220 ----

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Rhonda - 2007-04-06

Logged In: YES
user_id=1759249
Originator: YES

thanks for the newer version of the patch!! i'll apply this one as-is :-), and hopefully i can get it tested next week. (our sqa folks have been really busy for the last couple of weeks)

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Rhonda - 2007-04-09

Logged In: YES
user_id=1759249
Originator: YES

i got the test result today, and the patch does not fix the problem. ipfilter *seems* to ignore the FI_BAD result if the packet otherwise matches a rule. i can't be sure whether that's the real problem - i looked at the code a couple of years ago and couldn't find where the result of TCP flag validation is applied. probably i just missed it. but since it appeared that the existing code should've caught the bad combination, and this patch would *definitely* catch it, perhaps the problem is in the use of the info rather than the detection of the problem?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Darren - 2007-04-10

Logged In: YES
user_id=1448875
Originator: NO

You must include a specific rule to block bad packets.
Or you can do it like this:

pass in quick on <if> proto tcp/udp from any to any port = 22 with not bad

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Darren - 2007-04-10

status: open --> pending
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Rhonda - 2007-04-10

status: pending --> open
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Rhonda - 2007-04-10

Logged In: YES
user_id=1759249
Originator: YES

ok - thanks for the final answer!
i still think it's a bug to have to specifically allow only good packets, but i can see the blocking of bad packets being described as a feature...

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Darren - 2007-04-10

assigned_to: nobody --> darren_r

labels: --> packet filtering
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Darren - 2007-08-04

milestone: --> 4.1.*

priority: 5 --> 1

status: open --> open-wont-fix
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

ipfilter accepts packets with invalid TCP flag combinations

Group

Searches

Help

#1 ipfilter accepts packets with invalid TCP flag combinations

Discussion

Index: fil.c

Index: fil.c