Pull request #4205: sfip: set pointers to nullptr after deletion to avoid heap-use...

Merge in SNORT/snort3 from ~MMATIRKO/snort3:sfvar_mem to master

Squashed commit of the following:

commit 43ffbe3a7b41e0fd6198cf51444955ce6ea057c4
Author: Michael Matirko <mmatirko@cisco.com>
Date:   Wed Feb 14 16:36:53 2024 -0500

    sfip: remove references to unused mode feature

commit 81cabc672c4196bae2a56c112641c5a9807667bf
Author: Michael Matirko <mmatirko@cisco.com>
Date:   Tue Feb 13 16:34:46 2024 -0500

    sfip: zero out var/node pointers after operations to remedy heap-use-after-free on reload

diff --git a/src/helpers/discovery_filter.cc b/src/helpers/discovery_filter.cc
index 9ddd711fb6e4b6c7edda179b6b2c281d47c3f4d0..4e015de538c24c39e2da8841faa1df5c6df71afa 100644 (file)
--- a/src/helpers/discovery_filter.cc
+++ b/src/helpers/discovery_filter.cc
@@ -605,4 +605,24 @@ TEST_CASE("Discovery Filter Port Exclusion", "[portexclusion]")
     remove(conf.c_str());
 }
 
+TEST_CASE("Discovery Filter with Problem Config", "[df_problem_config]")
+{
+    // Checks a set of configs that previously caused
+    // a heap-use-after-free when initializing nodes
+
+    string conf("test_intf_ip.txt");
+    ofstream out_stream(conf.c_str());
+    out_stream << "config AnalyzeHost 10.0.0.0/24 -1\n";  // interface any
+    out_stream << "config AnalyzeHost 10.0.0.0/21 4\n";   // interface 4
+    out_stream << "config AnalyzeHost 192.8.8.0/24 -1\n"; // interface any
+    out_stream.close();
+
+
+    // Verifies the config loads with no issues - otherwise, ASAN
+    // will report leaks when constructing df below
+    DiscoveryFilter df(conf);
+
+    remove("test_intf_ip.txt");
+}
+
 #endif

diff --git a/src/sfip/sf_ipvar.cc b/src/sfip/sf_ipvar.cc
index 076cb1dd86e31fb43ba64b6a0afb4a3c08cb4a8a..f17ca3a9b593187ef4c6191097abdaf7d7ac7d99 100644 (file)
--- a/src/sfip/sf_ipvar.cc
+++ b/src/sfip/sf_ipvar.cc
@@ -19,7 +19,7 @@
 
 /*
  * Adam Keeton
- * sf_ipvar.c
+ * sf_ipvar.cc
  * 11/17/06
  *
  * Library for IP variables.
@@ -49,7 +49,7 @@ using namespace snort;
 
 static SfIpRet sfvar_list_compare(sfip_node_t*, sfip_node_t*);
 static inline void sfip_node_free(sfip_node_t*&);
-static inline void sfip_node_freelist(sfip_node_t*);
+static inline void sfip_node_freelist(sfip_node_t*&);
 
 static inline sfip_var_t* _alloc_var()
 {
@@ -67,15 +67,8 @@ void sfvar_free(sfip_var_t* var)
     if (var->value)
         snort_free(var->value);
 
-    if (var->mode == SFIP_LIST)
-    {
-        sfip_node_freelist(var->head);
-        sfip_node_freelist(var->neg_head);
-    }
-    else if (var->mode == SFIP_TABLE)
-    {
-        // FIXIT-L SFIP_TABLE free unimplemented
-    }
+    sfip_node_freelist(var->head);
+    sfip_node_freelist(var->neg_head);
 
     snort_free(var);
 }
@@ -116,7 +109,7 @@ static sfip_node_t* sfipnode_alloc(const char* str, SfIpRet* status)
         {
             if (status)
                 *status = SFIP_ARG_ERR;
-            snort_free(ret);
+            sfip_node_free(ret);
             return nullptr;
         }
 
@@ -164,13 +157,15 @@ static inline void sfip_node_free(sfip_node_t*& node)
         return;
 
     if ( node->ip )
+    {
         delete node->ip;
+    }
 
     snort_free(node);
     node = nullptr;
 }
 
-static inline void sfip_node_freelist(sfip_node_t* root)
+static inline void sfip_node_freelist(sfip_node_t*& root)
 {
     sfip_node_t* node;
 
@@ -261,8 +256,10 @@ static inline bool list_contains_node(sfip_node_t*& head, sfip_node_t*& tail, ui
         {
             head = tail = node;
             sfip_node_free(cur);
+            head->next = nullptr;
             return true;
         }
+
         sfip_node_free(cur); // overlaps removed so that caller can insert the node
         --count;
     }
@@ -279,7 +276,6 @@ sfip_var_t* sfvar_deep_copy(const sfip_var_t* var)
 
     ret = (sfip_var_t*)snort_calloc(sizeof(*ret));
 
-    ret->mode = var->mode;
     ret->head = _sfvar_deep_copy_list(var->head);
     ret->neg_head = _sfvar_deep_copy_list(var->neg_head);
     ret->head_count = var->head_count;
@@ -436,27 +432,26 @@ SfIpRet sfvar_add(sfip_var_t* dst, sfip_var_t* src)
     dst->neg_head = merge_lists(dst->neg_head, copiedvar->neg_head, dst->neg_head_count,
         copiedvar->neg_head_count, dst->neg_head_count);
 
+    // This needs to be snort_free rather than sfipvar_free since
+    // we don't want to free the nodes
     snort_free(copiedvar);
 
     return SFIP_SUCCESS;
 }
 
 // Adds the nodes in 'src' to the variable 'dst'
-// The mismatch of types is for ease-of-supporting Snort4 and
-// Snort6 simultaneously
+// The mismatch of types is for ease-of-supporting IPv4 and
+// IPv6 simultaneously
 static SfIpRet sfvar_add_node(sfip_var_t* var, sfip_node_t* node, int negated)
 {
-    sfip_node_t* p;
-    sfip_node_t* swp;
-    sfip_node_t** head;
+    sfip_node_t* p = nullptr;
+    sfip_node_t* swp = nullptr;
+    sfip_node_t** head = nullptr;
     uint32_t* count;
 
     if (!var || !node)
         return SFIP_ARG_ERR;
 
-    // As of this writing, 11/20/06, nodes are always added to
-    // the list, regardless of the mode (list or table).
-
     if (negated)
     {
         head = &var->neg_head;

diff --git a/src/sfip/sf_ipvar.h b/src/sfip/sf_ipvar.h
index a5c7bc25cb0a1ad5a5145d43f814ee828f3b50d3..2e085337685dc9312ff5d83de725857a01703ebf 100644 (file)
--- a/src/sfip/sf_ipvar.h
+++ b/src/sfip/sf_ipvar.h
@@ -43,34 +43,22 @@ struct SfIp;
 struct SfCidr;
 }
 
-/* Selects which mode a given variable is using to
- * store and lookup IP addresses */
-typedef enum _modes
-{
-    SFIP_LIST,
-    SFIP_TABLE
-} MODES;
-
-/* Used by the "list" mode.  A doubly linked list of SfIp objects. */
+/* A doubly linked list of SfIp objects. */
 typedef struct _ip_node
 {
-    snort::SfCidr* ip;
-#define ip_addr ip;   /* To ease porting Snort */
-    struct _ip_node* next;
-    int flags;
-    int addr_flags; /* Flags used exclusively by Snort */
+    snort::SfCidr* ip = nullptr;
+    struct _ip_node* next = nullptr;
+    int flags = 0;
+    int addr_flags = 0;
+                    /* Flags used exclusively by Snort */
                     /* Keeping these variables separate keeps
                      * this from stepping on Snort's toes. */
                     /* Should merge them later */
 } sfip_node_t;
 
-/* An IP variable onkect */
+/* An IP variable object */
 struct sfip_var_t
 {
-    /* Selects whether or not to use the list, the table,
-     * or any other method added later */
-    MODES mode;
-
     /* Linked lists.  Switch to something faster later */
     sfip_node_t* head;
     sfip_node_t* neg_head;