From 957eb28ce62a27e4af36e667cc55189b5e174968 Mon Sep 17 00:00:00 2001 From: "Davis McPherson -X (davmcphe - XORIANT CORPORATION at Cisco)" Date: Tue, 1 Apr 2025 15:53:39 +0000 Subject: [PATCH] Pull request #4654: snort3: resolve issues reported by Coverity static analysis Merge in SNORT/snort3 from ~DAVMCPHE/snort3:resolve_coverity_issues to master Squashed commit of the following: commit dbbb96a44df54ec5d8074befd0b2be937950ace8 Author: davis mcpherson Date: Sat Mar 15 17:16:36 2025 -0400 main: redirect stdin, stdout, stderr to /dev/null with the freopen system call main: check return code on mkdir system call and FatalError if it fails main: refactor signal handling switch statement to eliminate unreachable code commit 975bae48e44d038495e4649384dcf847dadf253d Author: davis mcpherson Date: Tue Mar 11 09:40:47 2025 -0400 loggers: allocate large buffer for writing unified2 extra data from heap instead of stack snort: in for loops that use auto keyword add & so the iterator assign a reference for each container element instead of doing a copy. coverity issue: AUTO_CAUSES_COPY filters: initialize struct fields when instance is defined unified2: use uint64_t to hold time values to eliminate Y2K38 time rollover issues managers: use std::move to pass shared ptr to new owner to avoid a copy commit 77bd1f1b7fc21d6fecf0d51682866bfa08149cf5 Author: davis mcpherson Date: Thu Mar 6 14:19:47 2025 -0500 flow: fix coverity SWAPPED ARGUMENTS and Y2K38_SAFETY issues helpers: validate input from conf file to verify port number string is valid digits host_tracker: recode while loop to avoid bogus coverity infinite loop warning ips_options: allocate large buffer for base64 decode from heap instead of on stack http: initialize class member variables in the ctor --- src/filters/sfrf.h | 35 +++++++++++++------ src/flow/expect_cache.cc | 4 ++- src/flow/flow_cache.cc | 2 +- src/flow/flow_cache.h | 2 +- src/helpers/discovery_filter.cc | 5 ++- src/host_tracker/host_cache_module.cc | 2 +- src/ips_options/ips_base64.cc | 14 ++++++-- src/loggers/unified2.cc | 34 +++++++++--------- src/main.cc | 12 ++++--- src/main/modules.cc | 4 +-- src/main/policy.cc | 4 +-- src/main/process.cc | 24 ++++++------- src/main/thread.cc | 5 ++- src/managers/action_manager.cc | 2 +- src/managers/module_manager.cc | 4 +-- src/managers/plugin_manager.cc | 2 +- src/profiler/rule_profiler.cc | 2 +- .../http_inspect/http_cutter.h | 4 +-- .../pps_ftp_telnet_protocol.cc | 2 +- .../snort2lua/preprocessor_states/pps_smtp.cc | 2 +- 20 files changed, 97 insertions(+), 68 deletions(-) diff --git a/src/filters/sfrf.h b/src/filters/sfrf.h index 0489186c9..c55b156fe 100644 --- a/src/filters/sfrf.h +++ b/src/filters/sfrf.h @@ -42,6 +42,7 @@ struct SnortConfig; typedef enum { + SFRF_TRACK_BY_NONE = 0, SFRF_TRACK_BY_SRC = 1, SFRF_TRACK_BY_DST, SFRF_TRACK_BY_RULE, @@ -65,20 +66,34 @@ typedef enum struct tSFRFConfigNode { - int tid; - unsigned gid; - unsigned sid; - PolicyId policyId; - SFRF_TRACK tracking; - unsigned count; - unsigned seconds; + int tid = 0; + unsigned gid = 0; + unsigned sid = 0; + PolicyId policyId = 0; + SFRF_TRACK tracking = SFRF_TRACK_BY_NONE; + unsigned count = 0; + unsigned seconds = 0; // Action that replaces original rule action on reaching threshold - snort::IpsAction::Type newAction; + snort::IpsAction::Type newAction = 0; // Threshold action duration in seconds before reverting to original rule action - unsigned timeout; - sfip_var_t* applyTo; + unsigned timeout = 0; + sfip_var_t* applyTo = nullptr; + + void init() + { + tid = 0; + gid = 0; + sid = 0; + policyId = 0; + tracking = SFRF_TRACK_BY_NONE; + count = 0; + seconds = 0; + newAction = 0; + timeout = 0; + applyTo = nullptr; + } }; struct tSFRFSidNode diff --git a/src/flow/expect_cache.cc b/src/flow/expect_cache.cc index c83c04e9e..ae31ae959 100644 --- a/src/flow/expect_cache.cc +++ b/src/flow/expect_cache.cc @@ -338,13 +338,15 @@ int ExpectCache::add_flow(const Packet *ctrlPkt, PktType type, IpProtocol ip_pro // This code assumes that the expected session is in the opposite direction of the control session // when groups are significant + int16_t expected_ingress_group = ctrlPkt->pkth->egress_group; + int16_t expected_egress_group = ctrlPkt->pkth->ingress_group; bool reversed_key = key.init(ctrlPkt->context->conf, type, ip_proto, cliIP, cliPort, srvIP, srvPort, vlanId, mplsId, ctrlPkt->pkth->address_space_id, #ifndef DISABLE_TENANT_ID ctrlPkt->pkth->tenant_id, #endif 0 != (ctrlPkt->pkth->flags & DAQ_PKT_FLAG_SIGNIFICANT_GROUPS), - ctrlPkt->pkth->egress_group, ctrlPkt->pkth->ingress_group); + expected_ingress_group, expected_egress_group); bool new_node = false; ExpectNode* node = static_cast ( hash_table->get_user_data(&key) ); if ( !node ) diff --git a/src/flow/flow_cache.cc b/src/flow/flow_cache.cc index f96e0d01c..238426522 100644 --- a/src/flow/flow_cache.cc +++ b/src/flow/flow_cache.cc @@ -451,7 +451,7 @@ void FlowCache::retire(Flow* flow) remove(flow); } -unsigned FlowCache::prune_idle(uint32_t thetime, const Flow* save_me) +unsigned FlowCache::prune_idle(time_t thetime, const Flow* save_me) { ActiveSuspendContext act_susp(Active::ASP_PRUNE); diff --git a/src/flow/flow_cache.h b/src/flow/flow_cache.h index 6aa5778fc..9caf05bb9 100644 --- a/src/flow/flow_cache.h +++ b/src/flow/flow_cache.h @@ -132,7 +132,7 @@ public: bool release(snort::Flow*, PruneReason = PruneReason::NONE, bool do_cleanup = true); - unsigned prune_idle(uint32_t thetime, const snort::Flow* save_me); + unsigned prune_idle(time_t thetime, const snort::Flow* save_me); unsigned prune_excess(const snort::Flow* save_me); bool prune_one(PruneReason, bool do_cleanup, uint8_t type = 0); unsigned timeout(unsigned num_flows, time_t cur_time); diff --git a/src/helpers/discovery_filter.cc b/src/helpers/discovery_filter.cc index 753978937..d1a915083 100644 --- a/src/helpers/discovery_filter.cc +++ b/src/helpers/discovery_filter.cc @@ -24,6 +24,7 @@ #include "discovery_filter.h" +#include #include #include #include @@ -132,7 +133,9 @@ DiscoveryFilter::DiscoveryFilter(const string& conf_path) string dir_str, proto_str, port_str, ip; line_stream >> dir_str >> proto_str >> port_str >> ip; - uint16_t port = strtol(port_str.c_str(), nullptr, 10); + uint16_t port = 0; + if ( std::all_of(port_str.begin(), port_str.end(), ::isdigit) ) + port = strtol(port_str.c_str(), nullptr, 10); if ( port == 0 ) { WarningMessage("Discovery Filter: Invalid port at line %u from %s;", diff --git a/src/host_tracker/host_cache_module.cc b/src/host_tracker/host_cache_module.cc index 8c5267ad9..2d898e398 100644 --- a/src/host_tracker/host_cache_module.cc +++ b/src/host_tracker/host_cache_module.cc @@ -392,7 +392,7 @@ bool HostCacheModule::set(const char*, Value& v, SnortConfig*) if (segments == 0 || (segments & (segments - 1)) != 0) { uint8_t highestBitSet = 0; - while (segments >>= 1) + while ((segments = segments >> 1)) highestBitSet++; segments = 1 << highestBitSet; LogMessage("== WARNING: host_cache segments is not the power of 2. setting to %d\n", segments); diff --git a/src/ips_options/ips_base64.cc b/src/ips_options/ips_base64.cc index a63db46ad..f1a0d2b28 100644 --- a/src/ips_options/ips_base64.cc +++ b/src/ips_options/ips_base64.cc @@ -31,6 +31,7 @@ #include "log/messages.h" #include "mime/decode_b64.h" #include "profiler/profiler.h" +#include "utils/util.h" #include "utils/util_unfold.h" using namespace snort; @@ -140,11 +141,14 @@ IpsOption::EvalStatus Base64DecodeOption::eval(Cursor& c, Packet* p) start_ptr += idx->offset; size -= idx->offset; - uint8_t base64_buf[DECODE_BLEN]; + uint8_t* base64_buf = (uint8_t*)snort_alloc(DECODE_BLEN); uint32_t base64_size = 0; - if (sf_unfold_header(start_ptr, size, base64_buf, sizeof(base64_buf), &base64_size, 0, nullptr) != 0) + if (sf_unfold_header(start_ptr, size, base64_buf, DECODE_BLEN, &base64_size, 0, nullptr) != 0) + { + snort_free(base64_buf); return NO_MATCH; + } if (idx->bytes_to_decode && (base64_size > idx->bytes_to_decode)) { @@ -153,8 +157,12 @@ IpsOption::EvalStatus Base64DecodeOption::eval(Cursor& c, Packet* p) if (sf_base64decode(base64_buf, base64_size, base64_decode_buffer.data, base64_decode_buffer.decode_blen, &base64_decode_buffer.len) != 0) - return NO_MATCH; + { + snort_free(base64_buf); + return NO_MATCH; + } + snort_free(base64_buf); return MATCH; } diff --git a/src/loggers/unified2.cc b/src/loggers/unified2.cc index 4595ce9c0..8a44202fa 100644 --- a/src/loggers/unified2.cc +++ b/src/loggers/unified2.cc @@ -73,7 +73,7 @@ struct U2 FILE* stream; unsigned int current; int base_proto; - uint32_t timestamp; + time_t timestamp; char filepath[STD_BUF]; }; @@ -111,11 +111,11 @@ static void Unified2InitFile(Unified2Config* config) char filepath[STD_BUF]; char* fname_ptr; - u2.timestamp = (uint32_t)time(nullptr); + u2.timestamp = time(nullptr); if (!config->nostamp) { - if (SnortSnprintf(filepath, sizeof(filepath), "%s.%u", + if (SnortSnprintf(filepath, sizeof(filepath), "%s.%lu", u2.filepath, u2.timestamp) != SNORT_SNPRINTF_SUCCESS) { FatalError("unified2 failed to copy file path.\n"); @@ -286,9 +286,7 @@ static void _WriteExtraData(Unified2Config* config, Serial_Unified2_Header hdr; SerialUnified2ExtraData alertdata; Unified2ExtraDataHdr alertHdr; - uint8_t write_buffer[MAX_XDATA_WRITE_BUF_LEN]; - uint8_t* ptr = nullptr; - + uint32_t write_len = sizeof(hdr) + sizeof(alertHdr); alertdata.sensor_id = htonl(tenant_id); @@ -304,7 +302,7 @@ static void _WriteExtraData(Unified2Config* config, alertHdr.event_type = htonl(EVENT_TYPE_EXTRA_DATA); alertHdr.event_length = htonl(write_len - sizeof(hdr)); - if (write_len > sizeof(write_buffer)) + if (write_len > MAX_XDATA_WRITE_BUF_LEN) return; if ( config->limit && (u2.current + write_len) > config->limit ) @@ -313,26 +311,26 @@ static void _WriteExtraData(Unified2Config* config, hdr.length = htonl(write_len - sizeof(hdr)); hdr.type = htonl(UNIFIED2_EXTRA_DATA); - ptr = write_buffer; - - memcpy_s(ptr, sizeof(write_buffer), &hdr, sizeof(hdr)); + uint8_t* write_buffer = (uint8_t*)snort_alloc(MAX_XDATA_WRITE_BUF_LEN); + uint8_t* ptr = write_buffer; + memcpy_s(ptr, MAX_XDATA_WRITE_BUF_LEN, &hdr, sizeof(hdr)); size_t offset = sizeof(hdr); - memcpy_s(ptr + offset, sizeof(write_buffer) - offset, &alertHdr, sizeof(alertHdr)); - + memcpy_s(ptr + offset, MAX_XDATA_WRITE_BUF_LEN - offset, &alertHdr, sizeof(alertHdr)); offset += sizeof(alertHdr); - memcpy_s(ptr + offset, sizeof(write_buffer) - offset, &alertdata, sizeof(alertdata)); - + memcpy_s(ptr + offset, MAX_XDATA_WRITE_BUF_LEN - offset, &alertdata, sizeof(alertdata)); offset += sizeof(alertdata); - memcpy_s(ptr + offset, sizeof(write_buffer) - offset, buffer, len); + memcpy_s(ptr + offset, MAX_XDATA_WRITE_BUF_LEN - offset, buffer, len); if (obf) obfuscate(ptr + offset, obf, type); Unified2Write(write_buffer, write_len, config); + + snort_free(write_buffer); } static void AlertExtraData( @@ -505,7 +503,7 @@ static void Unified2Write(uint8_t* buf, uint32_t buf_len, Unified2Config* config } else { - ErrorMessage("unified2 failed to write to file (%s.%u): %s\n", + ErrorMessage("unified2 failed to write to file (%s.%lu): %s\n", u2.filepath, u2.timestamp, get_error(error)); } @@ -556,7 +554,7 @@ static void Unified2Write(uint8_t* buf, uint32_t buf_len, Unified2Config* config } else { - ErrorMessage("unified2 rotated file: %s.%u\n", u2.filepath, u2.timestamp); + ErrorMessage("unified2 rotated file: %s.%lu\n", u2.filepath, u2.timestamp); } if (((fwcount = fwrite(buf, (size_t)buf_len, 1, u2.stream)) == 1) && @@ -580,7 +578,7 @@ static void Unified2Write(uint8_t* buf, uint32_t buf_len, Unified2Config* config } else { - ErrorMessage("unified2 failed to write to file (%s.%u): %s\n", + ErrorMessage("unified2 failed to write to file (%s.%lu): %s\n", u2.filepath, u2.timestamp, get_error(error)); } diff --git a/src/main.cc b/src/main.cc index 2913e47c4..8ab1ebcfa 100644 --- a/src/main.cc +++ b/src/main.cc @@ -879,10 +879,8 @@ static int signal_check() { PigSignal s = get_pending_signal(); - if ( s == PIG_SIG_NONE or s >= PIG_SIG_MAX ) - return 0; - - LogMessage("** caught %s signal\n", get_signal_name(s)); + if ( s > PIG_SIG_NONE and s < PIG_SIG_MAX ) + LogMessage("** caught %s signal\n", get_signal_name(s)); switch ( s ) { @@ -913,8 +911,12 @@ static int signal_check() case PIG_SIG_ROTATE_STATS: main_rotate_stats(); break; + default: - break; + // if signal is not handled, return 0 + if ( s >= PIG_SIG_MAX ) + LogMessage("** caught unknown signal - %u\n", s); + return 0; } proc_stats.signals++; return 1; diff --git a/src/main/modules.cc b/src/main/modules.cc index f0591d39a..9106c3238 100644 --- a/src/main/modules.cc +++ b/src/main/modules.cc @@ -1601,7 +1601,7 @@ class RateFilterModule : public Module { public: RateFilterModule() : Module("rate_filter", rate_filter_help, rate_filter_params, true) - { thdx.applyTo = nullptr; } + { thdx.init(); } ~RateFilterModule() override; bool set(const char*, Value&, SnortConfig*) override; @@ -1670,7 +1670,7 @@ bool RateFilterModule::set(const char*, Value& v, SnortConfig*) bool RateFilterModule::begin(const char*, int, SnortConfig* sc) { SFRF_Alloc(sc->rate_filter_config->memcap); - memset(&thdx, 0, sizeof(thdx)); + thdx.init(); return true; } diff --git a/src/main/policy.cc b/src/main/policy.cc index bd3b53b49..c568001d2 100644 --- a/src/main/policy.cc +++ b/src/main/policy.cc @@ -93,7 +93,7 @@ void NetworkPolicy::init(NetworkPolicy* other_network_policy, const char* exclud } user_inspection = other_network_policy->user_inspection; // Fix references to inspection_policy[0] - for ( auto p : other_network_policy->user_inspection ) + for ( const auto& p : other_network_policy->user_inspection ) { if ( p.second == other_network_policy->inspection_policy[0] ) user_inspection[p.first] = inspection_policy[0]; @@ -302,7 +302,7 @@ void PolicyMap::clone(PolicyMap *other_map, const char* exclude_name) shell_map = other_map->shell_map; // Fix references to network_policy[0] and inspection_policy[0] - for ( auto p : other_map->shell_map ) + for ( auto& p : other_map->shell_map ) { for ( unsigned idx = 0; idx < other_map->network_policy.size(); ++idx) { diff --git a/src/main/process.cc b/src/main/process.cc index 30b287baf..c0efd7495 100644 --- a/src/main/process.cc +++ b/src/main/process.cc @@ -568,19 +568,17 @@ void help_signals() static void snuff_stdio() { - bool err = (close(STDIN_FILENO) != 0); - err = err or (close(STDOUT_FILENO) != 0); - err = err or (close(STDERR_FILENO) != 0); - - /* redirect stdin/stdout/stderr to /dev/null */ - err = err or (open("/dev/null", O_RDWR) != STDIN_FILENO); // fd 0 - - err = err or (dup(STDIN_FILENO) != STDOUT_FILENO); // fd 0 => fd 1 - err = err or (dup(STDIN_FILENO) != STDERR_FILENO); // fd 0 => fd 2 - - if ( err ) - // message is hit or miss but we will exit with failure - FatalError("failed to snuff stdio - %s", get_error(errno)); + // Redirect stdin to /dev/null + if (freopen("/dev/null", "r", stdin) == nullptr) + FatalError("failed to snuff stdin - %s", get_error(errno)); + + // Redirect stdout to /dev/null + if (freopen("/dev/null", "w", stdout) == nullptr) + FatalError("failed to snuff stdout - %s", get_error(errno)); + + // Redirect stderr to /dev/null + if (freopen("/dev/null", "w", stderr) == nullptr) + FatalError("failed to snuff stderr - %s", get_error(errno)); } // All threads need to be created after daemonizing. If created in the diff --git a/src/main/thread.cc b/src/main/thread.cc index 5478472d9..744f49499 100644 --- a/src/main/thread.cc +++ b/src/main/thread.cc @@ -28,6 +28,7 @@ #include #include "log/messages.h" +#include "utils/util.h" #include "snort.h" #include "snort_config.h" @@ -157,7 +158,9 @@ const char* get_instance_file(std::string& file, const char* name) if ( stat(file.c_str(), &s) ) // FIXIT-L getting random 0750 or 0700 (umask not thread local)? - mkdir(file.c_str(), 0770); + if ( mkdir(file.c_str(), 0770) == -1 ) + ParseError("Failed to create directory %s - %s", + file.c_str(), get_error(errno)); } else if ( sep ) file += '_'; diff --git a/src/managers/action_manager.cc b/src/managers/action_manager.cc index 657cb90dd..a8ee6de82 100644 --- a/src/managers/action_manager.cc +++ b/src/managers/action_manager.cc @@ -241,7 +241,7 @@ void ActionManager::initialize_policies(SnortConfig* sc) if ( !policy ) continue; - for ( auto actor : s_actors ) + for ( const auto& actor : s_actors ) ActionManager::instantiate(actor.api, nullptr, sc, policy); } } diff --git a/src/managers/module_manager.cc b/src/managers/module_manager.cc index c9121fb8a..3c07b32a7 100644 --- a/src/managers/module_manager.cc +++ b/src/managers/module_manager.cc @@ -1762,7 +1762,7 @@ void ModuleManager::dump_rules(const char* pfx, const char* opts) { std::vector rule_set = get_rules(pfx); - for ( auto rp : rule_set ) + for ( const auto& rp : rule_set ) make_rule(cout, rp.mod, rp.rule, opts); if ( !rule_set.size() ) @@ -1773,7 +1773,7 @@ void ModuleManager::show_rules(const char* pfx, bool exact) { std::vector rule_set = get_rules(pfx, exact); - for ( auto rp : rule_set ) + for ( auto& rp : rule_set ) { cout << Markup::item(); cout << Markup::emphasis_on(); diff --git a/src/managers/plugin_manager.cc b/src/managers/plugin_manager.cc index e49be4b54..6f0f4ec09 100644 --- a/src/managers/plugin_manager.cc +++ b/src/managers/plugin_manager.cc @@ -216,7 +216,7 @@ static bool register_plugin( p.key = key; p.api = api; - p.handle = handle; + p.handle = std::move(handle); p.source = file; return true; diff --git a/src/profiler/rule_profiler.cc b/src/profiler/rule_profiler.cc index df01e894b..c9ecd87a6 100644 --- a/src/profiler/rule_profiler.cc +++ b/src/profiler/rule_profiler.cc @@ -112,7 +112,7 @@ static std::vector build_entries(const std::unordered_map entries; - for (auto stat : stats) + for (auto& stat : stats) if (stat.second.is_active()) entries.emplace_back(stat.second, stat.first); diff --git a/src/service_inspectors/http_inspect/http_cutter.h b/src/service_inspectors/http_inspect/http_cutter.h index b0ab22adb..4430f3593 100644 --- a/src/service_inspectors/http_inspect/http_cutter.h +++ b/src/service_inspectors/http_inspect/http_cutter.h @@ -138,8 +138,8 @@ private: uint8_t string_length = 0; z_stream* compress_stream = nullptr; ScriptFinder* const finder; - const uint8_t* match_string; - const uint8_t* match_string_upper; + const uint8_t* match_string = nullptr; + const uint8_t* match_string_upper = nullptr; }; class HttpBodyClCutter : public HttpBodyCutter diff --git a/tools/snort2lua/preprocessor_states/pps_ftp_telnet_protocol.cc b/tools/snort2lua/preprocessor_states/pps_ftp_telnet_protocol.cc index 200dfd941..0676e31c5 100644 --- a/tools/snort2lua/preprocessor_states/pps_ftp_telnet_protocol.cc +++ b/tools/snort2lua/preprocessor_states/pps_ftp_telnet_protocol.cc @@ -530,7 +530,7 @@ bool FtpServer::convert(std::istringstream& data_stream) { table_api.open_table("cmd_validity"); - for (auto c : commands) + for (auto& c : commands) { table_api.open_table(); bool tmpval1 = table_api.add_option("command", c.name); diff --git a/tools/snort2lua/preprocessor_states/pps_smtp.cc b/tools/snort2lua/preprocessor_states/pps_smtp.cc index b4d50333e..c91ed2f1b 100644 --- a/tools/snort2lua/preprocessor_states/pps_smtp.cc +++ b/tools/snort2lua/preprocessor_states/pps_smtp.cc @@ -339,7 +339,7 @@ bool Smtp::convert(std::istringstream& data_stream) { table_api.open_table("alt_max_command_line_len"); - for (auto c : commands) + for (const auto& c : commands) { table_api.open_table(); bool tmpval1 = table_api.add_option("command", c.name); -- 2.47.2