Linux is slightly more permissive wrt timestamps than many
other OS'. To avoid many events/issues with linux hosts, add an
option to allow for this slightly more permissive behavior.
Ideally the host-os config would be used, but in practice this
setting is rarely set up correctly, if at all.
This option is enabled by default.
(cherry picked from commit 01b7ccc224)
RFC 2883 specifies a special use of SACKs to indicate a host has
received a segment it considers a spurious retransmission.
(cherry picked from commit d79a926085)
On every accepted packet in established state, update next_seq if
packet seq+len is larger than existing next_seq. This allows it to
catch up after large gaps that are filled again a bit later.
Bug: #5877.
(cherry picked from commit 76225bf9ac)
An ACK that ACK'd older data while still being in-window could
lead to FIN_WAIT1 to FIN_WAIT2 state transition. Detect this
case and generally harden the check.
Bug: #5877.
(cherry picked from commit 67af94f2e0)
If a in-tree segment was partly before base_seq the overlap detection
miscalculated the data offset. This lead to memcmp comparing the wrong
data.
Bug: #5881.
(cherry picked from commit cbcd70537a)
When a "stream starter" packet finds an existing TCP flow, the flow will be
evaluated for reuse.
The following scenario wasn't handled well:
1. Suricata starts after a tool has just stopped using lots of connections
(e.g. ab stress testing a webserver)
2. even though the client is closed already, the server is still doing
connection cleanup sending many FINs and later RSTs
3. Suricata creates flows for these packets, but no TCP sessions
4. client resumes testing, creating flows that have the same 5 tuple as the
flows created for the FIN/RST packets
5. Suricata refuses to "reuse" the flows as the condition "tcp flow w/o session"
is not considered valid for session reuse
6. new TCP connection is not properly tracked and evaluated in parsing and
detection
There may be other vectors into this, like a flow w/o session because of
memcap issues.
Bug: #5843.
(cherry picked from commit 93c9594dd8)
Fix payload_len calculation post removal of the condition that returned
error code if the length to the decode fn did not match the length of
header from the UDP packet.
Bug 5379
(cherry picked from commit f941ceae2b)
If the packet is shorter than IP payload length we no longer flag it as an
invalid UDP packet. UDP packet can be therefore shorter than IP payload.
Keyword "udp.hlen_invalid" became outdated as we no longer flag short UDP
packets as invalid. The keyword's evaluation remains the same.
Inspired by commit: d18e52ed93
Redmine ticket: #5793
When the keyword config:logging disable,type tx is used,
OutputTxLog checks a flag to skip the transaction without logging
it, but AppLayerParserTransactionsCleanup waits for the
transaction to be marked as logged to clean it.
So, OutputTxLog now marks the tx as logged, so that it can
get cleaned away.
Ticket: #5456
(cherry picked from commit 0c0fcc9411)
Set the engine to ignore the stream.midstream-policy if stream.midstream
is enabled.
If we had both stream.midstream AND stream.midstream_policy enabled,
this could lead to midstream flows being dropped (or bypassed, or...)
instead of being accepted by the engine, as it was probably meant when
the user enabled midstream flows.
Bug #5765
(cherry picked from commit 8e9905e0d8)
cf https://redmine.openinfosecfoundation.org/issues/4125
This allows fuzz_applayerparser_parse to fuzz one specific
app-layer protocol based on the binary name, as is done
with the environment variable FUZZ_APPLAYER
That is if we rename/copy to fuzz_applayerparser_parse_smb,
it will fuzz only SMB protocol
This way, we can easily produce different fuzz targets for
each protocol in oss-fuzz
(cherry picked from commit e9b76a0e66)
This commit reduces the changes associated with adding the v14 api to
6.0.x
During the preparation of this commit, issues in the original backport
were corrected
- Failure to release a lock under error conditions
- Typo in an CPP ifdef
- Incorrect target for goto statement in an error handling case.
Issue: 5744
The unit test for content |aa bz| transforms in place the string
str to replace the 2 characters aa by one character 0xaa
Then, when z is not recognized as a valid hexadeicmal character,
the whole modified string is printed out, inclusing the non-ascii
0xaa
Ticket: #5558
(cherry picked from commit b281199e9a)
Tested on Fedora 37 with clang 15.
util-strlcatu.c:45:8: error: a function definition without a prototype is deprecated in all versions of C and is not supported in C2x [-Werror,-Wdeprecated-non-prototype]
size_t strlcat(dst, src, siz)
^
1 error generated.
(cherry picked from commit 41ca206fdf)
Tested on Fedora 37 with clang 15.
datasets.c:852:9: error: variable 'n' set but not used [-Werror,-Wunused-but-set-variable]
int n = 0;
^
1 error generated.
(cherry picked from commit 1bc6976a06)
Tested on Fedora 37 with clang 15.
app-layer.c:1055:27: error: a function declaration without a prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
void AppLayerSetupCounters()
^
void
app-layer.c:1176:29: error: a function declaration without a prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
void AppLayerDeSetupCounters()
^
void
2 errors generated.
(cherry picked from commit ebd8728219)
In the case port_line is first allocated and port_line_len is set,
Then a second request reaches memcap and frees port_line,
port_line_len should also be reset, because both will get used
by the response parsing.
Ticket: #5701
(cherry picked from commit 1660172a8b)
implicit conversion loses integer precision: 'int' to 'uint16_t'
because C shifts << translates automatically to signed integers
(cherry picked from commit ae6abd8ca3)
Make sure to exit the parser early on incomplete header data.
Additionally, make sure to not create duplicated tls frames in this
case.
Add a debug validation check for the header parser parsing too much
data, which should never happen.
(cherry picked from commit 1701a6b14c)
Improve unknown record handling. Inspired by Wireshark 'unknown record'
handling, we take a best effort approach for records with unknown content
types in TLS versions 1.0, 1.1 and 1.2.
Improve record length check and set 'invalid_record_length' event instead
of 'invalid_tls_header'.
(cherry picked from commit 69be41b241)
These tests are incompatible with the incomplete API usage and should
have been pcap based tests in the first place.
(cherry picked from commit 214e466b29)
TCP Buffering is now done in the app-layer using the incomplete API, on
the SSL/TLS record level. TLS level fragmentation will be implemented
separately.
(cherry picked from commit bcaf0f6f7d)
To avoid overhead of stream buffering for records we don't do
much with anyway, pass through application records instead of
buffering the entire record in the stream engine.
(cherry picked from commit 6076a51511)
The TLS record header is parsed in streaming mode still, but once the
record size is known we tell the app-layer API to give us the full
record.
Ticket: #5481
(cherry picked from commit 129fcb5c72)
Issue: 4581
This commit improves the runtime performance of rules with
tls.fingerprint by using the inspection logic from tls.cert_fingerprint.
(cherry picked from commit 63745a7879)
The check meant to see if the ip address part of the ip/cidr combo
was more specific than needed wasn't fully implemented, leading to
warnings being issued on completely valid and correct input.
This patch implements the same logic as in IPv4. If the ip address
as specified is different from the ip after the mask has been applied,
a warning is displayed.
Bug: #5747.
(cherry picked from commit 991f9fde32)
While updating the destination pointer, we were also adding the padded
bytes which are not a part of the decoded bytes. This led to running out
of space on the destination buffer.
Fix it by only incrementing destination buffer ptr by the number of
actual bytes that were decoded.
Ticket 5623
Ticket 5694
As per RFC 4648,
Implementations MUST reject the encoded data if it contains characters
outside the base alphabet when interpreting base-encoded data, unless
the specification referring to this document explicitly states
otherwise.
Add a new mode BASE64_MODE_RFC4648, and handle input strictly as per the
specification.
Bug 5223
(cherry picked from commit dad52f133d)
This is to make sure the files buffers are properly managed even
when there are no rules or when there are no file.data rules.
Bug: #5703.
(cherry picked from commit e601ebdfd8)
Fix PASS handling by setting and checking in the correct packet.
There are 3 types of packets:
1. tunnel packets (inner layer of encapsulation)
2. "root" packets (outmost layer of encapsulated packet)
3. normal packets (no encapsulation)
Tunnel packet have a pointer to their "root". The "root" is the packet
that is ultimately used by the capture method to issue a verdict:
DROP or ACCEPT (forward).
For tunnels:
DROP actions are always issued on the root packet.
The PASS action is issued on the packet currently in the detection
engine.
Non-tunnels:
DROP and PASS are both set in the current packet.
Bug: #5697.
If a protocol change was requested, the target did not handle
it as Suricata, as the target is meant to handle only one
app-layer protocol.
(cherry picked from commit ca054f7264)
Suricata already logs if AF_PACKET or Netmap are running in a mixed IPS
and IDS/TAP mode. As the behavior is undefined when these modes are
mixed, it is best to deprecate and to not allow this behavior. For now
warn that it will be unsupported and fail in Suricata 8.
Ticket: 5587
(cherry picked from commit 0c00f28ebc)
This commit modifies the Netmap packet handling to use API version 14.
@bmeeks8 contributed many changes instrumental to this effort.
(cherry picked from commit ca7d097225)
This commit adds another system include file based on autoconf to the
common Suricata include file for convenience.
(cherry picked from commit 3496e543af)
`PacketDrop` applied a DROP to the current packet unconditionally,
while in tunnel/encapsulated cases the DROP should have been applied
to the root packet.
Fixes: 6742ecbc9e ("decode: make PacketDrop use action as parameter")
Bug: #5600.
As is done in detect-threshold.c or in DETECT_RATE_REGEX
and is expected by switch (rule_type) which makes the same
for THRESHOLD_TYPE_THRESHOLD and THRESHOLD_TYPE_RATE
Ticket: #5327
(cherry picked from commit a2f857ed90)
For ICMPv4 error messages the vlan ids were always considered,
even if the 'vlan.use-for-tracking' option was disabled.
Ticket: #5330
(cherry picked from commit ce2775d331)
Before 6.0.6 if hex content was incomplete, Suricata didn't error out.
With 6.0.6 incomplete hex was detected and errored on which is a
breaking change in a release branch. Instead, only emit a warning
unless strict content checking has been requested.
To enable strict behaviour on incomplete content hex in a rule,
"--strict-rule-keywords=content" can be used on the command line.
Issue: #5546
Add a new setup return code, -4 which will print the error, but not
error out in test mode. This is to support allowing incomplete hex, as
there is no context as to which rule had the issue.
For context:
- -3: error ok, no message
- -2: error not ok, no message
- -4: error ok, message
For protocols with multi buffer inspection there could be multiple times
the same sid would be queued into the candidates queue. This triggered
a debug validation check.
W/o debug validation this would lead to duplicate work and possibly multiple
alerts where a single one would be appropriate.
Bug: 5419.
(cherry picked from commit 1bff888947)
This enables the usage of 'reject' as an exception policy. As for both
IPS and IDS modes the intended result of sending a reject packet is to
reject the related flow, this will effectively mean setting the reject
action to the packet that triggered the exception condition, and then
dropping the associated flow.
Task #5503
(cherry picked from commit bbd968c738)
This allows to set a midstream-policy that can:
- fail closed (stream.midstream-policy=drop-flow)
- fail open (stream.midstream-policy=pass-flow)
- bypass stream (stream.midstream-policy=bypass)
- do nothing (default behavior)
Usage and behavior:
If stream.midstream-policy is set then if Suricata identifies a midstream flow
it will apply the corresponding action associated with the policy.
No setting means Suricata will not apply such policies, either inspecting the
flow (if stream.midstream=true) or ignoring it stream.midstream=false.
Task #5468
(cherry picked from commit aa5bb2c329)
Make sure that when the policy is to drop the flow, we set no inspection
for payload and packet and disable applayer inspection as well.
Task #5468
(cherry picked from commit 242b8f7d65)
Before, if an invalid value was passed as exception policy, Suricata
would log a warning and set the exception policy to "ignore". This is a
very different result, than, say, dropping or bypassing a midstream flow.
Task #5504
(cherry picked from commit 58ef3cde7a)
Our unittests were only covering sig parsing for alert actions. As in
environments without LibNet the reject action will not work, we must
ensure that our parser properly fails in such cases, instead of silently
accepting an unsupported action.
Added tests for the reject and drop action.
Task #5496
(cherry picked from commit c81b78fd1c)
For testing purposes. Meant to simulate a reallocation failure when
dynamically growing the alert queue in DetectEngineThreadCtx, so we can
check that Suri's behavior doesn't break under such circumstances.
Task #5319
(cherry picked from commit 58928b249d)
Add unittests to check that packet flags are correctly updated after
detection finds drop or reject rules that match.
Related to
Bug #5458
(cherry picked from commit f897761ecb)
Bug 5458 states that the reject action is no longer working. While SV
tests that use the reject action still pass, it indeed seems that a
regression has happened with commit aa93984, because while the
function that applies rule actions to the flow (RuleActionToFlow) does
check for the reject action, the newly added function PacketApply
SignatureActions only checks for ACTION_DROP or ACTION_PASS when
deciding to call RuleActionToFlow.
Bug #5458
(cherry picked from commit 1f54e8611a)
A Packet may be dropped due to several different reasons. This change
adds action as a parameter, so we can update the packet action when we
drop it, instead of setting it to drop.
Related to
Bug #5458
(cherry picked from commit 1774ff18a6)
When in live-pcap mode, if the sniffed interface went down and up again,
Suri would enter an infinite and keep running, while not registering new
events. This fixes that behavior by allowing Suri to retry to open the
pcap in case of a retry on an already activated capture
('PCAP_ERROR_ACTIVATED').
This change is based on Zhiyuan Liao's work.
Bug #3846
(cherry picked from commit 2544be4672)
alert_queue_overflow and alerts_suppressed were not being
reinitialized when there was a reload of Suricata rules, leading to
non-valid stats counters if that happened.
Bug #5457
(cherry picked from commit 93c2c9743d)
Ticket: 5243
When switching from SMTP to TLS, and getting HTTP1 instead of
expected TLS, and HTTP1 requesting upgrade to HTTP2, we do not
overwrite the alproto_orig value so as not to have type confusion
in AppLayerParserStateProtoCleanup
(cherry picked from commit cedffdf14c)
Otherwise, if we log a first app_layer_event, then reset
with AppLayerDecoderEventsResetEvents for a new packet,
then get another event, it does not get logged
Ticket: #5391
(cherry picked from commit b0298dd046)
The SMTP parser should not supply lines w/o EOL chars to the mime
parser unless its in the BODY parsing stage. Mimic this in the fuzz
target by testing the state for inputs that have no EOL.
Additionally, make sure the delim cnt reflects the missing EOL.
(cherry picked from commit 816bbeb7dc)
Make sure a new body is not set up on empty lines unless it is
a body that is not encoded as base64/quoted printable.
(cherry picked from commit 41c2c1ed5a)
If the last data of the body was not a multple of 4 and not padded
to be a multiple of 4, it would not be processed.
(cherry picked from commit 0d6ab727c5)
Gap check would consider a GAP when the current data was in fact
exactly not a gap, but next segment(s) were already available.
(cherry picked from commit 100d821a9f)
Mime parser doesn't expect partial lines, which preprocessing can
provide. Add a check to let mime headers be handled by regular line
parsing.
(cherry picked from commit a38f2f2a52)
Lots of tests still use SigMatchSignatures as their main detection
entry function, which bypassed some logic. Make it match main logic
more closely.
(cherry picked from commit e36b9b89a1)
SigTestDropFlow04 was incorrectly expecting an alert in the packet
following a "drop" packet. The first drop is applied to the flow, so
it should lead to the 2nd packet being dropped before inspection is
run.
Clean up the test as well.
(cherry picked from commit 3874d08015)
Adds a framework for setting exception policies. These would be called
when the engine reaches some kind of exception condition, like hitting
a memcap or some traffic processing error.
The policy gives control over what should happen next: drop the packet,
drop the packet and flow, bypass, etc.
Implements the policy for:
stream: If stream session or reassembly memcaps are hit call the
memcap policy on the packet and flow.
flow: Apply policy when memcap is reached and no flow could be
freed up.
defrag: Apply policy when no tracker could be picked up.
app-layer: Apply ppolicy if a parser reaches an error state.
All options default to 'ignore', which means the default behavior
is unchanged.
Adds commandline options: add simulation options for exceptions. These
are only exposed if compiled with `--enable-debug`.
Ticket: #5214.
Ticket: #5215.
Ticket: #5216.
Ticket: #5218.
Ticket: #5194.
(cherry picked from commit 8580499ded)
Enforcing flow drops is now done earlier in the detection engine and
moved out of the IP-only engine where it didn't belong.
(cherry picked from commit 802c1ffee3)
In some situations bypass callback is called on already bypassed
flow. This allocates FlowBypassInfo structure for the flow but
does not check if the flow already has one.
Issue: #5368
(cherry picked from commit 05797c45bb)
AF-Packet bypass function in some situations allocates EBPF bypass data
for an already bypassed flow and assigns it to the flow without any checks
Issue: #5368
(cherry picked from commit 8845c07a90)
Due to the TIMEVAL_DIFF_SEC calculating the delta into an unsigned
integer, it would underflow to a high positive value leading to
and incorrect result if the packet timestamp was below the timestamp
for the threshold entry. In normal conditions this shouldn't happen,
but in offline mode each thread has its own concept of time which
might differ significantly based on the pcap. In this case the
overflow would be very common.
Changing it to a signed value calculation triggered fuzz undefined
behavior if the packet timeval was very high, so this patch takes a
new approach where it no longer calculates a diff but sets up the
"seconds" value we compare against as a timeval itself, and uses
that to compare.
Fixes: 9fafc1031c ("time: Add TIMEVAL_EARLIER and TIMEVAL_DIFF_SEC macros.")
Fixes: 82dc61f4c3 ("detect/threshold: Refactor threshold calculation to handle by_rule and by_both.")
Uses add `timeradd` specific version where available.
Bug: #5386.
(cherry picked from commit df2e408d96)
In the case of midstream SYN/ACK pickup, we reverse the flow based on
the SYN/ACK. If we then later get traffic that appears to be in the
reverse direction based on the app-layer, we would reverse it again.
This isn't correct. When we have the SYN/ACK we know the flow's real
direction.
(cherry picked from commit fea374626a)
Now that the engine understands the relation between SMB and DCERPC better
we can get rid of some of the special case handling in keywords.
(cherry picked from commit 84e26d670e)
So 'alert dcerpc' also matches if the DCERPC is over SMB.
Explicitly refuse smb keywords for the 'dcerpc' app proto setting:
`alert dceprc ... smb.share; ...` is rejected.
Remove a now useless special case in the stateless rule processing
matching for dcerpc/smb.
Bug: #5208.
(cherry picked from commit 7d38f5667d)
For connections that use TCP timestamps for which the first SYN packet
does not reach the server, any replies to retransmitted SYNs will be
tropped.
This is happening in StateSynSentValidateTimestamp, where the timestamp
value in a SYN-ACK packet must match the one from the SYN packet.
However, since the server never received the first SYN packet, it will
respond with an updated timestamp from any of the following SYN packets.
The timestamp value inside suricata is not being updated at any time
which should happen. This patch fixes that problem.
Bug: #4376.
Signed-off-by: Michael Tremer <michael.tremer@ipfire.org>
(cherry picked from commit f50af12068)
When trying to propegate the depth/offset, within/distance chains
a logic error would set too a restrictive depth on a pattern that
followed more than one "unchained" patterns.
Bug: #5162.
(cherry picked from commit 8d20b40cdd)
Use a macro to validate the ranges for overflows. This removes
the clutter of all the checks and warnings, and also no longer
puts the state machine in an undefined state when hitting such
a condition.
(cherry picked from commit 50d02ebc05)
Rules that look like they should be IP-only but contain a negated rule
address are now marked with an LIKE_IPONLY flag. This is so they are
treated like IPONLY rules with respect to flow action, but don't
interfere with other IPONLY processing like using the radix tree.
Ticket: #5361
(cherry picked from commit c8a5207083)
Addresses edge case: > 4 bytes at the end of the input with 2 or more
spaces.
Changes length type for remainder processing to allow for much longer
lines, which can happen in practice.
Adds a series of debug validation checks with real error handling
as well, to assist the fuzzer to find more edge cases.
(cherry picked from commit 30e47b2171)
So far, it was the job of caller to send the bae64 decoder a perfect
block of data and take care of the destination buffer (decoded data)
size. Now, make it the decoder's job to take care of any space
constraints that the destination buffer may have and return accordingly.
Also, handle space characters in base64 encoded data as per RFC 2045.
Update MIME parser accordingly to handle the base64 data.
Ticket: 5315
(cherry picked from commit 5b27619778)
The ideal line terminator for an SMTP line is <CRLF>. But, given that
bare LF is still allowed by many systems despite the prohibition by
standards, we have to consider that. In order to simplify things, we
consider bare CR as line terminators as well while updating the
delimiter parameter correctly if they were to be followed by a LF
immediately or as a part of next fragment.
This takes care of some edge cases that made base64 decoder error out
because unexpected data was sent to it at times.
Ticket: 5316
(cherry picked from commit 1e3282f363)
In the unlikely case of AlertQueueExpand failure, we were incrementing
the discarded alerts stats in AlertQueueAppend via the Packet member in the
DetectEngineThreadCtx, which may not be initialized yet.
Bug #5353
(cherry picked from commit 28ac75b505)
If a packet after the initialization would come with ACK flag set
but a ACK value of 0, the last_ack tracking could get confused. Fix
this by not checking for 0 but instead checking if the ACK flag
has been seen.
Bug: #4549.
(cherry picked from commit 1f43e1477f)
Instead of a loop over the rules in a group *per engine* do a single
loop in which all the engines are prepared in parallel.
(cherry picked from commit 9e6370ae2e)
A lot of time was spent in `SigMatchListSMBelongsTo` for the `mpm_sm`.
Optimize this by keeping the value at hand during Signature parsing and
detection engine setup.
(cherry picked from commit 3352c0bee4)
cppcheck:
src/util-memcmp.h:281:18: warning: Identical condition 'len-offset<16', second condition is always false [identicalConditionAfterEarlyExit]
if (diff < 16) {
^
src/util-memcmp.h:280:24: note: 'diff' is assigned value 'len-offset' here.
int diff = len - offset;
^
src/util-memcmp.h:269:33: note: If condition 'len-offset<16' is true, the function will return/exit
if (likely(len - offset < 16)) {
^
src/util-memcmp.h:281:18: note: Testing identical condition 'len-offset<16'
if (diff < 16) {
^
src/util-memcmp.h:344:18: warning: Identical condition 'len-offset<16', second condition is always false [identicalConditionAfterEarlyExit]
if (diff < 16) {
^
src/util-memcmp.h:343:24: note: 'diff' is assigned value 'len-offset' here.
int diff = len - offset;
^
src/util-memcmp.h:318:33: note: If condition 'len-offset<16' is true, the function will return/exit
if (likely(len - offset < 16)) {
^
src/util-memcmp.h:344:18: note: Testing identical condition 'len-offset<16'
if (diff < 16) {
^
src/util-memcmp.h:171:18: warning: Identical condition 'len-offset<16', second condition is always false [identicalConditionAfterEarlyExit]
if (diff < 16) {
^
src/util-memcmp.h:170:24: note: 'diff' is assigned value 'len-offset' here.
int diff = len - offset;
^
src/util-memcmp.h:159:33: note: If condition 'len-offset<16' is true, the function will return/exit
if (likely(len - offset < 16)) {
^
src/util-memcmp.h:171:18: note: Testing identical condition 'len-offset<16'
if (diff < 16) {
^
src/util-memcmp.h:233:18: warning: Identical condition 'len-offset<16', second condition is always false [identicalConditionAfterEarlyExit]
if (diff < 16) {
^
src/util-memcmp.h:232:24: note: 'diff' is assigned value 'len-offset' here.
int diff = len - offset;
^
src/util-memcmp.h:208:33: note: If condition 'len-offset<16' is true, the function will return/exit
if (likely(len - offset < 16)) {
^
src/util-memcmp.h:233:18: note: Testing identical condition 'len-offset<16'
if (diff < 16) {
^
(cherry picked from commit ca97ed4436)
Since GCC 12 the memcmp code using `_mm_blendv_epi8` failed to work.
Inspection of the disassembled objects suggests that it simply omits
the instruction on systems that are not AVX512 capable. On AVX512
it does replace it with VPCMPB logic that appears to work.
Luckily our use of blend is actually uncessary. A simple AND is sufficient.
Bug: #5312.
(cherry picked from commit 87c5d69437)
kill(0, ...) re-raises the signal to every processes in the process
group which may impact unrelated processes.
Concretely, in our CI pipeline, a segfaulting Suricata process killed
the test driver.
(cherry picked from commit b6407c4253)
Check is always true but confuses cppcheck:
src/log-pcap.c:1224:32: warning: Either the condition 'filename' is redundant or there is possible null pointer dereference: filename. [nullPointerRedundantCheck]
if ((pl->prefix = SCStrdup(filename)) == NULL) {
^
src/log-pcap.c:1421:9: note: Assuming that condition 'filename' is not redundant
if (filename) {
^
src/log-pcap.c:1224:32: note: Null pointer dereference
if ((pl->prefix = SCStrdup(filename)) == NULL) {
^
Bug: #5291.
(cherry picked from commit 3dfbf0bf11)
If the filename has to % sign and if pcap logging is using multi
mode, then the pcap capture will fail. So let's exit if ever this
is the case.
(cherry picked from commit 921d44b262)
cppcheck:
src/util-logopenfile.c:743:13: warning: %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
snprintf(threaded_name, len, "%s.%d.%s", tname, unique_id, ext);
^
src/util-logopenfile.c:752:9: warning: %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
snprintf(threaded_name, len, "%s.%d", original_name, unique_id);
^
Bug: #5291.
(cherry picked from commit 07d0ae04d3)
cppcheck:
src/util-ja3.c:197:28: warning: %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
(*buffer)->used += snprintf((*buffer)->data, (*buffer)->size, "%d",
^
src/util-ja3.c:201:28: warning: %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
(*buffer)->used += snprintf((*buffer)->data + (*buffer)->used,
^
Bug: #5291.
(cherry picked from commit 1e13f72785)
cppcheck:
src/source-af-packet.c:1762:19: warning: Size of pointer 'v2' used instead of size of its data. This is likely to lead to a buffer overflow. You probably intend to write 'sizeof(*v2)'. [pointerSize]
ptv->ring.v2 = SCMalloc(ptv->req.v2.tp_frame_nr * sizeof (union thdr *));
^
src/source-af-packet.c:1767:26: warning: Size of pointer 'v2' used instead of size of its data. This is likely to lead to a buffer overflow. You probably intend to write 'sizeof(*v2)'. [pointerSize]
memset(ptv->ring.v2, 0, ptv->req.v2.tp_frame_nr * sizeof (union thdr *));
^
scan-build:
CC source-af-packet.o
source-af-packet.c:1762:24: warning: Result of 'malloc' is converted to a pointer of type 'char', which is incompatible with sizeof operand type 'union thdr *' [unix.MallocSizeof]
ptv->ring.v2 = SCMalloc(ptv->req.v2.tp_frame_nr * sizeof (union thdr *));
^~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~
./util-mem.h:35:18: note: expanded from macro 'SCMalloc'
^~~~~~
1 warning generated.
Bug: #5291.
(cherry picked from commit fedced209d)
cppcheck:
src/util-time.c:255:18: warning: Either the condition 'str!=NULL' is redundant or there is possible null pointer dereference: str. [nullPointerRedundantCheck]
snprintf(str, size, "ts-error");
^
src/util-time.c:252:48: note: Assuming that condition 'str!=NULL' is not redundant
if (likely(t != NULL && fmt != NULL && str != NULL)) {
^
src/util-time.c:255:18: note: Null pointer dereference
snprintf(str, size, "ts-error");
^
Only `t` could possibly be NULL if `localtime_r` fails elsewhere.
Bug: #5291.
(cherry picked from commit 2f48e432cd)
cppcheck:
src/detect-engine.c:3643:5: warning: %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
snprintf(prefix, sizeof(prefix), "multi-detect.%d", tenant_id);
^
src/detect-engine.c:3707:5: warning: %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
snprintf(prefix, sizeof(prefix), "multi-detect.%d.reload.%d", tenant_id, reload_cnt);
^
src/detect-engine.c:4086:17: warning: %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
snprintf(prefix, sizeof(prefix), "multi-detect.%d", tenant_id);
^
Bug: #5291.
(cherry picked from commit 4fcb8740e7)
cppcheck:
src/util-reference-config.c:179:9: warning: Assignment of function parameter has no effect outside the function. Did you forget dereferencing it? [uselessAssignmentPtrArg]
fd = NULL;
^
Bug: #5291.
(cherry picked from commit 5a0bbb5289)
cppcheck:
src/util-runmodes.c:210:9: warning: %u in format string (no. 2) requires 'unsigned int' but the argument type is 'signed int'. [invalidPrintfArgType_uint]
snprintf(tname, sizeof(tname), "%s#%02u", thread_name_workers, thread+1);
^
src/util-runmodes.c:211:9: warning: %u in format string (no. 1) requires 'unsigned int' but the argument type is 'signed int'. [invalidPrintfArgType_uint]
snprintf(qname, sizeof(qname), "pickup%u", thread+1);
^
src/util-runmodes.c:455:9: warning: %u in format string (no. 2) requires 'unsigned int' but the argument type is 'signed int'. [invalidPrintfArgType_uint]
snprintf(tname, sizeof(tname), "%s#%02u", thread_name_workers, thread+1);
^
src/util-runmodes.c:457:9: warning: %u in format string (no. 1) requires 'unsigned int' but the argument type is 'signed int'. [invalidPrintfArgType_uint]
snprintf(qname, sizeof(qname), "pickup%u", thread+1);
^
src/runmode-erf-file.c:188:9: warning: %u in format string (no. 2) requires 'unsigned int' but the argument type is 'signed int'. [invalidPrintfArgType_uint]
snprintf(tname, sizeof(tname), "%s#%02u", thread_name_workers, thread+1);
^
src/runmode-erf-file.c:189:9: warning: %u in format string (no. 1) requires 'unsigned int' but the argument type is 'signed int'. [invalidPrintfArgType_uint]
snprintf(qname, sizeof(qname), "pickup%u", thread+1);
^
src/runmode-pcap-file.c:201:9: warning: %u in format string (no. 2) requires 'unsigned int' but the argument type is 'signed int'. [invalidPrintfArgType_uint]
snprintf(tname, sizeof(tname), "%s#%02u", thread_name_workers, thread+1);
^
src/runmode-pcap-file.c:202:9: warning: %u in format string (no. 1) requires 'unsigned int' but the argument type is 'signed int'. [invalidPrintfArgType_uint]
snprintf(qname, sizeof(qname), "pickup%u", thread+1);
^
Bug: #5291.
(cherry picked from commit 2965d809a4)
cppcheck:
src/util-mpm-ac-ks.c:1452:5: warning: %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
printf("Total states in the state table: %d\n", ctx->state_count);
^
src/util-mpm-ac-ks.c:606:34: error: Signed integer overflow for expression '1<<31'. [integerOverflow]
encoded_next_state |= (1 << 31);
^
Bug: #5291.
(cherry picked from commit a8d3cd6eb4)
cppcheck:
src/util-classification-config.c:189:9: warning: Assignment of function parameter has no effect outside the function. Did you forget dereferencing it? [uselessAssignmentPtrArg]
fd = NULL;
^
Bug: #5291.
(cherry picked from commit 9c672a805f)
Rearrange code slightly to make it more clear that `found` cannot
be NULL further down the loop.
cppcheck:
src/detect-engine-content-inspection.c:316:50: warning: Either the condition 'found!=NULL' is redundant or there is overflow in pointer subtraction. [nullPointerArithmeticRedundantCheck]
match_offset = (uint32_t)((found - buffer) + cd->content_len);
^
src/detect-engine-content-inspection.c:308:30: note: Assuming that condition 'found!=NULL' is not redundant
} else if (found != NULL && (cd->flags & DETECT_CONTENT_NEGATED)) {
^
src/detect-engine-content-inspection.c:316:50: note: Null pointer subtraction
match_offset = (uint32_t)((found - buffer) + cd->content_len);
^
Bug: #5291.
(cherry picked from commit 27e9a871d0)
cppcheck flagged this as:
src/detect-engine-analyzer.c:1359:13: warning: %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
fprintf(rule_engine_analysis_FD, " Rule contains %d content options, %d http content options, %d pcre options, and %d pcre options with http modifiers.\n", rule_content, rule_content_http, rule_pcre, rule_pcre_http);
^
src/detect-engine-analyzer.c:1359:13: warning: %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
fprintf(rule_engine_analysis_FD, " Rule contains %d content options, %d http content options, %d pcre options, and %d pcre options with http modifiers.\n", rule_content, rule_content_http, rule_pcre, rule_pcre_http);
^
src/detect-engine-analyzer.c:1359:13: warning: %d in format string (no. 3) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
fprintf(rule_engine_analysis_FD, " Rule contains %d content options, %d http content options, %d pcre options, and %d pcre options with http modifiers.\n", rule_content, rule_content_http, rule_pcre, rule_pcre_http);
^
src/detect-engine-analyzer.c:1359:13: warning: %d in format string (no. 4) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
fprintf(rule_engine_analysis_FD, " Rule contains %d content options, %d http content options, %d pcre options, and %d pcre options with http modifiers.\n", rule_content, rule_content_http, rule_pcre, rule_pcre_http);
^
Bug: #5291.
(cherry picked from commit a0847e6c69)
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.
(cherry picked from commit f8a0f3d9b9)
Remove useless allocation and free.
Found by cppcheck as a potential issue:
src/detect-engine-address-ipv6.c:385:12: warning: Either the condition 'tmp!=NULL' is redundant or there is possible null pointer dereference: tmp. [nullPointerRedundantCheck]
memset(tmp,0,sizeof(DetectAddress));
^
src/detect-engine-address-ipv6.c:525:13: note: Assuming that condition 'tmp!=NULL' is not redundant
if (tmp != NULL)
^
src/detect-engine-address-ipv6.c:385:12: note: Null pointer dereference
memset(tmp,0,sizeof(DetectAddress));
^
But code turned out not to do anything, so removed.
Bug: #5291.
(cherry picked from commit bad9005161)
Set correct direction for PORT mode, where the server connects
to the client.
The direction is not also strictly enforced. No data in the wrong
direction will be accepted to setup the file or to be added to the
file after setup.
This also fixes files getting closed twice.
Adds some general cleanups.
Bug: #3542.
(cherry picked from commit 07bf921451)
- Wrap the id in a new IPPairStorageId struct, to avoid id
confusion with other storage API calls.
- Formatting fixes by clang.
(cherry picked from commit cf516de587)
The first segment was not limited to the configured maximum line length
allowing it to be up to 65k. This could result in the next input length
being negative, which while handled properly by the code, did trigger a
debug validation assertion.
The fix is to be consistent and apply the limit to the first segment as
well, which does ensure the input_len could never be less than 0.
Ticket #5281
(cherry picked from commit 9645285dff)
Add a counter to our stats log with the total of alerts that have been
discarded due to packet alert queue overflow.
Task #5179
(cherry picked from commit 8616c90fe7)
Since we now only copy the PacketAlerts to the Packet's queue after
processing them, we no longer do packet alert appending from
detect-engine-alert, nor do we remove PacketAlerts from the queue (if
they're discarded by overflow or thresholding, they're not copied to the
final alert queue).
Task #4943
(cherry picked from commit e4e688a9b0)
Do all alert queue processing before actually appending
the PacketAlerts to the Packet's alert queue.
Adjusted changes to use macro instead of functions, in cases where the
latter didn't exist in this branch.
Task #4943
(cherry picked from commit 185b43edff)
The maximum of possible alerts triggered by a unique packet was
hardcoded to 15. With usage of 'noalert' rules, that limit could be
reached somewhat easily. Make that configurable via suricata.yaml.
Conf Bug#4941
Task #4207
(cherry picked from commit 3ace577d54)
Some unittests used SCMalloc for allocating new Packet the unittests.
While this is valid, it leads to segmentation faults when we move to
dynamic allocation of the maximum alerts allowed to be triggered by a
single packet.
This massive patch uses PacketGetFromAlloc, which initializes a Packet
in such a way that any dynamic allocated structures within will also be
initialized.
Backport: edit a few more files/unittests that were not present in 7.0.x
Related to
Task #4207
(cherry picked from commit a6bda3596b)
Fix rules from the 'match' list getting added to the tx candidates list
unsorted. In some cases this could lead to the same sid getting inspected
twice leading to a DEBUG_VALIDATION_BUG_ON trigger.
Bug: #5144.
(cherry picked from commit 4bb00964ac)
If a packet after the initialization would come with ACK flag set
but a ACK value of 0, the last_ack tracking could get confused. Fix
this by not checking for 0 but instead checking if the ACK flag
has been seen.
Bug: #4549.
(cherry picked from commit 1f43e1477f)
If the start of a line looks like it might be a mime boundary we
yield to the get line logic if we don't have enough data to be
conclusive.
(cherry picked from commit e7417a8e96)
The input data received in DATA and BDAT command modes can be huge and
could have important data, like a legit huge email. Therefore, exempt
these from the line buffering limits which were introduced to regulate
the size of lines that we buffer at any point in time.
As a part of this patch, anything that comes under DATA or BDAT is
processed early without buffering as and when it arrives. The ways of
processing remain the same as before.
(cherry picked from commit cf749fd450)
Issue
-----
So far, with the SMTP parser, we would buffer data up until an LF char
was found indicating the end of one line. This would happen in case of
fragmented data where a line might come broken into multiple chunks.
This was problematic if there was a really long line without any LF
character. It would mean that we'd keep buffering data up until we
encounter one such LF char which may be many many bytes of data later.
Fix
---
Fix this issue by setting an upper limit of 4KB on the buffering of
lines. If the limit is reached then we save the data into current line
and process it as if it were a regular request/response up until 4KB
only. Any data after 4KB is discarded up until there is a new LF char in
the received input.
Cases
-----
1. Fragmentation
The limit is enforced for any cases where a line of >= 4KB comes as diff
fragments that are each/some < 4KB.
2. Single too long line
The limit is also enforced for any cases where a single line exceeds the
limit of buffer.
Reported by Victor Julien.
Ticket 5023
(cherry picked from commit 078c251dea)
Also, remove tests that check for the removed buffers and any middle
states while parsing and buffering.
Ticket 4907
(cherry picked from commit 8918f53f6b)
FTP control commands will be buffered forever until a new line is seen,
this can lead to memory exhaustion in Suricata.
To fix, set an upper bound, 4096 bytes on the size of the command that
is saved in the transaction. The input continues to be parsed to find
the end of the command so the parser can continue to move onto the next
command.
The result is that the command data in the transaction is truncated,
which also shows up in the ftp transaction logs.
This value is configurable with the max-line-length field in the ftp
app-layer.protocols section.
As FTP doesn't have events at this time, add a new fields to eve-log
that specificy if the request, or the response has been truncated.
Ticket #5024
(cherry-picked from commit cf8ed576e0)
Ticket: 4972
As is done in detect-lua-extensions.
We can have a flow with alproto unknown, no state, and therefore
cannot run AppLayerParserGetTx which could try to run a NULL
function
(cherry picked from commit dccf2e4c30)
Ticket: 4972
Completes commit c3a220647
DETECT_CONFIG is added as DETECT_SM_LIST_POSTMATCH and not
as DETECT_SM_LIST_MATCH as other keywords handled in SignatureCreateMask
(cherry picked from commit 00da0d3420)
To recognize a protocol, Suricata first looks for
patterns, which can be confirmed by a probing parser.
If this does not work, Suricata can try to run
some probing parsers on some ports.
This is the case for SMB.
This commit makes handling the confirming and the probing
paser differently even if they share much code.
The confirmation parser knows that a pattern has been found.
So, it must not do the midstream case of looking for this
pattern in the whole buffer, but only check it at the beginning.
But it must reverse direction if needed.
Ticket #4849
Backported manually by jason.ish@oisf.net.
(cherry picked from commit 464ff80c6a)
Ticket: 4857
If a pattern such as GET is seen ine the beginning of the
file transferred over ftp-data, this flow will get recognized
as HTTP, and a HTTP state will be created during parsing.
Thus, we cannot override directly alproto's values
This solves the segfault, but not the logical bug that the flow
should be classified as FTP-DATA instead of HTTP
(cherry picked from commit dd32238667)
Victor Julien
Eric Leblond
Shivani Bhardwaj
Lukas Sismis
Philippe Antoine
Juliana Fajardini
Philippe Antoine
Jeff Lucovsky
Jeff Lucovsky
Sascha Steinbiss
Jason Ish
Jeff Lucovsky
Gleb Smirnoff
Victor Julien
Lukas Sismis
Michael Tremer
Shivani Bhardwaj
Juliana Fajardini
Arne Welzel
Eric Leblond
Modupe Falodun