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

Add DLL fieldset #679

Merged
merged 18 commits into from Feb 12, 2020
Merged

Add DLL fieldset #679

merged 18 commits into from Feb 12, 2020

Conversation

rw-access
Copy link
Contributor

@rw-access rw-access commented Dec 5, 2019

Closes #675

Added a field set for code libraries/modules, which could be used for DLL loads into a process, but also for driver loads into the kernel if we go that route. I think those should go in a separate field set.

  • dll.* chosen as the name, but I'm willing to change it for anything better we have in mind
  • dll.name
  • dll.process

PE fields (#676) can also be nested here, as well as hash.*. That'll likely happen in a PR that solves PE, or in this one, depending on which is done first.

@rw-access rw-access requested a review from webmat December 5, 2019 18:10
@rw-access rw-access self-assigned this Dec 6, 2019
Copy link
Contributor

@andrewstucki andrewstucki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least changes on removing the comment accidentally copied from schemas/process.yml into the new definition. The other stuff is more for discussion.

schemas/lib.yml Outdated Show resolved Hide resolved
schemas/lib.yml Outdated Show resolved Hide resolved
schemas/lib.yml Outdated Show resolved Hide resolved
@andrewstucki andrewstucki dismissed their stale review December 6, 2019 20:41

No more bad copy :)

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think APM should chime in on this as well. @simitt or @graphaelli do you think there would be a need for library.* as well in APM? Would the semantics proposed here be ok?

Note that we can keep building on this, missing fields wouldn't be a problem.

schemas/lib.yml Outdated Show resolved Hide resolved
schemas/lib.yml Outdated Show resolved Hide resolved
schemas/lib.yml Outdated Show resolved Hide resolved
@webmat
Copy link
Contributor

webmat commented Dec 9, 2019

Another thing to point out here is that typically we break down the file paths that may be suspicious to help with detections, and we have a single field with the full file path whenever the path is more informational and not necessarily suspicious.

With that in mind, do you think we should instead nest file at library.file.*? We can also consider this later. Adding library.file.* besides library.path is not a breaking change. We could deprecate the former and take it out in ECS 2.0

@rw-access rw-access changed the title Add library/module fieldset Add dll/library/module fieldset Dec 11, 2019
@rw-access rw-access changed the title Add dll/library/module fieldset Add dll/image/library/module fieldset Dec 11, 2019
@rw-access rw-access changed the title Add dll/image/library/module fieldset Add dll fieldset Jan 23, 2020
@rw-access rw-access requested a review from webmat January 23, 2020 19:48
@rw-access rw-access changed the title Add dll fieldset Add DLL fieldset Jan 23, 2020
@rw-access
Copy link
Contributor Author

I renamed to DLL to maintain clarity and avoid scope creep.

@rw-access rw-access added the endpoint Relevant to elastic endpoint security label Jan 24, 2020
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I'm good with the approach here, I have a few things I'd like to address first, though. See the comments.

Thanks Ross!

CHANGELOG.next.md Outdated Show resolved Hide resolved
schemas/dll.yml Outdated Show resolved Hide resolved
schemas/dll.yml Outdated Show resolved Hide resolved
schemas/dll.yml Outdated Show resolved Hide resolved
docs/field-details.asciidoc Outdated Show resolved Hide resolved
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these adjustments. I have one further tweak I'd like to discuss, in the phrasing of the field set description, as well as one nitpick.

Don't forget to run make to re-generate everything, on the last commit.

We can merge after this. It's looking good :-)

schemas/dll.yml Outdated Show resolved Hide resolved
schemas/dll.yml Outdated Show resolved Hide resolved
schemas/hash.yml Show resolved Hide resolved
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the adjustments!

@webmat webmat merged commit 0e4cdd3 into elastic:master Feb 12, 2020
@rw-access rw-access deleted the lib-fields branch February 12, 2020 21:01
dcode pushed a commit to dcode/ecs that referenced this pull request Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
endpoint Relevant to elastic endpoint security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Driver and DLL fields
3 participants