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.

(cherry picked from commit c93e698)

Author: catenacyber

doc/userguide/rules/tls-keywords.rst

@@ -214,6 +214,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
 ---------------
 

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++;

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 */

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;
 }