From c93e69830a51869a2eac3a4e79fb19d9f7fffde7 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Thu, 27 Nov 2025 15:58:15 +0100 Subject: [PATCH] detect/ssl: properly handle negation in ssl_version keyword Ticket: 3220 DetectSslVersionMatch did not handle properly negation. It could never match on a signatrue with ssl_version: !tls1.3 That is because, if we had such a signature and network traffic with tls1.1, we were looking into DetectSslVersionData field for tls1.1, which was not set, instead of looking at field for tls1.3 which was set with negated flag. Previous DetectSslVersionData was holding redundant information. It did not need to have it for each ssl version, but just globally. Also, it did not need to hold the version as a value in the array, as it was redundant with the index of the array. --- doc/userguide/rules/tls-keywords.rst | 4 ++ src/detect-ssl-version.c | 74 +++++++++++----------------- src/detect-ssl-version.h | 13 ++--- src/tests/detect-ssl-version.c | 10 ++-- 4 files changed, 42 insertions(+), 59 deletions(-) diff --git a/doc/userguide/rules/tls-keywords.rst b/doc/userguide/rules/tls-keywords.rst index b01c41cc19..b022d784c0 100644 --- a/doc/userguide/rules/tls-keywords.rst +++ b/doc/userguide/rules/tls-keywords.rst @@ -231,6 +231,10 @@ Example:: alert tls any any -> any any (msg:"match SSLv2 and SSLv3"; \ ssl_version:sslv2,sslv3; sid:200031;) +The list can be prefixed with ``!`` to match if version is different than +all the versions listed in the signature. Such a negation does not match +on a yet undetermined version. + tls.fingerprint --------------- diff --git a/src/detect-ssl-version.c b/src/detect-ssl-version.c index e9dd93bf93..a48e0936c0 100644 --- a/src/detect-ssl-version.c +++ b/src/detect-ssl-version.c @@ -98,7 +98,7 @@ static int DetectSslVersionMatch(DetectEngineThreadCtx *det_ctx, int ret = 0; uint16_t ver = 0; - uint8_t sig_ver = TLS_UNKNOWN; + bool sig_ver = false; const DetectSslVersionData *ssl = (const DetectSslVersionData *)m; SSLState *app_state = (SSLState *)state; @@ -119,29 +119,29 @@ static int DetectSslVersionMatch(DetectEngineThreadCtx *det_ctx, switch (ver) { case SSL_VERSION_2: - if (ver == ssl->data[SSLv2].ver) + if (ssl->data[SSLv2]) ret = 1; - sig_ver = SSLv2; + sig_ver = true; break; case SSL_VERSION_3: - if (ver == ssl->data[SSLv3].ver) + if (ssl->data[SSLv3]) ret = 1; - sig_ver = SSLv3; + sig_ver = true; break; case TLS_VERSION_10: - if (ver == ssl->data[TLS10].ver) + if (ssl->data[TLS10]) ret = 1; - sig_ver = TLS10; + sig_ver = true; break; case TLS_VERSION_11: - if (ver == ssl->data[TLS11].ver) + if (ssl->data[TLS11]) ret = 1; - sig_ver = TLS11; + sig_ver = true; break; case TLS_VERSION_12: - if (ver == ssl->data[TLS12].ver) + if (ssl->data[TLS12]) ret = 1; - sig_ver = TLS12; + sig_ver = true; break; case TLS_VERSION_13_DRAFT28: case TLS_VERSION_13_DRAFT27: @@ -157,35 +157,33 @@ static int DetectSslVersionMatch(DetectEngineThreadCtx *det_ctx, case TLS_VERSION_13_DRAFT17: case TLS_VERSION_13_DRAFT16: case TLS_VERSION_13_PRE_DRAFT16: - if (((ver >> 8) & 0xff) == 0x7f) - ver = TLS_VERSION_13; - /* fall through */ case TLS_VERSION_13: - if (ver == ssl->data[TLS13].ver) + if (ssl->data[TLS13]) ret = 1; - sig_ver = TLS13; + sig_ver = true; break; } - if (sig_ver == TLS_UNKNOWN) + if (!sig_ver) SCReturnInt(0); - SCReturnInt(ret ^ ((ssl->data[sig_ver].flags & DETECT_SSL_VERSION_NEGATED) ? 1 : 0)); + // matches if ret == 1 and negate is false + // or if ret == 0 and negate is true + SCReturnInt(ret ^ (ssl->negate ? 1 : 0)); } struct SSLVersionKeywords { const char *word; int index; - uint16_t value; }; struct SSLVersionKeywords ssl_version_keywords[TLS_SIZE] = { - { "sslv2", SSLv2, SSL_VERSION_2 }, - { "sslv3", SSLv3, SSL_VERSION_3 }, - { "tls1.0", TLS10, TLS_VERSION_10 }, - { "tls1.1", TLS11, TLS_VERSION_11 }, - { "tls1.2", TLS12, TLS_VERSION_12 }, - { "tls1.3", TLS13, TLS_VERSION_13 }, + { "sslv2", SSLv2 }, + { "sslv3", SSLv3 }, + { "tls1.0", TLS10 }, + { "tls1.1", TLS11 }, + { "tls1.2", TLS12 }, + { "tls1.3", TLS13 }, }; /** @@ -203,7 +201,6 @@ static DetectSslVersionData *DetectSslVersionParse(DetectEngineCtx *de_ctx, cons DetectSslVersionData *ssl = NULL; const char *tmp_str = str; size_t tmp_len = 0; - uint8_t found = 0; /* We have a correct ssl_version options */ ssl = SCCalloc(1, sizeof(DetectSslVersionData)); @@ -218,13 +215,12 @@ static DetectSslVersionData *DetectSslVersionParse(DetectEngineCtx *de_ctx, cons SCLogError("Invalid empty value"); goto error; } + if (tmp_str[0] == '!') { + ssl->negate = true; + tmp_str++; + } // iterate every version separated by comma while (tmp_str[0] != 0) { - uint8_t neg = 0; - if (tmp_str[0] == '!') { - neg = 1; - tmp_str++; - } // counts word length tmp_len = 0; while (tmp_str[tmp_len] != 0 && !isspace(tmp_str[tmp_len]) && tmp_str[tmp_len] != ',') { @@ -235,13 +231,11 @@ static DetectSslVersionData *DetectSslVersionParse(DetectEngineCtx *de_ctx, cons for (size_t i = 0; i < TLS_SIZE; i++) { if (tmp_len == strlen(ssl_version_keywords[i].word) && strncasecmp(ssl_version_keywords[i].word, tmp_str, tmp_len) == 0) { - if (ssl->data[ssl_version_keywords[i].index].ver != 0) { + if (ssl->data[ssl_version_keywords[i].index]) { SCLogError("Invalid duplicate value"); goto error; } - ssl->data[ssl_version_keywords[i].index].ver = ssl_version_keywords[i].value; - if (neg == 1) - ssl->data[ssl_version_keywords[i].index].flags |= DETECT_SSL_VERSION_NEGATED; + ssl->data[ssl_version_keywords[i].index] = true; is_keyword = true; break; } @@ -251,16 +245,6 @@ static DetectSslVersionData *DetectSslVersionParse(DetectEngineCtx *de_ctx, cons goto error; } - /* check consistency between negative and positive values : - * if there is a negative value, it overrides positive values - */ - if (found == 0) { - found |= 1 << neg; - } else if (found != 1 << neg) { - SCLogError("Invalid value mixing negative and positive forms"); - goto error; - } - tmp_str += tmp_len; while (isspace(tmp_str[0]) || tmp_str[0] == ',') { tmp_str++; diff --git a/src/detect-ssl-version.h b/src/detect-ssl-version.h index 6809178c5f..966c3c3b95 100644 --- a/src/detect-ssl-version.h +++ b/src/detect-ssl-version.h @@ -25,8 +25,6 @@ #ifndef DETECT_SSL_VERSION_H #define DETECT_SSL_VERSION_H -#define DETECT_SSL_VERSION_NEGATED 0x01 - enum { SSLv2 = 0, SSLv3 = 1, @@ -36,16 +34,13 @@ enum { TLS13 = 5, TLS_SIZE = 6, - TLS_UNKNOWN = 7, }; -typedef struct SSLVersionData_ { - uint16_t ver; /** ssl version to match */ - uint8_t flags; -} SSLVersionData; - typedef struct DetectSslVersionData_ { - SSLVersionData data[TLS_SIZE]; + // negate is global : `tls1.1, !tls1.0` does not make sense + bool negate; + // index is ssl version to match on + bool data[TLS_SIZE]; } DetectSslVersionData; /* prototypes */ diff --git a/src/tests/detect-ssl-version.c b/src/tests/detect-ssl-version.c index d4a5297655..da2ac89e16 100644 --- a/src/tests/detect-ssl-version.c +++ b/src/tests/detect-ssl-version.c @@ -33,7 +33,7 @@ static int DetectSslVersionTestParse01(void) DetectSslVersionData *ssl = NULL; ssl = DetectSslVersionParse(NULL, "SSlv3"); FAIL_IF_NULL(ssl); - FAIL_IF_NOT(ssl->data[SSLv3].ver == SSL_VERSION_3); + FAIL_IF_NOT(ssl->data[SSLv3]); DetectSslVersionFree(NULL, ssl); PASS; } @@ -73,13 +73,13 @@ static int DetectSslVersionTestParse03(void) DetectSslVersionData *ssl = NULL; ssl = DetectSslVersionParse(NULL, "SSlv3 , tls1.0"); FAIL_IF_NULL(ssl); - FAIL_IF_NOT(ssl->data[SSLv3].ver == SSL_VERSION_3); - FAIL_IF_NOT(ssl->data[TLS10].ver == TLS_VERSION_10); + FAIL_IF_NOT(ssl->data[SSLv3]); + FAIL_IF_NOT(ssl->data[TLS10]); DetectSslVersionFree(NULL, ssl); ssl = DetectSslVersionParse(NULL, " !tls1.2"); FAIL_IF_NULL(ssl); - FAIL_IF_NOT(ssl->data[TLS12].ver == TLS_VERSION_12); - FAIL_IF_NOT(ssl->data[TLS12].flags & DETECT_SSL_VERSION_NEGATED); + FAIL_IF_NOT(ssl->data[TLS12]); + FAIL_IF_NOT(ssl->negate); DetectSslVersionFree(NULL, ssl); PASS; }