detect-engine-address.c:1140:17: warning: Use of memory after it is freed [unix.Malloc]
r = DetectAddressCmp(ag, ag2);
^~~~~~~~~~~~~~~~~~~~~~~~~
detect-engine-address.c:1169:17: warning: Use of memory after it is freed [unix.Malloc]
r = DetectAddressCmp(ag, ag2);
^~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
detect-engine-port.c:1161:9: warning: Use of memory after it is freed [unix.Malloc]
DetectPortPrint(ag2);
^~~~~~~~~~~~~~~~~~~~
1 warning generated.
Bug: #3150.
Bug: #3151.
Cppcheck flagged this:
src/detect-engine-address.c:1035:48: warning: Either the condition 'ghn!=NULL' is redundant or there is possible null pointer dereference: gh. [nullPointerRedundantCheck]
int r = DetectAddressIsCompleteIPSpaceIPv4(gh->ipv4_head);
^
src/detect-engine-address.c:1297:17: note: Assuming that condition 'ghn!=NULL' is not redundant
if (ghn != NULL) {
^
src/detect-engine-address.c:1283:44: note: Calling function 'DetectAddressIsCompleteIPSpace', 1st argument 'ghn' value is 0
if (DetectAddressIsCompleteIPSpace(ghn)) {
^
src/detect-engine-address.c:1035:48: note: Null pointer dereference
int r = DetectAddressIsCompleteIPSpaceIPv4(gh->ipv4_head);
^
Cleanup code could only be reached with non-NULL pointers, so simplify checks.
Bug: #5291.
Only accept netmask in dotted quad notation if they can be turned
into a CIDR.
According to rfc 4632, CIDR (compat) netmasks are all that should be
used.
Bug: #5168.
If the ipaddress was not the address range start, it was not masked to turn
it into that. So 1.2.3.4/24 was not stored as address 1.2.3.0 with netmask 24,
but as 1.2.3.4 with netmask 24. This was then propagated into the radix tree,
where it was used as an exact key in exact lookups, giving unexpected results.
This patch implements the netmask handling for IPv4 and IPv6, and adds a set
of tests for it.
Bug: #5081.
Bug: #5066.
This commit makes a small optimization when comparing IPv4 and IPv6
addresses by making the host order value invariant and calculating the
value once, before entering the loop.
This commit improves support for large address variables. Without this
commit, address size was fixed at 8196 or less. This commit permits
larger sized address variables.
atoi() and related functions lack a mechanism for reporting errors for
invalid values. Replace them with calls to the appropriate
ByteExtractString* functions.
Partially closes redmine ticket #3053.
Fix address negation detection not resolving variables when
looking for the negation.
This patch makes use of the actual parsing routines to relay this
information to the signature parser.
Bug #3389.
Fixes: 92f08d85aa ("detect/iponly: improve negation handling in parsing")
As an optimization, reset bidirectional flag for rules with same src and dst.
If one created bidirectional rule like 'alert tcp any any <> any any ...',
the rule was checked twice (for each packet in every direction). This is
suboptimal and may give duplicated alerts. To avoid this, bidirectional
rules are now checked for the same src and dst (addresses and ports) and
if it's the case, the rule is treated as unidirectional and a corresponding
message is logged.
On MinGW the result of ntohl needs to be casted to uint32_t and
the result of ntohs to uint16_t. To avoid doing this everywhere
add SCNtohl and SCNtohs macros.
There is a memory-leak in DetectAddressTestConfVars. If the programm takes the "goto error"-path, the pointers gh and ghn will not be freed. This commit fixes bug #2345. Here is the ASAN-output:
=================================================================
ERROR: LeakSanitizer: detected memory leaks
Direct leak of 24 byte(s) in 1 object(s) allocated from:
0 0x7f4347cb1d28 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.3+0xc1d28)
1 0x55fe1fc8dcfc in DetectAddressHeadInit /root/suricata-1/src/detect-engine-address.c:1534
2 0x55fe1fc8c50a in DetectAddressTestConfVars /root/suricata-1/src/detect-engine-address.c:1306
3 0x55fe1ff356bd in PostConfLoadedSetup /root/suricata-1/src/suricata.c:2696
4 0x55fe1ff365eb in main /root/suricata-1/src/suricata.c:2884
5 0x7f43443892b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
Direct leak of 24 byte(s) in 1 object(s) allocated from:
0 0x7f4347cb1d28 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.3+0xc1d28)
1 0x55fe1fc8dcfc in DetectAddressHeadInit /root/suricata-1/src/detect-engine-address.c:1534
2 0x55fe1fc8c524 in DetectAddressTestConfVars /root/suricata-1/src/detect-engine-address.c:1310
3 0x55fe1ff356bd in PostConfLoadedSetup /root/suricata-1/src/suricata.c:2696
4 0x55fe1ff365eb in main /root/suricata-1/src/suricata.c:2884
5 0x7f43443892b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
SUMMARY: AddressSanitizer: 48 byte(s) leaked in 2 allocation(s).
Set flags by default:
-Wmissing-prototypes
-Wmissing-declarations
-Wstrict-prototypes
-Wwrite-strings
-Wcast-align
-Wbad-function-cast
-Wformat-security
-Wno-format-nonliteral
-Wmissing-format-attribute
-funsigned-char
Fix minor compiler warnings for these new flags on gcc and clang.
Some examples from wiki caused parsing errors.
For example, "[1:80,![2,4]]" was treated as a mistake.
Also fixed loop detection in variables declaration. For example,
'A: "HOME_NET, !$HOME_NET"' resulted in parsing error.
Fix rate_filter issues: if action was modified it wouldn't be logged
in EVE. To address this pass the PacketAlert structure to the threshold
code so it can flag the PacketAlert as modified. Use this in logging.
Update API to use const where possible. Fix a timout issue that this
uncovered.
Many rules have the same address vars, so instead of parsing them
each time use a hash to store the string and the parsed result.
Rules now reference the stored result in the hash table.
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.
Victor Julien
Philippe Antoine
Jeff Lucovsky
Victor Julien
Shivani Bhardwaj
Alexander Gozman
Wolfgang Hotwagner
Arturo Borrero Gonzalez
Jason Ish
bmeeks8
Ken Steele