[Winlogbeat] Improve ECS field mappings in Sysmon module. #18381

marc-gr · 2020-05-08T14:08:21Z

What does this PR do?

Improve ECS field mappings in Sysmon module.

related.hash, related.ip and related.user are now populated.
hashes are now also populated to the corresponding process.hash or file.hash
file.name, file.directory and file.extension are now populated.
rule.name is populated for all events when present.

Why is it important?

Sysmon module was not reporting some fields and was reporting others in a namespace that was not aligned with ECS. This adds changes to improve the ECS field mapping for it.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~I have made corresponding changes to the documentation~~
~~I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

Set related.hash, related.ip and related.user
Set file.extension/name/directory.
hash.* is not part of ECS. It should be used as file.hash., process.hash. or process.parent.hash.*.
Drop the rule.name field when it has a tack - value

NOTES:

process.parent.hash is not present in any event (you can check here), so nothing was done for it.
rule.name has been added to all events that can have it according to the previous schema, but will be ignored if empty or its value is -.
as mentioned in [Winlogbeat] More ECS changes for Sysmon #18364, the previous root level hash object will not be removed until 8.0 since it is a breaking change.

Related issues

Closes #18364

elasticmachine · 2020-05-08T14:08:33Z

Pinging @elastic/siem (Team:SIEM)

elasticmachine · 2020-05-08T14:45:04Z

💚 Build Succeeded

Expand to view the summary

Build stats

Build Cause: [Pull request [Winlogbeat] Improve ECS field mappings in Sysmon module. #18381 updated]
Start Time: 2020-05-14T11:30:34.597+0000
Duration: 37 min 51 sec (2211498)

Test stats 🧪

Test	Results
Failed	0
Passed	81
Skipped	0
Total	81

leehinman

Nice. Couple suggestions around using AppendTo for the arrays.

x-pack/winlogbeat/module/sysmon/config/winlogbeat-sysmon.js

AppendTo doesn't make an array when you just add one item, not what we want.

marc-gr · 2020-05-11T13:59:54Z

At the end it was changed to use AppendTo as @leehinman suggested since from ES standpoint there is no practical difference.

andrewkroh

Looks great, just one minor comment on behavior.

CHANGELOG.next.asciidoc

x-pack/winlogbeat/module/sysmon/config/winlogbeat-sysmon.js

andrewkroh · 2020-05-13T13:39:55Z

x-pack/winlogbeat/module/sysmon/config/winlogbeat-sysmon.js

-    var addHashes = function (evt, hashField) {
+    var setRelatedIP = function (evt) {
+        var sourceIP = evt.Get("source.ip");
+        if (sourceIP && net.isIP(sourceIP)) {


isIP is unnecessary since you are copying from source.ip a value that has already been validating by the convert processor and type: ip.

andrewkroh · 2020-05-13T13:42:24Z

x-pack/winlogbeat/module/sysmon/test/testdata/sysmon-11-filedelete.evtx.golden.json

+      "entity_id": "{42f11c3b-4664-5eba-91ae-000000000000}",
+      "executable": "C:\\Windows\\system32\\svchost.exe",
+      "hash": {
+        "imphash": "00000000000000000000000000000000",


Maybe we should drop these? @andrewstucki, does this value have any meaning?

So that looks odd to me--my guess is that sysmon is returning all zeroes in the case of not being able to grab the import symbols of the binary--I'd have to read the docs around it a bit more to be sure. That said, for sure that's not a valid value. Also--imphash itself is a pe specific thing and shouldn't be treated like other general purposes hashes here, we actually carved out a field for it in pe in ECS--so this should get:

Dropped if it's all zeroes

Potentially dropped from the related.hash field (clarifying with @webmat that that's meant for general purpose hashes, correct?)

Moved from process.hash.imphash to process.pe.imphash

btw, realized that imphash stuff is actually still not fully released (it's in unreleased ECS 1.6). It's not going away any time soon since the endpoint is using this field as well, but just making sure we're cool shipping this field a bit early?

https://www.elastic.co/guide/en/ecs/master/ecs-pe.html

Semantics of related.hash are very broad. Any hash should be added there, even if it's a very specific thing like imphash 👍

then, keeping it in related.hash and moving it from process.hash.imphash to process.pe.imphash sounds good, as far as it is good to ship process.pe.imphash now?

Forward compatibility on upcoming fields like process.pe.imphash is a product decision.

Although from the schema perspective, I've seen zero controversy about this new field. So I can say the risks of it changing until the ECS release are very slim. So this has my 👍

I think so.

For testing this I'd try seeing if you can ensure that bad imphash isn't in the output--and then try an generate another event that actually captures the proper imphash.

I poked around to find references to sysmon returning all zero hashes and this is the only thing I could find and it seems to indicate an "empty imphash value".

What's interesting is that I just grabbed the hash for svchost.exe from the Windows 2019 VM and it should be 247b9220e5d9b720a82b2c8b5069ad69--so I'm wondering if something is just misconfigured in sysmon? Either way we should be stripping out the 0000s--so a test case for inclusion of valid values and exclusion of the all zeros might be nice.

andrewkroh · 2020-05-14T11:12:56Z

x-pack/winlogbeat/module/sysmon/config/winlogbeat-sysmon.js

+            return true;
+        }
+
+        var match = emptyHashRegex.exec(value);


Can you use .test instead of .exec since you don't need matches. This is usually more efficient.

andrewkroh · 2020-05-14T11:14:23Z

x-pack/winlogbeat/module/sysmon/config/winlogbeat-sysmon.js

+        return namespace + ".hash." + hashKey;
+    };
+
+    var emptyHashRegex = new RegExp(/^0*$/);


Suggested change

var emptyHashRegex = new RegExp(/^0*$/);

var emptyHashRegex = /^0*$/;

Use literal notation when the regular expression will remain constant. Also with the constructor version the parameters are not enclosed in slashes IIRC.

- related.hash, related.ip, and related.user are now populated. - hashes are now also populated to the corresponding process.hash, process.pe.imphash, file.hash or file.pe.imphash - file.name, file.directory, and file.extension are now populated. - rule.name is populated for all events when present. Closes elastic#18364

andrewkroh

LGTM

- related.hash, related.ip, and related.user are now populated. - hashes are now also populated to the corresponding process.hash, process.pe.imphash, file.hash or file.pe.imphash - file.name, file.directory, and file.extension are now populated. - rule.name is populated for all events when present. Closes elastic#18364 (cherry picked from commit 096b88e)

…w-oss * upstream/master: (27 commits) Disable host fields for "cloud", panw, cef modules (elastic#18223) [docs] Rename monitoring collection from legacy internal collection to legacy collection (elastic#18504) Introduce auto detection of format (elastic#18095) Add additional fields to address issue elastic#18465 for googlecloud audit log (elastic#18472) Fix libbeat import path in seccomp policy template (elastic#18418) Address Okta input issue elastic#18530 (elastic#18534) [Ingest Manager] Avoid Chown on windows (elastic#18512) Fix Cisco ASA/FTD msgs that use a host name as NAT address (elastic#18376) [CI] Optimise stash/unstash performance (elastic#18473) Libbeat: Remove global loggers from libbeat/metric and libbeat/cloudid (elastic#18500) Fix PANW bad mapping of client/source and server/dest packets and bytes (elastic#18525) Add a file lock to the data directory on startup to prevent multiple agents. (elastic#18483) Followup to 12606 (elastic#18316) changed input from syslog to tcp/udp due to unsupported RFC (elastic#18447) Improve ECS field mappings in Sysmon module. (elastic#18381) [Elastic Agent] Cleaner output of inspect command (elastic#18405) [Elastic Agent] Pick up version from libbeat (elastic#18350) Update communitybeats.asciidoc (elastic#18470) [Metricbeat] Change visualization interval from 15m to >=15m (elastic#18466) docs: Fix typo in kerberos docs (elastic#18503) ...

- related.hash, related.ip, and related.user are now populated. - hashes are now also populated to the corresponding process.hash, process.pe.imphash, file.hash or file.pe.imphash - file.name, file.directory, and file.extension are now populated. - rule.name is populated for all events when present. Closes #18364 (cherry picked from commit 096b88e)

marc-gr added enhancement review Winlogbeat labels May 8, 2020

marc-gr requested a review from andrewkroh May 8, 2020 14:08

marc-gr requested a review from a team as a code owner May 8, 2020 14:08

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label May 8, 2020

marc-gr added the Team:SIEM label May 8, 2020

marc-gr removed the needs_team Indicates that the issue/PR needs a Team:* label label May 8, 2020

marc-gr force-pushed the winlogbeat-sysmon_ecs_changes branch from 21cfb4f to 92ae20a Compare May 8, 2020 14:23

leehinman previously requested changes May 8, 2020

View reviewed changes

x-pack/winlogbeat/module/sysmon/config/winlogbeat-sysmon.js Outdated Show resolved Hide resolved

x-pack/winlogbeat/module/sysmon/config/winlogbeat-sysmon.js Outdated Show resolved Hide resolved

marc-gr force-pushed the winlogbeat-sysmon_ecs_changes branch from 92ae20a to 61f7f7d Compare May 11, 2020 12:08

marc-gr requested a review from leehinman May 11, 2020 13:59

andrewkroh reviewed May 11, 2020

View reviewed changes

marc-gr force-pushed the winlogbeat-sysmon_ecs_changes branch 2 times, most recently from 5d4f37e to cd5e61a Compare May 12, 2020 07:16

marc-gr requested a review from andrewkroh May 12, 2020 07:19

marc-gr force-pushed the winlogbeat-sysmon_ecs_changes branch from cd5e61a to 5e4bec0 Compare May 13, 2020 06:36

marc-gr requested a review from a team May 13, 2020 06:59

andrewkroh reviewed May 13, 2020

View reviewed changes

marc-gr force-pushed the winlogbeat-sysmon_ecs_changes branch from 5e4bec0 to bcd220c Compare May 14, 2020 08:14

marc-gr requested review from andrewstucki, andrewkroh and webmat May 14, 2020 08:20

marc-gr force-pushed the winlogbeat-sysmon_ecs_changes branch from bcd220c to c5f19d3 Compare May 14, 2020 09:34

andrewkroh reviewed May 14, 2020

View reviewed changes

marc-gr force-pushed the winlogbeat-sysmon_ecs_changes branch from c5f19d3 to eef083f Compare May 14, 2020 11:30

marc-gr requested a review from andrewkroh May 14, 2020 12:21

andrewkroh approved these changes May 14, 2020

View reviewed changes

marc-gr merged commit 096b88e into elastic:master May 14, 2020

marc-gr deleted the winlogbeat-sysmon_ecs_changes branch May 14, 2020 13:03

marc-gr mentioned this pull request May 14, 2020

Cherry-pick #18381 to 7.8: Improve ECS field mappings in Sysmon module. (#18381) #18518

Merged

8 tasks

marc-gr restored the winlogbeat-sysmon_ecs_changes branch May 14, 2020 13:15

marc-gr mentioned this pull request May 14, 2020

Cherry-pick #18381 to 7.x: Improve ECS field mappings in Sysmon module. (#18381) #18519

Merged

8 tasks

marc-gr deleted the winlogbeat-sysmon_ecs_changes branch May 14, 2020 13:30

andrewkroh added the v7.8.0 label Sep 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Winlogbeat] Improve ECS field mappings in Sysmon module. #18381

[Winlogbeat] Improve ECS field mappings in Sysmon module. #18381

marc-gr commented May 8, 2020

elasticmachine commented May 8, 2020

elasticmachine commented May 8, 2020 •

edited

Loading

Build stats

Test stats 🧪

leehinman left a comment

marc-gr commented May 11, 2020

andrewkroh left a comment

andrewkroh May 13, 2020

andrewkroh May 13, 2020

andrewstucki May 13, 2020 •

edited

Loading

andrewstucki May 13, 2020

webmat May 13, 2020

marc-gr May 13, 2020

webmat May 13, 2020 •

edited

Loading

andrewstucki May 13, 2020

andrewkroh May 14, 2020

andrewkroh May 14, 2020 •

edited

Loading

andrewkroh left a comment

	var emptyHashRegex = new RegExp(/^0*$/);
	var emptyHashRegex = /^0*$/;

[Winlogbeat] Improve ECS field mappings in Sysmon module. #18381

[Winlogbeat] Improve ECS field mappings in Sysmon module. #18381

Conversation

marc-gr commented May 8, 2020

What does this PR do?

Why is it important?

Checklist

Author's Checklist

Related issues

elasticmachine commented May 8, 2020

elasticmachine commented May 8, 2020 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

leehinman left a comment

Choose a reason for hiding this comment

marc-gr commented May 11, 2020

andrewkroh left a comment

Choose a reason for hiding this comment

andrewkroh May 13, 2020

Choose a reason for hiding this comment

andrewkroh May 13, 2020

Choose a reason for hiding this comment

andrewstucki May 13, 2020 • edited Loading

Choose a reason for hiding this comment

andrewstucki May 13, 2020

Choose a reason for hiding this comment

webmat May 13, 2020

Choose a reason for hiding this comment

marc-gr May 13, 2020

Choose a reason for hiding this comment

webmat May 13, 2020 • edited Loading

Choose a reason for hiding this comment

andrewstucki May 13, 2020

Choose a reason for hiding this comment

andrewkroh May 14, 2020

Choose a reason for hiding this comment

andrewkroh May 14, 2020 • edited Loading

Choose a reason for hiding this comment

andrewkroh left a comment

Choose a reason for hiding this comment

elasticmachine commented May 8, 2020 •

edited

Loading

andrewstucki May 13, 2020 •

edited

Loading

webmat May 13, 2020 •

edited

Loading

andrewkroh May 14, 2020 •

edited

Loading