From 2696fda04168cb82bedc8920fb8a3cc7d55289de Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 24 May 2023 10:44:45 +0200 Subject: [PATCH] detect: use explicit rule types Instead of using flags to indicate a rule type, use an explicit `type` field. This will make it more clean in code paths what paths a rule is taking, and will allow easier debugging as well as analyzer output. Define the following fields: - SIG_TYPE_IPONLY: sig meets IP-only criteria and is handled by the IP-only engine. - SIG_TYPE_PDONLY: sig inspects protocol detection results only. - SIG_TYPE_DEONLY: sig inspects decoder events only. - SIG_TYPE_PKT: sig is inspected per packet. - SIG_TYPE_PKT_STREAM: sig is inspected against either packet payload or stream payload. - SIG_TYPE_STREAM: sig is inspected against the reassembled stream - SIG_TYPE_APPLAYER: sig is inspected against an app-layer property, but not against a tx engine. - SIG_TYPE_APP_TX: sig is inspected the tx aware inspection engine(s). Ticket: #6085. --- src/detect-app-layer-protocol.c | 13 +++--- src/detect-engine-alert.c | 15 +++++-- src/detect-engine-analyzer.c | 41 +++++++++++++++---- src/detect-engine-build.c | 70 ++++++++++++++++++++++++++------- src/detect-engine-iponly.c | 2 +- src/detect-engine.c | 24 +++++++++++ src/detect-engine.h | 1 + src/detect-parse.c | 2 +- src/detect-threshold.c | 5 +-- src/detect.h | 44 ++++++++++++++++----- 10 files changed, 170 insertions(+), 47 deletions(-) diff --git a/src/detect-app-layer-protocol.c b/src/detect-app-layer-protocol.c index 75693e6cd9..26a5ce6235 100644 --- a/src/detect-app-layer-protocol.c +++ b/src/detect-app-layer-protocol.c @@ -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); } @@ -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; } @@ -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; diff --git a/src/detect-engine-alert.c b/src/detect-engine-alert.c index d532e6d4be..3a23b68602 100644 --- a/src/detect-engine-alert.c +++ b/src/detect-engine-alert.c @@ -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); diff --git a/src/detect-engine-analyzer.c b/src/detect-engine-analyzer.c index 0ccc408ef7..2d78411bc7 100644 --- a/src/detect-engine-analyzer.c +++ b/src/detect-engine-analyzer.c @@ -863,9 +863,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"); } @@ -899,9 +896,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"); } @@ -1483,8 +1477,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"); diff --git a/src/detect-engine-build.c b/src/detect-engine-build.c index 106e80ce56..f82567e75d 100644 --- a/src/detect-engine-build.c +++ b/src/detect-engine-build.c @@ -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; @@ -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; @@ -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; + } else if ((s->flags & (SIG_FLAG_REQUIRE_PACKET | SIG_FLAG_REQUIRE_STREAM)) == + SIG_FLAG_REQUIRE_STREAM) { + s->type = SIG_TYPE_STREAM; + } 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; + } } } @@ -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) { @@ -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); } } diff --git a/src/detect-engine-iponly.c b/src/detect-engine-iponly.c index b041bb4e16..e2be468fca 100644 --- a/src/detect-engine-iponly.c +++ b/src/detect-engine-iponly.c @@ -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); diff --git a/src/detect-engine.c b/src/detect-engine.c index 4eb9acf749..522f1d50c1 100644 --- a/src/detect-engine.c +++ b/src/detect-engine.c @@ -106,6 +106,21 @@ static DetectEngineAppInspectionEngine *g_app_inspect_engines = NULL; static DetectEnginePktInspectionEngine *g_pkt_inspect_engines = NULL; static DetectEngineFrameInspectionEngine *g_frame_inspect_engines = NULL; +// clang-format off +const struct SignatureProperties signature_properties[SIG_TYPE_MAX] = { + /* SIG_TYPE_NOT_SET */ { SIG_PROP_FLOW_ACTION_PACKET, }, + /* SIG_TYPE_IPONLY */ { SIG_PROP_FLOW_ACTION_FLOW, }, + /* SIG_TYPE_LIKE_IPONLY */ { SIG_PROP_FLOW_ACTION_FLOW, }, + /* SIG_TYPE_PDONLY */ { SIG_PROP_FLOW_ACTION_FLOW, }, + /* SIG_TYPE_DEONLY */ { SIG_PROP_FLOW_ACTION_PACKET, }, + /* SIG_TYPE_PKT */ { SIG_PROP_FLOW_ACTION_PACKET, }, + /* SIG_TYPE_PKT_STREAM */ { SIG_PROP_FLOW_ACTION_FLOW_IF_STATEFUL, }, + /* SIG_TYPE_STREAM */ { SIG_PROP_FLOW_ACTION_FLOW_IF_STATEFUL, }, + /* SIG_TYPE_APPLAYER */ { SIG_PROP_FLOW_ACTION_FLOW, }, + /* SIG_TYPE_APP_TX */ { SIG_PROP_FLOW_ACTION_FLOW, }, +}; +// clang-format on + SCEnumCharMap det_ctx_event_table[] = { #ifdef UNITTESTS { "TEST", DET_CTX_EVENT_TEST }, @@ -1314,6 +1329,15 @@ bool DetectEngineBufferTypeSupportsMpmGetById(const DetectEngineCtx *de_ctx, con return map->mpm; } +bool DetectEngineBufferTypeSupportsFramesGetById(const DetectEngineCtx *de_ctx, const int id) +{ + const DetectBufferType *map = DetectEngineBufferTypeGetById(de_ctx, id); + if (map == NULL) + return false; + SCLogDebug("map %p id %d frame? %d", map, id, map->frame); + return map->frame; +} + void DetectBufferTypeRegisterSetupCallback(const char *name, void (*SetupCallback)(const DetectEngineCtx *, Signature *)) { diff --git a/src/detect-engine.h b/src/detect-engine.h index 9c26a60baa..6fef826173 100644 --- a/src/detect-engine.h +++ b/src/detect-engine.h @@ -72,6 +72,7 @@ bool DetectEngineBufferTypeSupportsMpmGetById(const DetectEngineCtx *de_ctx, con bool DetectEngineBufferTypeSupportsPacketGetById(const DetectEngineCtx *de_ctx, const int id); bool DetectEngineBufferTypeSupportsMultiInstanceGetById( const DetectEngineCtx *de_ctx, const int id); +bool DetectEngineBufferTypeSupportsFramesGetById(const DetectEngineCtx *de_ctx, const int id); const char *DetectEngineBufferTypeGetDescriptionById(const DetectEngineCtx *de_ctx, const int id); const DetectBufferType *DetectEngineBufferTypeGetById(const DetectEngineCtx *de_ctx, const int id); int DetectEngineBufferTypeGetByIdTransforms( diff --git a/src/detect-parse.c b/src/detect-parse.c index 8de7fba9a9..2c7d8f2ec6 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -2126,7 +2126,7 @@ static Signature *SigInitHelper(DetectEngineCtx *de_ctx, const char *sigstr, /* check what the type of this sig is */ SignatureSetType(de_ctx, sig); - if (sig->flags & SIG_FLAG_IPONLY) { + if (sig->type == SIG_TYPE_IPONLY) { /* For IPOnly */ if (IPOnlySigParseAddress(de_ctx, sig, parser.src, SIG_DIREC_SRC ^ dir) < 0) goto error; diff --git a/src/detect-threshold.c b/src/detect-threshold.c index 06d64fb441..25bda3556b 100644 --- a/src/detect-threshold.c +++ b/src/detect-threshold.c @@ -512,10 +512,7 @@ static int DetectThresholdTestSig1(void) SigGroupBuild(de_ctx); - if (s->flags & SIG_FLAG_IPONLY) { - printf("signature is ip-only: "); - goto end; - } + FAIL_IF(s->type == SIG_TYPE_IPONLY); DetectEngineThreadCtxInit(&th_v, (void *)de_ctx, (void *)&det_ctx); diff --git a/src/detect.h b/src/detect.h index 3a6c3b0e3a..49a920b03e 100644 --- a/src/detect.h +++ b/src/detect.h @@ -54,6 +54,36 @@ struct SCSigOrderFunc_; struct SCSigSignatureWrapper_; +enum SignatureType { + SIG_TYPE_NOT_SET = 0, + SIG_TYPE_IPONLY, // rule is handled by IPONLY engine + SIG_TYPE_LIKE_IPONLY, // rule is handled by pkt engine, has action effect like ip-only + /** Proto detect only signature. + * Inspected once per direction when protocol detection is done. */ + SIG_TYPE_PDONLY, // rule is handled by PDONLY engine + SIG_TYPE_DEONLY, + SIG_TYPE_PKT, + SIG_TYPE_PKT_STREAM, + SIG_TYPE_STREAM, + + SIG_TYPE_APPLAYER, // app-layer but not tx, e.g. appproto + SIG_TYPE_APP_TX, // rule is handled by TX engine + + SIG_TYPE_MAX, +}; + +enum SignaturePropertyFlowAction { + SIG_PROP_FLOW_ACTION_PACKET, + SIG_PROP_FLOW_ACTION_FLOW, + SIG_PROP_FLOW_ACTION_FLOW_IF_STATEFUL, +}; + +struct SignatureProperties { + enum SignaturePropertyFlowAction flow_action; +}; + +extern const struct SignatureProperties signature_properties[SIG_TYPE_MAX]; + /* The detection engine groups similar signatures/rules together. Internally a tree of different types of data is created on initialization. This is it's @@ -206,11 +236,7 @@ typedef struct DetectPort_ { #define SIG_FLAG_NOALERT BIT_U32(4) /**< no alert flag is set */ #define SIG_FLAG_DSIZE BIT_U32(5) /**< signature has a dsize setting */ -#define SIG_FLAG_APPLAYER BIT_U32(6) /**< signature applies to app layer instead of packets */ -#define SIG_FLAG_IPONLY BIT_U32(7) /**< ip only signature */ -#define SIG_FLAG_LIKE_IPONLY \ - BIT_U32(8) /**< signature that is almost ip only, but contains negation prevening some iponly \ - optimizations */ +#define SIG_FLAG_APPLAYER BIT_U32(6) /**< signature applies to app layer instead of packets */ // vacancy @@ -236,9 +262,8 @@ typedef struct DetectPort_ { #define SIG_FLAG_PREFILTER BIT_U32(23) /**< sig is part of a prefilter engine */ -/** Proto detect only signature. - * Inspected once per direction when protocol detection is done. */ -#define SIG_FLAG_PDONLY BIT_U32(24) +// vacancy + /** Info for Source and Target identification */ #define SIG_FLAG_SRC_IS_TARGET BIT_U32(25) /** Info for Source and Target identification */ @@ -247,7 +272,7 @@ typedef struct DetectPort_ { #define SIG_FLAG_HAS_TARGET (SIG_FLAG_DEST_IS_TARGET|SIG_FLAG_SRC_IS_TARGET) /* signature init flags */ -#define SIG_FLAG_INIT_DEONLY BIT_U32(0) /**< decode event only signature */ +// available 0 #define SIG_FLAG_INIT_PACKET BIT_U32(1) /**< signature has matches against a packet (as opposed to app layer) */ #define SIG_FLAG_INIT_FLOW BIT_U32(2) /**< signature has a flow setting */ #define SIG_FLAG_INIT_BIDIREC BIT_U32(3) /**< signature has bidirectional operator */ @@ -557,6 +582,7 @@ typedef struct SignatureInitData_ { typedef struct Signature_ { uint32_t flags; /* coccinelle: Signature:flags:SIG_FLAG_ */ + enum SignatureType type; AppProto alproto; -- 2.47.2