Skip to content

Commit fe1c363

Browse files
committed
SNT-377 review fixups
- Tighten BinaryMismatch decisionExtra assertion to lock the contract. - Document the critical-system-binaries short-circuit with the security property it relies on (CS_KILL/CS_HARD enforcement). - Drop redundant per-test mockCodesignChecker.teamID stubs in testTeamID(Allow|Block)Rule (the validateExecEvent fixture already mirrors target identity onto the mock); tighten the fixture comment to discourage adding new ones.
1 parent 9772037 commit fe1c363

3 files changed

Lines changed: 17 additions & 13 deletions

File tree

Source/santad/SNTExecutionControllerTest.mm

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -236,15 +236,21 @@ - (void)validateExecEvent:(SNTAction)wantAction
236236
messageSetupBlock(&esMsg);
237237
}
238238

239-
// Configure mockCodesignChecker to match procExec's identity so that
240-
// VerifyIdentity returns kMatch and does not short-circuit policy evaluation.
241-
// Tests in this file are not exercising identity verification; that is covered
242-
// by the +verifyIdentityForTargetProc:fd:csInfo: tests at the bottom of
239+
// Auto-mirror target identity onto the mock codesign checker so that
240+
// VerifyIdentity returns kMatch and doesn't short-circuit policy evaluation.
241+
// Tests in this file aren't exercising identity verification — that's covered
242+
// by the +verifyIdentityForTargetProc:fd:csInfo: tests in
243243
// SNTPolicyProcessorTest.mm.
244+
//
245+
// Tests should set target identity via messageSetupBlock and let this fixture
246+
// mirror it onto the mock. Do NOT add per-test OCMStubs for cdhash, teamID,
247+
// or signingID — OCMock resolves stubs FIFO, so a per-test stub added before
248+
// validateExecEvent runs would shadow the mirror below and silently trip
249+
// kMismatch if its value differs from what messageSetupBlock writes onto
250+
// target.
244251
{
245252
const es_process_t* target = esMsg.event.exec.target;
246253
NSString* cdhexStr = santa::StringToNSString(santa::BufToHexString(target->cdhash, 20));
247-
// Stubs below must mirror target identity; otherwise verifyIdentity short-circuits.
248254
OCMStub([self.mockCodesignChecker cdhash]).andReturn(cdhexStr);
249255
if (target->team_id.length > 0) {
250256
OCMStub([self.mockCodesignChecker teamID]).andReturn(@(target->team_id.data));
@@ -453,9 +459,6 @@ - (void)testSigningIDBlockRule {
453459
}
454460

455461
- (void)testTeamIDAllowRule {
456-
// Must match target->team_id set in messageSetup; otherwise verifyIdentity short-circuits.
457-
OCMStub([self.mockCodesignChecker teamID]).andReturn(@(kExampleTeamID));
458-
459462
SNTRule* rule = [[SNTRule alloc] init];
460463
rule.state = SNTRuleStateAllow;
461464
rule.type = SNTRuleTypeTeamID;
@@ -470,9 +473,6 @@ - (void)testTeamIDAllowRule {
470473
}
471474

472475
- (void)testTeamIDBlockRule {
473-
// Must match target->team_id set in messageSetup; otherwise verifyIdentity short-circuits.
474-
OCMStub([self.mockCodesignChecker teamID]).andReturn(@(kExampleTeamID));
475-
476476
SNTRule* rule = [[SNTRule alloc] init];
477477
rule.state = SNTRuleStateBlock;
478478
rule.type = SNTRuleTypeTeamID;

Source/santad/SNTPolicyProcessor.mm

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -589,7 +589,10 @@ + (IdentityVerifyResult)verifyIdentityForTargetProc:(const es_process_t*)targetP
589589
(NSDictionary* _Nullable (^_Nullable)(NSDictionary* _Nullable entitlements))
590590
entitlementsFilterCallback {
591591
// If the binary is a critical system binary, don't check its signature.
592-
// The binary was validated at startup when the rule table was initialized.
592+
// Critical binaries are SIP or tamper protected and were validated at
593+
// startup when the rule table was initialized; the kernel's CS_KILL/CS_HARD
594+
// enforcement guarantees the SigningID matched here can only be issued for
595+
// legitimately-signed binaries with that identity.
593596
SNTCachedDecision* systemCd = [self.ruleTable.criticalSystemBinaries[cd.signingID] copy];
594597
if (systemCd) {
595598
systemCd.decisionClientMode = configState.clientMode;

Source/santad/SNTPolicyProcessorTest.mm

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1358,7 +1358,8 @@ - (void)testOuter_Unsigned_StatMismatch_ReturnsBlockBinaryMismatch {
13581358
cachedDecision:nil];
13591359

13601360
XCTAssertEqual(cd.decision, SNTEventStateBlockBinaryMismatch);
1361-
XCTAssertNotNil(cd.decisionExtra);
1361+
XCTAssertEqualObjects(cd.decisionExtra,
1362+
@"Binary identity mismatch between ES event and on-disk file");
13621363
XCTAssertFalse(cd.holdAndAsk, @"mismatch must never be TouchID-overridable");
13631364
XCTAssertNotEqual(cd.decision & SNTEventStateBlock, (SNTEventState)0);
13641365
XCTAssertEqual(cd.decision & SNTEventStateAllow, (SNTEventState)0);

0 commit comments

Comments
 (0)