[M365 Defender] Adjust mappings for detection rules - #9860
Conversation
kcreddy
added
enhancement
🚀 Benchmarks reportPackage
|
| Data stream | Previous EPS | New EPS | Diff (%) | Result |
|---|---|---|---|---|
alert |
732.6 | 507.1 | -225.5 (-30.78%) | 💔 |
event |
704.72 | 261.51 | -443.21 (-62.89%) | 💔 |
incident |
640.2 | 364.17 | -276.03 (-43.12%) | 💔 |
To see the full report comment with /test benchmark fullreport
efd6
left a comment
efd6
left a comment
There was a problem hiding this comment.
The use of capitalised fields here makes me concerned.
The ECS allows the use of capitialised fields to prevent namespace collision as noted here. While this is allowed it weakens the ECS and opens up the potential for users to experience difficult to understand and harder to resolve problems with ingest and mapping.
The (unfortunately not explicitly stated) background for field capitalisation use is that we (Elastic) do not use capitalised fields, and so it is safe for anyone else to use them so long as they do not make that capitalisation public (i.e. the schema they use does not end up in publicly visible documents). This ensures that there is no possible way for two fields to have the same name with different mapping types except within a single org (that should have avoided it since it was entirely under their control). By providing a mapping with capitalised name in a publicly visible mapping definition, we (Elastic) break that contract.
The use of these fields to allow the ECS-ish to conform to the mappings used by another product is further troubling since it indicates a breakdown of component interface boundaries, where in this case aspects of our EDR contaminate how we use ECS. Ideally, if fields are missing from that ECS to properly capture the semantics of our EDR, fields should be added to the ECS to allow those semantics to be used more widely. The proliferation of fields with essentially identical semantics is also an issue (in particular dll.Ext.size which is perfectly captured by file.size in conjunction with the presence of any dll.* field).
I'm not going to block this, but I wanted to register that I don't agree with the approach that has been required here.
| }, | ||
| "pid": 5692, | ||
| "start": "2022-11-07T16:45:10.395Z" | ||
| "time": "2024-05-08T15:39:06.410Z", |
There was a problem hiding this comment.
What does "time" mean? It is not documented in the README or fields.yml.
There was a problem hiding this comment.
I am unable to find references to this field. My guess is its probably the time when event is made available in EventHub. We derive @timestamp from the event's Timestamp field.
There was a problem hiding this comment.
@kcreddy
It looks like time here is when the event was received by M365 Defender XDR
{
"records": [
{
"time": "<The time Microsoft Defender XDR received the event>"
"tenantId": "<The Id of the tenant that the event belongs to>"
"category": "<The Advanced Hunting table name with 'AdvancedHunting-' prefix>"
"properties": { <Microsoft Defender XDR Advanced Hunting event as Json> }
}
...
]
}This may be good for event.created maybe. Or an extended field in m365_defender.event
There was a problem hiding this comment.
Added description for time and category fields in: 8638954
|
Can .caseless be added ot process name as this is used in many pre built detections |
Hey @jvalente-salemstate Just checking if you ended up creating the issue? If not maybe we can move forward with this PR and create another for the new issue. |
|
@kcreddy Would you prefer the issue before or after this is merged? There were several items and a few were fixed (like the handling of initiating vs parent process) so I was holding off until this was merged to assess the entire scope of what was fixed and what wasn't. Either is fine with me. Perhaps after may be better since some items are just about dashboards and a few are also about the field naming @efd6 had mentioned. |
If there are things that need to be addressed that would result in additions here being removed, they should be dealt with before this is concretised into a released version. |
Sounds good. I'll pull these out separately into an issue and share it once posted. |
| if: ctx.event?.category != null && (ctx.event.category.contains('process') || ctx.m365_defender.event.category.toLowerCase().contains('deviceevents')) | ||
| if: ctx.event?.category != null && !ctx.event.category.contains('api') && !ctx.event.category.contains('driver') && (ctx.event.category.contains('process') || ctx.m365_defender.event.category.toLowerCase().contains('deviceevents')) | ||
| - set: | ||
| field: process.parent.name |
There was a problem hiding this comment.
@kcreddy This should also exclude deviceevents or map this to process.name. See #9993
This change does seem to fix dll load process events but this would still be an issue on device events. I could see this also introducing a large number of false positives with events where that specific process being a parent process is an IoC.
There was a problem hiding this comment.
After discussion, the change has been added to all 32 event.actions reported in the #9993.
Change is added in 09cc04d
@jvalente-salemstate Does the change look good? I also added some sample events corresponding to the event.action values you described in the issue.
| - name: process.Ext.api.parameters.desired_access_numeric | ||
| type: long | ||
| description: This parameter indicates the numeric value of the `DesiredAccess` field passed to `OpenProcess` or `OpenThread`. | ||
| - name: Target.process.name |
There was a problem hiding this comment.
@kcreddy I've read the discussion on the review by @efd6 so I get this may be a much larger discussion between internal Elastic teams than this PR; this is more for suggestions for future revisions. This may also need involvement with the ECS team.
ECS does lack a good way to demonstrate this but I am thinking this bit can be resolved by either/both:
-
Allowing/Using
target.*as a root level field (same approach here, without the capital T) -
Expanding where
targetcan be nested, and what can be nested undertarget. The ECS docs make a very good case for this regarding IAM, but I see a strong case in allowing others such asprocessandhost. That would allow for accurate representation of things like this as well as a wider range of EDR/XDR or administrative activities. Examples would be the target host for an isolation event. Target IP/URL for block/allow list changes. Usingtargetrather than the existing fields (source.*,destination.*, etc) also enables filtering for/against those details when it's specifically a target.
There was a problem hiding this comment.
I agree this would need discussing above possible solutions with ECS team. Also, in current ECS definitions, the target does appear to be nested in few places such as user.target.*, service.target.*, cloud.target.*, and this could be extended further.
|
@jvalente-salemstate Lets create a new public issue for |
💚 Build Succeeded
History
cc @kcreddy |
|
| if: ctx.json?.properties?.ActionType != null && (ctx.json.properties.ActionType.toLowerCase().contains('registrykeyrenamed') || ctx.json.properties.ActionType.toLowerCase().contains('registryvalueset')) | ||
|
|
||
| - set: | ||
| field: json.properties.ActionType |
There was a problem hiding this comment.
Should this be event.action?
There was a problem hiding this comment.
This is being set to event.action later in the pipeline.
|
Package m365_defender - 2.11.0 containing this change is available at https://epr.elastic.co/search?package=m365_defender |
A followup of PR #9860 to enhance ECS mappings for detection rules.
andrewkroh
added
the
Integration:ti_rapid7_threat_command
8 participants
Proposed commit message
Checklist
changelog.ymlfile.Author's Checklist
host.os.type: Need sample data to validate non-windowsOSPlatformvalues for: DeviceInfo, CloudAppEvents, IdentifyLogonEvents.How to test this PR locally
$ elastic-package stack down && elastic-package build && elastic-package stack up -d -v && eval "$(elastic-package stack shellinit)" && elastic-package test pipeline --generate -vRelated issues
process.parent. #9993