-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
auditbeat/module/file_integrity: add support for selinux and posix_acl_access xattrs #36310
Conversation
99560ee
to
9182000
Compare
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested that changing both selinux and acl attributes triggers an inotify CHMOD event.
This is missing a modification to diffEvents()
to detect if these new attributes changed.
beats/auditbeat/module/file_integrity/event.go
Lines 406 to 412 in 2797a04
// Test if metadata has changed. | |
if o, n := old.Info, new.Info; o != nil && n != nil { | |
// The owner and group names are ignored (they aren't persisted). | |
if o.Inode != n.Inode || o.UID != n.UID || o.GID != n.GID || o.SID != n.SID || | |
o.Mode != n.Mode || o.Type != n.Type || o.SetUID != n.SetUID || o.SetGID != n.SetGID { | |
result |= AttributesModified | |
} |
And it needs a modification in buildMetricbeatEvent()
to expose the new fields in events.
func buildMetricbeatEvent(e *Event, existedBefore bool) mb.Event { |
@@ -67,6 +68,11 @@ func NewMetadata(path string, info os.FileInfo) (*Metadata, error) { | |||
fileInfo.Owner = owner.Username | |||
} | |||
|
|||
getExtendedAttributes(path, map[string]*string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this is linux-only then can we guard this with a if runtime.GOOS == "linux"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The system.posix_acl_access
should in principle be available on other OSs.
56bfe12
to
f7ca8a7
Compare
} | ||
} | ||
return out | ||
} | ||
|
||
func aclText(s string) ([]string, error) { | ||
b, err := base64.StdEncoding.DecodeString(strings.TrimPrefix(s, "0s")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value coming back from FGet
is actually the raw binary data. It is not a base64 encoded value. The 0s
base64 must just be the representation used with getfattr
when the value binary. I think this also means the system.posix_acl_access
value should not be trimNull
ed.
I hacked in changes and re-tested, and I observed what I expected in the output event with 👍
"posix_acl_access": [
"user::rw-",
"user:landscape:rwx",
"group::r--",
"mask::rwx",
"other::r--"
],
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've kept the POSIXACLAccess
field as a string
. It sits right on the boundary of making a decision to change it to a []byte
, but I think it's just slightly better as string
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only impact of this decision to users will be the debug message that shows the old and new metadata. How does go encode a []byte
? IIRC it uses base64 which IMO would be slightly nicer in the logs.
{"log.level":"debug","@timestamp":"2023-08-16T10:30:29.388-0400","log.logger":"file_integrity","log.origin":{"file.name":"file_integrity/metricset.go","file.line":296},"message":"File changed since it was last seen","service.name":"auditbeat","file_path":"/foo.bar","took":280205,"event":{"action":"attributes_modified","old":{"timestamp":"2023-08-16T14:30:00.686192244Z","path":"/foo.bar","info":{"inode":71102,"uid":0,"gid":0,"sid":"","owner":"","group":"","size":0,"mtime":"2023-08-14T20:49:03.249161413Z","ctime":"2023-08-15T17:49:52.337561405Z","type":"file","mode":444,"setuid":false,"setgid":false,"origin":null,"selinux":"system_u:object_r:quota_db_t:s0","posix_acl_access":"\u0002\u0000\u0000\u0000\u0001\u0000\u0006\u0000\ufffd\ufffd\ufffd\ufffd\u0002\u0000\u0007\u0000o\u0000\u0000\u0000\u0004\u0000\u0004\u0000\ufffd\ufffd\ufffd\ufffd\u0010\u0000\u0007\u0000\ufffd\ufffd\ufffd\ufffd \u0000\u0004\u0000\ufffd\ufffd\ufffd\ufffd"},"source":"scan","action":"none","hash":{"sha1":"da39a3ee5e6b4b0d3255bfef95601890afd80709"}},"new":{"timestamp":"2023-08-16T14:30:29.388513038Z","path":"/foo.bar","info":{"inode":71102,"uid":0,"gid":0,"sid":"","owner":"root","group":"root","size":0,"mtime":"2023-08-14T20:49:03.249161413Z","ctime":"2023-08-16T14:30:29.384200342Z","type":"file","mode":444,"setuid":false,"setgid":false,"origin":null,"selinux":"system_u:object_r:quota_db_t:s0","posix_acl_access":"\u0002\u0000\u0000\u0000\u0001\u0000\u0006\u0000\ufffd\ufffd\ufffd\ufffd\u0002\u0000\u0003\u0000o\u0000\u0000\u0000\u0004\u0000\u0004\u0000\ufffd\ufffd\ufffd\ufffd\u0010\u0000\u0007\u0000\ufffd\ufffd\ufffd\ufffd \u0000\u0004\u0000\ufffd\ufffd\ufffd\ufffd"},"source":"fsnotify","action":"attributes_modified","hash":{"sha1":"da39a3ee5e6b4b0d3255bfef95601890afd80709"}},"ecs.version":"1.6.0"}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a MarshalText method to the POSIXACLAccess
field that renders it in the standard format. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that's not very different from keeping it as a []byte
. I think one of these should be done because note the \ufffd
. These are not in the original data; they are the unicode replacement rune because 0xffff is not a valid code point.
run elasticsearch-ci/docs |
…l_access xattrs (#36310) (#36350) (cherry picked from commit d771501) Co-authored-by: Dan Kortschak <[email protected]>
Proposed commit message
In environments where SELinux is employed then it is useful to monitor file metadata for changes to SELinux labels. A change to labeling can impact security posture. Similarly in environments where file ACLs are used (e.g. getfacl, setfacl) it is useful to monitor for changes to these ACLs (just like it is useful to monitor permissions in the file mode). So add support for the filesystem extended attributes (xattrs) named security.selinux and system.posix_acl_access that detail these.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs
Sample event: