Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upauditd-based file integrity monitoring #3492
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
@alessandrogario updated the pull request - view changes |
|
@alessandrogario updated the pull request - view changes |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
@alessandrogario updated the pull request - view changes |
|
@alessandrogario updated the pull request - view changes |
|
@alessandrogario updated the pull request - view changes |
|
@alessandrogario updated the pull request - view changes |
|
@alessandrogario updated the pull request - view changes |
alessandrogario
changed the title
|
@alessandrogario updated the pull request - view changes |
|
@alessandrogario updated the pull request - view changes |
|
I forgot to introduce some low-level syscalls (create/mknod/mknodat). They are now supported (the changes are smalls, as they are all handled the same way). |
|
ok to test |
|
The way I'm going to test this will be a little disorganized. Trying to move fast and include nitpick/style/approach/direction questions together, just want to apologize before hand. :P I've created a squashed version of all the commits and will be using that to track changes locally as I learn the new implementation and tweak/performance-monitor on my test hosts. |
|
How does the publisher shutdown? Seems like I need to |
|
I can't get the I used to be able to do: From reading the code I'm guessing you now need: But that's not working either? |
|
So if I try to initialize: Are there two audit-related publishers? Ideally there should be just one publisher that publishes to multiple subscribers. |
|
First past comments, nothing crazy. Some of my previous thread comments are more high-pri. |
| @@ -12,14 +12,21 @@ | |||
|
|
|||
| #include <libaudit.h> | |||
|
|
|||
| #include <atomic> | |||
theopolis
Aug 3, 2017
Member
If we aren't using the includes in the header's definitions, then let's move them to the implementation file.
If we aren't using the includes in the header's definitions, then let's move them to the implementation file.
| #include <vector> | ||
|
|
||
| #include <boost/algorithm/hex.hpp> | ||
| #include <boost/scoped_ptr.hpp> |
theopolis
Aug 3, 2017
Member
Same goes here, and all of the newly introduced C++ includes here.
Same goes here, and all of the newly introduced C++ includes here.
| @@ -0,0 +1,63 @@ | |||
| /* | |||
theopolis
Aug 3, 2017
Member
Could this entire file's function be implemented within the existing file_paths parser? If that name is not appropriate, let's find a way to squash both into 1 implementation file. The design of parsers allows for multiple top-level keys if that is not working as intended, let me know.
Could this entire file's function be implemented within the existing file_paths parser? If that name is not appropriate, let's find a way to squash both into 1 implementation file. The design of parsers allows for multiple top-level keys if that is not working as intended, let me know.
| }; | ||
|
|
||
| // | ||
| // Populated from the AUDIT_SYSCALL event record |
theopolis
Aug 3, 2017
Member
If you need multi-line comments use the long-form docblock:
/**
* @brief Here is a summary.
*
* Some more stuff yay!
*/
Only use the /// short-form for documenting a member or method that only needs a 1-liner.
If you need multi-line comments use the long-form docblock:
/**
* @brief Here is a summary.
*
* Some more stuff yay!
*/
Only use the /// short-form for documenting a member or method that only needs a 1-liner.
| @@ -0,0 +1,129 @@ | |||
| #pragma once | |||
theopolis
Aug 3, 2017
Member
nit, this will need a copywrite comment header.
nit, this will need a copywrite comment header.
alessandrogario
Aug 4, 2017
Author
Contributor
Added!
Added!
| * | ||
| */ | ||
|
|
||
| #include "osquery/tables/events/linux/auditd_fim_events.h" |
theopolis
Aug 3, 2017
Member
nit, ordering
nit, ordering
| translated_cwd = cwd; | ||
| } | ||
|
|
||
| translated_path = translated_cwd + "/" + translated_path; |
theopolis
Aug 3, 2017
Member
Does operator+ take a '/' char?
Does operator+ take a '/' char?
| // spawn other events). It is also possible that by the time we | ||
| // inspect the files on disk the state that caused this event has | ||
| // changed. | ||
| namespace boostfs = boost::filesystem; |
theopolis
Aug 3, 2017
Member
Avoid aliasing within code, prefer at the top, where we commonly use fs = boost::filesystem.
Avoid aliasing within code, prefer at the top, where we commonly use fs = boost::filesystem.
| emitted_row_list.clear(); | ||
|
|
||
| // Process the syscall events we received and emit the necessary rows | ||
| for (const SyscallEvent& syscall : syscall_event_list) { |
theopolis
Aug 3, 2017
Member
It seems like syscall could be shadowing method names in rare cases, mind choosing another name?
It seems like syscall could be shadowing method names in rare cases, mind choosing another name?
| for (const SyscallEvent& syscall : syscall_event_list) { | ||
| auto syscall_type = syscall.type; | ||
|
|
||
| switch (syscall_type) { |
theopolis
Aug 3, 2017
Member
This loop and switch are far too complex, consider moving some of the content into methods.
This loop and switch are far too complex, consider moving some of the content into methods.
|
The process_events table is working fine for me using the following flags: The reason that it may not work for you is that I have added a new flag to control whether it is enabled or not the same way socket_events works. The default setting is disabled. EDIT: I have also successfully tried with |
|
There is one AuditNetlink class fetching the events from the audit netlink and two publishers. The AuditNetlink class is generic and supports more than one consumer. The first publisher (audit.cpp, used by socket_events and process_events) was not suitable for my use case for the following reasons:
In order to avoid refactoring all the existing tables, a second publisher has been implemented. The AuditdFim component is a lot simpler, as it doesn't need to specifically keep track of which records the subscriber expects to receive (the syscall number is enough) because it makes use of the AUDIT_EOE terminator. This should also be faster than completing them manually. |
|
@alessandrogario updated the pull request - view changes |
|
@alessandrogario updated the pull request - view changes |
|
@alessandrogario updated the pull request - view changes |
|
@alessandrogario updated the pull request - view changes |
|
@alessandrogario updated the pull request - view changes |
|
@alessandrogario updated the pull request - view changes |
I have implemented file integrity monitoring using the auditd service. This can come in handy as inotify can only subscribe for a limited amount of entities, and can’t be easily changed at runtime by osquery without risking invalidation of the existing watchers.
Using auditd for file integrity monitoring offers the following advantages:
In general, our implementation can be easily expanded in the future to support more event publishers, since the audit netlink can now be shared. This should also empower subscribers, allowing them to use a more lightweight filtering logic suitable for the task they needs to perform. In order to make auditd suitable for file integrity monitoring, we overcame several performance issues when receiving and processing events from the audit netlink. Additionally, subscribers are no longer blocking each other and/or the netlink reader when processing the event records.
How auditd-based file integrity monitoring works
The new implementation works by tracking process/handle creation and destruction by monitoring the execve/exit/exit_group and open/close/dup syscalls.
The file descriptors are saved inside a handle table (one for each process) in the auditd_file_events subscriber. The tables are then used to solve the file descriptors when the following system calls are executed: read/write/mmap.
Rows are only inserted inside the table when a file changes state. Valid state changes are: Open -> Read -> Write -> Close.
Configuration is not yet supported but I plan on adding it, in a way similar to the existing file_events table.
Issues found and fixed in the original code
While I was separating the netlink reader and the AuditAssembler logic, I encountered and fixed the following issues:
[d075b79] Audit event IDs were not correctly handled; according to the documentation (see the commit message for more information) the primary key is made by both the event time and the event id. This caused the class to receive events that perceived as duplicates, tossing them away using an internal history of the last received events.
[ec57b07] Fixed syscall filtering. This caused subscribers to receive events for syscalls they did not subscribe for.
Modified or added components:
AuditNetlink (new)
This class is responsible for reading from the audit netlink, and has been implemented to make it possible to have more than one consumer.
This is needed because the existing AuditAssembler component is not capable of sharing the audit handle and it doesn’t support complex filtering logic (which makes it unsuitable for certain use-cases). Another issue is that it does not allow the consumers to subscribe for syscalls that receive different types (and amount) of records (i.e.: open() and mmap()).
Two threads are used, one for receiving the records, and another one for parsing them. Subscribers are given a handle to request new updates.
Configuration options have been all moved here, as having all settings here is better than implementing conflict-resolution logic to merge all the (possibly) different rules requested by each subscriber.
AuditFim (new)
This new publisher receives events from the AuditNetlink class, and uses record terminators (AUDIT_EOE) to isolate and dispatch file system changes to the auditd_file_events subscriber. This should have less overhead than attempting to complete the events manually (as the AuditAssembler class does), and was initially implemented because some of the syscalls I listen for (i.e.: read and write) are called very often.
AuditEventPublisher (changed)
This class has been updated to use the new AuditNetlink class. This component was not suitable for my use case (see the previous two points), so I decoupled it from the audit netlink reader.
auditd_file_events (new)
This table receives the events from the AuditFim publisher, building a file descriptor table for each running process. This is also where filtering is implemented, in order to only show file state changes (vs showing all syscalls).