Merge pull request #2972 in SNORT/snort3 from ~DIPANDIT/snort3:pinhole_fix to master

Squashed commit of the following:

commit 846148ae043eb8d919ae152b08ab467f726d55f7
Author: Dipto Pandit (dipandit) <dipandit@cisco.com>
Date:   Fri Jul 9 12:09:48 2021 -0400

    dce_rpc: fix crash when expected session comes after snort reload

diff --git a/src/service_inspectors/dce_rpc/dce_smb2.cc b/src/service_inspectors/dce_rpc/dce_smb2.cc
index 3b36190383fb995b05e6605c93754896cff0dbc9..7ed8dfabc22dc6b1419baf3e78360359855c1877 100644 (file)
--- a/src/service_inspectors/dce_rpc/dce_smb2.cc
+++ b/src/service_inspectors/dce_rpc/dce_smb2.cc
@@ -250,8 +250,7 @@ void Dce2Smb2SessionData::process_command(const Smb2Hdr* smb_hdr,
             if (neg_resp_hdr->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)
             {
                 Packet* p = DetectionEngine::get_current_packet();
-                Dce2SmbFlowData* fd =
-                    create_expected_smb_flow_data(p, (dce2SmbProtoConf *)sd.config);
+                Dce2SmbFlowData* fd = create_expected_smb_flow_data(p);
                 if (fd)
                 {
                     int result = Stream::set_snort_protocol_id_expected(p, PktType::TCP,
@@ -462,18 +461,17 @@ void Dce2Smb2SessionData::process()
     const uint8_t* data_ptr = p->data;
     uint16_t data_len = p->dsize;
 
-    // Check header length
-    if (data_len < sizeof(NbssHdr) + SMB2_HEADER_LENGTH)
-    {
-           SMB_DEBUG(dce_smb_trace, DEFAULT_TRACE_OPTION_ID, TRACE_CRITICAL_LEVEL, 
-           p, "Header error with data length %d\n",data_len);
-        dce2_smb_stats.v2_hdr_err++;
-        return;
-    }
-
     // Process the header
     if (p->is_pdu_start())
     {
+        // Check header length
+        if (data_len < sizeof(NbssHdr) + SMB2_HEADER_LENGTH)
+        {
+            SMB_DEBUG(dce_smb_trace, DEFAULT_TRACE_OPTION_ID, TRACE_CRITICAL_LEVEL, 
+                p, "Header error with data length %d\n",data_len);
+            dce2_smb_stats.v2_hdr_err++;
+            return;
+        }
         const Smb2Hdr* smb_hdr = (const Smb2Hdr*)(data_ptr + sizeof(NbssHdr));
         uint32_t next_command_offset;
         uint8_t compound_request_index = 0;

diff --git a/src/service_inspectors/dce_rpc/dce_smb_common.cc b/src/service_inspectors/dce_rpc/dce_smb_common.cc
index 026055772f4252b38d3da14cb1879f1486cfce88..83d2e22f3f39b98d6027ff11385422e008deecfe 100644 (file)
--- a/src/service_inspectors/dce_rpc/dce_smb_common.cc
+++ b/src/service_inspectors/dce_rpc/dce_smb_common.cc
@@ -48,14 +48,6 @@ Dce2SmbFlowData::Dce2SmbFlowData(Dce2SmbSessionData* ssd_v) : FlowData(inspector
     ssd = ssd_v;
 }
 
-void Dce2SmbFlowData::handle_expected(Packet* p)
-{
-    //we have a fd, but ssd not set, set it here in this flow
-    if (ssd)
-        delete ssd;
-    ssd = new Dce2Smb2SessionData(p, config);
-}
-
 Dce2SmbSessionData* Dce2SmbFlowData::upgrade(const Packet* p)
 {
     dce2SmbProtoConf* config =
@@ -120,16 +112,27 @@ static inline DCE2_SmbVersion get_smb_version(const Packet* p)
     return DCE2_SMB_VERSION_NULL;
 }
 
-Dce2SmbFlowData* create_expected_smb_flow_data(const Packet* p, dce2SmbProtoConf* config)
+Dce2SmbFlowData* create_expected_smb_flow_data(const Packet* p)
 {
     DCE2_SmbVersion smb_version = get_smb_version(p);
     if (DCE2_SMB_VERSION_2 == smb_version)
     {
-        return new Dce2SmbFlowData(config);
+        return new Dce2SmbFlowData();
     }
     return nullptr;
 }
 
+Dce2SmbSessionData* create_smb_session_data(Dce2SmbFlowData* flow_data, const Packet* p,
+    dce2SmbProtoConf* config)
+{
+    DCE2_SmbVersion smb_version = get_smb_version(p);
+    if (DCE2_SMB_VERSION_2 != smb_version)
+        return nullptr;
+    Dce2SmbSessionData* ssd = (Dce2SmbSessionData*)new Dce2Smb2SessionData(p, config);
+    flow_data->update_smb_session_data(ssd);
+    return ssd;
+}
+
 Dce2SmbSessionData* create_new_smb_session(const Packet* p,
     dce2SmbProtoConf* config)
 {
@@ -151,7 +154,8 @@ Dce2SmbSessionData* create_new_smb_session(const Packet* p,
 DCE2_SsnData* get_dce2_session_data(snort::Flow* flow)
 {
     Dce2SmbFlowData* fd = (Dce2SmbFlowData*)flow->get_flow_data(Dce2SmbFlowData::inspector_id);
-    return fd ? fd->get_smb_session_data()->get_dce2_session_data() : nullptr;
+    return fd ? fd->get_smb_session_data() ? fd->get_smb_session_data()->get_dce2_session_data() :
+        nullptr : nullptr;
 }
 
 inline FileContext* get_smb_file_context(const Packet* p)

diff --git a/src/service_inspectors/dce_rpc/dce_smb_common.h b/src/service_inspectors/dce_rpc/dce_smb_common.h
index 5ef7ce18c0887b81151a58129307fcd48c48e472..f0a8bdb329c448220879becf48aca872f0b16a28 100644 (file)
--- a/src/service_inspectors/dce_rpc/dce_smb_common.h
+++ b/src/service_inspectors/dce_rpc/dce_smb_common.h
@@ -270,12 +270,11 @@ class Dce2SmbFlowData : public snort::FlowData
 {
 public:
     Dce2SmbFlowData(Dce2SmbSessionData*);
-    Dce2SmbFlowData(dce2SmbProtoConf* cfg) : snort::FlowData(inspector_id)
+    Dce2SmbFlowData() : snort::FlowData(inspector_id)
     {
         dce2_smb_stats.concurrent_sessions++;
         if (dce2_smb_stats.max_concurrent_sessions < dce2_smb_stats.concurrent_sessions)
             dce2_smb_stats.max_concurrent_sessions = dce2_smb_stats.concurrent_sessions;
-        config = cfg;
         ssd = nullptr;
     }
 
@@ -291,19 +290,24 @@ public:
     { return ssd; }
 
     Dce2SmbSessionData* upgrade(const snort::Packet*);
+    void update_smb_session_data(Dce2SmbSessionData* ssd_v)
+    { 
+        if (ssd) delete ssd;
+        ssd = ssd_v;
+    }
     void handle_retransmit(snort::Packet*) override;
-    void handle_expected(snort::Packet*) override;
 
 public:
     static unsigned inspector_id;
 
 private:
     Dce2SmbSessionData* ssd;
-    dce2SmbProtoConf* config;
 };
 
-Dce2SmbFlowData* create_expected_smb_flow_data(const snort::Packet*, dce2SmbProtoConf*);
+Dce2SmbFlowData* create_expected_smb_flow_data(const snort::Packet*);
 Dce2SmbSessionData* create_new_smb_session(const snort::Packet*, dce2SmbProtoConf*);
+Dce2SmbSessionData* create_smb_session_data(Dce2SmbFlowData*, const snort::Packet*,
+    dce2SmbProtoConf*);
 DCE2_SsnData* get_dce2_session_data(snort::Flow*);
 snort::FileContext* get_smb_file_context(const snort::Packet*);
 snort::FileContext* get_smb_file_context(snort::Flow*, uint64_t, uint64_t, bool);

diff --git a/src/service_inspectors/dce_rpc/dce_smb_inspector.cc b/src/service_inspectors/dce_rpc/dce_smb_inspector.cc
index 1ce2430b23caf72dc7aabf98a4a7115280502160..9f0bd1e4b04011f8a9100be0f583e80740377926 100644 (file)
--- a/src/service_inspectors/dce_rpc/dce_smb_inspector.cc
+++ b/src/service_inspectors/dce_rpc/dce_smb_inspector.cc
@@ -69,7 +69,14 @@ void Dce2Smb::eval(Packet* p)
 
     Dce2SmbSessionData* smb_session_data;
     if (smb_flowdata)
+    {
         smb_session_data = smb_flowdata->get_smb_session_data();
+        // if flow data present but session data is not, it is an expected session
+        // try to update the session data, will only do for SMBv2. Ideally it should
+        // be done in handle_expected, but we dont have access to the config there.
+        if (!smb_session_data)
+            smb_session_data = create_smb_session_data(smb_flowdata, p, &config);
+    }
     else
         smb_session_data = create_new_smb_session(p, &config);