Victor Julien [Wed, 12 Apr 2023 17:21:53 +0000 (19:21 +0200)]
pcap: fix return check
The check that meant to check if pcap_dispatch processed fewer packets
than the desired number was inaccurate. It would also include all errors
(negative return values).
This patch considers only positive values for this check.
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.
Justin Azoff [Sat, 18 Feb 2023 02:11:46 +0000 (21:11 -0500)]
detect/iponly: Reduce the size of the SigNumArray bitsets
Instead of tracking ip only rules by the internal signum, track them by
a separate counter that starts at zero. This results in dense
SigNumArrays instead of sparse ones and a much smaller max_idx.
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.
Maxim Korotkov [Thu, 16 Feb 2023 17:16:19 +0000 (20:16 +0300)]
output: fix logic error
The logical error may have been made here. Comparison with the upper
bound of the variable type does not make sense. It may be worth adding
the cast of one of the multiplication operands to the 64-bit type for
avoiding overflow.
Found by Security Code with Svace static analyzer
Bug: #5789
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.
Jason Ish [Wed, 15 Mar 2023 22:23:49 +0000 (16:23 -0600)]
app-layer: add direction to transaction creation where needed
Build on Eric's but set the direction on transaction creation when
needed. I think this makes it a little more clear, and easier to
document when creating single direction transactions.
This also somewhat abstracts the inner-workings of a directional
transaction from the implementation.
Eric Leblond [Mon, 23 Jan 2023 19:08:07 +0000 (20:08 +0100)]
dnp3: activate unidirectional TX flag
By implementing the no inspection flag we can now set
the unidirectional TX flag. Which means that the alstate
progress function can now be simplified to always return
1 f the transaction is complete.
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.
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.
projects / thirdparty / suricata.git / log
