-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add column to es_process_events
for process codesigning flags
#7726
Add column to es_process_events
for process codesigning flags
#7726
Conversation
|
Hey @bgirardeau-figma, thanks for this! Are you having issues with the CLA? |
Thanks for checking in on it @sharvilshah! Yes I'm working on the CLA- our main reviewer for them will be back from PTO soon, so hopefully will be done later in the week (it's my first time contributing to open source at Figma) |
es_process_events
indicating if binary is ad-hoc signed
0b9ae91
to
7abcf90
Compare
Closing and reopening to get CI fix from master. |
@sharvilshah can you please review this now that the CLA has been completed? |
@@ -62,6 +62,7 @@ Status ESProcessEventSubscriber::Callback( | |||
r["signing_id"] = ec->signing_id; | |||
r["team_id"] = ec->team_id; | |||
r["cdhash"] = ec->cdhash; | |||
r["adhoc_signed"] = (ec->adhoc_signed) ? INTEGER(1) : INTEGER(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.
hey @bgirardeau-figma, what are your thoughts on renaming the column adhoc_signed
to codesign_flags
and it being TEXT
type?
This way, we can eventually surface all relevant flags.
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.
So the column would look like:
osquery> .mode line
osquery> select * from es_process_events;
codesign_flags = CS_ADHOC
And eventually we can extend it to include other flags from cs_blobs
, so this would allow us to make this a column which has comma separated values of all application cs flags, for example:
codesign_flags = CS_ADHOC, CS_VALID
There are other cs flags that are exposed in cs_blobs
that we can take a look at in the future:
/* code signing attributes of a process */
#define CS_VALID 0x00000001 /* dynamically valid */
#define CS_ADHOC 0x00000002 /* ad hoc signed */
#define CS_GET_TASK_ALLOW 0x00000004 /* has get-task-allow entitlement */
#define CS_INSTALLER 0x00000008 /* has installer entitlement */
#define CS_FORCED_LV 0x00000010 /* Library Validation required by Hardened System Policy */
#define CS_INVALID_ALLOWED 0x00000020 /* (macOS Only) Page invalidation allowed by task port policy */
#define CS_HARD 0x00000100 /* don't load invalid pages */
#define CS_KILL 0x00000200 /* kill process if it becomes invalid */
#define CS_CHECK_EXPIRATION 0x00000400 /* force expiration checking */
#define CS_RESTRICT 0x00000800 /* tell dyld to treat restricted */
...
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.
That's a good point, I like the idea of having a place to put more of the flags easily.
My main worry would be on the column size if we later added more flags that were commonly set on processes, to an already high-volume event table. I don't know how many of these flags are useful, but there are 24 defined.
What do you think about that option vs including the raw field (as an integer)? Individual flags would be queryable with SQL bitwise operations, and it would include all the available data compactly without needing future changes for new flags. But at the cost of some usability/human readability.
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.
That's a good point, I like the idea of having a place to put more of the flags easily.
Awesome! We definitely don't need all 24 flags (at least not yet), do you have any other flags in mind that would be immediately useful?
My main worry would be on the column size if we later added more flags that were commonly set on processes, to an already high-volume event table. I don't know how many of these flags are useful, but there are 24 defined.
I don't think the column size should be an issue here (especially since we likely won't be adding all 24 codesigning flags), and I think both path
and cmdline
column regularly are pretty big. I will try and load test this table a bit again, but I don't recall seeing any particular issues.
What do you think about that option vs including the raw field (as an integer)? Individual flags would be queryable with SQL bitwise operations, and it would include all the available data compactly without needing future changes for new flags. But at the cost of some usability/human readability.
I hear ya! This is a tricky balance, and we don't really have a good precedent for it either. The reason this was in the back of my mind was that both processes
and file
table have such flags, and both handle it differently.
osquery> select cpu_type, cpu_subtype from processes where pid = 6383;
cpu_type = 16777228
cpu_subtype = 0
osquery> select cpu_type, cpu_subtype from processes where pid = 3507;
cpu_type = 16777228
cpu_subtype = 2
osquery> select path, size, bsd_flags from file where path = '/Users/sharvil/Downloads/test.pdf';
path = /Users/sharvil/Downloads/test.pdf
size = 141581
bsd_flags = UF_IMMUTABLE
I don't think including the raw field is the right approach (see the processes
example above) -- when trying to troubleshoot or dig into something, it's hard to find documentation on what those flags mean. Contrast that with the file
table, where I know exactly what that flag means.
Sorry for the long answer, hopefully this provides some context -- would love your thoughts @bgirardeau-figma
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.
Yeah that's helpful context!
These are the flags I could see being most useful for security monitoring:
CS_VALID
: if missing, process is running with invalid code signature
CS_ADHOC
: if present, indicates code signed without a developer identity
CS_HARD
: if missing, process is at risk of tampering (loading invalid pages into memory)
CS_RESTRICT
: if missing, process is at risk of dynamic library injection
CS_REQUIRE_LV
: if missing, process can load dynamic libraries from any developer
CS_INSTALLER
: if present, process can bypass SIP filesystem protections
CS_NVRAM_UNRESTRICTED
: if present, process can manipulate/disable SIP (by writing to NVRAM)
Processes running in the hardened runtime should have CS_VALID, CS_HARD, CS_RESTRICT, CS_REQUIRE_LV
in the column, which is hopefully the common case (I can do some testing to double check). I agree up to ~7 flags doesn't seem like too much, so I support the human readable version.
I also like the idea of negating the hardening flags and dropping the CS prefix, so it's more high signal/efficient. My proposal would be: NOT_VALID
, ADHOC
, NOT_HARD
, NOT_RESTRICT
, NOT_REQUIRE_LV
, INSTALLER
, NVRAM_UNRESTRICTED
(where that's the order they would be included in the list if matched)
What do you think? I can update the PR/description if that sounds good
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.
That sounds great!
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.
Thanks @sharvilshah, updated the PR! After doing testing, I limited it to a smaller subset that I'm more confident in: NOT_VALID
, ADHOC
, NOT_RUNTIME
, INSTALLER
.
After testing, NOT_RUNTIME
is more useful than HARD
/RESTRICT
/REQUIRE_LV
because the hardened runtime enforces those settings for most apps, so it doesn't matter that those other flags are not set. There is a way for hardened runtime apps to opt out of these protections too, but it's based on entitlements rather than the flags, so it's not feasible to indicate in this table.
For NVRAM_UNRESTRICTED
, I omitted it because I'm not sure what binaries actually use this entitlement to test with, and there seem to be other similar entitlements to manipulate SIP NVRAM settings that aren't captured by it (ie csrutil which disables SIP has others).
I did keep INSTALLER
because this seems more common during system updates and is directly mentioned by the microsoft blog post about finding vulnerabilities in child processes of system_installd (which has CS_INSTALLER
set)
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.
This is great, thanks!
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 appreciate the review!
@@ -62,6 +62,7 @@ Status ESProcessEventSubscriber::Callback( | |||
r["signing_id"] = ec->signing_id; | |||
r["team_id"] = ec->team_id; | |||
r["cdhash"] = ec->cdhash; | |||
r["adhoc_signed"] = (ec->adhoc_signed) ? INTEGER(1) : INTEGER(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.
That's a good point, I like the idea of having a place to put more of the flags easily.
My main worry would be on the column size if we later added more flags that were commonly set on processes, to an already high-volume event table. I don't know how many of these flags are useful, but there are 24 defined.
What do you think about that option vs including the raw field (as an integer)? Individual flags would be queryable with SQL bitwise operations, and it would include all the available data compactly without needing future changes for new flags. But at the cost of some usability/human readability.
es_process_events
indicating if binary is ad-hoc signedes_process_events
for process codesigning flags
Adds column to the macOS es_process_events table to indicate information about codesigning flags for the process. This is useful data to include to detect processes that may be tampered with or more vulnerable to it.
This data is available in the es_process_t struct codesigning_flags field here, through the flags in the kernel headers here.
Only a few flags that are understood and relevant for monitoring are supported - more can be added later if use cases arise. These are:
NOT_VALID
: Process is running or exited with invalid code signatureADHOC
: "ad-hoc", without a code signing identity. Ad-hoc signing binaries do not require having an Apple developer account, so they may be less trustworthy (similar to unsigned binaries, which are already indicated in this table by a missing "cdhash" field).NOT_RUNTIME
: Processes that are not using the hardened runtime, which enforces key security protections by defaultINSTALLER
: Processes with a powerful system installer entitlement (can be inherited from parent) that can bypass SIP filesystem protectionsThere didn't seem to be existing test cases to add to, so I tested manually in a VM running macOS 12.5 (through Apple's virtualization framework) to make sure the table had the correct values for a normal signed binary, ad-hoc signed binary, system_installd, non-hardened runtime, and invalid code signature cases.