From 8fc0faf5c2622cd0685e07fbfd42ccc89aa08266 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Mon, 15 Jan 2024 12:39:34 +0530 Subject: [PATCH 1/7] util/streaming-buffer: remove unneeded fn param StreamingBuffer is not required to find the intersecting regions, so, don't pass it as a param to the fn. --- src/util-streaming-buffer.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/util-streaming-buffer.c b/src/util-streaming-buffer.c index 6ff4f438a40a..b396ef04c765 100644 --- a/src/util-streaming-buffer.c +++ b/src/util-streaming-buffer.c @@ -123,7 +123,7 @@ StreamingBufferBlock *SBB_RB_FIND_INCLUSIVE(struct SBB *head, StreamingBufferBlo * \brief does data region intersect with list region 'r' * Takes the max gap into account. */ -static inline bool RegionsIntersect(const StreamingBuffer *sb, const StreamingBufferConfig *cfg, +static inline bool RegionsIntersect(const StreamingBufferConfig *cfg, const StreamingBufferRegion *r, const uint64_t offset, const uint64_t re) { /* create the data range for the region, adding the max gap */ @@ -158,7 +158,7 @@ static StreamingBufferRegion *FindFirstRegionForOffset(const StreamingBuffer *sb StreamingBufferRegion *p = NULL; for (; r != NULL; r = r->next) { - if (RegionsIntersect(sb, cfg, r, offset, data_re) == true) { + if (RegionsIntersect(cfg, r, offset, data_re) == true) { *prev = p; return r; } @@ -182,7 +182,7 @@ static StreamingBufferRegion *FindLargestRegionForOffset(const StreamingBuffer * SCLogDebug("checking: %p/%" PRIu64 "/%" PRIu64 ", offset %" PRIu64 "/%" PRIu64, r, r->stream_offset, reg_re, offset, data_re); #endif - if (!RegionsIntersect(sb, cfg, r, offset, data_re)) + if (!RegionsIntersect(cfg, r, offset, data_re)) return candidate; if (r->buf_size > candidate->buf_size) { @@ -200,7 +200,7 @@ static StreamingBufferRegion *FindRightEdge(const StreamingBuffer *sb, const uint64_t data_re = offset + len; StreamingBufferRegion *candidate = r; for (; r != NULL; r = r->next) { - if (!RegionsIntersect(sb, cfg, r, offset, data_re)) { + if (!RegionsIntersect(cfg, r, offset, data_re)) { SCLogDebug( "r %p is out of scope: %" PRIu64 "/%u/%" PRIu64, r, offset, len, offset + len); return candidate; @@ -1433,11 +1433,11 @@ static StreamingBufferRegion *BufferInsertAtRegion(StreamingBuffer *sb, data_offset + data_len); ListRegions(sb); - if (RegionsIntersect(sb, cfg, &sb->region, data_offset, data_offset + data_len)) { + if (RegionsIntersect(cfg, &sb->region, data_offset, data_offset + data_len)) { SCLogDebug("data_offset %" PRIu64 ", data_len %u intersects with main region (next %p)", data_offset, data_len, sb->region.next); if (sb->region.next == NULL || - !RegionsIntersect(sb, cfg, sb->region.next, data_offset, data_offset + data_len)) { + !RegionsIntersect(cfg, sb->region.next, data_offset, data_offset + data_len)) { SCLogDebug( "data_offset %" PRIu64 ", data_len %u intersects with main region, no next or way before next region", From f6e1a202159720adc70e90bc413d3d3ae40cff6e Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Thu, 30 Nov 2023 14:32:08 +0100 Subject: [PATCH 2/7] detect: dns.opcode as first-class integer Ticket: 5446 That means it can accept ranges --- doc/userguide/rules/dns-keywords.rst | 11 ++ rust/src/detect/uint.rs | 2 +- rust/src/dns/detect.rs | 167 +++++++++------------------ src/detect-dns-opcode.c | 5 +- 4 files changed, 67 insertions(+), 118 deletions(-) diff --git a/doc/userguide/rules/dns-keywords.rst b/doc/userguide/rules/dns-keywords.rst index a514ae25195b..005164dfd159 100644 --- a/doc/userguide/rules/dns-keywords.rst +++ b/doc/userguide/rules/dns-keywords.rst @@ -28,12 +28,15 @@ dns.opcode This keyword matches on the **opcode** found in the DNS header flags. +dns.opcode uses an :ref:`unsigned 8-bit integer `. + Syntax ~~~~~~ :: dns.opcode:[!] + dns.opcode:[!]- Examples ~~~~~~~~ @@ -46,6 +49,14 @@ Match on DNS requests where the **opcode** is NOT 0:: dns.opcode:!0; +Match on DNS requests where the **opcode** is between 7 and 15, exclusively: + + dns.opcode:7-15; + +Match on DNS requests where the **opcode** is not between 7 and 15: + + dns.opcode:!7-15; + dns.query --------- diff --git a/rust/src/detect/uint.rs b/rust/src/detect/uint.rs index 7ce86d57f3e2..5b28830cea6a 100644 --- a/rust/src/detect/uint.rs +++ b/rust/src/detect/uint.rs @@ -42,7 +42,7 @@ pub enum DetectUintMode { DetectUintModeNegBitmask, } -#[derive(Debug)] +#[derive(Debug, PartialEq)] #[repr(C)] pub struct DetectUintData { pub arg1: T, diff --git a/rust/src/dns/detect.rs b/rust/src/dns/detect.rs index 5d9d945be0ce..452d4e8380e1 100644 --- a/rust/src/dns/detect.rs +++ b/rust/src/dns/detect.rs @@ -16,49 +16,15 @@ */ use super::dns::DNSTransaction; -use crate::core::*; -use std::ffi::CStr; -use std::os::raw::{c_char, c_void}; - -#[derive(Debug, PartialEq, Eq)] -pub struct DetectDnsOpcode { - negate: bool, - opcode: u8, -} - -/// Parse a DNS opcode argument returning the code and if it is to be -/// negated or not. -/// -/// For now only an indication that an error occurred is returned, not -/// the details of the error. -fn parse_opcode(opcode: &str) -> Result { - let mut negated = false; - for (i, c) in opcode.chars().enumerate() { - match c { - ' ' | '\t' => { - continue; - } - '!' => { - negated = true; - } - _ => { - let code: u8 = opcode[i..].parse().map_err(|_| ())?; - return Ok(DetectDnsOpcode { - negate: negated, - opcode: code, - }); - } - } - } - Err(()) -} +use crate::core::Direction; +use crate::detect::uint::{detect_match_uint, DetectUintData}; /// Perform the DNS opcode match. /// /// 1 will be returned on match, otherwise 0 will be returned. #[no_mangle] pub extern "C" fn rs_dns_opcode_match( - tx: &mut DNSTransaction, detect: &mut DetectDnsOpcode, flags: u8, + tx: &mut DNSTransaction, detect: &mut DetectUintData, flags: u8, ) -> u8 { let header_flags = if flags & Direction::ToServer as u8 != 0 { if let Some(request) = &tx.request { @@ -76,116 +42,87 @@ pub extern "C" fn rs_dns_opcode_match( // Not to server or to client?? return 0; }; + let opcode = ((header_flags >> 11) & 0xf) as u8; - match_opcode(detect, header_flags).into() -} - -fn match_opcode(detect: &DetectDnsOpcode, flags: u16) -> bool { - let opcode = ((flags >> 11) & 0xf) as u8; - if detect.negate { - detect.opcode != opcode - } else { - detect.opcode == opcode - } -} - -#[no_mangle] -pub unsafe extern "C" fn rs_detect_dns_opcode_parse(carg: *const c_char) -> *mut c_void { - if carg.is_null() { - return std::ptr::null_mut(); - } - let arg = match CStr::from_ptr(carg).to_str() { - Ok(arg) => arg, - _ => { - return std::ptr::null_mut(); - } - }; - - match parse_opcode(arg) { - Ok(detect) => Box::into_raw(Box::new(detect)) as *mut _, - Err(_) => std::ptr::null_mut(), - } -} - -#[no_mangle] -pub unsafe extern "C" fn rs_dns_detect_opcode_free(ptr: *mut c_void) { - if !ptr.is_null() { - std::mem::drop(Box::from_raw(ptr as *mut DetectDnsOpcode)); + if detect_match_uint(detect, opcode) { + return 1; } + return 0; } #[cfg(test)] mod test { use super::*; + use crate::detect::uint::{detect_parse_uint, DetectUintMode}; #[test] fn parse_opcode_good() { assert_eq!( - parse_opcode("1"), - Ok(DetectDnsOpcode { - negate: false, - opcode: 1 - }) - ); - assert_eq!( - parse_opcode("123"), - Ok(DetectDnsOpcode { - negate: false, - opcode: 123 - }) + detect_parse_uint::("1").unwrap().1, + DetectUintData { + mode: DetectUintMode::DetectUintModeEqual, + arg1: 1, + arg2: 0, + } ); assert_eq!( - parse_opcode("!123"), - Ok(DetectDnsOpcode { - negate: true, - opcode: 123 - }) + detect_parse_uint::("123").unwrap().1, + DetectUintData { + mode: DetectUintMode::DetectUintModeEqual, + arg1: 123, + arg2: 0, + } ); assert_eq!( - parse_opcode("!123"), - Ok(DetectDnsOpcode { - negate: true, - opcode: 123 - }) + detect_parse_uint::("!123").unwrap().1, + DetectUintData { + mode: DetectUintMode::DetectUintModeNe, + arg1: 123, + arg2: 0, + } ); - assert_eq!(parse_opcode(""), Err(())); - assert_eq!(parse_opcode("!"), Err(())); - assert_eq!(parse_opcode("! "), Err(())); - assert_eq!(parse_opcode("!asdf"), Err(())); + assert!(detect_parse_uint::("").is_err()); + assert!(detect_parse_uint::("!").is_err()); + assert!(detect_parse_uint::("! ").is_err()); + assert!(detect_parse_uint::("!asdf").is_err()); } #[test] fn test_match_opcode() { - assert!(match_opcode( - &DetectDnsOpcode { - negate: false, - opcode: 0, + assert!(detect_match_uint( + &DetectUintData { + mode: DetectUintMode::DetectUintModeEqual, + arg1: 0, + arg2: 0, }, 0b0000_0000_0000_0000, )); - assert!(!match_opcode( - &DetectDnsOpcode { - negate: true, - opcode: 0, + assert!(!detect_match_uint( + &DetectUintData { + mode: DetectUintMode::DetectUintModeNe, + arg1: 0, + arg2: 0, }, 0b0000_0000_0000_0000, )); - assert!(match_opcode( - &DetectDnsOpcode { - negate: false, - opcode: 4, + assert!(detect_match_uint( + &DetectUintData { + mode: DetectUintMode::DetectUintModeEqual, + arg1: 4, + arg2: 0, }, - 0b0010_0000_0000_0000, + ((0b0010_0000_0000_0000 >> 11) & 0xf) as u8, )); - assert!(!match_opcode( - &DetectDnsOpcode { - negate: true, - opcode: 4, + assert!(!detect_match_uint( + &DetectUintData { + mode: DetectUintMode::DetectUintModeNe, + arg1: 4, + arg2: 0, }, - 0b0010_0000_0000_0000, + ((0b0010_0000_0000_0000 >> 11) & 0xf) as u8, )); } } diff --git a/src/detect-dns-opcode.c b/src/detect-dns-opcode.c index 4baee19b8cd3..f5dcab700f27 100644 --- a/src/detect-dns-opcode.c +++ b/src/detect-dns-opcode.c @@ -19,6 +19,7 @@ #include "detect-parse.h" #include "detect-engine.h" +#include "detect-engine-uint.h" #include "detect-dns-opcode.h" #include "rust.h" @@ -35,7 +36,7 @@ static int DetectDnsOpcodeSetup(DetectEngineCtx *de_ctx, Signature *s, return -1; } - void *detect = rs_detect_dns_opcode_parse(str); + void *detect = DetectU8Parse(str); if (detect == NULL) { SCLogError("failed to parse dns.opcode: %s", str); return -1; @@ -57,7 +58,7 @@ static void DetectDnsOpcodeFree(DetectEngineCtx *de_ctx, void *ptr) { SCEnter(); if (ptr != NULL) { - rs_dns_detect_opcode_free(ptr); + rs_detect_u8_free(ptr); } SCReturn; } From 6de885c60379225e0b636a5a8df07fd2cfe6193d Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Tue, 30 Jan 2024 20:42:16 +0100 Subject: [PATCH 3/7] ci: update scorecard analysis workflow --- .github/workflows/scorecards-analysis.yml | 29 ++++++++++------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/.github/workflows/scorecards-analysis.yml b/.github/workflows/scorecards-analysis.yml index 3b6612849206..07d4eda8121f 100644 --- a/.github/workflows/scorecards-analysis.yml +++ b/.github/workflows/scorecards-analysis.yml @@ -17,39 +17,36 @@ jobs: permissions: # Needed to upload the results to code-scanning dashboard. security-events: write - actions: read - contents: read + id-token: write steps: - name: "Checkout code" - uses: actions/checkout@v3.5.3 - with: - persist-credentials: false + uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 - name: "Run analysis" - uses: ossf/scorecard-action@e38b1902ae4f44df626f11ba0734b14fb91f8f86 # v1.0.1 + uses: ossf/scorecard-action@0864cf19026789058feabb7e87baa5f140aac736 # v2.3.1 with: results_file: results.sarif results_format: sarif - # Read-only PAT token. To create it, - # follow the steps in https://github.com/ossf/scorecard-action#pat-token-creation. repo_token: ${{ secrets.SCORECARD_READ_TOKEN }} - # Publish the results to enable scorecard badges. For more details, see - # https://github.com/ossf/scorecard-action#publishing-results. - # For private repositories, `publish_results` will automatically be set to `false`, - # regardless of the value entered here. + # Scorecard team runs a weekly scan of public GitHub repos, + # see https://github.com/ossf/scorecard#public-data. + # Setting `publish_results: true` helps us scale by leveraging your workflow to + # extract the results instead of relying on our own infrastructure to run scans. + # And it's free for you! publish_results: true - # Upload the results as artifacts (optional). + # https://docs.github.com/en/actions/advanced-guides/storing-workflow-data-as-artifacts + # Optional. - name: "Upload artifact" - uses: actions/upload-artifact@0b7f8abb1508181956e8e162db84b466c27e18ce + uses: actions/upload-artifact@26f96dfa697d77e81fd5907df203aa23a56210a8 # v3 with: name: SARIF file path: results.sarif retention-days: 5 # Upload the results to GitHub's code scanning dashboard. - - name: "Upload to code-scanning" - uses: github/codeql-action/upload-sarif@v2 # v1.0.26 + - name: "Upload SARIF results" + uses: github/codeql-action/upload-sarif@cdcdbb579706841c47f7063dda365e292e5cad7a # v1 with: sarif_file: results.sarif From 264101ba22a3e28ffb2293e15e324412f97d5126 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Thu, 1 Feb 2024 15:20:15 +0530 Subject: [PATCH 4/7] detect: remove unused port in SigGroupHeadInitData port is not used and logically makes sense to not be in this struct as this struct is already referenced by DetectPort itself as a part of SigGroupHead. --- src/detect.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/detect.h b/src/detect.h index 0fc5d21fb1d8..e7cc0dfe935a 100644 --- a/src/detect.h +++ b/src/detect.h @@ -1434,9 +1434,6 @@ typedef struct SigGroupHeadInitData_ { /** Array with sig ptrs... size is sig_cnt * sizeof(Signature *) */ Signature **match_array; - - /* port ptr */ - struct DetectPort_ *port; } SigGroupHeadInitData; /** \brief Container for matching data for a signature group */ From 395c74d81eec59b07181c0015c6760845a9494e7 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Thu, 1 Feb 2024 16:44:33 +0530 Subject: [PATCH 5/7] detect/engine: set max sig ID per SGH Present scenario ---------------- Currently, as a part of setting signature count per SGH, a max_idx is passed which could be as high as the highest signature number (internal ID). Issue ----- Not every SGH needs to evaluate all the signatures while setting the signature count or while creating the match_array. In a nonideal scenario, when say, there are 2 SGHs and one SGH has 2 signatures and the other one has 60k, given the current scheme of evaluating max_idx, the max_idx will be set to 60k, and this shall later be passed on to SigGroupHeadSetSigCnt or SigGroupHeadBuildMatchArra which shall traverse over all the 60k sigs for either SGHs. Other info ---------- This is a very fast operation as the internal arithmetic is done bitwise. Patch ----- The functions SigGroupHeadSetSigCnt and SigGroupHeadBuildMatchArray can be optimized by storing the max signature id (internal) per SGH (which also seemed to be the initial intention as per fn comments). As a result of this, the sig_array is only walked up until the max sig id of that respective SGH. --- src/detect-engine-build.c | 23 ++++++++++++----------- src/detect-engine-siggroup.c | 11 +++++++---- src/detect.h | 1 + 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/detect-engine-build.c b/src/detect-engine-build.c index 604164b21c15..b99776bd13f3 100644 --- a/src/detect-engine-build.c +++ b/src/detect-engine-build.c @@ -1009,8 +1009,8 @@ static int RulesGroupByProto(DetectEngineCtx *de_ctx) if (lookup_sgh == NULL) { SCLogDebug("proto group %d sgh %p is the original", p, sgh_ts[p]); - SigGroupHeadSetSigCnt(sgh_ts[p], max_idx); - SigGroupHeadBuildMatchArray(de_ctx, sgh_ts[p], max_idx); + SigGroupHeadSetSigCnt(sgh_ts[p], 0); + SigGroupHeadBuildMatchArray(de_ctx, sgh_ts[p], 0); SigGroupHeadHashAdd(de_ctx, sgh_ts[p]); SigGroupHeadStore(de_ctx, sgh_ts[p]); @@ -1041,8 +1041,8 @@ static int RulesGroupByProto(DetectEngineCtx *de_ctx) if (lookup_sgh == NULL) { SCLogDebug("proto group %d sgh %p is the original", p, sgh_tc[p]); - SigGroupHeadSetSigCnt(sgh_tc[p], max_idx); - SigGroupHeadBuildMatchArray(de_ctx, sgh_tc[p], max_idx); + SigGroupHeadSetSigCnt(sgh_tc[p], 0); + SigGroupHeadBuildMatchArray(de_ctx, sgh_tc[p], 0); SigGroupHeadHashAdd(de_ctx, sgh_tc[p]); SigGroupHeadStore(de_ctx, sgh_tc[p]); @@ -1129,7 +1129,8 @@ static int RuleSetWhitelist(Signature *s) return wl; } -int CreateGroupedPortList(DetectEngineCtx *de_ctx, DetectPort *port_list, DetectPort **newhead, uint32_t unique_groups, int (*CompareFunc)(DetectPort *, DetectPort *), uint32_t max_idx); +int CreateGroupedPortList(DetectEngineCtx *de_ctx, DetectPort *port_list, DetectPort **newhead, + uint32_t unique_groups, int (*CompareFunc)(DetectPort *, DetectPort *)); int CreateGroupedPortListCmpCnt(DetectPort *a, DetectPort *b); static DetectPort *RulesGroupByPorts(DetectEngineCtx *de_ctx, uint8_t ipproto, uint32_t direction) @@ -1223,7 +1224,7 @@ static DetectPort *RulesGroupByPorts(DetectEngineCtx *de_ctx, uint8_t ipproto, u DetectPort *newlist = NULL; uint16_t groupmax = (direction == SIG_FLAG_TOCLIENT) ? de_ctx->max_uniq_toclient_groups : de_ctx->max_uniq_toserver_groups; - CreateGroupedPortList(de_ctx, list, &newlist, groupmax, CreateGroupedPortListCmpCnt, max_idx); + CreateGroupedPortList(de_ctx, list, &newlist, groupmax, CreateGroupedPortListCmpCnt); list = newlist; /* step 4: deduplicate the SGH's */ @@ -1243,8 +1244,8 @@ static DetectPort *RulesGroupByPorts(DetectEngineCtx *de_ctx, uint8_t ipproto, u if (lookup_sgh == NULL) { SCLogDebug("port group %p sgh %p is the original", iter, iter->sh); - SigGroupHeadSetSigCnt(iter->sh, max_idx); - SigGroupHeadBuildMatchArray(de_ctx, iter->sh, max_idx); + SigGroupHeadSetSigCnt(iter->sh, 0); + SigGroupHeadBuildMatchArray(de_ctx, iter->sh, 0); SigGroupHeadSetProtoAndDirection(iter->sh, ipproto, direction); SigGroupHeadHashAdd(de_ctx, iter->sh); SigGroupHeadStore(de_ctx, iter->sh); @@ -1541,7 +1542,8 @@ int CreateGroupedPortListCmpCnt(DetectPort *a, DetectPort *b) * The joingr is meant to be a catch all. * */ -int CreateGroupedPortList(DetectEngineCtx *de_ctx, DetectPort *port_list, DetectPort **newhead, uint32_t unique_groups, int (*CompareFunc)(DetectPort *, DetectPort *), uint32_t max_idx) +int CreateGroupedPortList(DetectEngineCtx *de_ctx, DetectPort *port_list, DetectPort **newhead, + uint32_t unique_groups, int (*CompareFunc)(DetectPort *, DetectPort *)) { DetectPort *tmplist = NULL, *joingr = NULL; char insert = 0; @@ -1560,8 +1562,7 @@ int CreateGroupedPortList(DetectEngineCtx *de_ctx, DetectPort *port_list, Detect list->next = NULL; groups++; - - SigGroupHeadSetSigCnt(list->sh, max_idx); + SigGroupHeadSetSigCnt(list->sh, 0); /* insert it */ DetectPort *tmpgr = tmplist, *prevtmpgr = NULL; diff --git a/src/detect-engine-siggroup.c b/src/detect-engine-siggroup.c index 4fadaac5c013..9bc992cb894a 100644 --- a/src/detect-engine-siggroup.c +++ b/src/detect-engine-siggroup.c @@ -347,7 +347,7 @@ int SigGroupHeadAppendSig(const DetectEngineCtx *de_ctx, SigGroupHead **sgh, /* enable the sig in the bitarray */ (*sgh)->init->sig_array[s->num / 8] |= 1 << (s->num % 8); - + (*sgh)->init->max_sig_id = MAX(s->num, (*sgh)->init->max_sig_id); return 0; error: @@ -405,6 +405,8 @@ int SigGroupHeadCopySigs(DetectEngineCtx *de_ctx, SigGroupHead *src, SigGroupHea if (src->init->score) (*dst)->init->score = MAX((*dst)->init->score, src->init->score); + if (src->init->max_sig_id) + (*dst)->init->max_sig_id = MAX((*dst)->init->max_sig_id, src->init->max_sig_id); return 0; error: @@ -422,9 +424,9 @@ int SigGroupHeadCopySigs(DetectEngineCtx *de_ctx, SigGroupHead *src, SigGroupHea void SigGroupHeadSetSigCnt(SigGroupHead *sgh, uint32_t max_idx) { uint32_t sig; - + sgh->init->max_sig_id = MAX(max_idx, sgh->init->max_sig_id); sgh->init->sig_cnt = 0; - for (sig = 0; sig < max_idx + 1; sig++) { + for (sig = 0; sig < sgh->init->max_sig_id + 1; sig++) { if (sgh->init->sig_array[sig / 8] & (1 << (sig % 8))) sgh->init->sig_cnt++; } @@ -492,12 +494,13 @@ int SigGroupHeadBuildMatchArray(DetectEngineCtx *de_ctx, SigGroupHead *sgh, return 0; BUG_ON(sgh->init->match_array != NULL); + sgh->init->max_sig_id = MAX(sgh->init->max_sig_id, max_idx); sgh->init->match_array = SCCalloc(sgh->init->sig_cnt, sizeof(Signature *)); if (sgh->init->match_array == NULL) return -1; - for (sig = 0; sig < max_idx + 1; sig++) { + for (sig = 0; sig < sgh->init->max_sig_id + 1; sig++) { if (!(sgh->init->sig_array[(sig / 8)] & (1 << (sig % 8))) ) continue; diff --git a/src/detect.h b/src/detect.h index e7cc0dfe935a..0707d8a5b211 100644 --- a/src/detect.h +++ b/src/detect.h @@ -1419,6 +1419,7 @@ typedef struct SigGroupHeadInitData_ { uint8_t protos[256]; /**< proto(s) this sgh is for */ uint32_t direction; /**< set to SIG_FLAG_TOSERVER, SIG_FLAG_TOCLIENT or both */ int score; /**< try to make this group a unique one */ + uint32_t max_sig_id; /**< max signature idx for this sgh */ MpmCtx **app_mpms; MpmCtx **pkt_mpms; From 7f89aaf772e65bc289528b59a7aee166c7c84f43 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Thu, 1 Feb 2024 16:58:48 +0530 Subject: [PATCH 6/7] detect: remove unneeded max_idx --- src/detect-engine-build.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/detect-engine-build.c b/src/detect-engine-build.c index b99776bd13f3..0c07150e696e 100644 --- a/src/detect-engine-build.c +++ b/src/detect-engine-build.c @@ -960,7 +960,6 @@ static int RulesGroupByProto(DetectEngineCtx *de_ctx) { Signature *s = de_ctx->sig_list; - uint32_t max_idx = 0; SigGroupHead *sgh_ts[256] = {NULL}; SigGroupHead *sgh_tc[256] = {NULL}; @@ -979,15 +978,12 @@ static int RulesGroupByProto(DetectEngineCtx *de_ctx) if (s->flags & SIG_FLAG_TOCLIENT) { SigGroupHeadAppendSig(de_ctx, &sgh_tc[p], s); - max_idx = s->num; } if (s->flags & SIG_FLAG_TOSERVER) { SigGroupHeadAppendSig(de_ctx, &sgh_ts[p], s); - max_idx = s->num; } } } - SCLogDebug("max_idx %u", max_idx); /* lets look at deduplicating this list */ SigGroupHeadHashFree(de_ctx); @@ -1140,7 +1136,6 @@ static DetectPort *RulesGroupByPorts(DetectEngineCtx *de_ctx, uint8_t ipproto, u * that belong to the SGH. */ DetectPortHashInit(de_ctx); - uint32_t max_idx = 0; const Signature *s = de_ctx->sig_list; DetectPort *list = NULL; while (s) { @@ -1199,7 +1194,6 @@ static DetectPort *RulesGroupByPorts(DetectEngineCtx *de_ctx, uint8_t ipproto, u p = p->next; } - max_idx = s->num; next: s = s->next; } From db99c45d239d5ca6e805094195f7ae39d3051e44 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Mon, 9 Oct 2023 14:49:54 +0200 Subject: [PATCH 7/7] detect: errors on 65k filestore signatures Errors when a detection engine gets 65k filestore signatures to avoid the hard limit to have 65k filestore per signature group head Ticket: #6393 --- src/detect-engine-siggroup.c | 3 +++ src/detect-filestore.c | 6 ++++++ src/detect.h | 3 +++ 3 files changed, 12 insertions(+) diff --git a/src/detect-engine-siggroup.c b/src/detect-engine-siggroup.c index 9bc992cb894a..52073cf0bfda 100644 --- a/src/detect-engine-siggroup.c +++ b/src/detect-engine-siggroup.c @@ -48,6 +48,7 @@ #include "util-error.h" #include "util-debug.h" +#include "util-validate.h" #include "util-cidr.h" #include "util-unittest.h" #include "util-unittest-helper.h" @@ -552,6 +553,8 @@ void SigGroupHeadSetupFiles(const DetectEngineCtx *de_ctx, SigGroupHead *sgh) } #endif if (SignatureIsFilestoring(s)) { + // should be insured by caller that we do not overflow + DEBUG_VALIDATE_BUG_ON(sgh->filestore_cnt == UINT16_MAX); sgh->filestore_cnt++; } } diff --git a/src/detect-filestore.c b/src/detect-filestore.c index 07bbd91ff199..c510544469aa 100644 --- a/src/detect-filestore.c +++ b/src/detect-filestore.c @@ -333,6 +333,11 @@ static int DetectFilestoreSetup (DetectEngineCtx *de_ctx, Signature *s, const ch static bool warn_not_configured = false; static uint32_t de_version = 0; + if (de_ctx->filestore_cnt == UINT16_MAX) { + SCLogError("Cannot have more than 65535 filestore signatures"); + return -1; + } + /* Check on first-time loads (includes following a reload) */ if (!warn_not_configured || (de_ctx->version != de_version)) { if (de_version != de_ctx->version) { @@ -466,6 +471,7 @@ static int DetectFilestoreSetup (DetectEngineCtx *de_ctx, Signature *s, const ch } s->flags |= SIG_FLAG_FILESTORE; + de_ctx->filestore_cnt++; if (match) pcre2_match_data_free(match); diff --git a/src/detect.h b/src/detect.h index 0707d8a5b211..76c6d2b66f03 100644 --- a/src/detect.h +++ b/src/detect.h @@ -1040,6 +1040,9 @@ typedef struct DetectEngineCtx_ { /* Track rule requirements for reporting after loading rules. */ SCDetectRequiresStatus *requirements; + + /* number of signatures using filestore, limited as u16 */ + uint16_t filestore_cnt; } DetectEngineCtx; /* Engine groups profiles (low, medium, high, custom) */