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

ECS normalization #62

Merged
merged 11 commits into from May 15, 2020
Merged

Conversation

andrewstucki
Copy link
Contributor

@andrewstucki andrewstucki commented May 13, 2020

So, unfortunately, the normalization file is pretty hard to review due to moving things around for the sake of ergonomics (i.e. ordering by similarly categorized events rather than alphabetically). Aside from the reorg and addition of ECS categorization fields--there is:

  1. a bug fix with the way we were unmarshaling yaml that was breaking yaml anchor node behavior.
  2. extending some of the normalization code, especially for compound events so that we have the associated syscall normalization information RE ECS categorization.
  3. removing the following normalization rule, which AFAIK doesn't actually do anything due to us stripping the AUDIT_ prefixes in our GetAuditMessageType code in the auparse subpackage.
  object:
    what: mac-config
  record_types:
  - AUDIT_DEV_ALLOC
  - AUDIT_DEV_DEALLOC
  - AUDIT_FS_RELABEL
  - AUDIT_USER_MAC_POLICY_LOAD
  - AUDIT_USER_MAC_CONFIG_CHANGE

@andrewstucki andrewstucki marked this pull request as draft May 13, 2020 20:36
@@ -52,7 +52,7 @@ type Strings struct {
func (s *Strings) UnmarshalYAML(unmarshal func(interface{}) error) error {
var singleValue string
if err := unmarshal(&singleValue); err == nil {
s.Values = append(s.Values, singleValue)
s.Values = []string{singleValue}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was causing bad anchor merge behavior. Basically, before this change, array sub-elements of an anchor node would get merged rather than overwritten when used, so:

macros:
  - &ecs-file
    category: file
    type: info

normalizations:
  - action: test
    ecs:
      <<: *ecs-file
      type: test

Would previously result in:

{
  ...
  "normalizations": [
    "action": "test",
    "ecs": {
      "category": ["file"],
      "type": ["info", "test"]
    }
  ]
}

rather than:

{
  ...
  "normalizations": [
    "action": "test",
    "ecs": {
      "category": ["file"],
      "type": ["test"]
    }
  ]
}

which is the intended (and to spec) result.

event.ECS.Event.Category = append(event.ECS.Event.Category, syscallNorm.ECS.Category.Values...)
event.ECS.Event.Type = append(event.ECS.Event.Type, syscallNorm.ECS.Type.Values...)
if event.Result == "fail" {
event.ECS.Event.Outcome = "failure"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're only explicit if something was a failure otherwise, unknown/success are implied based off of the context of the category/type

@andrewstucki andrewstucki marked this pull request as ready for review May 14, 2020 15:07
# AUDIT_ANOM_CRYPTO_FAIL - Crypto system test failure
# AUDIT_ANOM_MK_EXE - Make an executable
# AUDIT_ANOM_ACCESS_FS - Access of file or dir
# AUDIT_ANOM_ADD_ACCT - Adding an acct
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, we still have a bunch of ECS normalization we can add for some of these audit types, but I figured I could do that in a subsequent PR since this is already a ton of changes

- USER_TTY
- ecs: *ecs-process
syscalls:
- '*' # this is a catch all
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thought is that only a process can actually make syscalls anyway--so if we have a syscall where we haven't normalized it to a behavior (i.e. create a file)--then just say it's "process info" with ECS: event.category: process, event.type: info

@andrewstucki
Copy link
Contributor Author

I know that the yaml manifest is pretty hard to review--but would be appreciative if you took a look @webmat -- it's not as straightforward as last round in beats itself--but it should implement all the stuff we talked about in your former review + some.

Comment on lines 106 to 119
- action: read-file
object:
what: file
syscalls:
- read
ecs: *ecs-file
- action: wrote-to-file
object:
what: file
syscalls:
- write
ecs:
<<: *ecs-file
type: change
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two syscall normalizations were added

Copy link
Contributor

@leehinman leehinman left a comment

Choose a reason for hiding this comment

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

Nice.

couple of non-blocking questions

@@ -34,6 +34,13 @@ import (
// package does not have a constant defined for this.
const modeBlockDevice = 060000

// ECSEvent contains ECS-specific categorization fields
type ECSEvent struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Non blocking
Would event.kind be held somewhere else? or maybe the caller of the library is responsible for that?

What do you think about having something about ECS categorization in the struct type? That might provide clues to the type of fields in the struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my initial thought is that it could be in the caller--namely because I'm pretty sure we're working only with event kinds--but then again would "anomaly" events actually be alert kinds? If so, or if we just want to be explicit, I can add kind in a follow-up PR.

For the second question, I'm not entirely sure I follow--do you meaning adding constant enums for the categorization values themselves? If so I'd almost prefer to do that in the ECS go generated code from the ECS repo itself (which ideally we'd be using here too--but we still need to add tags and formalize on things like omitempty, pointer usage, etc. before doing that).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with it being the callers responsibility, I just wanted to double check that we weren't missing one of the ECS categorization fields.

I was thinking the name of the type might be better as ECSCategorization or ECSCat or something like that, since it really just holds the categorization info, nothing else about ECS.

Enums would be awesome and I agree it would be good to do that work once in ECS repo.

event.ECS.Event.Category = append(event.ECS.Event.Category, syscallNorm.ECS.Category.Values...)
event.ECS.Event.Type = append(event.ECS.Event.Type, syscallNorm.ECS.Type.Values...)
if event.Result == "fail" {
event.ECS.Event.Outcome = "failure"
Copy link
Contributor

Choose a reason for hiding this comment

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

non blocking

because "failure" is one of 3 constant values, it might be nice to use a constant for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ideally this would also be in an enum in the ECS generated go structures

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Wow I forgot just how many different events there can be. Nice work.

  • Add a changelog.
  • Drop a line into the README saying this provides Elastic Common Schema (ECS) 1.5 categorization (with link to docs) for the most prominent events (my vague way of saying not all events will be covered).
  • Update the example events on the README if you can (not sure if where those raw events came from).

aucoalesce/coalesce.go Show resolved Hide resolved
aucoalesce/normalizations.yaml Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants