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
typedef enum
{
+ SFRF_TRACK_BY_NONE = 0,
SFRF_TRACK_BY_SRC = 1,
SFRF_TRACK_BY_DST,
SFRF_TRACK_BY_RULE,
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
// 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 )
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);
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);
#include "discovery_filter.h"
+#include <algorithm>
#include <fstream>
#include <netdb.h>
#include <sstream>
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;",
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);
#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;
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))
{
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;
}
FILE* stream;
unsigned int current;
int base_proto;
- uint32_t timestamp;
+ time_t timestamp;
char filepath[STD_BUF];
};
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");
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);
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 )
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(
}
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));
}
}
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) &&
}
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));
}
{
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 )
{
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;
{
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;
bool RateFilterModule::begin(const char*, int, SnortConfig* sc)
{
SFRF_Alloc(sc->rate_filter_config->memcap);
- memset(&thdx, 0, sizeof(thdx));
+ thdx.init();
return true;
}
}
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];
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)
{
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
#include <sys/stat.h>
#include "log/messages.h"
+#include "utils/util.h"
#include "snort.h"
#include "snort_config.h"
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 += '_';
if ( !policy )
continue;
- for ( auto actor : s_actors )
+ for ( const auto& actor : s_actors )
ActionManager::instantiate(actor.api, nullptr, sc, policy);
}
}
{
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() )
{
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();
p.key = key;
p.api = api;
- p.handle = handle;
+ p.handle = std::move(handle);
p.source = file;
return true;
{
std::vector<View> entries;
- for (auto stat : stats)
+ for (auto& stat : stats)
if (stat.second.is_active())
entries.emplace_back(stat.second, stat.first);
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
{
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);
{
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);