Before:
If LF character was found, so far, we won't enforce the line limit on
the line. We only enforced limits in case of LF character missing in a
long line.
After this patch:
Line limit is enforced on the line if it is bigger than 4096 Bytes
irrespective of whether LF was found or not.
Victor Julien [Tue, 11 Apr 2023 09:40:35 +0000 (11:40 +0200)]
pcap: improve pcap_breakloop support
When pcap_breakloop has been issued on a handle, the current pcap_dispatch
call may return -2 (PCAP_ERROR_BREAK), but it can also return the number
of processed packets if lower than the desired number. So add this condition
as a check.
Philippe Antoine [Thu, 26 Jan 2023 08:28:46 +0000 (09:28 +0100)]
http: complete multipart until request.body-limit
In the case we are truncating a multipart file because of reaching
request.body-limit, we used to not consume the whole buffer, but
keep expected_boundary_len bytes in case a new boundary begins
in these bytes.
Even if we cannot check the complete boundary, we can still check
the first bytes, as will be done in the rust version.
Old behavior:
With RFC4648, the decoded bytes were reset to 0 in case an unusual
character was encountered in the encoded string. This worked out fine
for small test cases where there weren't many bytes to be decoded.
Problem:
If a big encoded string had a character outside of the base alphabet,
the processing would stop and the number of decoded bytes were set to 0.
However, even though the processing should stop at the invalid
character, the number of decoded bytes should correctly store the bytes
decoded up until the point an invalid characted was encountered.
New behavor:
For any base64 encoded string given to the base64 decoder in RFC4648
mode, we make sure that the number of decoded bytes correctly reflect
the number of bytes processed up until the string was valid. This makes
sure any further calculations/use of the decoded data is done correctly.
Eric Leblond [Mon, 23 Jan 2023 19:01:05 +0000 (20:01 +0100)]
app-layer: add flag to skip detection on TX
Stamus team did discover a problem were a signature can shadow
other signatures.
For example, on a PCAP only containing Kerberos protocol and where the
following signature is matching:
alert krb5 $HOME_NET any -> any any (msg:"krb match"; krb5_cname; content:"marlo"; sid:3; rev:1;)
If we add the following signature to the list of signature
alert ssh $HOME_NET any -> any any (msg:"rr"; content:"rr"; flow:established,to_server; sid:4; rev:2;)
Then the Kerberos signature is not matching anymore.
To understand this case, we need some information:
- The krb5_cname is a to_client keyword
- The signal on ssh is to_server
- Kerberos has unidirectional transaction
- kerberos application state progress is a function always returning 1
As the two signatures are in opposite side, they end up in separate
sig group head.
Another fact is that, in the PCAP, the to_server side of the session
is sent first to the detection. It thus hit the sig group head of
the SSH signature. When Suricata runs detection in this direction
the Kerberos application layer send the transaction as it is existing
and because the alstate progress function just return 1 if the transaction
exists. So Suricata runs DetectRunTx() and stops when it sees that
sgh->tx_engines is NULL.
But the transaction is consumed by the engine as it has been evaluated
in one direction and the kerberos transaction are unidirectional so
there is no need to continue looking at it.
This results in no matching of the kerberos signature as the match
should occur in the evaluation of the other side but the transaction
with the data is already seen has been handled.
This problem was discovered on this Kerberos signature but all
the application layer with unidirectional transaction are impacted.
This patch introduces a flag that can be used by application layer
to signal that the TX should not be inspected. By using this flag
on the directional detect_flags_[ts|tc] the application layer can
prevent the TX to be consumed in the wrong direction.
Application layers with unidirectional TX will be updated
in separate commits to set the flag on the direction opposite
to the one they are.
Eric Leblond [Fri, 20 Jan 2023 09:35:59 +0000 (10:35 +0100)]
app-layer-parser: give direction to progress func
The tx progress functions are expecting a direction and were given
a flow flags. As a result, they were not reporting correctly the
status if a DetectRunScratchPad flow_flags was containing some other
bits in the flag.
One case was when a signature was alterating the stream analysis
and triggering the addition of the STREAM_FLUSH flags.
The consequences are quite severe as the transactions are pilling
up waiting to be inspected causing sometimes a 10x performance hit
on pcap parsing. Also as the inspection was not done, Suricata is
missing a part of the alerts.
This was discovered when working on the following set of signatures:
alert ssh $HOME_NET any -> any any (msg:"pcre without content"; pcre:"/rabbit/"; sid:1; rev:1;)
alert smb $HOME_NET any -> any any (msg:"smb share content"; smb.share; content:"C"; sid:2; rev:1;)
When the first one is present the second is not triggering even
though the pcap file had no ssh inside. This is due to the fact
that the ssh signature was triggering the STREAM_FLUSH flag to
be set on the flowflags of the packet. But the application
layer will ask the smb state progress via
r = alp_ctx.ctxs[FlowGetProtoMapping(ipproto)][alproto].
StateGetProgress(alstate, flags);
passing it the flow flags but the smb function is expecting
a direction so we end up in a unplanned case
pub unsafe extern "C" fn rs_smb_tx_get_alstate_progress(tx: *mut ffi::c_void,
direction: u8)
...
if direction == Direction::ToServer as u8 && tx.request_done {
This leads the signature to not be evaluated correctly.
As flow.memcap-policy and defrag.memcap-policy do not support flow
actions, clarify that in the documentation. Also fix some typos, and
add missing values in some places where the exception policies were
explained.
exception/policy: use pkt action if no flow support
Defrag memcap and flow memcap do not support flow action for the
exception policies, as there is no flow when the exception condition is
hit. In such cases, the exception policy must be considered for the
packet only, when that makes sense, or should be ignored, in case of
`bypass`.
Updated all cases where flow_config.prealloc was used in a division.
*** CID 1524506: Integer handling issues (DIVIDE_BY_ZERO)
/src/flow-manager.c: 858 in FlowManager()
852 "flow_spare_q status: %" PRIu32 "%% flows at the queue",
853 spare_pool_len, flow_config.prealloc,
854 spare_pool_len * 100 / flow_config.prealloc);
855
856 /* only if we have pruned this "emergency_recovery" percentage
857 * of flows, we will unset the emergency bit */
>>> CID 1524506: Integer handling issues (DIVIDE_BY_ZERO)
>>> In expression "spare_pool_len * 100U / flow_config.prealloc", division by expression "flow_config.prealloc" which may be zero has undefined behavior.
858 if (spare_pool_len * 100 / flow_config.prealloc > flow_config.emergency_recovery) {
859 emerg_over_cnt++;
860 } else {
861 emerg_over_cnt = 0;
862 }
Jason Ish [Wed, 25 Jan 2023 18:02:27 +0000 (12:02 -0600)]
smb: remove duplicate tree_id logging
Remove the second occurrence of tree_id logging which appears to
always be a duplicate of the first tree_id logged, even though they
come from different data structures.
In a case of the line buffer being over 255 bytes, the consumed bytes
would reset to 0 as it was uint8_t. Fix this integer overflow by setting
the type to uint32_t.
Philippe Antoine [Wed, 18 Jan 2023 15:47:56 +0000 (16:47 +0100)]
smb: handles records with trailing nbss data
If a file (read/write) SMB record has padding/trailing data
after the buffer being read or written, and that Suricata falls
in one case where it skips the data, it should skip until
the very end of the NBSS record, meaning it should also skip the
padding/trailing data.
Otherwise, an attacker may smuggle some NBSS/SMB record in this
trailing data, that will be interpreted by Suricata, but not
by the SMB client/server, leading to evasions.
Philippe Antoine [Tue, 27 Dec 2022 16:34:47 +0000 (17:34 +0100)]
smb: checks against nbss records length
When Suricata handles files over SMB, it does not wait for the
NBSS record to be complete, and can stream the payload to the
file... But it did not check the consistency of the SMB record
length being read or written against the NBSS record length.
This could lead to an evasion where an attacker crafts a SMB
write with a too big Length field, and then sends its evil
payload, even if the server returned an error for the write request.
Bill Meeks [Thu, 23 Feb 2023 16:12:56 +0000 (11:12 -0500)]
netmap: packet stall
- Fix packet processing stall under high load when using netmap in IPS mode.
- Detect and generate Fatal Error exit for rare case when hardware NIC exposes
unmatched RX/TX queue counts. This is rare, but would result in some traffic
bypassing Suricata since it assumes NIC queue counts are symmetrical.
- Fix instance of missing unlock call for netmap device list when exiting due
to an error condition.
- Clean up existing code comments and add additional ones to better document
the new netmap v14 API code.
Jason Ish [Mon, 30 Jan 2023 23:13:04 +0000 (17:13 -0600)]
config: put version in configuration as a proper value
Adds a new field, "suricata-version" to the configuration file with
the major and minor version of the Suricata that generated the
configuration file.
This may be useful in the future for presenting warnings about
important changes, or even providing different defaults based on what
the user might expect.
Victor Julien [Tue, 21 Mar 2023 19:20:48 +0000 (20:20 +0100)]
eve/drop: don't log drops unless packet is dropped
In pass/drop combinations where the pass rule took precendence over
the drop, a "drop" false positive could still be logged due to the
storing of the drop record in the packet drop alert store.
Victor Julien [Thu, 9 Feb 2023 16:11:21 +0000 (17:11 +0100)]
stream: SYN queue support
Support case where there are multiple SYN retransmits, where
each has a new timestamp.
Before this patch, Suricata would only accept a SYN/ACK that
matches the last timestamp. However, observed behavior is that
the server may choose to only respond to the first. In IPS mode
this could lead to a connection timing out as Suricata drops
the SYN/ACK it considers wrong, and the server continues to
retransmit it.
This patch reuses the SYN/ACK queuing logic to keep a list
of SYN packets and their window, timestamp, wscale and sackok
settings. Then when the SYN/ACK arrives, it is first evaluated
against the normal session state. But if it fails due to a
timestamp mismatch, it will look for queued SYN's and see if
any of them match the timestamp. If one does, the ssn is updated
to use that SYN and the SYN/ACK is accepted.
Victor Julien [Wed, 22 Feb 2023 14:17:53 +0000 (15:17 +0100)]
stream: add liberal timetamps option
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.
Victor Julien [Sat, 18 Feb 2023 14:36:55 +0000 (15:36 +0100)]
stream: fix next_seq updates after temporary gap
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.
Victor Julien [Sat, 11 Feb 2023 12:14:53 +0000 (13:14 +0100)]
stream/tcp: fix wrong ACK trigger FIN1 to FIN2
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.
Victor Julien [Thu, 2 Feb 2023 13:45:30 +0000 (14:45 +0100)]
stream/tcp: allow tcp session reuse on null sessions
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.
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.
Lukas Sismis [Fri, 27 Jan 2023 11:34:37 +0000 (12:34 +0100)]
decode-udp: Allow shorter UDP packets than the remaining payload length
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.
Philippe Antoine [Mon, 25 Jul 2022 08:33:42 +0000 (10:33 +0200)]
detect: config keyword transaction logic fix
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.
exceptions: ignore policy if stream.midstream=true
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.
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
Jeff Lucovsky [Fri, 20 Jan 2023 14:16:05 +0000 (09:16 -0500)]
netmap: Fixup issues with v14+ backport
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.
Philippe Antoine [Thu, 15 Sep 2022 13:26:46 +0000 (15:26 +0200)]
test: do not output non ascii character
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
Victor Julien [Wed, 11 Jan 2023 20:07:16 +0000 (21:07 +0100)]
smb: fix post-trunc chunk behavior
After a gap in a file transaction, the file tracker is truncated. However
this did not clear any stored out of order chunks from memory or stop more
chunks to be stored, leading to accumulation of a large number of chunks.
This patches fixes this be clearing the stored chunks on trunc. It also
makes sure no more chunks are stored in the tracker after the trunc.
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.
Victor Julien [Mon, 16 Jan 2023 18:14:28 +0000 (19:14 +0100)]
src: fix strict-prototype warnings
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.
Philippe Antoine [Wed, 28 Dec 2022 14:57:12 +0000 (15:57 +0100)]
ftp: completely resets port_line
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.
Jason Ish [Fri, 13 Jan 2023 20:04:52 +0000 (14:04 -0600)]
rust: fix for loop over option
As of Rust 1.66 with strict mode enabled, a for loop over an option is
now an error. Replace the last occurrence of this pattern with an "if
let" statement.