Fix logging issue for transitive rules added from CLONE events (#827)

mlw · web-flow · commit ce791aa8fded · 2026-02-27T20:35:20.000-05:00
Fixes a (unreleased) crash when writing telemetry related to transitive rules generated by CLONE events. Introduced by: #762 Part of SNT-289
diff --git a/Source/santad/BUILD b/Source/santad/BUILD
@@ -151,6 +151,7 @@ objc_library(
         "//Source/common:SNTFileInfo",
         "//Source/common:SNTLogging",
         "//Source/common:SNTRule",
+        "//Source/common:String",
     ],
 )
 
diff --git a/Source/santad/Logs/EndpointSecurity/Logger.h b/Source/santad/Logs/EndpointSecurity/Logger.h
@@ -79,7 +79,8 @@ class Logger : public Timer<Logger> {
 
   virtual void Log(std::unique_ptr<santa::EnrichedMessage> msg);
 
-  void LogAllowlist(const santa::Message &msg, const std::string_view hash);
+  void LogAllowlist(const santa::Message &msg, const std::string_view hash,
+                    const std::string_view target_path);
 
   void LogBundleHashingEvents(NSArray<SNTStoredExecutionEvent *> *events);
 
diff --git a/Source/santad/Logs/EndpointSecurity/Logger.mm b/Source/santad/Logs/EndpointSecurity/Logger.mm
@@ -313,9 +313,10 @@
   }
 }
 
-void Logger::LogAllowlist(const Message &msg, const std::string_view hash) {
+void Logger::LogAllowlist(const Message &msg, const std::string_view hash,
+                          const std::string_view target_path) {
   if (ShouldLog(TelemetryEvent::kAllowlist)) {
-    writer_->Write(serializer_->SerializeAllowlist(msg, hash));
+    writer_->Write(serializer_->SerializeAllowlist(msg, hash, target_path));
   }
 }
 
diff --git a/Source/santad/Logs/EndpointSecurity/LoggerTest.mm b/Source/santad/Logs/EndpointSecurity/LoggerTest.mm
@@ -96,7 +96,8 @@
  public:
   MOCK_METHOD(std::vector<uint8_t>, SerializeMessage, (const EnrichedClose &msg));
 
-  MOCK_METHOD(std::vector<uint8_t>, SerializeAllowlist, (const Message &, const std::string_view));
+  MOCK_METHOD(std::vector<uint8_t>, SerializeAllowlist,
+              (const Message &, const std::string_view, const std::string_view));
 
   MOCK_METHOD(std::vector<uint8_t>, SerializeBundleHashingEvent, (SNTStoredExecutionEvent *));
   MOCK_METHOD(std::vector<uint8_t>, SerializeDiskAppeared, (NSDictionary *, bool));
@@ -287,13 +288,14 @@ - (void)testLogAllowList {
   auto mockWriter = std::make_shared<MockWriter>();
   es_message_t msg;
   std::string_view hash = "this_is_my_test_hash";
+  std::string_view target_path = "this_is_my_target_path";
 
   mockESApi->SetExpectationsRetainReleaseMessage();
-  EXPECT_CALL(*mockSerializer, SerializeAllowlist(testing::_, hash));
+  EXPECT_CALL(*mockSerializer, SerializeAllowlist(testing::_, hash, target_path));
   EXPECT_CALL(*mockWriter, Write);
 
   Logger(nil, nil, TelemetryEvent::kEverything, 1, 1, 1, mockSerializer, mockWriter)
-      .LogAllowlist(Message(mockESApi, &msg), hash);
+      .LogAllowlist(Message(mockESApi, &msg), hash, target_path);
 
   XCTBubbleMockVerifyAndClearExpectations(mockESApi.get());
   XCTBubbleMockVerifyAndClearExpectations(mockSerializer.get());
diff --git a/Source/santad/Logs/EndpointSecurity/Serializers/BasicString.h b/Source/santad/Logs/EndpointSecurity/Serializers/BasicString.h
@@ -84,7 +84,8 @@ class BasicString : public Serializer {
       std::optional<santa::EnrichedFile> enriched_event_target, FileAccessPolicyDecision decision,
       std::string_view operation_id) override;
 
-  std::vector<uint8_t> SerializeAllowlist(const santa::Message &, const std::string_view) override;
+  std::vector<uint8_t> SerializeAllowlist(const santa::Message &, const std::string_view,
+                                          const std::string_view) override;
 
   std::vector<uint8_t> SerializeBundleHashingEvent(SNTStoredExecutionEvent *) override;
 
diff --git a/Source/santad/Logs/EndpointSecurity/Serializers/BasicString.mm b/Source/santad/Logs/EndpointSecurity/Serializers/BasicString.mm
@@ -1111,15 +1111,16 @@ static inline void AppendStringToken(std::string &str, std::string key, es_strin
 }
 
 std::vector<uint8_t> BasicString::SerializeAllowlist(const Message &msg,
-                                                     const std::string_view hash) {
+                                                     const std::string_view hash,
+                                                     const std::string_view target_path) {
   std::string str = CreateDefaultString();
 
   str.append("action=ALLOWLIST|pid=");
   str.append(std::to_string(Pid(msg->process->audit_token)));
   str.append("|pidversion=");
   str.append(std::to_string(Pidversion(msg->process->audit_token)));
   str.append("|path=");
-  str.append(FilePath(santa::GetAllowListTargetFile(msg)).Sanitized());
+  str.append(SanitizableString(target_path.data(), target_path.size()).Sanitized());
   str.append("|sha256=");
   str.append(hash);
 
diff --git a/Source/santad/Logs/EndpointSecurity/Serializers/BasicStringTest.mm b/Source/santad/Logs/EndpointSecurity/Serializers/BasicStringTest.mm
@@ -1295,14 +1295,15 @@ - (void)testSerializeAllowlist {
   auto mockESApi = std::make_shared<MockEndpointSecurityAPI>();
   mockESApi->SetExpectationsRetainReleaseMessage();
 
-  std::vector<uint8_t> ret = BasicString::Create(mockESApi, nil, false)
-                                 ->SerializeAllowlist(Message(mockESApi, &esMsg), "test_hash");
+  std::vector<uint8_t> ret =
+      BasicString::Create(mockESApi, nil, false)
+          ->SerializeAllowlist(Message(mockESApi, &esMsg), "test_hash", "target_path");
 
   XCTAssertTrue(testing::Mock::VerifyAndClearExpectations(mockESApi.get()),
                 "Expected calls were not properly mocked");
 
   std::string got(ret.begin(), ret.end());
-  std::string want = "action=ALLOWLIST|pid=12|pidversion=34|path=foo"
+  std::string want = "action=ALLOWLIST|pid=12|pidversion=34|path=target_path"
                      "|sha256=test_hash|machineid=my_id\n";
 
   XCTAssertCppStringEqual(got, want);
diff --git a/Source/santad/Logs/EndpointSecurity/Serializers/Empty.h b/Source/santad/Logs/EndpointSecurity/Serializers/Empty.h
@@ -76,7 +76,8 @@ class Empty : public Serializer {
       std::optional<santa::EnrichedFile> enriched_event_target, FileAccessPolicyDecision decision,
       std::string_view operation_id) override;
 
-  std::vector<uint8_t> SerializeAllowlist(const santa::Message &, const std::string_view) override;
+  std::vector<uint8_t> SerializeAllowlist(const santa::Message &, const std::string_view,
+                                          const std::string_view) override;
 
   std::vector<uint8_t> SerializeBundleHashingEvent(SNTStoredExecutionEvent *) override;
 
diff --git a/Source/santad/Logs/EndpointSecurity/Serializers/Empty.mm b/Source/santad/Logs/EndpointSecurity/Serializers/Empty.mm
@@ -164,7 +164,8 @@
   return {};
 }
 
-std::vector<uint8_t> Empty::SerializeAllowlist(const Message &msg, const std::string_view hash) {
+std::vector<uint8_t> Empty::SerializeAllowlist(const Message &msg, const std::string_view hash,
+                                               const std::string_view target_path) {
   return {};
 }
 
diff --git a/Source/santad/Logs/EndpointSecurity/Serializers/EmptyTest.mm b/Source/santad/Logs/EndpointSecurity/Serializers/EmptyTest.mm
@@ -43,7 +43,7 @@ - (void)testAllSerializersReturnEmptyVector {
   XCTAssertEqual(e->SerializeMessage(*(santa::EnrichedUnlink *)&fake).size(), 0);
   XCTAssertEqual(e->SerializeMessage(*(santa::EnrichedCSInvalidated *)&fake).size(), 0);
 
-  XCTAssertEqual(e->SerializeAllowlist(*(santa::Message *)&fake, "").size(), 0);
+  XCTAssertEqual(e->SerializeAllowlist(*(santa::Message *)&fake, "", "").size(), 0);
   XCTAssertEqual(e->SerializeBundleHashingEvent(nil).size(), 0);
   XCTAssertEqual(e->SerializeDiskAppeared(nil, true).size(), 0);
   XCTAssertEqual(e->SerializeDiskDisappeared(nil).size(), 0);
diff --git a/Source/santad/Logs/EndpointSecurity/Serializers/Protobuf.h b/Source/santad/Logs/EndpointSecurity/Serializers/Protobuf.h
@@ -88,7 +88,8 @@ class Protobuf : public Serializer {
       std::optional<santa::EnrichedFile> enriched_event_target, FileAccessPolicyDecision decision,
       std::string_view operation_id) override;
 
-  std::vector<uint8_t> SerializeAllowlist(const santa::Message &, const std::string_view) override;
+  std::vector<uint8_t> SerializeAllowlist(const santa::Message &, const std::string_view,
+                                          const std::string_view) override;
 
   std::vector<uint8_t> SerializeBundleHashingEvent(SNTStoredExecutionEvent *) override;
 
diff --git a/Source/santad/Logs/EndpointSecurity/Serializers/Protobuf.mm b/Source/santad/Logs/EndpointSecurity/Serializers/Protobuf.mm
@@ -1404,7 +1404,8 @@ static inline void EncodeFileInfo(::pbv1::FileInfo *pb_file, es_string_token_t p
   return FinalizeProto(santa_msg);
 }
 
-std::vector<uint8_t> Protobuf::SerializeAllowlist(const Message &msg, const std::string_view hash) {
+std::vector<uint8_t> Protobuf::SerializeAllowlist(const Message &msg, const std::string_view hash,
+                                                  const std::string_view target_path) {
   Arena arena;
   ::pbv1::SantaMessage *santa_msg = CreateDefaultProto(&arena);
 
@@ -1419,6 +1420,11 @@ static inline void EncodeFileInfo(::pbv1::FileInfo *pb_file, es_string_token_t p
   EncodeFileInfo(pb_allowlist->mutable_target(), es_file, enriched_file,
                  [NSString stringWithFormat:@"%s", hash.data()]);
 
+  // The real path might not match the path that was used in `EncodeFileInfo`. This is because
+  // we know the stat information will be appropriate, but the path might've been built from
+  // components in the ES message.
+  *pb_allowlist->mutable_target()->mutable_path() = std::string(target_path);
+
   return FinalizeProto(santa_msg);
 }
 
diff --git a/Source/santad/Logs/EndpointSecurity/Serializers/ProtobufTest.mm b/Source/santad/Logs/EndpointSecurity/Serializers/ProtobufTest.mm
@@ -1878,7 +1878,7 @@ - (void)testSerializeAllowlist {
         return false;
       },
       ^std::vector<uint8_t>(std::shared_ptr<Serializer> serializer, const Message &msg) {
-        return serializer->SerializeAllowlist(msg, "hash_value");
+        return serializer->SerializeAllowlist(msg, "hash_value", "close_file");
       });
 }
 
diff --git a/Source/santad/Logs/EndpointSecurity/Serializers/Serializer.h b/Source/santad/Logs/EndpointSecurity/Serializers/Serializer.h
@@ -100,7 +100,7 @@ class Serializer {
       const santa::EnrichedProcess &enriched_process, size_t target_index,
       std::optional<santa::EnrichedFile> enriched_event_target, FileAccessPolicyDecision decision);
 
-  virtual std::vector<uint8_t> SerializeAllowlist(const santa::Message &,
+  virtual std::vector<uint8_t> SerializeAllowlist(const santa::Message &, const std::string_view,
                                                   const std::string_view) = 0;
 
   virtual std::vector<uint8_t> SerializeBundleHashingEvent(SNTStoredExecutionEvent *) = 0;
diff --git a/Source/santad/Logs/EndpointSecurity/Serializers/Utilities.mm b/Source/santad/Logs/EndpointSecurity/Serializers/Utilities.mm
@@ -160,6 +160,7 @@
   switch (msg->event_type) {
     case ES_EVENT_TYPE_NOTIFY_CLOSE: return msg->event.close.target;
     case ES_EVENT_TYPE_NOTIFY_RENAME: return msg->event.rename.source;
+    case ES_EVENT_TYPE_NOTIFY_CLONE: return msg->event.clone.source;
     default:
       // This is a programming error
       [NSException raise:@"Unexpected type"
diff --git a/Source/santad/Logs/EndpointSecurity/Serializers/UtilitiesTest.mm b/Source/santad/Logs/EndpointSecurity/Serializers/UtilitiesTest.mm
@@ -57,6 +57,15 @@ - (void)testGetAllowListTargetFile {
     XCTAssertEqual(target, &renameSourceFile);
   }
 
+  {
+    es_file_t cloneSourceFile = MakeESFile("clone_source");
+    esMsg.event_type = ES_EVENT_TYPE_NOTIFY_CLONE;
+    esMsg.event.clone.source = &cloneSourceFile;
+    Message msg(mockESApi, &esMsg);
+    es_file_t *target = GetAllowListTargetFile(msg);
+    XCTAssertEqual(target, &cloneSourceFile);
+  }
+
   {
     esMsg.event_type = ES_EVENT_TYPE_NOTIFY_EXIT;
     Message msg(mockESApi, &esMsg);
diff --git a/Source/santad/SNTCompilerController.mm b/Source/santad/SNTCompilerController.mm
@@ -26,6 +26,7 @@
 #import "Source/common/SNTFileInfo.h"
 #import "Source/common/SNTLogging.h"
 #import "Source/common/SNTRule.h"
+#include "Source/common/String.h"
 #import "Source/santad/DataLayer/SNTRuleTable.h"
 #import "Source/santad/SNTDatabaseController.h"
 #import "Source/santad/SNTDecisionCache.h"
@@ -209,7 +210,8 @@ - (void)createTransitiveRule:(const Message &)esMsg
             LOGE(@"Unable to add new transitive rule to database: %@", error.localizedDescription);
           }
         } else {
-          logger->LogAllowlist(esMsg, [targetFile.SHA256 UTF8String]);
+          logger->LogAllowlist(esMsg, santa::NSStringToUTF8StringView(targetFile.SHA256),
+                               santa::NSStringToUTF8StringView(targetFile.path));
         }
       }
     }

Original file line number	Diff line number	Diff line change
`@@ -151,6 +151,7 @@ objc_library(`
`151`	`151`	`"//Source/common:SNTFileInfo",`
`152`	`152`	`"//Source/common:SNTLogging",`
`153`	`153`	`"//Source/common:SNTRule",`
	`154`	`+ "//Source/common:String",`
`154`	`155`	`],`
`155`	`156`	`)`
`156`	`157`
Original file line number	Diff line number	Diff line change
`@@ -313,9 +313,10 @@`
`313`	`313`	`}`
`314`	`314`	`}`
`315`	`315`
`316`		`-void Logger::LogAllowlist(const Message &msg, const std::string_view hash) {`
	`316`	`+void Logger::LogAllowlist(const Message &msg, const std::string_view hash,`
	`317`	`+ const std::string_view target_path) {`
`317`	`318`	`if (ShouldLog(TelemetryEvent::kAllowlist)) {`
`318`		`- writer_->Write(serializer_->SerializeAllowlist(msg, hash));`
	`319`	`+ writer_->Write(serializer_->SerializeAllowlist(msg, hash, target_path));`
`319`	`320`	`}`
`320`	`321`	`}`
`321`	`322`
Original file line number	Diff line number	Diff line change
`@@ -164,7 +164,8 @@`
`164`	`164`	`return {};`
`165`	`165`	`}`
`166`	`166`
`167`		`-std::vector<uint8_t> Empty::SerializeAllowlist(const Message &msg, const std::string_view hash) {`
	`167`	`+std::vector<uint8_t> Empty::SerializeAllowlist(const Message &msg, const std::string_view hash,`
	`168`	`+ const std::string_view target_path) {`
`168`	`169`	`return {};`
`169`	`170`	`}`
`170`	`171`