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

[WIP][Auditbeat] auditd ECS categorization update #18028

Closed

Conversation

andrewstucki
Copy link
Contributor

What does this PR do?

Putting this up here to get some eyes on it. This is some of the work around initial categorization changes to support ECS 1.5 for the auditd module. Some things to note:

  1. We need to continue shipping invalid ECS categorization fields as part of the array of event.category and event.type values until 8.x so we don't make a breaking change.
  2. It appears like we might have some misclassification of event categories aucoalesce.AuditEventType based off of the mapping code here, and the corresponding header values from the linux kernel/audit userspace headers. Namely: AUDIT_USER_CHAUTHTOK, AUDIT_ROLE_ASSIGN, and AUDIT_ROLE_REMOVE. This could be a bug in the original code that was ported for the go-libaudit library, but I'm wondering if it's worth fixing as a bug on our side.
  3. I tried to expand the aucoalesce.AuditEventType values to explicitly enumerate what auditd types each encompassed and copied the comments from the headers documenting what each value is. This isn't all-inclusive of everything that we'd expect to be picked up, but should be informative for what sort of events we're actually working with for categorization purposes.
  4. I threw some commented proposed categorization fields in the yml file that we can talk about adopting into the ECS spec if we deem them useful. Specifically: config, device, policy, anomaly, and check.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • 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.

Related issues

@andrewstucki andrewstucki requested a review from a team as a code owner April 27, 2020 20:50
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 27, 2020
@andrewstucki andrewstucki added Auditbeat enhancement Team:SIEM and removed needs_team Indicates that the issue/PR needs a Team:* label labels Apr 27, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/siem (Team:SIEM)

@andrewkroh
Copy link
Member

I haven't taken a deep look at this yet, but my initial thought is that maybe this will be easier to maintain over the long run if the data from go-libaudit came with all of this information. This way all the logic and mappings are in one place.

@andrewstucki
Copy link
Contributor Author

@andrewkroh agree that things would be a bit cleaner if we put some of this in go-libaudit--main concern there is whether or not we're introducing too much ECS-specific domain knowledge to what seems to otherwise be a general-purpose library.

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 @MikePaquette and @dainperkins should look at ecs_categorization.yml as well.

I've answered a few questions in the comments, I'm challenging a few categorizations & so on. But this is looking great!

Do you think this will be portable to the Filebeat auditd module as well?

Comment on lines +144 to +145
"event.category": []string{"authentication", "user-login"},
"event.type": []string{"start", "authentication_failure"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these second ones (user-login and auth_failure) marked as deprecated for deletion in 8.0?

Comment on lines +16 to +17
# AUDIT_ACCT_LOCK - User's account locked by admin
# AUDIT_ACCT_UNLOCK - User's account unlocked by admin
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 thinking these two should warrant an event.type of "change", wdyt?

Comment on lines +20 to +21
default_types:
- info
Copy link
Contributor

Choose a reason for hiding this comment

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

Does event.type: info get added to all of these IAM events? Or is this a fallback that's used only when there's no entry under types: below?

# AUDIT_CRED_DISP - User credential disposed
# AUDIT_USER_START - User session start
# AUDIT_USER_END - User session end
# AUDIT_USER_CHAUTHTOK - User acct password or pin changed // should this be classified EventTypeUserAccount?
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be classified EventTypeUserAccount?

I would say yes to that.

Comment on lines +51 to +52
# AUDIT_USER_START - User session start
# AUDIT_USER_END - User session end
Copy link
Contributor

Choose a reason for hiding this comment

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

Been a long time since we worked on this. But I remember we wanted to treat sessions and authorizations differently than authentications. In other words, there's no category published for it yet.

Am I remembering this right, @dainperkins and @MikePaquette?

# AUDIT_CONFIG_CHANGE - Audit system configuration change
# AUDIT_NETFILTER_CFG - Netfilter chain modifications
# AUDIT_FEATURE_CHANGE - audit log listing feature changes
# AUDIT_REPLACE - Replace auditd if this packet unanswerd
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# AUDIT_REPLACE - Replace auditd if this packet unanswerd
# AUDIT_REPLACE - Replace auditd if this packet unanswered

# AUDIT_FEATURE_CHANGE - audit log listing feature changes
# AUDIT_REPLACE - Replace auditd if this packet unanswerd
categories:
# - config
Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine not to assign a category for now, until we define a more appropriate one 👍

categories:
# - policy

EventTypeAnomoly:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
EventTypeAnomoly:
EventTypeAnomaly:

Copy link
Contributor

Choose a reason for hiding this comment

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

Some of the anomalies below sound like they could be associated with existing categories, such as "iam", "file", "process", "authentication" and so on.

Then if we later add an "anomaly" category, these events could also be assigned that category.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that was my thought too--some of them are policy violations (like exceeding some threshold of failed logins), some are device access/modifications, and some are restricted process spawning, file or iam creation. So I'd be happy with categorizing them there.

Wondering though if it makes sense to add a secondary category in these cases--they're only being signaled because auditd has flagged them as "anomalous" - so they aren't your standard file/creation events, etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, just re-read your comment, you were thinking the exact same thing as me :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but it still brings a good point.

If this is a second "anomaly" event, will this mean this anomalous IAM action yield two auditd events? One for the regular IAM event, and this second one for the anomaly?

But I'd err on the side of capturing both as IAM, and then we / the user can dedupe if that's the case.

Moreover, if a user turns off "normal" IAM events to reduce noise, this IAM anomaly should be categorized as IAM.

- process
types:
- end
# are these mis-classified?
Copy link
Contributor

Choose a reason for hiding this comment

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

are these mis-classified?

I think the whole list of associations in this messageTypes structure looks good

MessageTypes map[string]messageType `yaml:"messageTypes"`
}

const fileTemplate = `
Copy link
Contributor

Choose a reason for hiding this comment

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

Meta-programming in Golang 🤯 😂

@andrewstucki
Copy link
Contributor Author

@webmat thanks for the reviews/ideas, going to close this in favor of adding this stuff to go-libaudit for the next release as I discussed offline with @andrewkroh but I'll take the comments over there, reference back here and ping you when the work is done on that side.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants