-
Notifications
You must be signed in to change notification settings - Fork 375
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
[M365 Defender] Adjust mappings for detection rules #9860
base: main
Are you sure you want to change the base?
Conversation
🚀 Benchmarks reportPackage
|
Data stream | Previous EPS | New EPS | Diff (%) | Result |
---|---|---|---|---|
incident |
620.73 | 518.67 | -102.06 (-16.44%) | 💔 |
log |
4975.12 | 3246.75 | -1728.37 (-34.74%) | 💔 |
Package ti_rapid7_threat_command
👍(1) 💚(1) 💔(1)
Expand to view
Data stream | Previous EPS | New EPS | Diff (%) | Result |
---|---|---|---|---|
alert |
3412.97 | 2232.14 | -1180.83 (-34.6%) | 💔 |
To see the full report comment with /test benchmark fullreport
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
packages/m365_defender/data_stream/event/_dev/test/pipeline/test-device.log-expected.json
Outdated
Show resolved
Hide resolved
- name: dll.Ext.size | ||
type: long | ||
description: Size of the dll executable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to use file.size
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like file.size
is already populated with same value. The value is also copied into this field (taken from endpoint-dev's ECS flat file), which is required for detection rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The detection rules probably should be made to conform to the ECS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The detection rules probably should be made to conform to the ECS.
The proposed changes are meant to adjust previous work on the integration while enhancing compatibility with our own EDR. Detection rules use the data. Definitions and background around mapping decisions within Elastic Defend can be discussed with endpoint-dev.
packages/m365_defender/data_stream/event/elasticsearch/ingest_pipeline/pipeline_device.yml
Outdated
Show resolved
Hide resolved
packages/m365_defender/data_stream/event/elasticsearch/ingest_pipeline/pipeline_device.yml
Outdated
Show resolved
Hide resolved
packages/m365_defender/data_stream/event/elasticsearch/ingest_pipeline/pipeline_device.yml
Outdated
Show resolved
Hide resolved
packages/m365_defender/data_stream/event/elasticsearch/ingest_pipeline/pipeline_device.yml
Outdated
Show resolved
Hide resolved
packages/m365_defender/data_stream/event/elasticsearch/ingest_pipeline/pipeline_device.yml
Outdated
Show resolved
Hide resolved
packages/m365_defender/data_stream/event/elasticsearch/ingest_pipeline/pipeline_device.yml
Outdated
Show resolved
Hide resolved
packages/m365_defender/data_stream/event/elasticsearch/ingest_pipeline/pipeline_device.yml
Outdated
Show resolved
Hide resolved
packages/m365_defender/data_stream/event/elasticsearch/ingest_pipeline/pipeline_device.yml
Outdated
Show resolved
Hide resolved
- set: | ||
field: process.pe.sections.physical_size | ||
copy_from: m365_defender.event.file.size | ||
ignore_empty_value: true | ||
tag: set_process_pe_sections_physical_size | ||
if: ctx.event?.category != null && !ctx.m365_defender.event.category.toLowerCase().contains('deviceimageloadevents') && (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.m365_defender.event.category.toLowerCase().contains('deviceimageloadevents') && (ctx.event.category.contains('process') || ctx.m365_defender.event.category.toLowerCase().contains('deviceevents')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the same issue. The field is being used incorrectly.
I have filed an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to me. @w0rk3r WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objections, we don't use process.pe.sections.physical_size
anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed reference to *pe.sections.physical_size
in 69e9734
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "time" mean? It is not documented in the README or fields.yml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
- set: | ||
field: process.parent.start | ||
copy_from: m365_defender.event.initiating_process.creation_time | ||
ignore_empty_value: true | ||
tag: set_process_parent_start | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@w0rk3r WDYT?
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
target
can 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 asprocess
andhost
. 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. Usingtarget
rather 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
💔 Build Failed
Failed CI StepsHistory
cc @kcreddy |
Proposed commit message
See title
Checklist
changelog.yml
file.Author's Checklist
host.os.type
: Need sample data to validate non-windowsOSPlatform
values 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 -v
Related issues