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
santad: add critical system binaries #296
Conversation
fileSHA256:nil | ||
certificateSHA256:csInfo.leafCertificate.SHA256]; | ||
cd.certCommonName = csInfo.leafCertificate.commonName; | ||
cd.certCommonName = csInfo.leafCertificate.commonName; | ||
} |
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 think this might be easier to follow if it were structured as
if (!cd) {
// declare csInfo
if (binInfo.isMachO) {
// get csInfo
}
// get cd from policyProcessor
}
instead of having two separate checks for !cd
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.
done
cd.certCommonName = csInfo.leafCertificate.commonName; | ||
|
||
bins[binInfo.SHA256] = cd; | ||
} |
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.
log error if signing info doesn't match?
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.
good idea
// TODO(tburgin): Add the Santa components to this feature and remove the santadCertSHA rule. | ||
NSMutableDictionary *bins = [NSMutableDictionary dictionary]; | ||
for (NSString *path in @[ @"/usr/libexec/trustd" ]) { | ||
SNTFileInfo *binInfo = [[SNTFileInfo alloc] initWithPath:path]; |
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.
Does this fail the right way if there's no file at path? I'm guessing csInfo will end up nil and then the call to signingInformationMatches: will return NO?
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, if csInfo is nil, sending any message to it will return nil.
@@ -116,21 +116,28 @@ - (void)validateBinaryWithMessage:(santa_message_t)message { | |||
} | |||
|
|||
// Get codesigning info about the file but only if it's a Mach-O. | |||
// If the binary is a critical system binary, don't check its signiture. The binary was validated |
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.
typo: signiture -> signature
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.
oops
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.
PTAL
// TODO(tburgin): Add the Santa components to this feature and remove the santadCertSHA rule. | ||
NSMutableDictionary *bins = [NSMutableDictionary dictionary]; | ||
for (NSString *path in @[ @"/usr/libexec/trustd" ]) { | ||
SNTFileInfo *binInfo = [[SNTFileInfo alloc] initWithPath:path]; |
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, if csInfo is nil, sending any message to it will return nil.
cd.certCommonName = csInfo.leafCertificate.commonName; | ||
|
||
bins[binInfo.SHA256] = cd; | ||
} |
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.
good idea
@@ -116,21 +116,28 @@ - (void)validateBinaryWithMessage:(santa_message_t)message { | |||
} | |||
|
|||
// Get codesigning info about the file but only if it's a Mach-O. | |||
// If the binary is a critical system binary, don't check its signiture. The binary was validated |
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.
oops
fileSHA256:nil | ||
certificateSHA256:csInfo.leafCertificate.SHA256]; | ||
cd.certCommonName = csInfo.leafCertificate.commonName; | ||
cd.certCommonName = csInfo.leafCertificate.commonName; | ||
} |
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.
done
PTAL |
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.
LGTM
Thanks a ton for this fix @tburgin I was wondering when the next release would be? :) |
A couple of hours, most likely |
Awesome, Thanks again! |
* santad: add critical system binaries * review updates * use a getter
/usr/libexec/trustd
needs to run in order to check arbitrary code signatures. This may be a departure from previous version of macOS. Iftrustd
is killed, Santa can cause a deadlock. This fix pre-validates and whiteliststrustd
atsantad
startup time.