SSLv2Decode() consumed six fixed CLIENT_HELLO body bytes (version +
cipher_spec_length + session_id_length) guarded only by input_len >= 6,
which checks the buffer remainder but not the record itself. When an
attacker sends a record whose declared record_length is < 7 (e.g. 1),
bytes_processed advances past record_lengths_length + record_length,
leading an integer underflow.
Address by adding a record_length >= 7 check to the CLIENT_HELLO case.
Also add another check: bail if bytes_processed > record_length +
record_lengths_length.
Ticket: #8853.
TLS parsers use x509-parser crate which parses X.509 certificates that
use ASN.1 DER encoding that can allow arbitrary byte sequences. An
attacker could inject null byte in a certificate anywhere to stump the
common language parsers terminating the string at a null byte leading to
a bypass of a possibly malicious certificate.
So far, the rust TLS parser for "subjectaltname" used a pattern that involved:
-> Get ASN.1 DER encoded raw data from the x509-parser crate
-> Convert this raw data to a decoded string (Rust)
-> Convert the Rust string to CString
-- The problem lies here. CString only accepts proper strings/byte
buffers and converts it into an owned C-compatible, null-terminated
string. However, if any null byte occurs in the string passed to the
CString then it panics.
In the rust TLS parser, this panic is handled by returning NULL.
This means that the parser will error out during the decoding of the
certificate. However, Suricata must be able to detect the null byte
injection attack being an IDS/IPS.
Hence, replace all such string patterns w.r.t. TLS SAN with a byte
array.
Bug 7887
TLS parsers use x509-parser crate which parses X.509 certificates that
use ASN.1 DER encoding that can allow arbitrary byte sequences. An
attacker could inject null byte in a certificate anywhere to stump the
common language parsers terminating the string at a null byte leading to
a bypass of a possibly malicious certificate.
So far, the rust TLS parser for "issuerdn" used a pattern that involved:
-> Get ASN.1 DER encoded raw data from the x509-parser crate
-> Convert this raw data to a decoded string (Rust)
-> Convert the Rust string to CString
-- The problem lies here. CString only accepts proper strings/byte
buffers and converts it into an owned C-compatible, null-terminated
string. However, if any null byte occurs in the string passed to the
CString then it panics.
In the rust TLS parser, this panic is handled by returning NULL.
This means that the parser will error out during the decoding of the
certificate. However, Suricata must be able to detect the null byte
injection attack being an IDS/IPS.
Hence, replace all such string patterns w.r.t. TLS IssuerDN with a byte
array.
Bug 7887
TLS parsers use x509-parser crate which parses X.509 certificates that
use ASN.1 DER encoding that can allow arbitrary byte sequences. An
attacker could inject null byte in a certificate anywhere to stump the
common language parsers terminating the string at a null byte leading to
a bypass of a possibly malicious certificate.
So far, the rust TLS parser for "Subject" used a pattern that involved:
-> Get ASN.1 DER encoded raw data from the x509-parser crate
-> Convert this raw data to a decoded string (Rust)
-> Convert the Rust string to CString
-- The problem lies here. CString only accepts proper strings/byte
buffers and converts it into an owned C-compatible, null-terminated
string. However, if any null byte occurs in the string passed to the
CString then it panics.
In the rust TLS parser, this panic is handled by returning NULL.
This means that the parser will error out during the decoding of the
certificate. However, Suricata must be able to detect the null byte
injection attack being an IDS/IPS.
Hence, replace all such string patterns w.r.t. TLS Subject with a byte
array.
Bug 7887
SSL connp maintains all the state and certificate data that was
parsed/decoded successfully and it must retain that for later usage.
There should be just one place to free this object which is SSLStateFree
for both the directions. By freeing the connp data during parsing error,
there is room for memory errors.
This works so far because the field parsed after this cannot error out so
if there's an error parsing this, it anyway does not exist. However, this
is incorrect and leaves scope for mistakes.
Remove this extra free and treat SAN like all other TLS keywords.
Bug 7996
The functions around TriggerRawStreamReassembly are misnomers in the
current layout of the code. The functions were named appropriately when
they were created as per the structural and logical layout of the code
at the time.
These functions in today's code are being used to track, update and
trigger progress of inspection in the raw stream only. Hence, rename them
to TriggerRawStreamInspection.
Ticket: 6695
"server_handshake" which logs the following:
1. TLS version used during handshake
2. The chosen cipher suite, excluding GREASE
3. TLS extensions, excluding GREASE
This commit is designed in preparation of enabling the handshake object
to log it's own contents rather than being done on the C side.
Moving the tls versions functionality to rust has a couple of uses:
1. Allows both rust and C side to use these fields
2. Moves more of the tls related logic to rust
3. C side can still use these values because of cbindgen
Ticket: 6695
With the introduction of the HandshakeParams object we're able to
utilise the theory further by using it as the object to track the ALPNs.
The HandshakeParams object is now responsible for holding all ALPNS. The
user of this HandshakeParams object i.e. JA4, can use whichever fields
are needed. So only when we generate a JA4 hash do we use the first ALPN
and require to format it. Other users of HandshakeParams may opt to use
all ALPN's i.e. during TlsAlpnGetData().
Ticket: 6695
This introduction splits the use of the handshake parameters into their
own object, HandshakeParams, which is populated by the TLS decoder. The
JA4 object is now very simple. It's a simple String object (the JA4
Hash) which is generated during new().
This introduction is part of a larger idea, which is to enable
outputting these raw parameters without JA3/JA4. These handshake
parameters are the components used to generate the JA4 hash, thus it
makes sense for it to be a user of HandshakeParams.
The default value for TLS encryption is generic and not precise.
The new keyword, "track-only" substitues the old keyword, being
more on point what action is actually taken when the TLS flow
reaches an encrypted state.
If not all data is ACK'd during the FIN session shutdown, the last calls
to the parser can be with a non-NULL data pointer, but a input length of
0. This wasn't considered by the EOF check, which then lead to it being
seen as an error. No event was raised, but the tls error stats were
incremented.
Bug: #7554.
To optimize detection, and logging, to avoid going through
all the live transactions when only a few were modified.
Two boolean fields are added to the tx data: updated_tc and ts
The app-layer parsers are now responsible to set these when
needed, and the logging and detection uses them to skip
transactions that were not updated.
There may some more optimization remaining by when we set
both updated_tc and updated_ts in functions returning
a mutable transaction, by checking if all the callers
are called in one direction only (request or response)
Ticket: 7087
Introduce a common function for mapping names to IDs that performs
bounds checking.
Note: For event IDs in the enum that are larger than a uint8_t, -1
will be returned instead of -4. -4 has special meaning during
signature parsin that means requirements were not met. -4 has no
special handling prior to requirements, or the meaning has been lost.
So far, the SANs were available as a part of IssuerDN via x509_parser
crate but SANs were not available to the SSLState* to be directly used
to setup and match against a sticky buffer.
Expose it to SSLStateConnp.
Feature 5234
Victor Julien
Philippe Antoine
Shivani Bhardwaj
Richard McConnell
Jason Ish
Lukas Sismis
Sascha Steinbiss
Juliana Fajardini
Jeff Lucovsky