Add identity verification step to exec policy evaluation#927
Closed
mlw wants to merge 10 commits into
Closed
Conversation
github-actions
Bot
added
comp/santad
mlw
force-pushed
the
mlw/snt-377-verify-opened-file-matches-es-event-vnode-cdhashfstat
branch
from
f0e7396 to
6299971
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements binary identity verification between Endpoint Security process metadata and on-disk code-signing information. It extends MOLCodesignChecker to support CPU type hints for per-slice validation in universal binaries, introduces file-descriptor-based initialization, and integrates identity comparison into SNTPolicyProcessor's decision flow, emitting a new SNTEventStateBlockBinaryMismatch decision on mismatch. Changes
Sequence DiagramsequenceDiagram
participant ES as Endpoint Security
participant SEC as SNTExecutionController
participant FI as SNTFileInfo
participant MCS as MOLCodesignChecker
participant SP as SNTPolicyProcessor
ES->>SEC: exec event (cdhash, signing_id, team_id, etc.)
SEC->>FI: initWithEndpointSecurityExecEvent:<br/>(pass exec event + cpu type hint)
FI->>MCS: codesignCheckerWithError:<br/>(fd + cpuType hint)
MCS->>MCS: extract per-slice signing dict<br/>(if cpuType != CPU_TYPE_ANY)
MCS-->>FI: return checker with<br/>slice-specific signing info
FI-->>SEC: return file info<br/>(cpuType, cpuSubtype, signing certs)
SEC->>SP: decisionForFileInfo:<br/>(...with fd + csInfo)
SP->>SP: verifyIdentityForTargetProc:<br/>compare ES vs on-disk identity
alt identity mismatch
SP-->>SEC: kMismatch
else identity drift allowed
SP-->>SEC: kDriftAllowed
else identity match
SP-->>SEC: kMatch (continue evaluation)
end
SEC-->>ES: decision<br/>(Block / Allow / SNTEventStateBlockBinaryMismatch)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Source/santad/SNTExecutionController.mm (1)
174-174: Make binary-mismatch conversion explicit inBlockToAllowDecision.The new state is counted here, but conversion currently relies on
defaultinBlockToAllowDecision. Adding an explicit case will avoid hidden fallback behavior and future drift.♻️ Proposed follow-up
static SNTEventState BlockToAllowDecision(SNTEventState blockDecision) { switch (blockDecision) { case SNTEventStateBlockUnknown: return SNTEventStateAllowUnknown; case SNTEventStateBlockBinary: return SNTEventStateAllowBinary; case SNTEventStateBlockCertificate: return SNTEventStateAllowCertificate; case SNTEventStateBlockScope: return SNTEventStateAllowScope; case SNTEventStateBlockTeamID: return SNTEventStateAllowTeamID; case SNTEventStateBlockSigningID: return SNTEventStateAllowSigningID; case SNTEventStateBlockCDHash: return SNTEventStateAllowCDHash; case SNTEventStateBlockCELFallback: return SNTEventStateAllowCELFallback; case SNTEventStateBlockLongPath: return SNTEventStateAllowUnknown; // No direct equivalent + case SNTEventStateBlockBinaryMismatch: + return SNTEventStateAllowUnknown; // No direct equivalent default: return SNTEventStateAllowUnknown; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/santad/SNTExecutionController.mm` at line 174, The new enum value SNTEventStateBlockBinaryMismatch is handled in the switch that maps eventTypeStr but BlockToAllowDecision still relies on its default branch to translate states; add an explicit case in BlockToAllowDecision that maps SNTEventStateBlockBinaryMismatch to the correct allow/deny decision (the same semantic intent as kBlockBinaryMismatch) instead of falling through to default, updating the switch in BlockToAllowDecision to include a case for SNTEventStateBlockBinaryMismatch so behavior is explicit and future-proof.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/common/MOLCodesignChecker.mm`:
- Around line 160-170: The per-architecture validation in
universalSigningInformationForBinaryPath:fileDescriptor: must use the same
fd-based path used for the primary SecStaticCode binding; update the per-arch
SecStaticCodeCreateWithPath calls to build their CFURL from the same secCodePath
(i.e. "/dev/fd/N" when fileDescriptor != -1) instead of the original path
variable so slices are validated against the descriptor-bound vnode; locate the
per-arch code ref creation in
universalSigningInformationForBinaryPath:fileDescriptor: and replace usage of
path with the fd-derived secCodePath (or a new local constructed string based on
fileDescriptor) when fileDescriptor != -1.
---
Nitpick comments:
In `@Source/santad/SNTExecutionController.mm`:
- Line 174: The new enum value SNTEventStateBlockBinaryMismatch is handled in
the switch that maps eventTypeStr but BlockToAllowDecision still relies on its
default branch to translate states; add an explicit case in BlockToAllowDecision
that maps SNTEventStateBlockBinaryMismatch to the correct allow/deny decision
(the same semantic intent as kBlockBinaryMismatch) instead of falling through to
default, updating the switch in BlockToAllowDecision to include a case for
SNTEventStateBlockBinaryMismatch so behavior is explicit and future-proof.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fc2fab3-0ab3-4692-8003-60ef34f7b14c
📒 Files selected for processing (31)
MODULE.bazelSource/common/MOLCodesignChecker.hSource/common/MOLCodesignChecker.mmSource/common/MOLCodesignCheckerTest.mmSource/common/SNTBlockMessage.mmSource/common/SNTCommonEnums.hSource/common/SNTFileInfo.mmSource/common/ScopedFile.hSource/common/ScopedFileTest.mmSource/common/santa.protoSource/gui/Resources/de.lproj/Localizable.stringsSource/gui/Resources/en.lproj/Localizable.stringsSource/gui/Resources/es.lproj/Localizable.stringsSource/gui/Resources/fr-CA.lproj/Localizable.stringsSource/gui/Resources/fr.lproj/Localizable.stringsSource/gui/Resources/ja.lproj/Localizable.stringsSource/gui/Resources/ru.lproj/Localizable.stringsSource/gui/Resources/uk.lproj/Localizable.stringsSource/santad/BUILDSource/santad/Logs/EndpointSecurity/Serializers/BasicString.mmSource/santad/Logs/EndpointSecurity/Serializers/BasicStringTest.mmSource/santad/Logs/EndpointSecurity/Serializers/Protobuf.mmSource/santad/Logs/EndpointSecurity/Serializers/ProtobufTest.mmSource/santad/SNTExecutionController.hSource/santad/SNTExecutionController.mmSource/santad/SNTExecutionControllerTest.mmSource/santad/SNTPolicyProcessor.hSource/santad/SNTPolicyProcessor.mmSource/santad/SNTPolicyProcessorTest.mmSource/santasyncservice/ProtoTraits.hSource/santasyncservice/SNTSyncEventUpload.mm
mlw
added a commit
that referenced
this pull request
Apr 26, 2026
MOLCodesignChecker: route per-arch SecStaticCodeCreateWithPathAndAttributes through /dev/fd/N when an fd is supplied, matching the primary code-ref binding so universal-binary slice validation operates on the same vnode (CodeRabbit on PR #927). SantadTest: mirror the on-disk binary's identity (cdhash, teamID, signingID, signedness) into the ES-side proc fields in checkBinaryExecution: so the new identity verifier sees matching ES vs disk. Drops the kNoRuleMatchSigningID override pattern — the disk binaries' actual signing_ids already aren't keyed in rules.db, preserving the "no signing-id match" intent. SNTCommandFileInfoTest: stub initWithBinaryPath:fileDescriptor:error: with ignoringNonObjectArgs(); SNTFileInfo.codesignCheckerWithError: invokes the 3-arg form now, so the old 2-arg stubs no longer matched.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Source/santactl/Commands/SNTCommandFileInfoTest.mm (1)
130-132: Consider extracting repeatedOCMStubsetup into a test helper.The same stub chain is repeated many times; a helper would reduce noise and future churn when the initializer signature changes again.
♻️ Suggested refactor
+ - (void)stubCodesignCheckerInitReturningError:(NSError *)err { + OCMStub([self.cscMock initWithBinaryPath:OCMOCK_ANY + fileDescriptor:0 + error:[OCMArg setTo:err]]) + .ignoringNonObjectArgs() + .andReturn(self.cscMock); + } - OCMStub([self.cscMock initWithBinaryPath:OCMOCK_ANY fileDescriptor:0 error:[OCMArg setTo:err]]) - .ignoringNonObjectArgs() - .andReturn(self.cscMock); + [self stubCodesignCheckerInitReturningError:err];Also applies to: 139-141, 148-150, 157-159, 166-168, 175-177, 184-186, 193-195, 202-204, 211-213, 220-222, 229-231, 238-240, 247-249, 256-258, 265-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/santactl/Commands/SNTCommandFileInfoTest.mm` around lines 130 - 132, Tests repeat the same OCMStub setup for [self.cscMock initWithBinaryPath:OCMOCK_ANY fileDescriptor:0 error:[OCMArg setTo:err]] .ignoringNonObjectArgs().andReturn(self.cscMock); — extract that chain into a single test helper (e.g., a private method on the test class like stubCscInitWithError: or stubCscInitReturning:) and replace each repeated OCMStub call with a single call to that helper; ensure the helper accepts the error pointer or return mock as parameters so callers can vary behavior without duplicating the whole stub chain, and update uses referenced in methods that currently call initWithBinaryPath:... to call the new helper instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Source/santactl/Commands/SNTCommandFileInfoTest.mm`:
- Around line 130-132: Tests repeat the same OCMStub setup for [self.cscMock
initWithBinaryPath:OCMOCK_ANY fileDescriptor:0 error:[OCMArg setTo:err]]
.ignoringNonObjectArgs().andReturn(self.cscMock); — extract that chain into a
single test helper (e.g., a private method on the test class like
stubCscInitWithError: or stubCscInitReturning:) and replace each repeated
OCMStub call with a single call to that helper; ensure the helper accepts the
error pointer or return mock as parameters so callers can vary behavior without
duplicating the whole stub chain, and update uses referenced in methods that
currently call initWithBinaryPath:... to call the new helper instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4bd1f48d-241d-4bae-bb94-003650d7099d
📒 Files selected for processing (3)
Source/common/MOLCodesignChecker.mmSource/santactl/Commands/SNTCommandFileInfoTest.mmSource/santad/SantadTest.mm
🚧 Files skipped from review as they are similar to previous changes (1)
- Source/common/MOLCodesignChecker.mm
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/santad/SNTPolicyProcessor.mm`:
- Around line 679-695: The code allows UpdateCachedDecisionSigningInfo to
overwrite ES-side identity with on-disk identity after verifyIdentity returns
IdentityVerifyResult::kDriftAllowed, causing CreateRuleIDs to evaluate rules
against a mixed identity; modify the kDriftAllowed branch so we do NOT apply
on-disk signing info to cd (either skip calling UpdateCachedDecisionSigningInfo
when verifyIdentity(...) == IdentityVerifyResult::kDriftAllowed, or set/clear a
flag on cd such as cd.identityDriftAllowed and prevent
UpdateCachedDecisionSigningInfo from overwriting cd.sha256 and related fields),
and ensure CreateRuleIDs checks that flag (or presence of original ES cdhash)
and performs rule lookups using the original ES identity only.
- Around line 807-813: The verifyIdentity block is only set when
existingDecision == nil, so cached decisions (including
SNTActionRespondAllowNoCache) skip the vnode identity re-check; move or create
the verifyIdentity block unconditionally in decisionForFileInfo:targetProcess:…
so it is available for mismatch protection regardless of existingDecision, e.g.
always assign verifyIdentity = ^IdentityVerifyResult(MOLCodesignChecker*
_Nullable csInfo) { return [SNTPolicyProcessor
verifyIdentityForTargetProc:targetProc fd:fileInfo.fileHandle.fileDescriptor
csInfo:csInfo]; }; and keep the later mismatch check (lines referencing
verifyIdentity) unchanged so cached responses still trigger re-verification via
verifyIdentityForTargetProc.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 581f9fa8-ffa4-4c2c-bbb9-1d2bde4b07ef
📒 Files selected for processing (3)
Source/common/MOLCodesignCheckerTest.mmSource/santad/SNTPolicyProcessor.mmSource/santad/SNTPolicyProcessorTest.mm
✅ Files skipped from review due to trivial changes (1)
- Source/santad/SNTPolicyProcessorTest.mm
…SNT-377) At policy-evaluation time, compare the kernel's view of the about-to-execute binary (targetProc from the ES event) against Santa's on-disk view (MOLCodesignChecker for signed; fstat(fd) for unsigned). When the two views disagree, deny with a new SNTEventStateBlockBinaryMismatch decision. CDHash drift within a matching (TeamID, SigningID) is allowed to accommodate concurrent package installs. Plumbs the new event state through telemetry proto (REASON_BINARY_MISMATCH), sync proto (BLOCK_BINARY_MISMATCH v1+v2, protos pin -> 54add5f7), BasicString serializer, GUI block reasons, and the exec counter. ScopedFile gains a size accessor and keep-path mode used by integration tests.
When a fd is supplied, construct SecStaticCode via /dev/fd/N instead of re-resolving the path. Avoids a redundant path lookup and aligns disk-side identity with the caller's open vnode — consumed by SNT-377's verifier.
- 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.
MOLCodesignChecker: route per-arch SecStaticCodeCreateWithPathAndAttributes through /dev/fd/N when an fd is supplied, matching the primary code-ref binding so universal-binary slice validation operates on the same vnode (CodeRabbit on PR #927). SantadTest: mirror the on-disk binary's identity (cdhash, teamID, signingID, signedness) into the ES-side proc fields in checkBinaryExecution: so the new identity verifier sees matching ES vs disk. Drops the kNoRuleMatchSigningID override pattern — the disk binaries' actual signing_ids already aren't keyed in rules.db, preserving the "no signing-id match" intent. SNTCommandFileInfoTest: stub initWithBinaryPath:fileDescriptor:error: with ignoringNonObjectArgs(); SNTFileInfo.codesignCheckerWithError: invokes the 3-arg form now, so the old 2-arg stubs no longer matched.
Threads cputype/cpusubtype from es_event_exec_t through SNTFileInfo into a new MOLCodesignChecker initializer so signing identity (cdhash, teamID, signingID, certificates) reflects the slice the kernel actually loaded. Without the hint, behavior is unchanged - the new path is opt-in via the new SNTFileInfo exec-event initializer. For per-arch-inconsistent universals, the active slice's identity now populates regardless of cross-slice consistency. The cross-slice check remains as an informational error signal.
mlw
force-pushed
the
mlw/snt-377-verify-opened-file-matches-es-event-vnode-cdhashfstat
branch
from
bbee9e3 to
6571d15
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Source/santad/SNTPolicyProcessor.mm (2)
667-690:⚠️ Potential issue | 🔴 CriticalRe-run identity verification on cache-hit paths too.
Line 807 only creates
verifyIdentityfor fresh evaluations, and Lines 667-690 only invoke it while repopulating signing info. Any re-eval path with cached signing material therefore skips the new ES-vs-disk mismatch gate entirely, so stale cached decisions can survive an in-place vnode mutation.Suggested direction
- NSError* csInfoError; - if (!cd.certSHA256.length) { - MOLCodesignChecker* csInfo = [fileInfo codesignCheckerWithError:&csInfoError]; + NSError* csInfoError; + MOLCodesignChecker* csInfo = nil; + if (verifyIdentity || !cd.certSHA256.length) { + csInfo = [fileInfo codesignCheckerWithError:&csInfoError]; + } + + if (verifyIdentity) { + switch (verifyIdentity(csInfo)) { + case IdentityVerifyResult::kMismatch: + cd.decision = SNTEventStateBlockBinaryMismatch; + cd.decisionExtra = @"Binary identity mismatch between ES event and on-disk file"; + return cd; + case IdentityVerifyResult::kDriftAllowed: + cd.decisionExtra = @"CDHash drift allowed by matching TeamID/SigningID"; + break; + case IdentityVerifyResult::kMatch: + break; + } + } + + if (!cd.certSHA256.length) { - if (verifyIdentity) { - switch (verifyIdentity(csInfo)) { - case IdentityVerifyResult::kMismatch: - cd.decision = SNTEventStateBlockBinaryMismatch; - cd.decisionExtra = @"Binary identity mismatch between ES event and on-disk file"; - return cd; - case IdentityVerifyResult::kDriftAllowed: - cd.decisionExtra = @"CDHash drift allowed by matching TeamID/SigningID"; - break; - case IdentityVerifyResult::kMatch: break; - } - } - if (csInfo && csInfo.cdhash.length > 0) { UpdateCachedDecisionSigningInfo(cd, csInfo, platformBinaryState, entitlementsFilterCallback); } else if (csInfoError) { ... } }- VerifyIdentityBlock verifyIdentity = nil; - if (!existingDecision) { - int fd = fileInfo.fileHandle.fileDescriptor; - verifyIdentity = ^IdentityVerifyResult(MOLCodesignChecker* _Nullable csInfo) { - return [SNTPolicyProcessor verifyIdentityForTargetProc:targetProc fd:fd csInfo:csInfo]; - }; - } + int fd = fileInfo.fileHandle.fileDescriptor; + VerifyIdentityBlock verifyIdentity = ^IdentityVerifyResult(MOLCodesignChecker* _Nullable csInfo) { + return [SNTPolicyProcessor verifyIdentityForTargetProc:targetProc fd:fd csInfo:csInfo]; + };Also applies to: 807-813
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/santad/SNTPolicyProcessor.mm` around lines 667 - 690, The identity re-check is only performed for fresh evaluations so cached-signing paths skip the ES-vs-disk mismatch gate; ensure verifyIdentity is invoked for cache-hit/repopulating signing-info flows as well by making the verifyIdentity callable available before the cache-check (or by creating it in the cached-path) and calling verifyIdentity(csInfo) in the block that handles !cd.certSHA256.length, handling the three results exactly as the fresh-eval branch (set cd.decision = SNTEventStateBlockBinaryMismatch and cd.decisionExtra for kMismatch, set cd.decisionExtra for kDriftAllowed, and treat kMatch as no-op) so stale cached decisions cannot survive an in-place vnode mutation.
685-695:⚠️ Potential issue | 🟠 MajorDon't mix disk-only rule IDs into
kDriftAlloweddecisions.Once Line 685 allows drift, Lines 692-695 still hydrate certificate data from the on-disk file while
cdhash,teamID, andsigningIDcontinue to describe the executing image.CreateRuleIDs(cd)then evaluates binary/certificate rules against mixed identity.Suggested direction
- if (verifyIdentity) { - switch (verifyIdentity(csInfo)) { + IdentityVerifyResult identityResult = IdentityVerifyResult::kMatch; + if (verifyIdentity) { + identityResult = verifyIdentity(csInfo); + switch (identityResult) { case IdentityVerifyResult::kMismatch: cd.decision = SNTEventStateBlockBinaryMismatch; cd.decisionExtra = @"Binary identity mismatch between ES event and on-disk file"; return cd; case IdentityVerifyResult::kDriftAllowed: cd.decisionExtra = @"CDHash drift allowed by matching TeamID/SigningID"; break; case IdentityVerifyResult::kMatch: break; } } - if (csInfo && csInfo.cdhash.length > 0) { + if (identityResult == IdentityVerifyResult::kMatch && csInfo && csInfo.cdhash.length > 0) { // Identity was extracted — populate signing details and keep whatever // signingStatus the kernel reported, even if csInfoError is set. UpdateCachedDecisionSigningInfo(cd, csInfo, platformBinaryState, entitlementsFilterCallback); + } else if (identityResult == IdentityVerifyResult::kDriftAllowed) { + cd.sha256 = nil; + cd.certSHA256 = nil; + cd.certCommonName = nil; + cd.certChain = nil; } else if (csInfoError) { cd.decisionExtra = [NSString stringWithFormat:@"Signature ignored due to error: %ld", (long)csInfoError.code];
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/santad/SantadTest.mm`:
- Around line 198-205: Currently the code treats a nil MOLCodesignChecker
(csInfo) the same as an unsigned binary; instead, detect csInfo == nil as a
test/setup failure and fail fast (e.g., call the test failure helper or
return/abort the test) so real fixture errors are not masked, and only use the
unsigned path when csInfo is non-nil and csInfo.cdhash.length == 0 before
clearing proc.codesigning_flags; update the logic around MOLCodesignChecker
initialization (csInfo) to assert/fail on nil, then handle the empty cdhash case
separately.
---
Duplicate comments:
In `@Source/santad/SNTPolicyProcessor.mm`:
- Around line 667-690: The identity re-check is only performed for fresh
evaluations so cached-signing paths skip the ES-vs-disk mismatch gate; ensure
verifyIdentity is invoked for cache-hit/repopulating signing-info flows as well
by making the verifyIdentity callable available before the cache-check (or by
creating it in the cached-path) and calling verifyIdentity(csInfo) in the block
that handles !cd.certSHA256.length, handling the three results exactly as the
fresh-eval branch (set cd.decision = SNTEventStateBlockBinaryMismatch and
cd.decisionExtra for kMismatch, set cd.decisionExtra for kDriftAllowed, and
treat kMatch as no-op) so stale cached decisions cannot survive an in-place
vnode mutation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c555433-3253-462a-b567-5a95040dd7e9
📒 Files selected for processing (36)
MODULE.bazelSource/common/BUILDSource/common/MOLCodesignChecker.hSource/common/MOLCodesignChecker.mmSource/common/MOLCodesignCheckerTest.mmSource/common/SNTBlockMessage.mmSource/common/SNTCommonEnums.hSource/common/SNTFileInfo.hSource/common/SNTFileInfo.mmSource/common/SNTFileInfoTest.mmSource/common/ScopedFile.hSource/common/ScopedFileTest.mmSource/common/santa.protoSource/gui/Resources/de.lproj/Localizable.stringsSource/gui/Resources/en.lproj/Localizable.stringsSource/gui/Resources/es.lproj/Localizable.stringsSource/gui/Resources/fr-CA.lproj/Localizable.stringsSource/gui/Resources/fr.lproj/Localizable.stringsSource/gui/Resources/ja.lproj/Localizable.stringsSource/gui/Resources/ru.lproj/Localizable.stringsSource/gui/Resources/uk.lproj/Localizable.stringsSource/santactl/Commands/SNTCommandFileInfoTest.mmSource/santad/BUILDSource/santad/Logs/EndpointSecurity/Serializers/BasicString.mmSource/santad/Logs/EndpointSecurity/Serializers/BasicStringTest.mmSource/santad/Logs/EndpointSecurity/Serializers/Protobuf.mmSource/santad/Logs/EndpointSecurity/Serializers/ProtobufTest.mmSource/santad/SNTExecutionController.hSource/santad/SNTExecutionController.mmSource/santad/SNTExecutionControllerTest.mmSource/santad/SNTPolicyProcessor.hSource/santad/SNTPolicyProcessor.mmSource/santad/SNTPolicyProcessorTest.mmSource/santad/SantadTest.mmSource/santasyncservice/ProtoTraits.hSource/santasyncservice/SNTSyncEventUpload.mm
✅ Files skipped from review due to trivial changes (15)
- Source/gui/Resources/ja.lproj/Localizable.strings
- Source/gui/Resources/fr.lproj/Localizable.strings
- Source/gui/Resources/de.lproj/Localizable.strings
- Source/gui/Resources/ru.lproj/Localizable.strings
- Source/santad/SNTExecutionController.h
- Source/gui/Resources/fr-CA.lproj/Localizable.strings
- Source/gui/Resources/es.lproj/Localizable.strings
- Source/santasyncservice/SNTSyncEventUpload.mm
- Source/santad/Logs/EndpointSecurity/Serializers/BasicString.mm
- Source/santad/Logs/EndpointSecurity/Serializers/ProtobufTest.mm
- MODULE.bazel
- Source/common/SNTCommonEnums.h
- Source/santasyncservice/ProtoTraits.h
- Source/gui/Resources/en.lproj/Localizable.strings
- Source/gui/Resources/uk.lproj/Localizable.strings
🚧 Files skipped from review as they are similar to previous changes (5)
- Source/santad/Logs/EndpointSecurity/Serializers/Protobuf.mm
- Source/common/SNTBlockMessage.mm
- Source/common/ScopedFileTest.mm
- Source/santad/BUILD
- Source/santactl/Commands/SNTCommandFileInfoTest.mm
…th-based codesignCheckerWithError: now uses the fd-bound MOLCodesignChecker initializer only when the SNTFileInfo was constructed from an exec event (cputype hint set). Diagnostic callers (santactl, compiler controller, GUI, sync) fall back to the path-based init so SecStaticCode has full bundle context, producing richer signing-status output. Convert MOLCodesignChecker's fat-header read to pread so the caller's file offset is preserved across the call. Document on SNTFileInfo's fileHandle property that the offset is unspecified after codesignCheckerWithError: (SecStaticCode reads through /dev/fd/N share the descriptor's offset), and add a regression test pinning that pread-based readers see stable bytes before and after.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/common/MOLCodesignChecker.mm (1)
541-559:⚠️ Potential issue | 🟠 MajorRetry interrupted
pread()calls in the fd-offset reader.Lines 546 and 559 treat any
pread()failure as fatal.pread(2)can return-1witherrno == EINTR, so a transient signal will makearchitectureAndOffsetsForFileDescriptor:returnnilspuriously and suppress the new exec-time fd/slice-bound path.🔧 Proposed fix
const uint8* headerBytes = (const uint8*)alloca(len); - if (pread(fd, (void*)headerBytes, len, 0) != (ssize_t)len) return nil; + ssize_t bytesRead; + do { + bytesRead = pread(fd, (void*)headerBytes, len, 0); + } while (bytesRead == -1 && errno == EINTR); + if (bytesRead != (ssize_t)len) return nil; struct fat_header* fh = (struct fat_header*)headerBytes; @@ len = use64 ? sizeof(struct fat_arch_64) * archCount : sizeof(struct fat_arch) * archCount; const uint8* archBytes = (const uint8*)alloca(len); - if (pread(fd, (void*)archBytes, len, sizeof(struct fat_header)) != (ssize_t)len) return nil; + do { + bytesRead = pread(fd, (void*)archBytes, len, sizeof(struct fat_header)); + } while (bytesRead == -1 && errno == EINTR); + if (bytesRead != (ssize_t)len) return nil;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/common/MOLCodesignChecker.mm` around lines 541 - 559, The pread() calls that read headerBytes and archBytes in architectureAndOffsetsForFileDescriptor: treat any return != len as fatal; change both to robust read loops that handle EINTR and partial reads by repeatedly calling pread() until the requested len bytes are read or a non-recoverable error occurs, accumulating bytes read in a ssize_t total and advancing the buffer pointer and file offset argument accordingly; on failure propagate nil only for non-EINTR errors or short reads after the loop. Ensure you update the two sites that call pread(fd, headerBytes, len, 0) and pread(fd, archBytes, len, sizeof(struct fat_header)) to use this retry/accumulate pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Source/common/MOLCodesignChecker.mm`:
- Around line 541-559: The pread() calls that read headerBytes and archBytes in
architectureAndOffsetsForFileDescriptor: treat any return != len as fatal;
change both to robust read loops that handle EINTR and partial reads by
repeatedly calling pread() until the requested len bytes are read or a
non-recoverable error occurs, accumulating bytes read in a ssize_t total and
advancing the buffer pointer and file offset argument accordingly; on failure
propagate nil only for non-EINTR errors or short reads after the loop. Ensure
you update the two sites that call pread(fd, headerBytes, len, 0) and pread(fd,
archBytes, len, sizeof(struct fat_header)) to use this retry/accumulate pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e0eda02-3a18-48ce-bf91-ea0881cff7ff
📒 Files selected for processing (5)
Source/common/BUILDSource/common/MOLCodesignChecker.mmSource/common/SNTFileInfo.hSource/common/SNTFileInfo.mmSource/common/SNTFileInfoTest.mm
- SNTExecutionController: explicit BlockBinaryMismatch arm in BlockToAllowDecision so the mapping is locked rather than relying on the default-fall-through. - MOLCodesignChecker: factor a PreadFull helper that retries on EINTR and short reads, and route the fat-header / arch-table reads in architectureAndOffsetsForFileDescriptor: through it. - SantadTest: distinguish a MOL init failure (XCTFail + return NO) from a genuine unsigned binary (non-nil checker with empty cdhash) so fixture problems surface instead of silently exercising the unsigned path.
sharvilshah
reviewed
Apr 28, 2026
Contributor
There was a problem hiding this comment.
Should we not add one more check here: && !identityExtracted? (Or call UpdateCachedDecisionSigningInfo like we do on line 695)
sharvilshah
previously approved these changes
Apr 28, 2026
sharvilshah
left a comment
sharvilshah
left a comment
Contributor
There was a problem hiding this comment.
Just a question, but lgtm
tburgin
reviewed
Apr 28, 2026
Comment on lines
+595
to
+597
| if (CdhashEqual(targetProc->cdhash, csInfo.cdhash)) { | ||
| return IdentityVerifyResult::kMatch; | ||
| } |
Contributor
There was a problem hiding this comment.
Should we test for CDHash equality first thing inside of the if (esHasSID) branch?
|
|
||
| @note The file offset will be set to the amount of bytes read while parsing the header. | ||
| The descriptor must refer to a regular-file Mach-O. SecStaticCode operates | ||
| on the descriptor's vnode (via /dev/fd/N) rather than re-resolving |
Contributor
There was a problem hiding this comment.
Are we sure that having SecStaticCode looking /dev/fd/N paths is not masking signature failing errors behind non-failing errors like errSecCSInfoPlistFailed?
Contributor
Author
|
This is being superseded by #942 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Also includes a
MOLCodesignCheckerchange: when a file descriptor is supplied toinitWithBinaryPath:fileDescriptor:, routeSecStaticCodeCreateWithPaththrough/dev/fd/Nso the static code ref operates on the caller's vnode. This avoids a redundant path resolution and aligns disk-side identity with the bytes the caller already has open.