From: Victor Julien <victor@inliniac.net>
Date: Wed, 1 Aug 2018 15:32:34 +0000 (+0200)
Subject: detect/dce: fix false positives in detection
X-Git-Tag: suricata-4.1.0-rc2~174
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=a2b8ea57fc89231a29c70186c43b8cc2f9e80077;p=thirdparty%2Fsuricata.git

detect/dce: fix false positives in detection

If a signature didn't explicitly specified 'dcerpc' or 'smb' as the
app proto, false positives on other traffic could happen. This was
caused by the sig not having a app_proto set. This isn't set as the
rule is supposed to match against either ALPROTO_DCERPC or ALPROTO_SMB.

To avoid adding runtime costs for checking for both protocols, this
patch adds a new flag for DCERPC in the 'mask' logic. The flag is set
on the sig if dce_* keywords are present and set on the packet if the
flow's app proto is either ALPROTO_DCERPC or ALPROTO_SMB.

Bug #2559

Reported-by: Jason Taylor
---

diff --git a/src/detect-engine-build.c b/src/detect-engine-build.c
index 74bd70a728..81755e2567 100644
--- a/src/detect-engine-build.c
+++ b/src/detect-engine-build.c
@@ -427,12 +427,49 @@ PacketCreateMask(Packet *p, SignatureMask *mask, AppProto alproto,
         SCLogDebug("packet has flow");
         (*mask) |= SIG_MASK_REQUIRE_FLOW;
     }
+
+    if (alproto == ALPROTO_SMB || alproto == ALPROTO_DCERPC) {
+        SCLogDebug("packet will be inspected for DCERPC");
+        (*mask) |= SIG_MASK_REQUIRE_DCERPC;
+    }
+}
+
+static int g_dce_generic_list_id = -1;
+static int g_dce_stub_data_buffer_id = -1;
+
+static bool SignatureNeedsDCERPCMask(const Signature *s)
+{
+    if (g_dce_generic_list_id == -1) {
+        g_dce_generic_list_id = DetectBufferTypeGetByName("dce_generic");
+        SCLogDebug("g_dce_generic_list_id %d", g_dce_generic_list_id);
+    }
+    if (g_dce_stub_data_buffer_id == -1) {
+        g_dce_stub_data_buffer_id = DetectBufferTypeGetByName("dce_stub_data");
+        SCLogDebug("g_dce_stub_data_buffer_id %d", g_dce_stub_data_buffer_id);
+    }
+
+    if (g_dce_generic_list_id >= 0 &&
+            s->init_data->smlists[g_dce_generic_list_id] != NULL)
+    {
+        return true;
+    }
+    if (g_dce_stub_data_buffer_id >= 0 &&
+            s->init_data->smlists[g_dce_stub_data_buffer_id] != NULL)
+    {
+        return true;
+    }
+    return false;
 }
 
 static int SignatureCreateMask(Signature *s)
 {
     SCEnter();
 
+    if (SignatureNeedsDCERPCMask(s)) {
+        s->mask |= SIG_MASK_REQUIRE_DCERPC;
+        SCLogDebug("sig requires DCERPC");
+    }
+
     if (s->init_data->smlists[DETECT_SM_LIST_PMATCH] != NULL) {
         s->mask |= SIG_MASK_REQUIRE_PAYLOAD;
         SCLogDebug("sig requires payload");
diff --git a/src/detect.c b/src/detect.c
index 0f9a1822fa..1dea649a13 100644
--- a/src/detect.c
+++ b/src/detect.c
@@ -371,7 +371,7 @@ DetectPrefilterBuildNonPrefilterList(DetectEngineThreadCtx *det_ctx, SignatureMa
          * so build the non_mpm array only for match candidates */
         const SignatureMask rule_mask = det_ctx->non_pf_store_ptr[x].mask;
         const uint8_t rule_alproto = det_ctx->non_pf_store_ptr[x].alproto;
-        if ((rule_mask & mask) == rule_mask && (rule_alproto == 0 || rule_alproto == alproto)) { // TODO dce?
+        if ((rule_mask & mask) == rule_mask && (rule_alproto == 0 || rule_alproto == alproto)) {
             det_ctx->non_pf_id_array[det_ctx->non_pf_id_cnt++] = det_ctx->non_pf_store_ptr[x].id;
         }
     }
diff --git a/src/detect.h b/src/detect.h
index e239a913fc..11ad5dae89 100644
--- a/src/detect.h
+++ b/src/detect.h
@@ -266,7 +266,7 @@ typedef struct DetectPort_ {
 #define SIG_MASK_REQUIRE_FLAGS_INITDEINIT   (1<<2)    /* SYN, FIN, RST */
 #define SIG_MASK_REQUIRE_FLAGS_UNUSUAL      (1<<3)    /* URG, ECN, CWR */
 #define SIG_MASK_REQUIRE_NO_PAYLOAD         (1<<4)
-//
+#define SIG_MASK_REQUIRE_DCERPC             (1<<5)    /* require either SMB+DCE or raw DCE */
 #define SIG_MASK_REQUIRE_ENGINE_EVENT       (1<<7)
 
 /* for now a uint8_t is enough */