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

exception: in ids mode, only REJECT the packet - v1 #8970

Closed
wants to merge 1 commit into from

Conversation

jufajardini
Copy link
Contributor

In case of 'EXCEPTION_POLICY_REJECT', we were applying the same behavior regardless of being in IDS or IPS mode.
This meant that at least the 'flow.action' was changed to drop when we hit an exception policy in IDS mode. This minor fix makes the SV test pass, but I'm afraid the bug can mean more than that.

Bug #6109

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

Describe changes:

  • Check if we are in IPS mode before falling through drop actions when applying REJECT exception policy.
    This fix 6109 test, but... is that enough?

I'm not confident about how REJECT works as a whole, so I feel this might not be enough to ensure that we'll still reject things properly. Also not sure if this would be the right approach in cases where we should reject the flow in IDS mode.
Makes me wonder if we should have REJECT for flow or packet, like we have for DROP and PASS.

SV_BRANCH=pr/1229

OISF/suricata-verify#1229

@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #8970 (8d1e93c) into master (6154bab) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 8d1e93c differs from pull request most recent head eb69e10. Consider uploading reports for the commit eb69e10 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8970      +/-   ##
==========================================
- Coverage   82.42%   82.41%   -0.01%     
==========================================
  Files         969      969              
  Lines      273476   273478       +2     
==========================================
- Hits       225410   225392      -18     
- Misses      48066    48086      +20     
Flag Coverage Δ
fuzzcorpus 64.93% <0.00%> (-0.01%) ⬇️
suricata-verify 60.49% <100.00%> (-0.02%) ⬇️
unittests 62.91% <0.00%> (-0.01%) ⬇️

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 14247

@victorjulien
Copy link
Member

"reject-flow" would depend on https://redmine.openinfosecfoundation.org/issues/960

We don't have this.

@victorjulien
Copy link
Member

I think the code looks good, but the commit message needs to be fearless :)

In case of 'EXCEPTION_POLICY_REJECT', we were applying the same behavior
regardless of being in IDS or IPS mode.
This meant that (at least) the 'flow.action' was changed to drop when we
hit an exception policy in IDS mode.

Bug OISF#6109
@jufajardini
Copy link
Contributor Author

😅 roger that.

@jufajardini
Copy link
Contributor Author

"reject-flow" would depend on https://redmine.openinfosecfoundation.org/issues/960

We don't have this.

oh! (best part is that I am subscribed to that ticket, and had no recollection of that >__<')

@jufajardini
Copy link
Contributor Author

I think the code looks good, but the commit message needs to be fearless :)

Does this one work? eb69e10

@suricata-qa
Copy link

Information:

ERROR: QA failed on SURI_TLPW1_files_sha256.

field baseline test %
SURI_TLPR1_stats_chk
.http.memuse 2104 0 -

Pipeline 14293

@jufajardini jufajardini marked this pull request as ready for review June 7, 2023 13:14
@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPR1_stats_chk
.flow.memuse 519171056 665403936 128.17%

Pipeline 14320

@victorjulien victorjulien mentioned this pull request Jun 8, 2023
@victorjulien
Copy link
Member

Merged in #8994, thanks!

@jufajardini jufajardini deleted the 6109-bug/v1 branch June 12, 2023 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants