Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

De cleanups/20241010/v2 #11937

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 24 additions & 41 deletions src/detect-engine-build.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
#include "util-conf.h"

/* Magic numbers to make the rules of a certain order fall in the same group */
#define DETECT_PGSCORE_RULE_PORT_WHITELISTED 111 /* Rule port group contains a whitelisted port */
#define DETECT_PGSCORE_RULE_PORT_PRIORITIZED 111 /* Rule port group contains a priority port */
#define DETECT_PGSCORE_RULE_MPM_FAST_PATTERN 99 /* Rule contains an MPM fast pattern */
#define DETECT_PGSCORE_RULE_MPM_NEGATED 77 /* Rule contains a negated MPM */
#define DETECT_PGSCORE_RULE_NO_MPM 55 /* Rule does not contain MPM */
Expand Down Expand Up @@ -487,27 +487,11 @@ static int SignatureCreateMask(Signature *s)
{
DetectFlagsData *fl = (DetectFlagsData *)sm->ctx;

if (fl->flags & TH_SYN) {
if (fl->flags & MASK_TCP_INITDEINIT_FLAGS) {
s->mask |= SIG_MASK_REQUIRE_FLAGS_INITDEINIT;
SCLogDebug("sig requires SIG_MASK_REQUIRE_FLAGS_INITDEINIT");
}
if (fl->flags & TH_RST) {
s->mask |= SIG_MASK_REQUIRE_FLAGS_INITDEINIT;
SCLogDebug("sig requires SIG_MASK_REQUIRE_FLAGS_INITDEINIT");
}
if (fl->flags & TH_FIN) {
s->mask |= SIG_MASK_REQUIRE_FLAGS_INITDEINIT;
SCLogDebug("sig requires SIG_MASK_REQUIRE_FLAGS_INITDEINIT");
}
if (fl->flags & TH_URG) {
s->mask |= SIG_MASK_REQUIRE_FLAGS_UNUSUAL;
SCLogDebug("sig requires SIG_MASK_REQUIRE_FLAGS_UNUSUAL");
}
if (fl->flags & TH_ECN) {
s->mask |= SIG_MASK_REQUIRE_FLAGS_UNUSUAL;
SCLogDebug("sig requires SIG_MASK_REQUIRE_FLAGS_UNUSUAL");
}
if (fl->flags & TH_CWR) {
if (fl->flags & MASK_TCP_UNUSUAL_FLAGS) {
s->mask |= SIG_MASK_REQUIRE_FLAGS_UNUSUAL;
SCLogDebug("sig requires SIG_MASK_REQUIRE_FLAGS_UNUSUAL");
}
Expand Down Expand Up @@ -969,7 +953,7 @@ static void RulesDumpGrouping(const DetectEngineCtx *de_ctx,
fclose(fp);
}

static int RulesGroupByProto(DetectEngineCtx *de_ctx)
static int RulesGroupByIPProto(DetectEngineCtx *de_ctx)
{
Signature *s = de_ctx->sig_list;

Expand All @@ -980,15 +964,16 @@ static int RulesGroupByProto(DetectEngineCtx *de_ctx)
if (s->type == SIG_TYPE_IPONLY)
continue;

int p;
for (p = 0; p < 256; p++) {
/* traverse over IP protocol list from libc */
for (int p = 0; p < 256; p++) {
if (p == IPPROTO_TCP || p == IPPROTO_UDP) {
continue;
}
if (!(s->proto.proto[p / 8] & (1<<(p % 8)) || (s->proto.flags & DETECT_PROTO_ANY))) {
continue;
}

/* Signatures that are !IP only, ICMP, SCTP are handlnd here */
if (s->flags & SIG_FLAG_TOCLIENT) {
SigGroupHeadAppendSig(de_ctx, &sgh_tc[p], s);
}
Expand Down Expand Up @@ -1079,15 +1064,14 @@ static int RulesGroupByProto(DetectEngineCtx *de_ctx)
return 0;
}

static int PortIsWhitelisted(const DetectEngineCtx *de_ctx,
const DetectPort *a, int ipproto)
static int PortIsPriority(const DetectEngineCtx *de_ctx, const DetectPort *a, int ipproto)
{
DetectPort *w = de_ctx->tcp_whitelist;
DetectPort *w = de_ctx->tcp_priorityports;
if (ipproto == IPPROTO_UDP)
w = de_ctx->udp_whitelist;
w = de_ctx->udp_priorityports;

while (w) {
/* Make sure the whitelist port falls in the port range of a */
/* Make sure the priority port falls in the port range of a */
DEBUG_VALIDATE_BUG_ON(a->port > a->port2);
if (a->port == w->port && w->port2 == a->port2) {
return 1;
Expand All @@ -1098,7 +1082,7 @@ static int PortIsWhitelisted(const DetectEngineCtx *de_ctx,
return 0;
}

static int RuleSetWhitelist(Signature *s)
static int RuleSetScore(Signature *s)
{
DetectPort *p = NULL;
if (s->flags & SIG_FLAG_TOSERVER)
Expand All @@ -1109,27 +1093,27 @@ static int RuleSetWhitelist(Signature *s)
return 0;

/* for sigs that don't use 'any' as port, see if we want to
* whitelist poor sigs */
* prioritize poor sigs */
int wl = 0;
if (!(p->port == 0 && p->port2 == 65535)) {
/* pure pcre, bytetest, etc rules */
if (RuleInspectsPayloadHasNoMpm(s)) {
SCLogDebug("Rule %u MPM has 1 byte fast_pattern. Whitelisting SGH's.", s->id);
SCLogDebug("Rule %u MPM has 1 byte fast_pattern. Prioritizing SGH's.", s->id);
wl = DETECT_PGSCORE_RULE_MPM_FAST_PATTERN;

} else if (RuleMpmIsNegated(s)) {
SCLogDebug("Rule %u MPM is negated. Whitelisting SGH's.", s->id);
SCLogDebug("Rule %u MPM is negated. Prioritizing SGH's.", s->id);
wl = DETECT_PGSCORE_RULE_MPM_NEGATED;

/* one byte pattern in packet/stream payloads */
} else if (s->init_data->mpm_sm != NULL &&
s->init_data->mpm_sm_list == DETECT_SM_LIST_PMATCH &&
RuleGetMpmPatternSize(s) == 1) {
SCLogDebug("Rule %u No MPM. Payload inspecting. Whitelisting SGH's.", s->id);
SCLogDebug("Rule %u No MPM. Payload inspecting. Prioritizing SGH's.", s->id);
wl = DETECT_PGSCORE_RULE_NO_MPM;

} else if (DetectFlagsSignatureNeedsSynOnlyPackets(s)) {
SCLogDebug("Rule %u Needs SYN, so inspected often. Whitelisting SGH's.", s->id);
SCLogDebug("Rule %u Needs SYN, so inspected often. Prioritizing SGH's.", s->id);
wl = DETECT_PGSCORE_RULE_SYN_ONLY;
}
}
Expand Down Expand Up @@ -1247,7 +1231,7 @@ static int CreateGroupedPortList(DetectEngineCtx *de_ctx, DetectPort *port_list,

/* insert the ports into the tmplist, where it will
* be sorted descending on 'cnt' and on whether a group
* is whitelisted. */
* is prioritized. */
tmplist = port_list;
SortGroupList(&groups, &tmplist, SortCompare);
uint32_t left = unique_groups;
Expand Down Expand Up @@ -1535,8 +1519,7 @@ static DetectPort *RulesGroupByPorts(DetectEngineCtx *de_ctx, uint8_t ipproto, u

int wl = s->init_data->score;
while (p) {
int pwl = PortIsWhitelisted(de_ctx, p, ipproto) ? DETECT_PGSCORE_RULE_PORT_WHITELISTED
: 0;
int pwl = PortIsPriority(de_ctx, p, ipproto) ? DETECT_PGSCORE_RULE_PORT_PRIORITIZED : 0;
pwl = MAX(wl,pwl);

DetectPort *lookup = DetectPortHashLookup(de_ctx, p);
Expand Down Expand Up @@ -1633,11 +1616,11 @@ static DetectPort *RulesGroupByPorts(DetectEngineCtx *de_ctx, uint8_t ipproto, u
}
#if 0
for (iter = list ; iter != NULL; iter = iter->next) {
SCLogInfo("PORT %u-%u %p (sgh=%s, whitelisted=%s/%d)",
SCLogInfo("PORT %u-%u %p (sgh=%s, prioritized=%s/%d)",
iter->port, iter->port2, iter->sh,
iter->flags & PORT_SIGGROUPHEAD_COPY ? "ref" : "own",
iter->sh->init->whitelist ? "true" : "false",
iter->sh->init->whitelist);
iter->sh->init->score ? "true" : "false",
iter->sh->init->score);
}
#endif
SCLogPerf("%s %s: %u port groups, %u unique SGH's, %u copies",
Expand Down Expand Up @@ -1802,7 +1785,7 @@ int SigPrepareStage1(DetectEngineCtx *de_ctx)
DetectContentPropagateLimits(s);
SigParseApplyDsizeToContent(s);

RuleSetWhitelist(s);
RuleSetScore(s);

/* if keyword engines are enabled in the config, handle them here */
if (!g_skip_prefilter && de_ctx->prefilter_setting == DETECT_PREFILTER_AUTO &&
Expand Down Expand Up @@ -1907,7 +1890,7 @@ int SigPrepareStage2(DetectEngineCtx *de_ctx)
de_ctx->flow_gh[0].udp = RulesGroupByPorts(de_ctx, IPPROTO_UDP, SIG_FLAG_TOCLIENT);

/* Setup the other IP Protocols (so not TCP/UDP) */
RulesGroupByProto(de_ctx);
RulesGroupByIPProto(de_ctx);

/* now for every rule add the source group to our temp lists */
for (Signature *s = de_ctx->sig_list; s != NULL; s = s->next) {
Expand Down
17 changes: 8 additions & 9 deletions src/detect-engine-proto.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* Copyright (C) 2007-2010 Open Information Security Foundation
/* Copyright (C) 2007-2024 Open Information Security Foundation
*
* You can copy, redistribute or modify this Program under the terms of
* the GNU General Public License version 2 as published by the Free
Expand All @@ -24,14 +24,13 @@
#ifndef SURICATA_DETECT_PROTO_H
#define SURICATA_DETECT_PROTO_H

#define DETECT_PROTO_ANY (1 << 0) /**< Indicate that given protocol
is considered as IP */
#define DETECT_PROTO_ONLY_PKT (1 << 1) /**< Indicate that we only care
about packet payloads. */
#define DETECT_PROTO_ONLY_STREAM (1 << 2) /**< Indicate that we only care
about stream payloads. */
#define DETECT_PROTO_IPV4 (1 << 3) /**< IPv4 only */
#define DETECT_PROTO_IPV6 (1 << 4) /**< IPv6 only */
// clang-format off
#define DETECT_PROTO_ANY BIT_U8(0) /**< Indicate that given protocol is considered as IP */
#define DETECT_PROTO_ONLY_PKT BIT_U8(1) /**< Indicate that we only care about packet payloads. */
#define DETECT_PROTO_ONLY_STREAM BIT_U8(2) /**< Indicate that we only care about stream payloads. */
#define DETECT_PROTO_IPV4 BIT_U8(3) /**< IPv4 only */
#define DETECT_PROTO_IPV6 BIT_U8(4) /**< IPv6 only */
// clang-format on

typedef struct DetectProto_ {
uint8_t proto[256/8]; /**< bit array for 256 protocol bits */
Expand Down
44 changes: 21 additions & 23 deletions src/detect-engine.c
Original file line number Diff line number Diff line change
Expand Up @@ -2662,8 +2662,8 @@ void DetectEngineCtxFree(DetectEngineCtx *de_ctx)
#endif
}

DetectPortCleanupList(de_ctx, de_ctx->tcp_whitelist);
DetectPortCleanupList(de_ctx, de_ctx->udp_whitelist);
DetectPortCleanupList(de_ctx, de_ctx->tcp_priorityports);
DetectPortCleanupList(de_ctx, de_ctx->udp_priorityports);

DetectBufferTypeFreeDetectEngine(de_ctx);
SCClassConfDeinit(de_ctx);
Expand Down Expand Up @@ -2915,55 +2915,53 @@ static int DetectEngineCtxLoadConf(DetectEngineCtx *de_ctx)
}
}

/* parse port grouping whitelisting settings */
/* parse port grouping priority settings */

const char *ports = NULL;
(void)ConfGet("detect.grouping.tcp-whitelist", &ports);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to handle old configs as well, which means we first read the new tcp-priorityports option, but if it doesn't exist, we try tcp-whitelist

Btw tcp-priority-ports looks better to me.

(void)ConfGet("detect.grouping.tcp-priorityports", &ports);
if (ports) {
SCLogConfig("grouping: tcp-whitelist %s", ports);
SCLogConfig("grouping: tcp-priorityports %s", ports);
} else {
ports = "53, 80, 139, 443, 445, 1433, 3306, 3389, 6666, 6667, 8080";
SCLogConfig("grouping: tcp-whitelist (default) %s", ports);

SCLogConfig("grouping: tcp-priorityports (default) %s", ports);
}
if (DetectPortParse(de_ctx, &de_ctx->tcp_whitelist, ports) != 0) {
if (DetectPortParse(de_ctx, &de_ctx->tcp_priorityports, ports) != 0) {
SCLogWarning("'%s' is not a valid value "
"for detect.grouping.tcp-whitelist",
"for detect.grouping.tcp-priorityports",
ports);
}
DetectPort *x = de_ctx->tcp_whitelist;
DetectPort *x = de_ctx->tcp_priorityports;
for ( ; x != NULL; x = x->next) {
if (x->port != x->port2) {
SCLogWarning("'%s' is not a valid value "
"for detect.grouping.tcp-whitelist: only single ports allowed",
"for detect.grouping.tcp-priorityports: only single ports allowed",
ports);
DetectPortCleanupList(de_ctx, de_ctx->tcp_whitelist);
de_ctx->tcp_whitelist = NULL;
DetectPortCleanupList(de_ctx, de_ctx->tcp_priorityports);
de_ctx->tcp_priorityports = NULL;
break;
}
}

ports = NULL;
(void)ConfGet("detect.grouping.udp-whitelist", &ports);
(void)ConfGet("detect.grouping.udp-priorityports", &ports);
if (ports) {
SCLogConfig("grouping: udp-whitelist %s", ports);
SCLogConfig("grouping: udp-priorityports %s", ports);
} else {
ports = "53, 135, 5060";
SCLogConfig("grouping: udp-whitelist (default) %s", ports);

SCLogConfig("grouping: udp-priorityports (default) %s", ports);
}
if (DetectPortParse(de_ctx, &de_ctx->udp_whitelist, ports) != 0) {
if (DetectPortParse(de_ctx, &de_ctx->udp_priorityports, ports) != 0) {
SCLogWarning("'%s' is not a valid value "
"for detect.grouping.udp-whitelist",
"for detect.grouping.udp-priorityports",
ports);
}
for (x = de_ctx->udp_whitelist; x != NULL; x = x->next) {
for (x = de_ctx->udp_priorityports; x != NULL; x = x->next) {
if (x->port != x->port2) {
SCLogWarning("'%s' is not a valid value "
"for detect.grouping.udp-whitelist: only single ports allowed",
"for detect.grouping.udp-priorityports: only single ports allowed",
ports);
DetectPortCleanupList(de_ctx, de_ctx->udp_whitelist);
de_ctx->udp_whitelist = NULL;
DetectPortCleanupList(de_ctx, de_ctx->udp_priorityports);
de_ctx->udp_priorityports = NULL;
break;
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/detect.h
Original file line number Diff line number Diff line change
Expand Up @@ -974,8 +974,8 @@ typedef struct DetectEngineCtx_ {

HashListTable *dport_hash_table;

DetectPort *tcp_whitelist;
DetectPort *udp_whitelist;
DetectPort *tcp_priorityports;
DetectPort *udp_priorityports;

/** table for storing the string representation with the parsers result */
HashListTable *address_table;
Expand Down
6 changes: 3 additions & 3 deletions suricata.yaml.in
Original file line number Diff line number Diff line change
Expand Up @@ -1701,12 +1701,12 @@ detect:
default: mpm

# the grouping values above control how many groups are created per
# direction. Port whitelisting forces that port to get its own group.
# direction. Port priority setting forces that port to get its own group.
# Very common ports will benefit, as well as ports with many expensive
# rules.
grouping:
#tcp-whitelist: 53, 80, 139, 443, 445, 1433, 3306, 3389, 6666, 6667, 8080
#udp-whitelist: 53, 135, 5060
#tcp-priorityports: 53, 80, 139, 443, 445, 1433, 3306, 3389, 6666, 6667, 8080
#udp-priorityports: 53, 135, 5060

# Thresholding hash table settings.
thresholds:
Expand Down
Loading