Skip to content

Detect rule types hooks/v18 #8906

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.

Sign up for GitHub

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

Closed
victorjulien wants to merge 3 commits into from
Closed

Detect rule types hooks/v18 #8906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
437788d
detect: minor cleanup
victorjulien Jul 29, 2022
30ca7d9
detect/pcre: remove redundant applayer flag set
victorjulien Aug 2, 2022
6775e90
detect: use explicit rule types
victorjulien Apr 28, 2022
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
13 changes: 6 additions & 7 deletions src/detect-app-layer-protocol.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,8 @@ static int DetectAppLayerProtocolPacketMatch(
const DetectAppLayerProtocolData *data = (const DetectAppLayerProtocolData *)ctx;

/* if the sig is PD-only we only match when PD packet flags are set */
if ((s->flags & SIG_FLAG_PDONLY) &&
(p->flags & (PKT_PROTO_DETECT_TS_DONE|PKT_PROTO_DETECT_TC_DONE)) == 0)
{
if (s->type == SIG_TYPE_PDONLY &&
(p->flags & (PKT_PROTO_DETECT_TS_DONE | PKT_PROTO_DETECT_TC_DONE)) == 0) {
SCLogDebug("packet %"PRIu64": flags not set", p->pcap_cnt);
SCReturnInt(0);
}
Expand Down Expand Up @@ -258,7 +257,7 @@ static int PrefilterSetupAppProto(DetectEngineCtx *de_ctx, SigGroupHead *sgh)

static bool PrefilterAppProtoIsPrefilterable(const Signature *s)
{
if (s->flags & SIG_FLAG_PDONLY) {
if (s->type == SIG_TYPE_PDONLY) {
SCLogDebug("prefilter on PD %u", s->id);
return true;
}
Expand Down Expand Up @@ -548,9 +547,9 @@ static int DetectAppLayerProtocolTest14(void)
FAIL_IF(data->negated);

SigGroupBuild(de_ctx);
FAIL_IF_NOT(s1->flags & SIG_FLAG_PDONLY);
FAIL_IF_NOT(s2->flags & SIG_FLAG_PDONLY);
FAIL_IF(s3->flags & SIG_FLAG_PDONLY); // failure now
FAIL_IF_NOT(s1->type == SIG_TYPE_PDONLY);
FAIL_IF_NOT(s2->type == SIG_TYPE_PDONLY);
FAIL_IF(s3->type == SIG_TYPE_PDONLY); // failure now

DetectEngineCtxFree(de_ctx);
PASS;
Expand Down
15 changes: 12 additions & 3 deletions src/detect-engine-alert.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,10 +335,19 @@ static inline void FlowApplySignatureActions(
* - match is in applayer
* - match is in stream */
if (s->action & (ACTION_DROP | ACTION_PASS)) {
if ((pa->flags & (PACKET_ALERT_FLAG_STATE_MATCH | PACKET_ALERT_FLAG_STREAM_MATCH)) ||
(s->flags & (SIG_FLAG_IPONLY | SIG_FLAG_LIKE_IPONLY | SIG_FLAG_PDONLY |
SIG_FLAG_APPLAYER))) {
DEBUG_VALIDATE_BUG_ON(s->type == SIG_TYPE_NOT_SET);
DEBUG_VALIDATE_BUG_ON(s->type == SIG_TYPE_MAX);

enum SignaturePropertyFlowAction flow_action = signature_properties[s->type].flow_action;
if (flow_action == SIG_PROP_FLOW_ACTION_FLOW) {
pa->flags |= PACKET_ALERT_FLAG_APPLY_ACTION_TO_FLOW;
} else if (flow_action == SIG_PROP_FLOW_ACTION_FLOW_IF_STATEFUL) {
if (pa->flags & (PACKET_ALERT_FLAG_STATE_MATCH | PACKET_ALERT_FLAG_STREAM_MATCH)) {
pa->flags |= PACKET_ALERT_FLAG_APPLY_ACTION_TO_FLOW;
}
}

if (pa->flags & PACKET_ALERT_FLAG_APPLY_ACTION_TO_FLOW) {
SCLogDebug("packet %" PRIu64 " sid %u action %02x alert_flags %02x (set "
"PACKET_ALERT_FLAG_APPLY_ACTION_TO_FLOW)",
p->pcap_cnt, s->id, s->action, pa->flags);
Expand Down
104 changes: 96 additions & 8 deletions src/detect-engine-analyzer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include "suricata-common.h"
#include "suricata.h"
#include "rust.h"
#include "action-globals.h"
#include "detect.h"
#include "detect-parse.h"
#include "detect-engine.h"
Expand Down Expand Up @@ -841,6 +842,68 @@ void EngineAnalysisRules2(const DetectEngineCtx *de_ctx, const Signature *s)
}
jb_close(ctx.js);

jb_open_object(ctx.js, "policy");
Copy link
Copy Markdown
Member Author

@victorjulien victorjulien May 23, 2023

Choose a reason for hiding this comment

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

@jasonish @jufajardini we should probably consider this and the verdict output discussion we had. It would be good to make these things similar if not the same, and at least use one set of keywords.

Copy link
Copy Markdown
Member

@jasonish jasonish May 23, 2023

Choose a reason for hiding this comment

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

Given we brainstormed on this for a while, can you fit this new "policy" into #8596 (comment)?

Copy link
Copy Markdown
Contributor

@jufajardini jufajardini Jun 2, 2023

Choose a reason for hiding this comment

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

I'm sorry, this escaped me completely last week.
I noticed that this part seems to not have made it to the merged PR (#8928).

Well defining the keywords is one of the first steps, in my opinion, as at the moment 'policy', for instance, only echoes "exception policy" in my head xD

So in engine analysis mode, one would be able to see all actions that are available for rules, and what each of them means, if triggered (aka the 'verdict')?

jb_open_array(ctx.js, "actions");
if (s->action & ACTION_ALERT) {
jb_append_string(ctx.js, "alert");
}
if (s->action & ACTION_DROP) {
jb_append_string(ctx.js, "drop");
}
if (s->action & ACTION_PASS) {
jb_append_string(ctx.js, "pass");
}
jb_close(ctx.js);
enum SignaturePropertyFlowAction flow_action = signature_properties[s->type].flow_action;
switch (flow_action) {
case SIG_PROP_FLOW_ACTION_PACKET:
jb_set_string(ctx.js, "scope", "packet");
break;
case SIG_PROP_FLOW_ACTION_FLOW:
jb_set_string(ctx.js, "scope", "flow");
break;
case SIG_PROP_FLOW_ACTION_FLOW_IF_STATEFUL:
jb_set_string(ctx.js, "scope", "flow_if_stateful");
break;
}
jb_close(ctx.js);

switch (s->type) {
case SIG_TYPE_NOT_SET:
jb_set_string(ctx.js, "type", "unset");
break;
case SIG_TYPE_IPONLY:
jb_set_string(ctx.js, "type", "ip_only");
break;
case SIG_TYPE_LIKE_IPONLY:
jb_set_string(ctx.js, "type", "like_ip_only");
break;
case SIG_TYPE_PDONLY:
jb_set_string(ctx.js, "type", "pd_only");
break;
case SIG_TYPE_DEONLY:
jb_set_string(ctx.js, "type", "de_only");
break;
case SIG_TYPE_PKT:
jb_set_string(ctx.js, "type", "pkt");
break;
case SIG_TYPE_PKT_STREAM:
jb_set_string(ctx.js, "type", "pkt_stream");
break;
case SIG_TYPE_STREAM:
jb_set_string(ctx.js, "type", "stream");
break;
case SIG_TYPE_APPLAYER:
jb_set_string(ctx.js, "type", "app_layer");
break;
case SIG_TYPE_APP_TX:
jb_set_string(ctx.js, "type", "app_tx");
break;
case SIG_TYPE_MAX:
jb_set_string(ctx.js, "type", "error");
break;
}

jb_open_array(ctx.js, "flags");
if (s->flags & SIG_FLAG_SRC_ANY) {
jb_append_string(ctx.js, "src_any");
Expand All @@ -863,9 +926,6 @@ void EngineAnalysisRules2(const DetectEngineCtx *de_ctx, const Signature *s)
if (s->flags & SIG_FLAG_APPLAYER) {
jb_append_string(ctx.js, "applayer");
}
if (s->flags & SIG_FLAG_IPONLY) {
jb_append_string(ctx.js, "ip_only");
}
if (s->flags & SIG_FLAG_REQUIRE_PACKET) {
jb_append_string(ctx.js, "need_packet");
}
Expand Down Expand Up @@ -899,9 +959,6 @@ void EngineAnalysisRules2(const DetectEngineCtx *de_ctx, const Signature *s)
if (s->flags & SIG_FLAG_PREFILTER) {
jb_append_string(ctx.js, "prefilter");
}
if (s->flags & SIG_FLAG_PDONLY) {
jb_append_string(ctx.js, "proto_detect_only");
}
if (s->flags & SIG_FLAG_SRC_IS_TARGET) {
jb_append_string(ctx.js, "src_is_target");
}
Expand Down Expand Up @@ -1483,8 +1540,39 @@ void EngineAnalysisRules(const DetectEngineCtx *de_ctx,
fprintf(rule_engine_analysis_FD, "== Sid: %u ==\n", s->id);
fprintf(rule_engine_analysis_FD, "%s\n", line);

if (s->flags & SIG_FLAG_IPONLY) fprintf(rule_engine_analysis_FD, " Rule is ip only.\n");
if (s->flags & SIG_FLAG_PDONLY) fprintf(rule_engine_analysis_FD, " Rule is PD only.\n");
switch (s->type) {
case SIG_TYPE_NOT_SET:
break;
case SIG_TYPE_IPONLY:
fprintf(rule_engine_analysis_FD, " Rule is ip only.\n");
break;
case SIG_TYPE_LIKE_IPONLY:
fprintf(rule_engine_analysis_FD, " Rule is like ip only.\n");
break;
case SIG_TYPE_PDONLY:
fprintf(rule_engine_analysis_FD, " Rule is PD only.\n");
break;
case SIG_TYPE_DEONLY:
fprintf(rule_engine_analysis_FD, " Rule is DE only.\n");
break;
case SIG_TYPE_PKT:
fprintf(rule_engine_analysis_FD, " Rule is packet inspecting.\n");
break;
case SIG_TYPE_PKT_STREAM:
fprintf(rule_engine_analysis_FD, " Rule is packet and stream inspecting.\n");
break;
case SIG_TYPE_STREAM:
fprintf(rule_engine_analysis_FD, " Rule is stream inspecting.\n");
break;
case SIG_TYPE_APPLAYER:
fprintf(rule_engine_analysis_FD, " Rule is app-layer inspecting.\n");
break;
case SIG_TYPE_APP_TX:
fprintf(rule_engine_analysis_FD, " Rule is App-layer TX inspecting.\n");
break;
case SIG_TYPE_MAX:
break;
}
if (rule_ipv6_only) fprintf(rule_engine_analysis_FD, " Rule is IPv6 only.\n");
if (rule_ipv4_only) fprintf(rule_engine_analysis_FD, " Rule is IPv4 only.\n");
if (packet_buf) fprintf(rule_engine_analysis_FD, " Rule matches on packets.\n");
Expand Down
78 changes: 59 additions & 19 deletions src/detect-engine-build.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -989,7 +989,7 @@ static int RulesGroupByProto(DetectEngineCtx *de_ctx)
SigGroupHead *sgh_tc[256] = {NULL};

for ( ; s != NULL; s = s->next) {
if (s->flags & SIG_FLAG_IPONLY)
if (s->type == SIG_TYPE_IPONLY)
continue;

int p;
Expand Down Expand Up @@ -1168,7 +1168,7 @@ static DetectPort *RulesGroupByPorts(DetectEngineCtx *de_ctx, uint8_t ipproto, u
DetectPort *list = NULL;
while (s) {
/* IP Only rules are handled separately */
if (s->flags & SIG_FLAG_IPONLY)
if (s->type == SIG_TYPE_IPONLY)
goto next;
if (!(s->proto.proto[ipproto / 8] & (1<<(ipproto % 8)) || (s->proto.flags & DETECT_PROTO_ANY)))
goto next;
Expand Down Expand Up @@ -1300,21 +1300,65 @@ static DetectPort *RulesGroupByPorts(DetectEngineCtx *de_ctx, uint8_t ipproto, u

void SignatureSetType(DetectEngineCtx *de_ctx, Signature *s)
{
BUG_ON(s->type != SIG_TYPE_NOT_SET);
int iponly = 0;

/* see if the sig is dp only */
if (SignatureIsPDOnly(de_ctx, s) == 1) {
s->flags |= SIG_FLAG_PDONLY;
s->type = SIG_TYPE_PDONLY;

/* see if the sig is ip only */
/* see if the sig is ip only */
} else if ((iponly = SignatureIsIPOnly(de_ctx, s)) > 0) {
if (iponly == 1) {
s->flags |= SIG_FLAG_IPONLY;
s->type = SIG_TYPE_IPONLY;
} else if (iponly == 2) {
s->flags |= SIG_FLAG_LIKE_IPONLY;
s->type = SIG_TYPE_LIKE_IPONLY;
}
} else if (SignatureIsDEOnly(de_ctx, s) == 1) {
s->init_data->init_flags |= SIG_FLAG_INIT_DEONLY;
s->type = SIG_TYPE_DEONLY;

} else {
const bool has_match = s->init_data->smlists[DETECT_SM_LIST_MATCH] != NULL;
const bool has_pmatch = s->init_data->smlists[DETECT_SM_LIST_PMATCH] != NULL;
bool has_buffer_frame_engine = false;
bool has_buffer_packet_engine = false;
bool has_buffer_app_engine = false;

for (uint32_t x = 0; x < s->init_data->buffer_index; x++) {
const uint32_t id = s->init_data->buffers[x].id;

if (DetectEngineBufferTypeSupportsPacketGetById(de_ctx, id)) {
has_buffer_packet_engine = true;
} else if (DetectEngineBufferTypeSupportsFramesGetById(de_ctx, id)) {
has_buffer_frame_engine = true;
} else {
has_buffer_app_engine = true;
}
}

if (has_buffer_packet_engine) {
s->type = SIG_TYPE_PKT;
} else if (has_buffer_frame_engine || has_buffer_app_engine) {
s->type = SIG_TYPE_APP_TX;
} else if (has_pmatch) {
if ((s->flags & (SIG_FLAG_REQUIRE_PACKET | SIG_FLAG_REQUIRE_STREAM)) ==
SIG_FLAG_REQUIRE_PACKET) {
s->type = SIG_TYPE_PKT; // TODO review
Copy link
Copy Markdown
Member Author

@victorjulien victorjulien May 23, 2023

Choose a reason for hiding this comment

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

looks correct

} else if ((s->flags & (SIG_FLAG_REQUIRE_PACKET | SIG_FLAG_REQUIRE_STREAM)) ==
SIG_FLAG_REQUIRE_STREAM) {
s->type = SIG_TYPE_STREAM; // TODO review
Copy link
Copy Markdown
Member Author

@victorjulien victorjulien May 23, 2023

Choose a reason for hiding this comment

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

looks correct

} else {
s->type = SIG_TYPE_PKT_STREAM;
}
} else if (has_match) {
s->type = SIG_TYPE_PKT;

/* app-layer but no inspect engines */
} else if (s->flags & SIG_FLAG_APPLAYER) {
s->type = SIG_TYPE_APPLAYER;
} else {
s->type = SIG_TYPE_PKT;
}
}
}

Expand Down Expand Up @@ -1354,15 +1398,15 @@ int SigAddressPrepareStage1(DetectEngineCtx *de_ctx)

SCLogDebug("Signature %" PRIu32 ", internal id %" PRIu32 ", ptrs %p %p ", s->id, s->num, s, de_ctx->sig_array[s->num]);

if (s->flags & SIG_FLAG_PDONLY) {
if (s->type == SIG_TYPE_PDONLY) {
SCLogDebug("Signature %"PRIu32" is considered \"PD only\"", s->id);
} else if (s->flags & SIG_FLAG_IPONLY) {
} else if (s->type == SIG_TYPE_IPONLY) {
SCLogDebug("Signature %"PRIu32" is considered \"IP only\"", s->id);
cnt_iponly++;
} else if (SignatureIsInspectingPayload(de_ctx, s) == 1) {
SCLogDebug("Signature %"PRIu32" is considered \"Payload inspecting\"", s->id);
cnt_payload++;
} else if (s->init_data->init_flags & SIG_FLAG_INIT_DEONLY) {
} else if (s->type == SIG_TYPE_DEONLY) {
SCLogDebug("Signature %"PRIu32" is considered \"Decoder Event only\"", s->id);
cnt_deonly++;
} else if (s->flags & SIG_FLAG_APPLAYER) {
Expand Down Expand Up @@ -1684,11 +1728,9 @@ int SigAddressPrepareStage2(DetectEngineCtx *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) {
SCLogDebug("s->id %"PRIu32, s->id);
if (s->flags & SIG_FLAG_IPONLY) {
if (s->type == SIG_TYPE_IPONLY) {
IPOnlyAddSignature(de_ctx, &de_ctx->io_ctx, s);
}

if (s->init_data->init_flags & SIG_FLAG_INIT_DEONLY) {
} else if (s->type == SIG_TYPE_DEONLY) {
DetectEngineAddDecoderEventSig(de_ctx, s);
}
}
Expand Down Expand Up @@ -1872,8 +1914,7 @@ static int SigMatchPrepare(DetectEngineCtx *de_ctx)
DetectEngineAppInspectionEngine2Signature(de_ctx, s);

/* built-ins */
int type;
for (type = 0; type < DETECT_SM_LIST_MAX; type++) {
for (int type = 0; type < DETECT_SM_LIST_MAX; type++) {
/* skip PMATCH if it is used in a stream 'app engine' instead */
if (type == DETECT_SM_LIST_PMATCH && (s->init_data->init_flags & SIG_FLAG_INIT_STATE_MATCH))
continue;
Expand All @@ -1888,16 +1929,15 @@ static int SigMatchPrepare(DetectEngineCtx *de_ctx)
EngineAnalysisRules2(de_ctx, s);
}
/* free lists. Ctx' are xferred to sm_arrays so won't get freed */
uint32_t i;
for (i = 0; i < DETECT_SM_LIST_MAX; i++) {
for (uint32_t i = 0; i < DETECT_SM_LIST_MAX; i++) {
SigMatch *sm = s->init_data->smlists[i];
while (sm != NULL) {
SigMatch *nsm = sm->next;
SigMatchFree(de_ctx, sm);
sm = nsm;
}
}
for (i = 0; i < (uint32_t)s->init_data->transforms.cnt; i++) {
for (uint32_t i = 0; i < (uint32_t)s->init_data->transforms.cnt; i++) {
if (s->init_data->transforms.transforms[i].options) {
int transform = s->init_data->transforms.transforms[i].transform;
sigmatch_table[transform].Free(
Expand Down
2 changes: 1 addition & 1 deletion src/detect-engine-iponly.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1524,7 +1524,7 @@ void IPOnlyPrepare(DetectEngineCtx *de_ctx)
void IPOnlyAddSignature(DetectEngineCtx *de_ctx, DetectEngineIPOnlyCtx *io_ctx,
Signature *s)
{
if (!(s->flags & SIG_FLAG_IPONLY))
if (!(s->type == SIG_TYPE_IPONLY))
return;

SigIntId mapped_signum = IPOnlyTrackSigNum(io_ctx, s->num);
Expand Down
Loading