Add transitive whitelisting to Santa #224
Conversation
…hitelisting. cleaned up the rest of the prototype code, though left a few debugging things in for now.
|
For testing purposes: |
There was a problem hiding this comment.
Nice work so far!
As discussed we should be able to get rid of the compiler queue and use the decision queue instead. This only works if we are able to reduce the number of compiler messages sent over the great divide by filtering "file closes" based on known compiler PIDs. These known PIDs would be reported to the kernel by the exec decision from santad and invalidated by either a polling kernel thread or another message from santad (triggered by the kqueue syscall (funny we don't have access to this in kernel 😭 )).
This is a large change. It removes most of the boilerplate queue code and a lot of the user-space code.
I have added a few comments on things I don't think need removing for this new approach.
I will do a thorough review once we confirm the new plan works and the pull is updated.
Santa.xcodeproj/project.pbxproj
Outdated
| @@ -1051,11 +1057,31 @@ | |||
| attributes = { | |||
| LastUpgradeCheck = 0730; | |||
| TargetAttributes = { | |||
| 0D0016A1192BCD3C005E7FCD = { | |||
| DevelopmentTeam = 9L79GAXCBC; | |||
There was a problem hiding this comment.
Remove any DevelopmentTeam additions
| // because that was the first unused integer from sys/kauth.h. The reason that we don't | ||
| // simply use the KAUTH_VNODE_WRITE_DATA constant as is is because it overlaps with the other | ||
| // KAUTH_FILEOP constants. | ||
| #define KAUTH_FILEOP_WRITE 8 |
There was a problem hiding this comment.
If this is still needed we should give the actions used by KAUTH_SCOPE_FILEOP room to grow. I don't really like the idea of adding our own action, but if we need to make it more than 8 incase fileop adds a new action in the future.
Maybe KAUTH_FILEOP_DELETE + 32 or something
| } | ||
|
|
||
| - (void)runWithArguments:(NSArray *)arguments { | ||
| SNTConfigurator *config = [SNTConfigurator configurator]; | ||
| if ([config syncBaseURL] && ![arguments containsObject:@"--check"]) { | ||
| // TODO: --force flag was added so that we could manually add compiler rules during testing. |
There was a problem hiding this comment.
You can add the --force flag in an #ifdef DEBUG block so it can only be used on debug builds.
…D_ALLOW_COMPILER value stored in decision cache. Moved responsibility of cacheing compiler pids to kernel-space in SantaDecisionManager.cc
…ision cache. Set expiration of cached allow transitive decisions to one day. Don't add transitive rules when another rule already exists in database. Add code to check process ancestors for compiler status.
…xecution decisions
…at process ancestors by default.
|
PTAL. Also for testing with Xcode: |
Source/common/SNTRule.h
Outdated
| /// | ||
| /// Sets timestamp of rule to the current time. | ||
| /// | ||
| - (void)refreshTimestamp; |
There was a problem hiding this comment.
It's odd to have a rw property as well another method that updates the property. I suggest removing this method and let the caller set the property.
Alternatively you could make the property readonly in the interface and keep this method. If so, small nit: resetTimestamp sounds a little closer to the effective action.
There was a problem hiding this comment.
Renamed refreshTimestamp to resetTimestamp. I made timestamp into a readonly property, but SNTRuleTable needs to be able to set it with results from the rules database. So this meant adding an extra initializer for SNTRule.
I like having the method to reset the timestamp inside of SNTRule so that the logic for setting the timestamp value is self-contained. Otherwise we'd have to duplicate the code to set the timestamp in two places, which could lead to errors if we ever decided to change the format.
Source/common/SNTRule.m
Outdated
| return ([self.shasum isEqual:o.shasum] && | ||
| self.state == o.state && | ||
| self.type == o.type && | ||
| self.timestamp == o.timestamp); |
There was a problem hiding this comment.
Do we ever need to differentiate between two SNTRuleStateWhitelistTransitive rules that have the same shasum but different timestamps?
There was a problem hiding this comment.
Nope, removed.
| // FileOpCallback method. The KAUTH_FILEOP_* constants are defined in | ||
| // sys/kauth.h and run from 1--7. KAUTH_VNODE_WRITE_DATA is already defined as | ||
| // 4 so it overlaps with the other KAUTH_FILEOP_* constants and can't be used. | ||
| // We define KAUTH_FILEOP_WRITE as something >> 7. |
There was a problem hiding this comment.
nit: > 7 (>> 7 makes me think of right shift 7)
There was a problem hiding this comment.
Fixed. Sorry -- bad shorthand for" much greater than"
| @@ -286,6 +297,13 @@ santa_action_t SantaDecisionManager::GetFromCache(uint64_t identifier) { | |||
| decision_cache->remove(identifier); | |||
| return ACTION_UNSET; | |||
| } | |||
| } else if (result == ACTION_RESPOND_ALLOW_TRANSITIVE) { | |||
There was a problem hiding this comment.
Why the need to remove these from the cache after a certain amount of time? They are whitelisted binaries as far as we are concerned. They should follow the same cache invalidation procedures as standard whitelisted binaries.
There was a problem hiding this comment.
It's because the the timestamps on the transitive rules only get updated when the rules are retrieved from the rules database. If a transitive rule ends up in the decision cache and manages to stay in the cache for a long period of time, then its timestamp will never be updated even though the user might be running the binary it applies to fairly often. Setting an expiration time on the transitive rules forces them to be periodically retrieved from the database, keeping its last access timestamp fresh.
| struct timespec ts; // time to sleep between checks | ||
| SantaDecisionManager *sdm; // reference to SantaDecisionManager | ||
| } pid_monitor_info; | ||
|
|
There was a problem hiding this comment.
Fixed. (BTW, this struct definition and the following function might should be moved somewhere else since they're not part of the class, I just liked having all the process monitoring code close together).
Source/santad/SNTApplication.m
Outdated
| case ACTION_NOTIFY_CLOSE: | ||
| // Determine if we should add a transitive whitelisting rule for this new file. | ||
| // Requires that writing process was a compiler and that new file is executable. | ||
| [self.compilerController checkForNewExecutable:message]; |
There was a problem hiding this comment.
checkForNewExecutable: is not very exact. I would expect that name to return a BOOL. Maybe something like createTransitiveRule:?
|
|
||
| // Check if this file is an executable. | ||
| SNTFileInfo *fi = [[SNTFileInfo alloc] initWithPath:@(target)]; | ||
| if (fi.isExecutable) { |
There was a problem hiding this comment.
nit: if (!fi.isExecutable) return;
Then the meat of the method does not have to be indented.
| // Check if there is an existing (non-transitive) rule for this file. We leave existing rules | ||
| // alone, so that a whitelist or blacklist rule can't be overwritten by a transitive one. | ||
| SNTRuleTable *ruleTable = [SNTDatabaseController ruleTable]; | ||
| SNTRule *prevRule = [ruleTable ruleForBinarySHA256:fi.SHA1 certificateSHA256:nil]; |
There was a problem hiding this comment.
Yikes! Done.
| customMsg:@""]; | ||
|
|
||
| // Add the new rule to the rules database. | ||
| NSError *err = [[NSError alloc] init]; |
Source/santad/SNTPolicyProcessor.h
Outdated
| /// artifacts on disk. Simply pass nil to fileInfo and pass in the desired SHA256s. | ||
| /// | ||
| - (nonnull SNTCachedDecision *)decisionForFileInfo:(nullable SNTFileInfo *)fileInfo | ||
| fileSHA256:(nullable NSString *)fileSHA256 |
… set the enable_transitive_whitelisting flag. updated logic for when new rules trigger a decision cache flush.
|
Fixed all the small stuff, plus added in code for getting compiler rules and transitive whitelisting config from server. Need to spend some more time thinking about how to deal with the abandoned thread issue and keeping the transitive rule timestamps fresh. |
…es during logging. clear out old transitive rules. treat transitive rules as unknown if transitive whitelisting is disabled
| } else { | ||
| if (failed_compiler_queue_requests_ > 0) { | ||
| failed_compiler_queue_requests_--; | ||
| } |
There was a problem hiding this comment.
You can remove this pop and re-queue bit, it's unnecessary.
| proc_t proc = proc_find(pid); | ||
| if (!proc) break; | ||
| proc_rele(proc); | ||
| msleep(param, NULL, 0, "", &ts); |
There was a problem hiding this comment.
As nothing is waking this up, use IOSleep instead
There was a problem hiding this comment.
And make the timeout a constant in SantaDecisionManager.h
There was a problem hiding this comment.
Done. Added kPidMonitorSleepTimeMilliseconds constant as a private member, and PidMonitorSleepTimeMilliseconds() as a public method to return it.
| // This keeps track of all pids associated with compiler processes. It is defined as a global | ||
| // variable so that the pid monitor threads can access it without needing to reference | ||
| // our instance of SantaDecisionManager. | ||
| static SantaCache<bool> *compiler_pid_set_ = new SantaCache<bool>(500, 5); |
There was a problem hiding this comment.
As discussed:
- make this a property of SDM
- add a counter to SDM (like listener_invocations_) to track the number of threads that are outstanding
- atomically inc/dec that counter before making and terminating the threads
- during SDM dealloc (or ClientDisconnect, maybe?) make this cache pointer NULL to make the threads begin terminating and then sleep-loop until the number of outstanding threads drops to 0.
- use a struct to pass the SDM reference and the pid to the threads when they start
There was a problem hiding this comment.
Done. Added pid_monitor_thread_count_ to track the number of threads. Added PidMonitorThreadsShouldExit() function for the threads to check, since cache pointer is private. Currently calling StopPidMonitorThreads() from SDM's free() method. Alternately I think maybe this could be called from SantaDriver::stop() similar to StopListener. Not sure which is better.
| #import <Foundation/Foundation.h> | ||
| #import "SNTKernelCommon.h" | ||
|
|
||
| @class SNTDriverManager; |
| } else if (cd.decision == SNTEventStateAllowTransitive) { | ||
| // We also upgrade transitive rule decisions. These are filtered by SNTPolicyProcessor before | ||
| // they get here so that we only receive them if transitive whitelisting is enabled. | ||
| // TODO(nguyenphillip): may not need to distinguish transitive allows in the kernel anymore. |
There was a problem hiding this comment.
Nope. Got rid of ACTION_RESPOND_ALLOW_TRANSITIVE and just store SNTEventStateAllowTransitive decisions as ACTION_RESPOND_ALLOW in SantaDecisionManager's decision cache.
| if (cd.decision == SNTEventStateAllowCompiler && | ||
| [[SNTConfigurator configurator] transitiveWhitelistingEnabled]) { | ||
| // If rule indicated that the allowed binary was a compiler and Santa is configured to allow | ||
| // transitive whitelisting, upgrade the action to ACTION_RESPOND_ALLOW_COMPILER. |
There was a problem hiding this comment.
Could SNTPolicyProcessor not make this determination?
There was a problem hiding this comment.
Moved the check for transitiveWhitelistingEnabled to SNTPolicyProcessor so that if it's not enabled, the decision passed to SNTExecutionController from a SNTRuleStateWhitelistCompiler rule will be downgraded to SNTEventStateAllow.
…er_pid_set_doesn't have to be a global variable. Also keep track of number of pid monitor threads in SantaDecisionManager and wait for them to exit before freed. * Don't store ACTION_RESPOND_ALLOW_TRANSITIVE in the kernel decision cache any more. The kernel doesn't need to know about transitive rules. * Switch from msleep to IOSleep.
|
PTAL |
| /// whitelist any executables that they produce. If NO, SNTRuleStateWhitelistCompiler rules are | ||
| /// interpreted as if they were simply SNTRuleStateWhitelist rules. Defaults to NO. | ||
| /// | ||
| @property BOOL transitiveWhitelistingEnabled; |
There was a problem hiding this comment.
There doesn't seem to be any corresponding keys in the .m for this, so it's not configurable in a mobileconfig or persisted between runs from a sync server. I think it should probably be one of those keys that can be configured with a mobileconfig but can be overridden by a server, so you'll need to add a key constant and add it to both the arrays in init.
There was a problem hiding this comment.
Done. added key to both with a DEBUG guard around the syncServerKeys entry to be removed when the sync server supports it.
| // termination and then remove the process pid from cache of compiler pids. | ||
| static void pid_monitor(void *param, __unused wait_result_t wait_result) { | ||
| pid_monitor_info *info = (pid_monitor_info *)param; | ||
| uint32_t sleep_time = info->sdm->PidMonitorSleepTimeMilliseconds(); |
There was a problem hiding this comment.
This should be inside your if statement below
| static void pid_monitor(void *param, __unused wait_result_t wait_result) { | ||
| pid_monitor_info *info = (pid_monitor_info *)param; | ||
| uint32_t sleep_time = info->sdm->PidMonitorSleepTimeMilliseconds(); | ||
| if (info->sdm) { |
There was a problem hiding this comment.
To be on the safe side I'd check info is not nullptr first too
|
|
||
| // Keep track of how many times we've slept waiting for the pid monitor | ||
| // threads to exit. If we sleep for more than 5 seconds, give up on waiting. | ||
| int wait_count = 0; |
There was a problem hiding this comment.
This worries me - if the thread didn't decrement the thread count because it hasn't been scheduled there's a possibility it'll be scheduled after SDM is freed and then attempt to access it which will almost certainly panic.
There was a problem hiding this comment.
Took out the counter. I was worried that SDM might get stuck waiting forever if something went wrong with the thread count, but I suppose that is probably preferable to a kernel panic. Added code to increase the sleep time to 5 sec if the wait loop is still going after 10 sec, in case this does get stuck.
|
|
||
| pid_t client_pid_; | ||
|
|
||
| kauth_listener_t vnode_listener_; | ||
| kauth_listener_t fileop_listener_; | ||
|
|
||
| struct timespec ts_; | ||
|
|
||
| // When set to true, Santa will check all ancestors of a process to determine | ||
| // if it is a compiler. |
There was a problem hiding this comment.
There doesn't appear to be a way to make this true - is it just for testing or are we going to add a driver method to enable it?
There was a problem hiding this comment.
Eventually there should be a way to change it along with other parameters that are currently defined as constants. Added a TODO for this and moved this to live with the other constants.
| // Arguments that are passed to pid_monitor thread. | ||
| typedef struct { | ||
| pid_t pid; // process to monitor | ||
| SantaDecisionManager *sdm; // reference to SantaDecisionManager |
There was a problem hiding this comment.
Can you add a TODO somewhere appropriate to split this out of SDM at some point? I'd like to make SDM smaller and I think this can almost entirely live on its own.
Don't give up on pid monitor thread cleanup, but increase check time instead.
…en out by compilers. The rule will be deleted from the cache once SNTCompilerController finishes checking and hashing the file. This prevents race condition from blocking binaries that are in the process of being whitelisted.
to be a compiler only if its pid is in the set and it still has the same parent. This should help prevent a fork bomb from managing to steal a compiler pid from a compiler that has just exited but not been reaped from the compiler_pid_set_ yet.
There was a problem hiding this comment.
PTAL. Also I'm wondering if SNTCompilerController should be deleted and its few methods moved into something else, like SNTApplication or SNTPolicyProcessor maybe. It made sense as a class back when it was doing more, but most of that work is now in the kernel, so it's really just a single method without any stored state. If moved into SNTPolicyProcessor, we'd just need to instantiate that class in SNTApplication and pass it off to SNTExecutionController as opposed to SNTExecutionController owning its own instance.
Source/common/SNTKernelCommon.h
Outdated
| (x == ACTION_RESPOND_ALLOW || \ | ||
| x == ACTION_RESPOND_DENY || \ | ||
| x == ACTION_RESPOND_ALLOW_COMPILER || \ | ||
| x == ACTION_RESPOND_ALLOW_TEMPORARY) |
There was a problem hiding this comment.
Does ACTION_RESPOND_TOOLONG need to be a valid response?
|
Also, here's an example of what the logs look like now for executing "go run hello.go" where a temporary allow rule is being stored in the kernel while SNTCompilerController figures out the permanent rule. [2018-02-01T21:14:15.339Z] I santad: action=EXEC|decision=ALLOW|reason=COMPILER|sha256=9f16595bf07331ddb52968c67a478b3a4bc2a3c234a8d1b12341ff0d80b92e57|pid=32312|ppid=32 [2018-02-01T21:14:15.440Z] I santad: action=EXEC|decision=ALLOW|reason=PENDING_TRANSITIVE|sha256=pending|pid=32313|ppid=32306|uid=484912|user=nguyenphillip|gid=5000|grou [2018-02-01T21:14:15.452Z] I santad: action=WHITELIST|pid=32312|path=/private/var/folders/9f/0snyvwy57519np346scs_fh800fsjh/T/go-build271032812/command-line-arguments/_o |
… since it's more specific
…CLOSE for the purposes of whitelisting. Renamed ACTION_NOTIFY_CLOSE to the more generic ACTION_NOTIFY_WHITELIST.
|
PTAL. Started monitoring file rename events as well as file close because under Xcode 9, if the output file already exists, ld writes to a new file and renames it, but never explicitly closes it. |
|
Adding in some logs to show the behavior of ld with Xcode 9. This is every fileop and vnode scope event that occurs during execution of "clang -o hello hello.c"
KAUTH_FILEOP_EXEC, uid=484912, pid=22122, ppid=674, process=clang, vnode=0x7fffff806e434c98, path=/usr/bin/clang
KAUTH_FILEOP_EXEC, uid=484912, pid=24788, ppid=674, process=clang, vnode=0x7fffff806e434c98, path=/usr/bin/clang |
|
I would be curious to see what OpenBSM logs look like when performing |
|
From OpenBSM, looking at all events, but filtering on the path:
process=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang|header, 130, 11, access(2), 0, Fri Jun 1 18:37:26 2018, + 839 msec|path, hello.c|path, /KauthTest/hello.c|return, success, 0|
process=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang|header, 130, 11, access(2), 0, Fri Jun 1 18:37:33 2018, + 260 msec|path, hello.c|path, /KauthTest/hello.c|return, success, 0|
process=/usr/libexec/taskgated|header, 139, 11, open(2) - read, 0, Fri Jun 1 18:40:18 2018, + 303 msec|argument, 2, 0x1100004, flags|path, /KauthTest|path, /KauthTest|return, success, 3| process=/System/Library/CoreServices/Finder.app/Contents/MacOS/Finder|header, 125, 11, getattrlist(), 0, Fri Jun 1 18:40:29 2018, + 625 msec|path, /KauthTest|path, /KauthTest|return, success, 0| |
Tests/KernelTests/main.mm
Outdated
| @@ -57,6 +57,9 @@ | |||
| exit(1); \ | |||
| } while (0) | |||
|
|
|||
| const char *ldPath = | |||
There was a problem hiding this comment.
This is brittle. Instead of hardcoding, use this:
xcrun -f ld, which prints the full path to the active ld
Tests/KernelTests/main.mm
Outdated
| IOConnectCallStructMethod(self.connection, kSantaUserClientAllowBinary, | ||
| &vnodeid, sizeof(vnodeid), 0, 0); | ||
| break; | ||
| case ACTION_RESPOND_DENY: |
There was a problem hiding this comment.
case indents are wrong
Add transitive whitelisting. Binaries may be identified with WHITELIST_COMPILER rules. Any executable they output will then be marked locally with a transitive whitelist rule and allowed to run if the TransitiveWhitelistingEnabled config key is true.
Creates a new queue to listen for compiler-related events in santa-driver (exec, close, and rename), and these are passed on to SNTCompilerController which uses them to determine when it should auto-whitelist compiled binaries.