Suricata crashed when variable (either address or port)
referred to itself or if one created a looped chain of
variables. For instance:
HOME_NET: "!$EXTERNAL_NET"
EXTERNAL_NET: "!$HOME_NET"
Or:
Var1: "$Var2"
Var2: "$Var3"
Var3: "$Var1"
I found three somewhat serious IPv6 address bugs within the Suricata 2.0.x source code. Two are in the source module "detect-engine-address.c", and the third is in "util-radix-tree.c".
The first bug occurs within the function DetectAddressParse2(). When parsing an address string and a negated block is encountered (such as when parsing !$HOME_NET, for example), any corresponding IPv6 addresses were not getting added to the Group Heads in the DetectAddressList. Only IPv4 addresses were being added.
I discovered another bug related to IPv6 address ranges in the Signature Match Address Array comparison code for IPv6 addresses. The function DetectAddressMatchIPv6() walks a signature's source or destination match address list comparing each to the current packet's corresponding address value. The match address list consists of value pairs representing a lower and upper IP address range. If the packet's address is within that range (including equal to either the lower or upper bound), then a signature match flag is returned.
The original test of each signature match address to the packet was performed using a set of four compounded AND comparisons looking at each of the four 32-bit blocks that comprise an IPv6 address. The problem with the old comparison is that if ANY of the four 32-bit blocks failed the test, then a "no-match" was returned. This is incorrect. If one or more of the more significant 32-bit blocks met the condition, then it is a match no matter if some of the less significant 32-bit blocks did not meet the condition. Consider this example where Packet represents the packet address being checked, and Target represents the upper bound of a match address pair. We are testing if Packet is less than Target.
Packet -- 2001:0470 : 1f07:00e2 : 1988:01f1 : d468:27ab
Target -- 2001:0470 : 1f07:00e2 : a48c:2e52 : d121:101e
In this example the Packet's address is less than the target and it should give a match. However, the old code would compare each 32-bit block (shown spaced out above for clarity) and logically AND the result with the next least significant block comparison. If any of the four blocks failed the comparison, that kicked out the whole address. The flaw is illustrated above. The first two blocks are 2001:0470 and 1f07:00e2 and yield TRUE; the next less significant block is 1988:01f1 and a48c:2e52, and also yields TRUE (that is, Packet is less than Target); but the last block compare is FALSE (d468:27ab is not less than d121:101e). That last block is the least significant block, though, so its FALSE determination should not invalidate a TRUE from any of the more significant blocks. However, in the previous code using the compound logical AND block, that last least significant block would invalidate the tests done with the more significant blocks.
The other bug I found for IPv6 occurs when trying to parse and insert an IPv6 address into a Radix Tree using the function SCRadixAddKeyIPV6String(). The test for min and max values for an IPv6 CIDR mask incorrectly tests the upper limit as 32 when it should be 128 for an IPv6 address. I think this perhaps is an old copy-paste error if the IPv6 version of this function was initially copied from the corresponding IPv4 version directly above it in the code. Without this patch, the function will return null when you attempt to add an IPv6 network whose CIDR mask is larger than 32 (for example, the popular /64 mask will cause the function to return the NULL error condition).
(amended by Victor Julien)
Fix issue where negating a range containing a negation would fail.
E.g. HOME_NET: [192.168.0.0/16,!192.168.10.0], can be used in a rule
as !$HOME_NET.
Also, fix another parsing issue:
If the negation range would be bigger than the 'positive' range, parsing
wouldn't be correct. Now this case is rejected.
E.g. [192.168.1.3,!192.168.0.0/16] is now explicitly rejected
Ticket 1079.
When handling error case on SCMallog, SCCalloc or SCStrdup
we are in an unlikely case. This patch adds the unlikely()
expression to indicate this to gcc.
This patch has been obtained via coccinelle. The transformation
is the following:
@istested@
identifier x;
statement S1;
identifier func =~ "(SCMalloc|SCStrdup|SCCalloc)";
@@
x = func(...)
... when != x
- if (x == NULL) S1
+ if (unlikely(x == NULL)) S1
Arturo Borrero Gonzalez
Jason Ish
Victor Julien
Alexander Gozman
bmeeks8
Ken Steele
Anoop Saldanha
Last G
Eric Leblond
Anoop Saldanha
William Metcalf
Gerardo Iglesias Galvan