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

Draft/RFC: Engine analysis format #10136

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

regit
Copy link
Contributor

@regit regit commented Jan 9, 2024

The detect-engine-analyzer.c file is getting really long and I propose to update the signature match structure to include a dump function. This will permit to have the JSON dump function in the detect file of the keyword and with some luck will force people to code it when there is new keyword addition.

I'm opening this PR to sync with @jufajardini and @hadiqaalamdar

Link to redmine ticket:

Describe changes:

  • Add pattern for pcre output
  • Add basic flow information
  • Add dataset information
  • Factorize code

@suricata-qa
Copy link

ERROR:

ERROR: QA failed on build_asan.

Pipeline 17423

This is really helping when trying to understand a signature from
the engine analysis file.
This seems not to be needed and it is triggering a circular include
when using JsonBuilder in detect.h (which is coming in next patch).
@suricata-qa
Copy link

ERROR:

ERROR: QA failed on build_asan.

Pipeline 17426

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 17427

@regit regit marked this pull request as draft January 9, 2024 14:19
@catenacyber
Copy link
Contributor

The idea looks good to me :-)
The CI is red :-(

What is expected out of this draft ?
Is there a redmine ticket ?

@regit
Copy link
Contributor Author

regit commented Jan 18, 2024

The idea looks good to me :-) The CI is red :-(

What is expected out of this draft ?

Be sure there is an interest and ack on the factorization.

Is there a redmine ticket ?

I'm opening one. Then I will resubmit the MR with more keywords support and CI fixes.

Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

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

I think this needs a bit more thought it general. I'm reworking the content inspection code, and one of the aspects of that work is that it treats things less like a list of conditions, and more like a "chain" of related sigmatches. So only dumping thing per single sigmatch may prove too limiting. I think it may be wise to wait a bit for my branch to flesh out more.

@@ -42,6 +42,8 @@
typedef struct DetectPcreData_ {
DetectParseRegex parse_regex;
int thread_ctx_id;
/* String for logging */
char *pcre_str;
Copy link
Member

Choose a reason for hiding this comment

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

I want to avoid things like this. The match structures need to be as minimal and "lean" as possible for the matching. This type of data isn't used at matching and shouldn't affect the memory layout.

@@ -1278,6 +1280,8 @@ typedef struct SigTableElmt_ {
int (*SetupPrefilter)(DetectEngineCtx *de_ctx, struct SigGroupHead_ *sgh);

void (*Free)(DetectEngineCtx *, void *);

void (*JsonDump)(JsonBuilder *js, const void *detectdata);
Copy link
Member

Choose a reason for hiding this comment

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

can we use SigMatchData instead?

@jufajardini
Copy link
Contributor

I clearly have an e-mail notification overload x_x
Sorry for missing this RFC until now. Also think that this is a good idea, as something in this direction would make having keyword info available to engine-analysis as part of the process of adding keywords. I see that the ticket was created, but considering Victor's comment, I will try to find out if the re-work that Victor mentioned has a ticket, and link the two.

@catenacyber
Copy link
Contributor

@catenacyber catenacyber added the needs rebase Needs rebase to master label Apr 30, 2024
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 17427

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Needs rebase to master
5 participants