From: Michael Matirko (mmatirko) Date: Tue, 20 Feb 2024 23:38:05 +0000 (+0000) Subject: Pull request #4205: sfip: set pointers to nullptr after deletion to avoid heap-use... X-Git-Tag: 3.1.82.0~21 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3a7b02f9ef61b683d0775ed141b545fb61548ef8;p=thirdparty%2Fsnort3.git Pull request #4205: sfip: set pointers to nullptr after deletion to avoid heap-use-after-free on reload Merge in SNORT/snort3 from ~MMATIRKO/snort3:sfvar_mem to master Squashed commit of the following: commit 43ffbe3a7b41e0fd6198cf51444955ce6ea057c4 Author: Michael Matirko Date: Wed Feb 14 16:36:53 2024 -0500 sfip: remove references to unused mode feature commit 81cabc672c4196bae2a56c112641c5a9807667bf Author: Michael Matirko 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 9ddd711fb..4e015de53 100644 --- 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 076cb1dd8..f17ca3a9b 100644 --- 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 a5c7bc25c..2e0853376 100644 --- 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;