Skip to content

Detect rule types hooks/v18#8906

Closed
victorjulien wants to merge 3 commits into
OISF:masterfrom
victorjulien:detect-rule-types-hooks/v18
Closed

Detect rule types hooks/v18#8906
victorjulien wants to merge 3 commits into
OISF:masterfrom
victorjulien:detect-rule-types-hooks/v18

Conversation

@victorjulien
Copy link
Copy Markdown
Member

@victorjulien victorjulien commented May 22, 2023

For review and QA

SV_BRANCH=pr/1213

Instead of using flags to indicate a rule type, use an explicit
`type` field.

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).

Update the analyzer output to add the type.

Per rule type record properties of the type. This is currently only about
whether a pass/drop action applies to the flow.

Example output:

  "policy": {
    "actions": [
      "drop"
    ],
    "scope": "flow"
  },
  "type": "ip_only",
@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2023

Codecov Report

Merging #8906 (6775e90) into master (ebe0a7b) will increase coverage by 0.05%.
The diff coverage is 82.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8906      +/-   ##
==========================================
+ Coverage   82.30%   82.35%   +0.05%     
==========================================
  Files         969      969              
  Lines      273335   273452     +117     
==========================================
+ Hits       224961   225203     +242     
+ Misses      48374    48249     -125     
Flag Coverage Δ
fuzzcorpus 64.70% <41.02%> (+0.05%) ⬆️
suricata-verify 60.51% <82.05%> (+0.05%) ⬆️
unittests 62.95% <40.62%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link
Copy Markdown

Information: QA ran without warnings.

Pipeline 13885

}
jb_close(ctx.js);

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

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

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

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')?

Comment thread src/detect-engine-build.c
} 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

Choose a reason for hiding this comment

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

looks correct

Comment thread src/detect-engine-build.c
s->type = SIG_TYPE_PKT; // TODO review
} 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

Choose a reason for hiding this comment

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

looks correct

Comment thread src/suricata.c
#if defined(SC_ADDRESS_SANITIZER)
strlcat(features, "ASAN ", sizeof(features));
#endif
strlcat(features, "RULE_TYPES ", sizeof(features));
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

remove

@jasonish
Copy link
Copy Markdown
Member

Any ticket/discussion of why?

@victorjulien
Copy link
Copy Markdown
Member Author

Any ticket/discussion of why?

https://redmine.openinfosecfoundation.org/issues/6085

@victorjulien
Copy link
Copy Markdown
Member Author

replaced by #8928

@victorjulien victorjulien deleted the detect-rule-types-hooks/v18 branch July 17, 2023 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants