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: debug validation for list ids overflows #6316

Closed

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/4403

Describe changes:

  • Adds debug validation for list id integer overflow

975062c#diff-99eda33658bd0778da7bf89acbb4e7bbdb9ce82b0ab93486e1643691925f4091L600 has changed sm_list from int to int16_t
But there are no checks against integer overflow on this shorter integer

@catenacyber catenacyber requested a review from a team as a code owner August 30, 2021 20:06
@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #6316 (eb077ae) into master (7551247) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6316      +/-   ##
==========================================
- Coverage   76.95%   76.94%   -0.01%     
==========================================
  Files         611      611              
  Lines      185955   185955              
==========================================
- Hits       143102   143085      -17     
- Misses      42853    42870      +17     
Flag Coverage Δ
fuzzcorpus 52.87% <ø> (+0.01%) ⬆️
suricata-verify 51.09% <ø> (-0.03%) ⬇️
unittests 63.12% <ø> (ø)

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

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 3960

@victorjulien victorjulien mentioned this pull request Aug 31, 2021
@catenacyber
Copy link
Contributor Author

And after a long quadratic time, I get the debug assert to trigger...
Let's see if oss-fuzz is getting it the same way

@victorjulien
Copy link
Member

The id increases with the number of rules using unique combinations of transactions, I think. Is that what you're seeing?

This was referenced Sep 1, 2021
@victorjulien
Copy link
Member

Merged in #6324, thanks!

@catenacyber
Copy link
Contributor Author

Let's now wait for google/oss-fuzz#6362 to get oss-fuzz back to build latest suricata and see if this is the bug getting triggered

@catenacyber
Copy link
Contributor Author

The id increases with the number of rules using unique combinations of transactions, I think. Is that what you're seeing?

The unique combination of transforms ...
That is indeed what I am seeing (with the script from the redline ticket to generate 65k rules with different combinations of transforms)
Maybe there is another way to increase this id...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants