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

fix: |re matching is not done correctly, so use is_match instead of find_iter #1212

Merged
merged 5 commits into from
Nov 11, 2023

Conversation

fukusuket
Copy link
Collaborator

@fukusuket fukusuket commented Nov 8, 2023

What Changed

Evidence

Enviroment

  • OS: macOS Sonoma version 14.0

Test

I run the steps to reproduce #1211 and confirmed that 6 cases were detected as follows.

% ./hayabusa csv-timeline -d ../hayabusa-sample-evtx -r test.yml -w -o out.csv -C -q
...
Results Summary:

Events with hits / Total events: 6 / 47,465 (Data reduction: 47,459 events (99.99%))

Total | Unique detections: 6 | 1
Total | Unique critical detections: 0 (0.00%) | 0 (0.00%)
Total | Unique high detections: 0 (0.00%) | 0 (0.00%)
Total | Unique medium detections: 0 (0.00%) | 0 (0.00%)
Total | Unique low detections: 0 (0.00%) | 0 (0.00%)
Total | Unique informational detections: 6 (100.00%) | 1 (100.00%)

I would appreciate it if you could review when you have time🙏

@fukusuket fukusuket self-assigned this Nov 8, 2023
@fukusuket fukusuket added the bug Something isn't working label Nov 8, 2023
@fukusuket fukusuket changed the title fix: '|re' matching is not done correctly, so use is_match instead of find_iter fix: |re matching is not done correctly, so use is_match instead of find_iter Nov 8, 2023
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7006a2d) 83.60% compared to head (5b030e0) 83.61%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1212      +/-   ##
==========================================
+ Coverage   83.60%   83.61%   +0.01%     
==========================================
  Files          26       26              
  Lines       23818    23835      +17     
==========================================
+ Hits        19912    19929      +17     
  Misses       3906     3906              
Files Coverage Δ
src/detections/rule/matchers.rs 97.35% <100.00%> (+0.01%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@hitenkoku hitenkoku left a comment

Choose a reason for hiding this comment

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

LGTM

@fukusuket
Copy link
Collaborator Author

@hitenkoku Thank you so much for review :) Sorry... There was a regression case, so I'll draft it once...

@fukusuket fukusuket marked this pull request as draft November 9, 2023 00:13
@fukusuket
Copy link
Collaborator Author

The detection result diff of hayabusa-sample-evtx was as follows.
It seems that there is a regression in case Renamed Exe File , so I will check...

Rule title level main This PR
Invoke-Obfuscation Obfuscated IEX Invocation - PowerShell high 0 9
Invoke-Obfuscation Via Use Clip - Powershell high 0 1
Potential AD User Enumeration From Non-Machine Account med 5 17
Powershell Token Obfuscation - Powershell high 0 10
Renamed Exe File low 6 0

@fukusuket
Copy link
Collaborator Author

@hitenkoku @YamatoSecurity
The diff in theRenamed Exe File case was not caused by a regression, but by the issue below.
Yamato-Security/hayabusa-rules#526

Therefore, the difference in the number of detections above does not seem to be a problem :)

@fukusuket fukusuket marked this pull request as ready for review November 9, 2023 09:47
Copy link
Collaborator

@YamatoSecurity YamatoSecurity left a comment

Choose a reason for hiding this comment

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

@fukusuket Sorry it took some time. I took some benchmarks and this PR is slightly faster, uses about the same memory and detects more! So LGTM!

@YamatoSecurity YamatoSecurity merged commit 143aea3 into main Nov 11, 2023
9 checks passed
@fukusuket fukusuket deleted the 1211-fix-invalid-regex-match branch November 12, 2023 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] |re modifier rule does not detect logs correctly
3 participants