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

[M365 Defender] Adjust mappings for detection rules #9860

Open
wants to merge 51 commits into
base: main
Choose a base branch
from

Conversation

kcreddy
Copy link
Contributor

@kcreddy kcreddy commented May 14, 2024

Proposed commit message

See title

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

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

--- Test results for package: m365_defender - START ---
╭───────────────┬─────────────┬───────────┬─────────────────────────────────────┬────────┬──────────────╮
│ PACKAGE       │ DATA STREAM │ TEST TYPE │ TEST NAME                           │ RESULT │ TIME ELAPSED │
├───────────────┼─────────────┼───────────┼─────────────────────────────────────┼────────┼──────────────┤
│ m365_defender │ alert       │ pipeline  │ test-alert.log                      │ PASS   │  11.464875ms │
│ m365_defender │ event       │ pipeline  │ test-alert.log                      │ PASS   │    3.17075ms │
│ m365_defender │ event       │ pipeline  │ test-app-and-identity.log           │ PASS   │   4.927334ms │
│ m365_defender │ event       │ pipeline  │ test-device.log                     │ PASS   │  50.800875ms │
│ m365_defender │ event       │ pipeline  │ test-email.log                      │ PASS   │  10.704625ms │
│ m365_defender │ incident    │ pipeline  │ test-incident.log                   │ PASS   │  18.932917ms │
│ m365_defender │ log         │ pipeline  │ test-m365-defender-empty-ndjson.log │ PASS   │   1.284125ms │
│ m365_defender │ log         │ pipeline  │ test-m365-defender-ndjson.log       │ PASS   │   8.650125ms │
╰───────────────┴─────────────┴───────────┴─────────────────────────────────────┴────────┴──────────────╯
--- Test results for package: m365_defender - END   ---
Done

Related issues

@kcreddy kcreddy self-assigned this May 14, 2024
@elasticmachine
Copy link

elasticmachine commented May 14, 2024

🚀 Benchmarks report

Package m365_defender 👍(2) 💚(0) 💔(2)

Expand to view
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

@kcreddy kcreddy marked this pull request as ready for review May 17, 2024 10:46
@kcreddy kcreddy requested a review from a team as a code owner May 17, 2024 10:46
@kcreddy kcreddy added the Team:Security-Service Integrations Security Service Integrations Team label May 17, 2024
@elasticmachine
Copy link

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

Comment on lines +867 to +869
- name: dll.Ext.size
type: long
description: Size of the dll executable.
Copy link
Contributor

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?

Copy link
Contributor Author

@kcreddy kcreddy May 20, 2024

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Comment on lines 1322 to 1327
- 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'))
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

@kcreddy kcreddy May 24, 2024

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

@kcreddy kcreddy requested a review from efd6 May 24, 2024 09:57
Copy link
Contributor

@efd6 efd6 left a 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",
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link

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

Copy link
Contributor Author

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

@the-pixel-hunter
Copy link

Can .caseless be added ot process name as this is used in many pre built detections

@efd6
Copy link
Contributor

efd6 commented May 26, 2024

@kcreddy
Copy link
Contributor Author

kcreddy commented May 27, 2024

Thanks, I'll collect all the other items and drop in an issue.

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.

@jvalente-salemstate
Copy link

jvalente-salemstate commented May 27, 2024

@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.

@efd6
Copy link
Contributor

efd6 commented May 27, 2024

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.

@jvalente-salemstate
Copy link

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
Copy link

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.

Copy link
Contributor Author

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

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 under target. The ECS docs make a very good case for this regarding IAM, but I see a strong case in allowing others such as process and host. 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. Using target rather than the existing fields (source.*, destination.*, etc) also enables filtering for/against those details when it's specifically a target.

Copy link
Contributor Author

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.

@elasticmachine
Copy link

💔 Build Failed

Failed CI Steps

History

cc @kcreddy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants