]> git.ipfire.org Git - thirdparty/snort3.git/commitdiff
Pull request #4654: snort3: resolve issues reported by Coverity static analysis
authorDavis McPherson -X (davmcphe - XORIANT CORPORATION at Cisco) <davmcphe@cisco.com>
Tue, 1 Apr 2025 15:53:39 +0000 (15:53 +0000)
committerSteven Baigal (sbaigal) <sbaigal@cisco.com>
Tue, 1 Apr 2025 15:53:39 +0000 (15:53 +0000)
Merge in SNORT/snort3 from ~DAVMCPHE/snort3:resolve_coverity_issues to master

Squashed commit of the following:

commit dbbb96a44df54ec5d8074befd0b2be937950ace8
Author: davis mcpherson <davmcphe@cisco.com>
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 <davmcphe@cisco.com>
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 <davmcphe@cisco.com>
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

20 files changed:
src/filters/sfrf.h
src/flow/expect_cache.cc
src/flow/flow_cache.cc
src/flow/flow_cache.h
src/helpers/discovery_filter.cc
src/host_tracker/host_cache_module.cc
src/ips_options/ips_base64.cc
src/loggers/unified2.cc
src/main.cc
src/main/modules.cc
src/main/policy.cc
src/main/process.cc
src/main/thread.cc
src/managers/action_manager.cc
src/managers/module_manager.cc
src/managers/plugin_manager.cc
src/profiler/rule_profiler.cc
src/service_inspectors/http_inspect/http_cutter.h
tools/snort2lua/preprocessor_states/pps_ftp_telnet_protocol.cc
tools/snort2lua/preprocessor_states/pps_smtp.cc

index 0489186c9894a5dcbb346b33dd0896f6e36f29af..c55b156fea3e4296d0f4a63ec757bff676e5067b 100644 (file)
@@ -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
index c83c04e9e5c15e3b6c2f8538d6658e40ec7bc75e..ae31ae959e8f0be724587856482e21ceaed79497 100644 (file)
@@ -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<ExpectNode*> ( hash_table->get_user_data(&key) );
     if ( !node )
index f96e0d01cf816e0d70d2b2de6782958b33f257c3..238426522c88fca1508eb50997fbf06a14009dfd 100644 (file)
@@ -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);
 
index 6aa5778fcacaf53f1679f665a8cd8aee8f2e3e8c..9caf05bb9625c765115cd1a2cfcf09e4492da44e 100644 (file)
@@ -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);
index 7539789379ffade6b4746fa9412e049d21e6ba05..d1a915083f0f67af0e1a1d184aac126c8d736841 100644 (file)
@@ -24,6 +24,7 @@
 
 #include "discovery_filter.h"
 
+#include <algorithm>
 #include <fstream>
 #include <netdb.h>
 #include <sstream>
@@ -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;",
index 8c5267ad99f5a67722b5fc98e5da81841af2aad5..2d898e3986fbd44685880b499c5701d5a21321c6 100644 (file)
@@ -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);
index a63db46ad57bb24b15f9fc9af43835a82fc6fd2e..f1a0d2b2846bb9a2bd4144b40d3c9874b39c83bc 100644 (file)
@@ -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;
 }
 
index 4595ce9c077a917a0896ac1e913dbb0bb41aad7c..8a44202fa8aed82aed9a86e1b3d9afa91d285b3a 100644 (file)
@@ -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));
                 }
 
index 2913e47c4e3641c5870e0b2b4ebb9218cea6749a..8ab1ebcfa8c6c3139261ffcd7fcfb7bab4b04bf3 100644 (file)
@@ -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;
index f0591d39adab0ae56d4e78b38bc79ea57759c245..9106c3238ce8e285dc8bb4247d2120eaf4a3f53b 100644 (file)
@@ -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;
 }
 
index bd3b53b49e84f2603578ec97aa96d70539de538e..c568001d237db18508a7bcdbbe0237d3128c8407 100644 (file)
@@ -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)
         {
index 30b287baf682093ea1d3a0f201cbab6eec7adfdb..c0efd749592cab93c94b050892fbae282f67cec9 100644 (file)
@@ -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
index 5478472d9db007bafbbc516dfd2ecc59c62fa169..744f4949934defb7ccf1840aa3a0c5cf044f9458 100644 (file)
@@ -28,6 +28,7 @@
 #include <sys/stat.h>
 
 #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 += '_';
index 657cb90dd5531cbf6a7619da1f5de5e23127f059..a8ee6de82b6104b204c94b1b634f3a0e62d298a2 100644 (file)
@@ -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);
     }
 }
index c9121fb8aa0a529febfb63ed01982cd15dfce326..3c07b32a751eeecc9f50fafd8fc4d1225f5c024c 100644 (file)
@@ -1762,7 +1762,7 @@ void ModuleManager::dump_rules(const char* pfx, const char* opts)
 {
     std::vector<RulePtr> 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<RulePtr> rule_set = get_rules(pfx, exact);
 
-    for ( auto rp : rule_set )
+    for ( auto& rp : rule_set )
     {
         cout << Markup::item();
         cout << Markup::emphasis_on();
index e49be4b54f20f70ebe9468ecabcc841b6ccb5269..6f0f4ec09c80aa1261817c4e50f0002c011d0a1e 100644 (file)
@@ -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;
index df01e894b19ff1260b153a3e5abbfc655252fa82..c9ecd87a6384f6386a4f07e214fe97def7cf50ae 100644 (file)
@@ -112,7 +112,7 @@ static std::vector<View> build_entries(const std::unordered_map<SigInfo*, OtnSta
 {
     std::vector<View> entries;
 
-    for (auto stat : stats)
+    for (auto& stat : stats)
         if (stat.second.is_active())
             entries.emplace_back(stat.second, stat.first);
 
index b0ab22adb30e867d6e31b02396dd50cb8385880c..4430f3593182436c24490422c1ab25ea89f42b6e 100644 (file)
@@ -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
index 200dfd941c4d7f4e92b5ff41f74bd14b22ae6827..0676e31c5b60f686e2f3f03020e7098179f72a70 100644 (file)
@@ -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);
index b4d50333ed7c3720a8cadd56c7b4655e0ec92b92..c91ed2f1b4b46807bcb271e102881fa6b0d222e2 100644 (file)
@@ -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);