fix(sentinelone): handle missing signature data gracefully in matching (#415)#421
Draft
Seb-MIGUEL wants to merge 1 commit into
Draft
fix(sentinelone): handle missing signature data gracefully in matching (#415)#421Seb-MIGUEL wants to merge 1 commit into
Seb-MIGUEL wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes SentinelOne expectation matching for static threats when some signature data (notably parent_process_name from events/DV) is missing, avoiding silent mismatches and improving diagnostics so detections still correlate correctly in OpenAEV.
Changes:
- Skip signature-type checks when the threat provides no corresponding OAEV data for that signature type, rather than failing the entire match.
- Add a guard clause requiring at least one signature type to be verifiably matched to prevent false positives when all data is missing.
- Add two regression tests covering (1) hostname-only matching for static threats without events and (2) no-match when no signature data is available.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
sentinelone/src/services/expectation_service.py |
Makes signature matching resilient to missing threat data and improves matching error logging. |
sentinelone/tests/services/test_expectation_service.py |
Adds regression tests for static threats without DV events and for the “no verifiable signature” guard clause. |
Comment on lines
+612
to
+618
| self.logger.info( | ||
| f"{LOG_PREFIX} Expectation {expectation.inject_expectation_id} requires " | ||
| f"'{sig_type}' but threat {threat.threat_id} has no data for it " | ||
| f"(available: {list(filtered_oaev_data.keys())}). " | ||
| f"Skipping this signature check — match will rely on remaining signatures." | ||
| ) | ||
| continue |
Contributor
Author
There was a problem hiding this comment.
Fixed — lowered to DEBUG and improved the log message to show verified/skipped counts.
Comment on lines
+655
to
+657
| self.logger.debug( | ||
| f"{LOG_PREFIX} All signatures matched for expectation {expectation.inject_expectation_id} vs threat {threat.threat_id}" | ||
| f"{LOG_PREFIX} All verifiable signatures matched ({matched_count}/{len(signature_groups)}) " | ||
| f"for expectation {expectation.inject_expectation_id} vs threat {threat.threat_id}" |
Contributor
Author
There was a problem hiding this comment.
Fixed — lowered to DEBUG and improved the log message to show verified/skipped counts.
When a threat lacks data for a required signature type (e.g. static threats without Deep Visibility have no parent_process_name), the matching now skips that check and relies on remaining signatures instead of silently failing via KeyError. This fixes the detection gap for static threats (e.g. ART droppers blocked during download) where S1 detects and blocks the payload but the collector couldn't match it because: 1. Static threats need DV for parent_process_name (disabled by default) 2. Missing key caused KeyError caught by generic except -> False Also: - Added matched_count guard: at least one signature must be verified - Improved error logging with exc_info for debugging - Removed leftover breakpoint() comments - Added 2 regression tests for the fix Refs: #415
Seb-MIGUEL
force-pushed
the
seb/fix/sentinelone-detection-gap
branch
from
a119c80 to
723bfbb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #415
Root Cause Analysis
The SentinelOne collector was failing to detect static threats (e.g. ART droppers blocked during download) due to two issues in the matching logic:
Bug 1: KeyError in signature matching (silent failure)
In
_expectation_matches_threat_data(), when iterating over expected signature types, the code assumed all signature types would have corresponding data from the threat:When
parent_process_namewasn't available (no events), this raised aKeyErrorcaught by a genericexcept Exceptionthat silently returnedFalse(no match).Bug 2: Static threats without DV have no parent_process_name
Static threats (file-based detections like droppers) require Deep Visibility to get process information. When DV is disabled (default), static threats have NO events, thus no
parent_process_namedata → Bug 1 triggers → silent mismatch.Fix
exc_info=Trueand the actual expectation/threat IDsbreakpoint()commentsImpact
With this fix, a static threat detected by S1 will now match the expectation based on the available
target_hostname_addresssignature, even whenparent_process_namecan't be verified (because DV is disabled or events are empty).Tests
test_static_threat_matches_on_hostname_without_process_name: verifies dropper scenariotest_no_match_when_all_signature_data_unavailable: verifies no false positives