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 74bd70a728150170bc9885e858945e944930e286..81755e2567fb16c0aa20be83fbea7311a0bb3e72 100644 (file)
--- 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 0f9a1822fac8717ee1d5a5f5ca090c30d337696d..1dea649a131da68c69be580e1cec9fb44ce1b1dc 100644 (file)
--- 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 e239a913fcdc6af2f8882dbfc8c1f448090b5985..11ad5dae89d514634c3bc0a27f1af021a3b7217f 100644 (file)
--- 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 */