[Auditbeat] Determine event.action based on diff against state #22170

andrewkroh · 2020-10-27T00:45:55Z

What does this PR do?

Rather than using an event.action that is derived from the OS flags provided in the
file notification event the FIM module will determine the event.action based on the
diff between the stored state for the path and the new state.

Why is it important?

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.

elasticmachine · 2020-10-27T00:56:48Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #22170 updated
- Start Time: 2021-02-01T14:30:18.496+0000
Duration: 30 min 5 sec
Commit: 0542651

Test stats 🧪

Test	Results
Failed	0
Passed	978
Skipped	222
Total	1200

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	978
Skipped	222
Total	1200

andrewkroh · 2020-10-27T01:52:59Z

The initial attempt with

	if event.Action != InitialScan {
		event.Action = action
	}

doesn't work because you lose actions for renames and deletes (they just appear as attribute_modified.

So perhaps we should only use the diff'ed action for updates.

andrewkroh · 2020-11-05T02:36:32Z

Relates: #17347

adriansr · 2020-11-27T00:47:13Z

auditbeat/module/file_integrity/fileinfo_windows.go

+	if err != nil {
+		return sid, owner, errors.Wrapf(err, "failed to convert path:'%s' to UTF16", path)
+	}
+	if err = GetNamedSecurityInfo(pathW, FileObject,


The problem here was:

Some applications will open files for writing in exclusive mode (non FILE_READ_SHARED).

ReadOpen above wants read access, so it fails with permission denied.

As GetSecurityInfo doesn't need read access to the file, but only the metadata (READ_CONTROL), I tried opening the file with just that flag. This also fails because the underlying CreateFile will internally add more permissions to the request (SYNCHRONIZE|FILE_READ_ATTRIBUTES) which causes it to still fail for files open with exclusivity.

Finally I came across this SO thread: https://stackoverflow.com/questions/58393139/createfilew-for-read-control-fails-with-access-is-denied-despite-being-owner-o that points to using GetNamedSecurityInfo, which internally does an NtOpenFile call with the minimum required permissions.

adriansr · 2020-11-27T00:52:48Z

jenkins, test this

adriansr · 2020-12-01T08:06:16Z

jenkins, test this

elasticmachine · 2020-12-21T16:22:53Z

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

andrewkroh · 2020-12-23T02:45:18Z

With the current changes I am seeing attributes_modified when only mtime changed. 👍 @adriansr

One thing I noticed is that I get a sequence of created->deleted->created for new files. Is that explained by some OS behavior? I saw it with both a text editor saving a file and a firefox download on Windows.

auditbeat-fim-create-delete-create.json.txt

adriansr · 2020-12-29T15:44:07Z

@andrewkroh I've tested under Windows server 2016 and didn't get the created->deleted->created sequence.

This is saving a file with notepad:

Also got similar result with notepad++, but lost the screen capture.

Download a file with iexplore:

Download a file with Firefox:

My understanding is that Windows server 2016 and Windows 10 should behave similarly? Maybe differences in the underlying filesystem?

andrewkroh · 2020-12-30T02:15:21Z

Your test results look good. I was testing on GCP. The machine is still running if you wanted to look at it.

adriansr · 2020-12-30T12:17:51Z

Seems like a different behavior under Windows server 2019:

This sequence is indeed what we're getting from fsnotify:

2020-12-30T10:48:45.254Z INFO instance/beat.go:454 auditbeat start running.
2020-12-30T10:48:45.254Z INFO [monitoring] log/log.go:118 Starting metrics logging every 30s
2020-12-30T10:48:51.583Z INFO [file_integrity] file_integrity/eventreader_fsnotify.go:97 Started fsnotify watcher {"file_path": ["C:\Users\adrian_serrano\Documents"], "recursive": true}
2020-12-30T10:49:05.625Z DEBUG [file_integrity] file_integrity/eventreader_fsnotify.go:138 Received fsnotify event {"file_path": "C:\Users\adrian_serrano\Documents\doc.txt", "event_flags": "CREATE"}
2020-12-30T10:49:05.628Z DEBUG [file_integrity] file_integrity/eventreader_fsnotify.go:138 Received fsnotify event {"file_path": "C:\Users\adrian_serrano\Documents\doc.txt", "event_flags": "REMOVE"}
2020-12-30T10:49:05.631Z DEBUG [file_integrity] file_integrity/metricset.go:281 File changed since it was last seen {"file_path": "C:\Users\adrian_serrano\Documents\doc.txt", "took": 0, "event": {"action": "created", "old": null, "new": {"timestamp":"2020-12-30T10:49:05.6280538Z","path":"C:\Users\adrian_serrano\Documents\doc.txt","info":null,"source":"fsnotify","action":"created"}}}
2020-12-30T10:49:05.734Z DEBUG [file_integrity] file_integrity/eventreader_fsnotify.go:138 Received fsnotify event {"file_path": "C:\Users\adrian_serrano\Documents\doc.txt", "event_flags": "CREATE"}
2020-12-30T10:49:05.740Z DEBUG [file_integrity] file_integrity/eventreader_fsnotify.go:138 Received fsnotify event {"file_path": "C:\Users\adrian_serrano\Documents\doc.txt", "event_flags": "WRITE"}
2020-12-30T10:49:05.740Z DEBUG [file_integrity] file_integrity/metricset.go:281 File changed since it was last seen {"file_path": "C:\Users\adrian_serrano\Documents\doc.txt", "took": 0, "event": {"action": "deleted", "old": null, "new": {"timestamp":"2020-12-30T10:49:05.6319632Z","path":"C:\Users\adrian_serrano\Documents\doc.txt","info":null,"source":"fsnotify","action":"deleted"}}}
2020-12-30T10:49:05.745Z DEBUG [file_integrity] file_integrity/metricset.go:281 File changed since it was last seen {"file_path": "C:\Users\adrian_serrano\Documents\doc.txt", "took": 3906500, "event": {"action": "created", "old": null, "new": {"timestamp":"2020-12-30T10:49:05.7354821Z","path":"C:\Users\adrian_serrano\Documents\doc.txt","info":{"inode":3096224743861787,"uid":0,"gid":0,"sid":"S-1-5-21-3277996792-4287217755-2489893845-1001","owner":"AKROH-WINDOWS\adrian_serrano","group":"","size":12,"mtime":"2020-12-30T10:49:05.7101082Z","ctime":"2020-12-30T10:49:05.6251136Z","type":"file","mode":438,"setuid":false,"setgid":false,"origin":null},"source":"fsnotify","action":"created","hash":{"sha1":"2ef7bde608ce5404e97d5f042f95f89f1c232871"}}}}

Looking at it via Process Monitor shows that when saving a file for the first time (Save...), it is first created and deleted, then created and written to:

When saved using the Save option (no file-selection dialog) it results in just an update event:

I believe the first create-delete sequence is the file-selection dialog verifying that the target file can be created before the dialog is closed.

For the Firefox issue however, I couldn't reproduce the created->deleted->created, but I got something that I believe is wrong:

It's missing a final event for the target file (moved/created/updated?). I'm looking into it.

adriansr · 2021-01-13T19:51:19Z

jenkins test this please

Rather than using an event.action that is derived from the OS flags provided in the file notification event the FIM module will determine the event.action based on the diff between the stored state for the path and the new state.

Update the file-state tracking to support the case where we receive a DELETE event but we observe the file as already re-created (event.Info is not nil and hashes are likely populated). Before this change, we would report a deletion but at the same store the hashes and file info. Then, a following CREATION event would be ignored because the diff-ing loggic doesn't take the previous action into account (in this case prev.action==Deleted). The best is to ignore the deletion and report on the observed file changes (if any). Otherwise we have to deal with complex logic in the cases where the OS event includes multiple actions ( ...|DELETED|...).

andrewkroh · 2021-02-01T13:50:02Z

I tested out the a build of this on Windows 2019 (same one in GCP). The results LGTM🥇

Thank you @adriansr!

Saving a file with no changes:

Downloading a file with FF:

adriansr · 2021-02-01T14:22:43Z

Will add a changelog entry

…ic#22170) Rather than using an event.action that is derived from the OS flags provided in the file notification event the FIM module will determine the event.action based on the diff between the stored state for the path and the new state. ---- Update the file-state tracking to support the case where we receive a DELETE event but we observe the file as already re-created (event.Info is not nil and hashes are likely populated). Before this change, we would report a deletion but at the same store the hashes and file info. Then, a following CREATION event would be ignored because the diff-ing logic doesn't take the previous action into account (in this case prev.action==Deleted). The best is to ignore the deletion and report on the observed file changes (if any). Otherwise we have to deal with complex logic in the cases where the OS event includes multiple actions ( ...|DELETED|...). Co-authored-by: Adrian Serrano <adrisr83@gmail.com> (cherry picked from commit 4a44fac)

… (#23788) Rather than using an event.action that is derived from the OS flags provided in the file notification event the FIM module will determine the event.action based on the diff between the stored state for the path and the new state. ---- Update the file-state tracking to support the case where we receive a DELETE event but we observe the file as already re-created (event.Info is not nil and hashes are likely populated). Before this change, we would report a deletion but at the same store the hashes and file info. Then, a following CREATION event would be ignored because the diff-ing logic doesn't take the previous action into account (in this case prev.action==Deleted). The best is to ignore the deletion and report on the observed file changes (if any). Otherwise we have to deal with complex logic in the cases where the OS event includes multiple actions ( ...|DELETED|...). Co-authored-by: Adrian Serrano <adrisr83@gmail.com> (cherry picked from commit 4a44fac) Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>

andrewkroh added Auditbeat Team:Security-External Integrations labels Oct 27, 2020

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Oct 27, 2020

andrewkroh force-pushed the bugfix/ab/use-computed-diff-as-action branch from f783ba4 to ee60d0d Compare October 27, 2020 02:06

adriansr reviewed Nov 27, 2020

View reviewed changes

andrewkroh marked this pull request as ready for review December 21, 2020 16:22

andrewkroh requested a review from a team as a code owner December 21, 2020 16:22

adriansr self-assigned this Dec 28, 2020

andrewkroh and others added 9 commits January 13, 2021 21:33

Determine event.action based on diff against state

f44c186

Rather than using an event.action that is derived from the OS flags provided in the file notification event the FIM module will determine the event.action based on the diff between the stored state for the path and the new state.

Update diff based action for updates only

eb23cc3

Fetch file owner for non-readable files

0105770

Report event errors in error.message

a527e32

Act on failed hashing (file in use)

2f56b5a

Detect when files change size during hashing

df271c2

Don't miss non-Update actions from OS

575cccc

Minor correction

73de601

adriansr force-pushed the bugfix/ab/use-computed-diff-as-action branch from b0effb9 to b30f19a Compare January 13, 2021 20:35

Skip hashing error test under root

908cd4c

andrewkroh added the needs_backport PR is waiting to be backported to other branches. label Feb 1, 2021

adriansr approved these changes Feb 1, 2021

View reviewed changes

Changelog entry

0542651

adriansr merged commit 4a44fac into elastic:master Feb 1, 2021

adriansr mentioned this pull request Feb 1, 2021

Cherry-pick #22170 to 7.x: [Auditbeat] Determine event.action based on diff against state #23788

Merged

6 tasks

adriansr added v7.12.0 and removed needs_backport PR is waiting to be backported to other branches. labels Feb 1, 2021

mdelapenya mentioned this pull request Feb 3, 2021

[Elastic Agent] Monitoring filebeat and metricbeat not connecting to Agent over GRPC #23833

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Auditbeat] Determine event.action based on diff against state #22170

[Auditbeat] Determine event.action based on diff against state #22170

andrewkroh commented Oct 27, 2020

elasticmachine commented Oct 27, 2020 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

Test stats 🧪

andrewkroh commented Oct 27, 2020

andrewkroh commented Nov 5, 2020

adriansr Nov 27, 2020

adriansr commented Nov 27, 2020

adriansr commented Dec 1, 2020

elasticmachine commented Dec 21, 2020

andrewkroh commented Dec 23, 2020

adriansr commented Dec 29, 2020 •

edited

Loading

andrewkroh commented Dec 30, 2020

adriansr commented Dec 30, 2020

adriansr commented Jan 13, 2021

andrewkroh commented Feb 1, 2021

adriansr commented Feb 1, 2021

[Auditbeat] Determine event.action based on diff against state #22170

[Auditbeat] Determine event.action based on diff against state #22170

Conversation

andrewkroh commented Oct 27, 2020

What does this PR do?

Why is it important?

Checklist

elasticmachine commented Oct 27, 2020 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

Test stats 🧪

andrewkroh commented Oct 27, 2020

andrewkroh commented Nov 5, 2020

adriansr Nov 27, 2020

Choose a reason for hiding this comment

adriansr commented Nov 27, 2020

adriansr commented Dec 1, 2020

elasticmachine commented Dec 21, 2020

andrewkroh commented Dec 23, 2020

adriansr commented Dec 29, 2020 • edited Loading

andrewkroh commented Dec 30, 2020

adriansr commented Dec 30, 2020

adriansr commented Jan 13, 2021

andrewkroh commented Feb 1, 2021

adriansr commented Feb 1, 2021

elasticmachine commented Oct 27, 2020 •

edited by jenkins-beats-ci bot

Loading

adriansr commented Dec 29, 2020 •

edited

Loading