-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add malware_signature for Memory protection alert #155
Conversation
@@ -469,6 +469,8 @@ fields: | |||
exceptionable: true | |||
status: | |||
exceptionable: true | |||
malware_signature: | |||
fields: "*" |
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.
Everything else here looks like this:
fields: "*" | |
fields: {} |
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.
We should update the docs on "*"
vs {}
because I always forget haha. {}
only works when the field is a leaf aka if we only wanted malware_signature.all_names
in the mapping we could write it like:
malware_signature:
fields:
all_names: {}
Since we want everything under malware_signature
and fields
is an array we have to use the "*"
syntax like Chan has here.
@@ -469,6 +469,8 @@ fields: | |||
exceptionable: true | |||
status: | |||
exceptionable: true | |||
malware_signature: |
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.
Should this go into memory_protection_event, since Memory Scan is presented to the user as part of Memory Protection? https://github.com/elastic/endpoint-package/blob/c23a052ef81ea39ab2934e2e1ea5b32bff102196/custom_subsets/elastic_endpoint/alerts/memory_protection_event.yaml
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 should stay where it is at the very least for malware (FileScore) events. We could duplicate it to another location as well for use by memory alerts though I'd vote against that.
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 take it back. I now see you're commenting on process.Ext.malware_signature
not file.Exe.malware_signature
.
Currently Linux only sends file activity based malware (FileScore) alerts (open, write, etc) so wouldn't ever need this location. But Windows and macOS can send process based malware alerts (process start). So I still think this is needed for Windows and macOS signature based FileScore alerts (and wonder why testing has been OK without this so far).
But that's only whether or not this update is beneficial to malware alerts. My presumption would be that a standardized key name for yara signature matches regardless of the protection type would be better than it going in different places for different alert types. But I don't feel strongly, if my presumption is wrong I'll stay silent.
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.
Per our discussion, I will remove this line from this PR. We will add it back in 7.15
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.
Thanks for the PR Chan! Couple questions and changes:
From the example that Kevin linked (memory_signature_agentTesla.json.zip) it looks like memory_region
is included in malware_signature
:
"memory_region": {
"allocation_base": 4194304,
"allocation_protection": "RWX",
"allocation_size": 245760,
"allocation_type": "PRIVATE",
"bytes_address": 4194304,
"bytes_allocation_offset": 0,
do we need to update https://github.com/elastic/endpoint-package/blob/master/custom_schemas/custom_memory_region.yml#L11 to include it within this schema as well?
@@ -469,6 +469,8 @@ fields: | |||
exceptionable: true | |||
status: | |||
exceptionable: true | |||
malware_signature: | |||
fields: "*" |
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.
We should update the docs on "*"
vs {}
because I always forget haha. {}
only works when the field is a leaf aka if we only wanted malware_signature.all_names
in the mapping we could write it like:
malware_signature:
fields:
all_names: {}
Since we want everything under malware_signature
and fields
is an array we have to use the "*"
syntax like Chan has here.
|
||
- name: secondary | ||
level: custom | ||
type: array |
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.
In Elasticsearch there isn't an array
type. Everything can be an array out of the box: https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-types.html#types-array-handling
Could we change this to the structure and type that will be contained within secondary
? If secondary
will be the exact same as primary
, you could abstract it out to a new file and have it overlaid as both primary
and secondary
using the ECS reuse
mechanism.
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.
secondary
will be a list of object that are subset of the primary
. Should I just say object
here then?
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.
If we use object
then users won't be able to search for it beyond just using a search based on its existence. If we don't expect users to search for the secondary
fields then you could exclude it from the definition here. The only reason we want to define it in the endpoint package is so that users can search for it. If you want to include it just as a description of a field that could exist in an alert, we can add enabled: false
(e.g. https://github.com/elastic/endpoint-package/blob/master/custom_schemas/custom_endpoint.yml#L28) that way Elasticsearch will not index it.
I'm not sure if we need to include them. IMO creating an Exception based on those values would be too much of information. |
I guess what I mean is, based on the example that Kevin gave I'm assuming the endpoint will send those memory region values. If we expect the user to want to search for alerts based on @kevinlog @pzl could you take a look at the mappings when you get a chance. |
@jonathan-buttner thanks for bubbling this up.
There are two questions to answer here:
I don't think we need to index this field for searching. However, we may want it included in the Index pattern so that we can write exceptions for it. @gogochan @gabriellandau @ferullo any thoughts on writing Exceptions? If we DO want to write Exceptions, we can |
@joe-desimone has thoughts on what signature match fields should be available for exceptions. |
I believe users should have the ability to search for and create exceptions from the following fields: @dstepanic17 can you double check my work Also side note, the memory_region stuff is landing in production at 7.15 at the earliest. Not applicable to linux file yara scanning which is landing in 7.14. |
@joe-desimone Here, are a couple fields I did not see on your list. I believe the
|
Thank you. I actually had allocation_base on there by mistake, and left off region_base intentionally. My thinking was these are going to be generally random so not too helpful from an exceptionlist point of view (but useful for response). allocation_offset I waffled on. In some situations it could be useful so I added it now. Also, we dont have it yet but we should add malware_signature.memory_region.memory_pe.imphash to match shellcode alert schema more closely. We would want to search+exception on that as well so I added it to my original list. |
Just want to point out that the current workflow does exception list checking before reading process memory, thus it lacks We can alter the behavior, but it means that we are checking the exception list after scanning process memory. |
@jonathan-buttner We will make a separate PR to address this. I've adjusted the PR per comments provided. Let me know if this is good. |
@joe-desimone per our recent conversations, do you think we should put |
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.
Left a few more comments about changes I think we need
Thanks @jonathan-buttner for the suggestion. It looks better now. |
Endpoint should definitely check the exception list after it scans memory and can generate the alert but before it takes action. Checking exceptions earlier than this is a good optimization but this final check is always needed for all protections. |
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 good! Just one question.
top_level: false | ||
expected: | ||
- process.Ext | ||
- file.Ext |
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.
Just a heads up it doesn't look like malware_signature
is being placed under file.Ext
🤔 that might be because the file subset file doesn't include it 🤷♂️ is that ok?
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 updated the title. All fields added here are for Memory Protection plugin.
Agree here. I had the understanding that we do some early exceptionlist tests which would allow us better performance like https://github.com/elastic/global-security-exceptionlist/pull/257/files. Might be a good place for unit test. |
Yes which would help makes things more consistent between shellcode alerts and memory scan alerts |
Created an issue to specifically discuss this proposal https://github.com/elastic/security-team/issues/1496 |
This may require further modification based on https://github.com/elastic/security-team/issues/1496 |
Added changes proposed in https://github.com/elastic/security-team/issues/1496 |
order: 1 | ||
top_level: false | ||
expected: | ||
- memory_region |
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.
👍
@@ -9,6 +9,7 @@ | |||
order: 1 | |||
top_level: false | |||
expected: | |||
- process.Ext |
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.
Just curious, should we be calling it start_address_details
within process.Ext
too like in process.thread.Ext
?
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 pushed changes that addressed my concerns. LGTM.
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.
Question about renaming start_address_details
@@ -262,27 +262,6 @@ fields: | |||
start_address_bytes: {} | |||
start_address_bytes_disasm: {} | |||
start_address_bytes_disasm_hash: {} | |||
start_address_details: |
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 will result in a breaking change since we're renaming these fields. Has the endpoint already started populating these fields in 7.14 or prior? If it has, do any of our detection rules use these fields? If they do we'll be causing them to no longer work. Do you think it's likely that users have custom detection rules triggering based on these fields?
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.
Has the endpoint already started populating these fields in 7.14 or prior?
Only for diagnostic. The shellcode feature isn't going live until 7.15.
If it has, do any of our detection rules use these fields?
Negative. Not until 7.15.
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.
Ok perfect! Thanks Gabe!
Issue: https://github.com/elastic/security-team/issues/1214
Adding additional fields that are part of yara signature. The fields are used in FileScore alert and MemoryScan Alert.
malware_signature.primary.signature.hash.sha256malware_signature.primary.matchesversionidentifier