From 77c21b05d2cb77cb618ec8dc29e5baecb52dccc0 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Fri, 10 Oct 2025 19:41:39 +0530 Subject: [PATCH] tls/subject: use byte array instead of string 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 --- rust/src/x509/mod.rs | 22 +++++++++++++++++----- src/app-layer-ssl.c | 13 +++++++------ src/app-layer-ssl.h | 3 ++- src/detect-tls-cert-subject.c | 4 ++-- src/detect-tls.c | 4 +++- src/log-tlsstore.c | 13 ++++++++----- src/output-json-tls.c | 13 +++++++++++-- src/util-lua-tls.c | 2 +- 8 files changed, 51 insertions(+), 23 deletions(-) diff --git a/rust/src/x509/mod.rs b/rust/src/x509/mod.rs index 4873f392b2..d15f61d0d8 100644 --- a/rust/src/x509/mod.rs +++ b/rust/src/x509/mod.rs @@ -1,4 +1,4 @@ -/* Copyright (C) 2019-2020 Open Information Security Foundation +/* Copyright (C) 2019-2025 Open Information Security Foundation * * You can copy, redistribute or modify this Program under the terms of * the GNU General Public License version 2 as published by the Free @@ -85,13 +85,17 @@ pub unsafe extern "C" fn SCX509Decode( } #[no_mangle] -pub unsafe extern "C" fn SCX509GetSubject(ptr: *const X509) -> *mut c_char { +pub unsafe extern "C" fn SCX509GetSubject(ptr: *const X509, subject_name: *mut *mut u8, subject_len: *mut u32) { if ptr.is_null() { - return std::ptr::null_mut(); + *subject_len = 0; + *subject_name = std::ptr::null_mut(); + return; } let x509 = cast_pointer! {ptr, X509}; - let subject = x509.0.tbs_certificate.subject.to_string(); - rust_string_to_c(subject) + let subject = x509.0.tbs_certificate.subject().to_string().into_bytes(); + + *subject_len = subject.len() as u32; + *subject_name = Box::into_raw(subject.into_boxed_slice()) as *mut u8; } #[no_mangle] @@ -182,6 +186,14 @@ pub unsafe extern "C" fn SCX509Free(ptr: *mut X509) { drop(Box::from_raw(ptr)); } +#[no_mangle] +pub unsafe extern "C" fn SCX509ArrayFree(ptr: *mut u8, len: u32) { + if ptr.is_null() { + return; + } + drop(Box::from_raw(std::ptr::slice_from_raw_parts_mut(ptr, len as usize))); +} + fn x509_parse_error_to_errcode(e: &Err) -> X509DecodeError { match e { Err::Incomplete(_) => X509DecodeError::InvalidLength, diff --git a/src/app-layer-ssl.c b/src/app-layer-ssl.c index 62c9fb3167..f79e58bb65 100644 --- a/src/app-layer-ssl.c +++ b/src/app-layer-ssl.c @@ -490,14 +490,13 @@ static int TlsDecodeHSCertificate(SSLState *ssl_state, SSLStateConnp *connp, goto next; } - char *str = SCX509GetSubject(x509); - if (str == NULL) { + SCX509GetSubject(x509, &connp->cert0_subject, &connp->cert0_subject_len); + if (connp->cert0_subject == NULL) { err_code = ERR_EXTRACT_SUBJECT; goto error; } - connp->cert0_subject = str; - str = SCX509GetIssuer(x509); + char *str = SCX509GetIssuer(x509); if (str == NULL) { err_code = ERR_EXTRACT_ISSUER; goto error; @@ -2855,7 +2854,8 @@ static void SSLStateFree(void *p) SSLCertsChain *item; if (ssl_state->client_connp.cert0_subject) - SCRustCStringFree(ssl_state->client_connp.cert0_subject); + SCX509ArrayFree( + ssl_state->client_connp.cert0_subject, ssl_state->client_connp.cert0_subject_len); if (ssl_state->client_connp.cert0_issuerdn) SCRustCStringFree(ssl_state->client_connp.cert0_issuerdn); if (ssl_state->client_connp.cert0_serial) @@ -2870,7 +2870,8 @@ static void SSLStateFree(void *p) SCFree(ssl_state->client_connp.hs_buffer); if (ssl_state->server_connp.cert0_subject) - SCRustCStringFree(ssl_state->server_connp.cert0_subject); + SCX509ArrayFree( + ssl_state->server_connp.cert0_subject, ssl_state->server_connp.cert0_subject_len); if (ssl_state->server_connp.cert0_issuerdn) SCRustCStringFree(ssl_state->server_connp.cert0_issuerdn); if (ssl_state->server_connp.cert0_serial) diff --git a/src/app-layer-ssl.h b/src/app-layer-ssl.h index 8d37f866a2..0c200d90fe 100644 --- a/src/app-layer-ssl.h +++ b/src/app-layer-ssl.h @@ -184,9 +184,10 @@ typedef struct SSLStateConnp_ { uint16_t session_id_length; uint8_t random[TLS_RANDOM_LEN]; - char *cert0_subject; char *cert0_issuerdn; char *cert0_serial; + uint8_t *cert0_subject; + uint32_t cert0_subject_len; int64_t cert0_not_before; int64_t cert0_not_after; char *cert0_fingerprint; diff --git a/src/detect-tls-cert-subject.c b/src/detect-tls-cert-subject.c index ce84cbba84..61a9405d7c 100644 --- a/src/detect-tls-cert-subject.c +++ b/src/detect-tls-cert-subject.c @@ -138,8 +138,8 @@ static InspectionBuffer *GetData(DetectEngineThreadCtx *det_ctx, return NULL; } - const uint32_t data_len = (uint32_t)strlen(connp->cert0_subject); - const uint8_t *data = (uint8_t *)connp->cert0_subject; + const uint32_t data_len = connp->cert0_subject_len; + const uint8_t *data = connp->cert0_subject; InspectionBufferSetupAndApplyTransforms( det_ctx, list_id, buffer, data, data_len, transforms); diff --git a/src/detect-tls.c b/src/detect-tls.c index d08833e5a0..fe8212f958 100644 --- a/src/detect-tls.c +++ b/src/detect-tls.c @@ -187,7 +187,9 @@ static int DetectTlsSubjectMatch (DetectEngineThreadCtx *det_ctx, SCLogDebug("TLS: Subject is [%s], looking for [%s]\n", connp->cert0_subject, tls_data->subject); - if (strstr(connp->cert0_subject, tls_data->subject) != NULL) { + if (SpmSearch(connp->cert0_subject, connp->cert0_subject_len, + (const uint8_t *)tls_data->subject, + (uint16_t)strlen(tls_data->subject)) != NULL) { if (tls_data->flags & DETECT_CONTENT_NEGATED) { ret = 0; } else { diff --git a/src/log-tlsstore.c b/src/log-tlsstore.c index de848500f0..3467399bb6 100644 --- a/src/log-tlsstore.c +++ b/src/log-tlsstore.c @@ -234,11 +234,14 @@ static void LogTlsLogPem(LogTlsStoreLogThread *aft, const Packet *p, SSLState *s goto end_fwrite_fpmeta; } - if (fprintf(fpmeta, - "TLS SUBJECT: %s\n" - "TLS ISSUERDN: %s\n" - "TLS FINGERPRINT: %s\n", - connp->cert0_subject, connp->cert0_issuerdn, connp->cert0_fingerprint) < 0) + char *subject = CreateStringFromByteArray(connp->cert0_subject, connp->cert0_subject_len); + int r = fprintf(fpmeta, + "TLS SUBJECT: %s\n" + "TLS ISSUERDN: %s\n" + "TLS FINGERPRINT: %s\n", + subject ? subject : "", connp->cert0_issuerdn, connp->cert0_fingerprint); + SCFree(subject); + if (r < 0) goto end_fwrite_fpmeta; fclose(fpmeta); diff --git a/src/output-json-tls.c b/src/output-json-tls.c index f5af87c5fb..cfd6c84245 100644 --- a/src/output-json-tls.c +++ b/src/output-json-tls.c @@ -132,7 +132,12 @@ typedef struct JsonTlsLogThread_ { static void JsonTlsLogSubject(SCJsonBuilder *js, SSLState *ssl_state) { if (ssl_state->server_connp.cert0_subject) { - SCJbSetString(js, "subject", ssl_state->server_connp.cert0_subject); + if (ssl_state->server_connp.cert0_subject_len == 0) { + SCJbSetString(js, "subject", ""); + } else { + SCJbSetStringFromBytes(js, "subject", ssl_state->server_connp.cert0_subject, + ssl_state->server_connp.cert0_subject_len); + } } } @@ -338,7 +343,11 @@ static void JsonTlsLogClientCert( SCJsonBuilder *js, SSLStateConnp *connp, const bool log_cert, const bool log_chain) { if (connp->cert0_subject != NULL) { - SCJbSetString(js, "subject", connp->cert0_subject); + if (connp->cert0_subject_len == 0) { + SCJbSetString(js, "subject", ""); + } else { + SCJbSetStringFromBytes(js, "subject", connp->cert0_subject, connp->cert0_subject_len); + } } if (connp->cert0_issuerdn != NULL) { SCJbSetString(js, "issuerdn", connp->cert0_issuerdn); diff --git a/src/util-lua-tls.c b/src/util-lua-tls.c index d657d5062d..6805019832 100644 --- a/src/util-lua-tls.c +++ b/src/util-lua-tls.c @@ -174,7 +174,7 @@ static int GetCertInfo(lua_State *luastate, bool client, const SSLState *ssl_sta SSLVersionToString(ssl_state->server_connp.version, ssl_version); int r = LuaPushStringBuffer(luastate, (uint8_t *)ssl_version, strlen(ssl_version)); - r += LuaPushStringBuffer(luastate, (uint8_t *)connp->cert0_subject, strlen(connp->cert0_subject)); + r += LuaPushStringBuffer(luastate, connp->cert0_subject, connp->cert0_subject_len); r += LuaPushStringBuffer(luastate, (uint8_t *)connp->cert0_issuerdn, strlen(connp->cert0_issuerdn)); r += LuaPushStringBuffer(luastate, (uint8_t *)connp->cert0_fingerprint, strlen(connp->cert0_fingerprint)); return r;