Audit: Implement support for fork/vfork/clone/execveat #5701

alessandrogario · 2019-08-12T19:18:20Z

This fixes issue #5691

linux-foundation-easycla · 2019-08-12T19:18:25Z

The committers are authorized under a signed CLA.

✅ Teddy Reed (a56b43e)
✅ Shim Myeongseob (7abf354)
✅ Alessandro Gario (7d88db8, 98f892a, a0304ec, af5346e, 2740267, 87cf5b4, c1b209d, 3f562cf, ac037bd, 15c05ac, be23950, de8d0e0, 616135a, 428c4f1)

theopolis · 2019-08-13T23:25:02Z

Wow, cool!

loqpa · 2019-08-16T09:30:30Z

What do you think about filling mode column with info from file table in case there are no PATH records?

theopolis · 2019-08-15T14:31:28Z

osquery/events/linux/auditdnetlink.cpp

+    }
+
+  } else if (FLAGS_audit_allow_fork_process_events) {
+    LOG(WARNING) << "--audit_allow_fork_process_events is ignored without "


We should investigate if this dependency can be expressed using the gflags APIs.

This can be compared to enabling --audit_allow_config without also having the command line flag that enables Audit.

If we want to conform to the rest of the code, we can just remove the warning log. Otherwise, we could implement a flag validator and make sure that both flags are active.

Which one would you prefer?

Ah sorry, my comment was too general and not helpful. You are right in that we should address/explore gflags more completely later.

Always your call!

theopolis · 2019-08-17T00:28:39Z

osquery/tables/events/linux/process_events.cpp


-      row["cmdline"] += DecodeAuditPathValues(arg.second);
+    if (row["cmdline"].size() > 0) {
+      row["cmdline"] += " ";


You can also += ' '

theopolis · 2019-08-17T00:28:57Z

osquery/tables/events/linux/process_events.cpp

+    return Status::failure("Malformed AUDIT_EXECVE event");
+  }
+
+  row["cmdline"] = "";


Is this the same as .clear().

theopolis · 2019-08-17T00:30:33Z

osquery/tables/events/linux/process_events.cpp

+    return Status::failure("Malformed AUDIT_CWD event");
+  }
+
+  CopyFieldFromMap(row, cwd_event_record->fields, "cwd", "");


I see common calls with "" and "0" as the third argument. Is it worth making this API more explicit with an overload + fall through?

Those arguments are the default value to adopt in case the specified field is missing in the field map, and varies depending on the context. How would you change this?

theopolis

Take a look at the inline notes I left. Overall this looks good, GetProcessIDs looks complex but also well tested.

alessandrogario · 2019-08-22T10:50:34Z

What do you think about filling mode column with info from file table in case there are no PATH records?

Are you referring to something similar to this? https://github.com/osquery/osquery/pull/5701/files#diff-ba6fdbb94c1ce5b8f9a3950608fbe86fR59

In general, I think we should avoid having too many SELECTs on other tables inside the code (which also makes testing harder). We could maybe achieve the same using a JOIN in the query?

Also implement an initial test target for the process_events table

loqpa · 2019-08-22T11:06:09Z

What do you think about filling mode column with info from file table in case there are no PATH records?

Are you referring to something similar to this? https://github.com/osquery/osquery/pull/5701/files#diff-ba6fdbb94c1ce5b8f9a3950608fbe86fR59

In general, I think we should avoid having too many SELECTs on other tables inside the code (which also makes testing harder). We could maybe achieve the same using a JOIN in the query?

I am not entirely sure how selectAllFrom works, but i think something like
if (row["mode"]=="") row["mode"] = qd.front().at("mode")
at line 67 should not introduce any new SELECTs.

If i am wrong - it is definitely achievable with joins, i already use almost the same logic while joining processes table which does not have mode column.

However i understand that in my case mode column would be a little inconsistent (4 digits records from file vs 7 digits from PATH). This is the main reason behind my question.

alessandrogario · 2019-08-22T11:25:10Z

What do you think about filling mode column with info from file table in case there are no PATH records?

Are you referring to something similar to this? https://github.com/osquery/osquery/pull/5701/files#diff-ba6fdbb94c1ce5b8f9a3950608fbe86fR59
In general, I think we should avoid having too many SELECTs on other tables inside the code (which also makes testing harder). We could maybe achieve the same using a JOIN in the query?

I am not entirely sure how selectAllFrom works, but i think something like
if (row["mode"]=="") row["mode"] = qd.front().at("mode")
at line 67 should not introduce any new SELECTs.

If i am wrong - it is definitely achievable with joins, i already use almost the same logic while joining processes table which does not have mode column.

I see what you mean now! Thanks for the explanation 👍

alessandrogario · 2019-08-22T12:22:17Z

I know it may seem weird but GitHub had no idea what was happening in the PR until we forced it to update by changing the base branch.... 🤔

All seems good now

alessandrogario requested a review from Smjert August 12, 2019 19:19

alessandrogario added events Related to osquery's evented tables or eventing subsystem Linux process auditing labels Aug 12, 2019

alessandrogario force-pushed the alessandro/feature/fork-process-events branch 2 times, most recently from 0352539 to ce6a318 Compare August 13, 2019 21:24

alessandrogario requested a review from theopolis August 14, 2019 00:32

alessandrogario marked this pull request as ready for review August 14, 2019 00:32

theopolis reviewed Aug 17, 2019

View reviewed changes

theopolis previously approved these changes Aug 19, 2019

View reviewed changes

alessandrogario dismissed theopolis’s stale review via 27a1320 August 20, 2019 11:00

alessandrogario force-pushed the alessandro/feature/fork-process-events branch from 35fbc19 to 27a1320 Compare August 20, 2019 11:00

theopolis previously approved these changes Aug 21, 2019

View reviewed changes

alessandrogario added 13 commits August 22, 2019 13:03

Audit: Implement support for fork/vfork/clone/execveat

7d88db8

Also implement an initial test target for the process_events table

Refactor

98f892a

Refactor

a0304ec

Refactor

af5346e

Refactor

2740267

Refactor

87cf5b4

Refactor

c1b209d

Fix pid and ppid acquisition

3f562cf

Refactor

ac037bd

Refactor

15c05ac

Refactor

be23950

Code review changes

de8d0e0

Code review changes

616135a

Code review changes

428c4f1

alessandrogario dismissed theopolis’s stale review via 428c4f1 August 22, 2019 11:30

alessandrogario changed the base branch from master to thirdparty-submodules August 22, 2019 12:20

alessandrogario changed the base branch from thirdparty-submodules to master August 22, 2019 12:20

theopolis approved these changes Aug 23, 2019

View reviewed changes

alessandrogario merged commit 444b2cc into osquery:master Aug 23, 2019

alessandrogario deleted the alessandro/feature/fork-process-events branch August 23, 2019 14:09

Smjert mentioned this pull request Oct 25, 2020

sshd child not present in process_events #4220

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit: Implement support for fork/vfork/clone/execveat #5701

Audit: Implement support for fork/vfork/clone/execveat #5701

alessandrogario commented Aug 12, 2019 •

edited

linux-foundation-easycla bot commented Aug 12, 2019 •

edited

theopolis commented Aug 13, 2019

loqpa commented Aug 16, 2019

theopolis Aug 15, 2019

alessandrogario Aug 20, 2019

theopolis Aug 21, 2019

theopolis Aug 17, 2019

theopolis Aug 17, 2019

theopolis Aug 17, 2019

alessandrogario Aug 20, 2019

theopolis left a comment

alessandrogario commented Aug 22, 2019

loqpa commented Aug 22, 2019 •

edited

alessandrogario commented Aug 22, 2019

alessandrogario commented Aug 22, 2019

Audit: Implement support for fork/vfork/clone/execveat #5701

Audit: Implement support for fork/vfork/clone/execveat #5701

Conversation

alessandrogario commented Aug 12, 2019 • edited

linux-foundation-easycla bot commented Aug 12, 2019 • edited

theopolis commented Aug 13, 2019

loqpa commented Aug 16, 2019

theopolis Aug 15, 2019

Choose a reason for hiding this comment

alessandrogario Aug 20, 2019

Choose a reason for hiding this comment

theopolis Aug 21, 2019

Choose a reason for hiding this comment

theopolis Aug 17, 2019

Choose a reason for hiding this comment

theopolis Aug 17, 2019

Choose a reason for hiding this comment

theopolis Aug 17, 2019

Choose a reason for hiding this comment

alessandrogario Aug 20, 2019

Choose a reason for hiding this comment

theopolis left a comment

Choose a reason for hiding this comment

alessandrogario commented Aug 22, 2019

loqpa commented Aug 22, 2019 • edited

alessandrogario commented Aug 22, 2019

alessandrogario commented Aug 22, 2019

alessandrogario commented Aug 12, 2019 •

edited

linux-foundation-easycla bot commented Aug 12, 2019 •

edited

loqpa commented Aug 22, 2019 •

edited