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

detect/content: Consider distance in validation #6846

Closed
wants to merge 3 commits into from

Conversation

jlucovsky
Copy link
Contributor

@jlucovsky jlucovsky commented Jan 22, 2022

Continuation of #6838

This commit modifies the validation callback to include the distance
during validation.

Values of distance that cause the right edge to be exceeded are
considered an error and the signature will be rejected.

Link to redmine ticket: 2982

Describe changes:

  • Centralized content/dsize validation

Updates:

  • clang-format fixup

suricata-verify-pr: 673
#suricata-verify-repo:
#suricata-verify-branch:
#suricata-update-pr:
#suricata-update-repo:
#suricata-update-branch:
#libhtp-pr:
#libhtp-repo:
#libhtp-branch:

Ticket: 2982

This commit validates that the content usage in a rule will not exceed
the dsize value.

Values of distance that cause the right edge to be exceeded are
considered an error and the signature will be rejected.
This commit replaces a SCMalloc/memset with SCCalloc
@codecov
Copy link

codecov bot commented Jan 22, 2022

Codecov Report

Merging #6846 (916178b) into master (7b44485) will decrease coverage by 0.01%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #6846      +/-   ##
==========================================
- Coverage   77.70%   77.69%   -0.02%     
==========================================
  Files         628      628              
  Lines      187247   187281      +34     
==========================================
+ Hits       145503   145508       +5     
- Misses      41744    41773      +29     
Flag Coverage Δ
fuzzcorpus 57.98% <82.85%> (+0.02%) ⬆️
suricata-verify 53.26% <90.32%> (-0.29%) ⬇️
unittests 62.84% <80.64%> (+<0.01%) ⬆️

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

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.

Minor comment inline.

I guess the bigger issue is to work with QA (and perhaps ET?) to get all checks to pass...

@@ -402,6 +400,17 @@ bool DetectContentPMATCHValidateCallback(const Signature *s)

uint32_t max_right_edge = (uint32_t)max_right_edge_i;

int min_dsize_required = SigParseMaxRequiredDsize(s);
if (min_dsize_required >= 0) {
SCLogNotice("min_dsize %d; max_right_edge %d", min_dsize_required, max_right_edge);
Copy link
Member

Choose a reason for hiding this comment

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

debug

@suricata-qa
Copy link

ERROR: Invalid Signature config error in tlpr1_asan_cfg QA test

ERROR: QA failed on tlpr1_asan_cfg.

Pipeline 5887

@jlucovsky
Copy link
Contributor Author

Continued in #6939

@jlucovsky jlucovsky closed this Feb 7, 2022
@jlucovsky jlucovsky deleted the 2982/4 branch November 21, 2022 13:21
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.

3 participants