From 2fea195e3a0ddec8c51955abecbd758b622ec350 Mon Sep 17 00:00:00 2001 From: Christophe Nasarre Date: Wed, 24 Jun 2026 17:30:32 +0200 Subject: [PATCH 1/3] - Fix possible elevation attack - Fix invalid data injection --- .../ETW/EtwEventsHandler.cpp | 32 ++- .../ETW/IpcClient.cpp | 5 +- .../ETW/IpcServer.cpp | 4 +- .../EtwEventsManager.cpp | 47 ++- .../EtwEventsManagerTest.cpp | 270 ++++++++++++++++-- 5 files changed, 323 insertions(+), 35 deletions(-) diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/EtwEventsHandler.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/EtwEventsHandler.cpp index d6335a03fe0c..048e67f44e37 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/EtwEventsHandler.cpp +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/EtwEventsHandler.cpp @@ -101,13 +101,20 @@ void EtwEventsHandler::OnConnect(HANDLE hPipe) else if (message->CommandId == Commands::ClrEvents) { - if (message->Size > readSize) + // The bytes received might come from an untrusted client (any local process can connect to + // the inbound pipe), so the embedded length fields must be validated against the bytes + // actually read before they are used to index into the receive buffer. + // + // Bytes required before the variable-length ETW payload: + // IpcHeader + EVENT_HEADER + the EtwUserDataLength field. + const DWORD clrEventsHeaderSize = sizeof(IpcHeader) + sizeof(EVENT_HEADER) + sizeof(uint16_t); + if (readSize < clrEventsHeaderSize) { std::stringstream builder; - builder << "Invalid format: read size " << readSize << " bytes is smaller than supposed message size " << message->Size + sizeof(IpcHeader); + builder << "Invalid format: read size " << readSize << " bytes is smaller than the minimum ClrEvents message size " << clrEventsHeaderSize; _logger->Error(builder.str()); - // TODO: maybe we should stop the communication??? + // malformed message: skip it but keep listening continue; } @@ -125,6 +132,18 @@ void EtwEventsHandler::OnConnect(HANDLE hPipe) uint16_t userDataLength = pPayload->EtwUserDataLength; uint8_t* pUserData = (uint8_t*)((byte*)&(pPayload->EtwPayload)); + // The payload length is attacker-controllable: make sure it does not exceed the bytes + // actually received so downstream parsing cannot read past the end of the receive buffer. + const DWORD availablePayload = readSize - clrEventsHeaderSize; + if (userDataLength > availablePayload) + { + std::stringstream builder; + builder << "Invalid format: ETW payload length " << userDataLength << " exceeds received bytes " << availablePayload; + _logger->Error(builder.str()); + + continue; + } + if ((keyword == KEYWORD_GC) && (id == EVENT_ALLOCATION_TICK)) { // TODO: set a breakpoint here to check the content of the payload where the type name should be visible @@ -203,6 +222,13 @@ bool EtwEventsHandler::ReadEvents(HANDLE hPipe, uint8_t* pBuffer, DWORD bufferSi } else { + // the message must at least contain a full IPC header before its fields are inspected + if (readSize < sizeof(IpcHeader)) + { + _logger->Error("Message smaller than the IPC header..."); + return false; + } + if (!IsMessageValid(pMessage)) { _logger->Error("Invalid Magic signature in message from Agent..."); diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/IpcClient.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/IpcClient.cpp index 93ad0498b665..9c89431bee34 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/IpcClient.cpp +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/IpcClient.cpp @@ -135,13 +135,16 @@ HANDLE IpcClient::GetEndPoint(IIpcLogger* pLogger, const std::string& portName, pLogger->Info(builder.str()); } + // SECURITY_SQOS_PRESENT | SECURITY_ANONYMOUS prevents a malicious process that squats the + // well-known pipe name from impersonating this (potentially high-privilege) process via + // ImpersonateNamedPipeClient. Without it, the connection defaults to SecurityImpersonation. HANDLE hPipe = ::CreateFileA( portName.c_str(), GENERIC_READ | GENERIC_WRITE, 0, nullptr, OPEN_EXISTING, - FILE_FLAG_NO_BUFFERING | FILE_FLAG_WRITE_THROUGH, + SECURITY_SQOS_PRESENT | SECURITY_ANONYMOUS | FILE_FLAG_NO_BUFFERING | FILE_FLAG_WRITE_THROUGH, nullptr); return hPipe; diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/IpcServer.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/IpcServer.cpp index 1dcc2b369c09..13716df502f2 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/IpcServer.cpp +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/IpcServer.cpp @@ -43,13 +43,15 @@ void IpcServer::Stop() if (_hNamedPipe != nullptr) { // connecting to the server pipe will unblock the ConnectNamedPipe() call + // SECURITY_SQOS_PRESENT | SECURITY_ANONYMOUS guards against a process that squats our pipe + // name during the shutdown race from impersonating us via ImpersonateNamedPipeClient. HANDLE hPipe = ::CreateFileA( _portName.c_str(), GENERIC_READ | GENERIC_WRITE, 0, nullptr, OPEN_EXISTING, - FILE_FLAG_NO_BUFFERING | FILE_FLAG_WRITE_THROUGH, + SECURITY_SQOS_PRESENT | SECURITY_ANONYMOUS | FILE_FLAG_NO_BUFFERING | FILE_FLAG_WRITE_THROUGH, nullptr); if (hPipe != INVALID_HANDLE_VALUE) { diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/EtwEventsManager.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/EtwEventsManager.cpp index 8a88920deae8..369d424a4f12 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/EtwEventsManager.cpp +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/EtwEventsManager.cpp @@ -3,6 +3,7 @@ #include "EtwEventsManager.h" +#include "EventsParserHelper.h" #include "FrameStore.h" #include "IContentionListener.h" #include "IAllocationsListener.h" @@ -14,6 +15,7 @@ #include "Windows.h" +#include #include #include #include @@ -218,8 +220,39 @@ void EtwEventsManager::OnEvent( // // but no object size... // Since the events are received asynchronously, it is not even possible - // to assume that the object that was stored at the received Adress field + // to assume that the object that was stored at the received Address field // is still there: it could have been moved by a garbage collection + // + // Since the payload can be received from an untrusted source (any local process can + // connect to the inbound pipe), so it must be validated before being parsed: + // - the buffer must be large enough to hold the fixed-size fields, and + // - the type name must be NUL-terminated within the received bytes. + // Reading the name with EventsParserHelper::ReadWideString is also cross-platform + const uint32_t fixedFieldsSize = static_cast(offsetof(AllocationTickV3Payload, FirstCharInName)); + if (cbEventData < fixedFieldsSize) + { + if (_isDebugLogEnabled) + { + std::cout << "AllocationTick payload too small (" << cbEventData << " bytes)" << std::endl; + } + + pThreadInfo->ClearLastEventId(); + return; + } + + ULONG nameOffset = fixedFieldsSize; + const WCHAR* pTypeName = EventsParserHelper::ReadWideString(pEventData, cbEventData, &nameOffset); + if (pTypeName == nullptr) + { + if (_isDebugLogEnabled) + { + std::cout << "AllocationTick type name is not properly terminated" << std::endl; + } + + pThreadInfo->ClearLastEventId(); + return; + } + auto* pPayload = reinterpret_cast(pEventData); pThreadInfo->AllocationTickTimestamp = timestamp; pThreadInfo->AllocationKind = pPayload->AllocationKind; @@ -238,7 +271,7 @@ void EtwEventsManager::OnEvent( pThreadInfo->AllocationAmount = pPayload->AllocationAmount64; // TODO: should we use a buffer allocated once and reused to avoid memory allocations due to std::string? - pThreadInfo->AllocatedType = shared::ToString(shared::WSTRING(&(pPayload->FirstCharInName))); + pThreadInfo->AllocatedType = shared::ToString(shared::WSTRING(pTypeName)); // wait for the sibling StackWalk event to create the sample } @@ -302,8 +335,14 @@ void EtwEventsManager::AttachCallstack(std::vector& stack, uint16_t u } StackWalkPayload* pPayload = (StackWalkPayload*)pUserData; - // size of all frames + payload size - size of the first frame not counted twice - if (userDataLength < pPayload->FrameCount * sizeof(uintptr_t) + sizeof(StackWalkPayload) - sizeof(uintptr_t)) + + // FrameCount might come from an untrusted payload. Compute the number of frames that actually fit + // in the received bytes using a division so the bound cannot be bypassed by an integer overflow + // of FrameCount * sizeof(uintptr_t) (which would truncate to 32 bits on x86). + // sizeof(StackWalkPayload) already accounts for the first Stack[0] entry, hence - sizeof(uintptr_t). + const size_t headerSize = sizeof(StackWalkPayload) - sizeof(uintptr_t); + const size_t maxFrames = (static_cast(userDataLength) - headerSize) / sizeof(uintptr_t); + if (pPayload->FrameCount > maxFrames) { return; } diff --git a/profiler/test/Datadog.Profiler.Native.Tests/EtwEventsManagerTest.cpp b/profiler/test/Datadog.Profiler.Native.Tests/EtwEventsManagerTest.cpp index f54b1c132fb4..9a64cddf4092 100644 --- a/profiler/test/Datadog.Profiler.Native.Tests/EtwEventsManagerTest.cpp +++ b/profiler/test/Datadog.Profiler.Native.Tests/EtwEventsManagerTest.cpp @@ -14,7 +14,14 @@ #include "profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/EtwEventsManager.h" +#include #include +#include +#include +#include +#include +#include +#include using ::testing::_; using ::testing::ElementsAre; @@ -24,6 +31,151 @@ using ::testing::ReturnRef; using namespace std::chrono_literals; +namespace +{ +constexpr uint32_t AllocationTickTypeNameOffset = offsetof(AllocationTickV3Payload, FirstCharInName); + +struct TestLogger : public IIpcLogger +{ + void Info(std::string line) const override + { + } + + void Warn(std::string line) const override + { + } + + void Error(std::string line) const override + { + } +}; + +class TestEtwEventsReceiver : public IEtwEventsReceiver +{ +public: + uint32_t EventCount = 0; + uint32_t LastEventDataSize = 0; + + void OnEvent( + etw_timestamp timestamp, + uint32_t tid, + uint32_t version, + uint64_t keyword, + uint8_t level, + uint32_t id, + uint32_t cbEventData, + const uint8_t* pEventData) override + { + EventCount++; + LastEventDataSize = cbEventData; + } + + void OnStop() override + { + } +}; + +std::vector CreateAllocationTickPayload(const WCHAR* typeName, bool includeNullTerminator) +{ + const size_t typeNameLength = std::char_traits::length(typeName); + const size_t typeNameCharCount = typeNameLength + (includeNullTerminator ? 1 : 0); + std::vector buffer(AllocationTickTypeNameOffset + (typeNameCharCount * sizeof(WCHAR))); + auto* payload = reinterpret_cast(buffer.data()); + + payload->AllocationAmount = 21; + payload->AllocationKind = 0x0; + payload->ClrInstanceId = 1; + payload->AllocationAmount64 = 21; + memcpy(&payload->FirstCharInName, typeName, typeNameLength * sizeof(WCHAR)); + if (includeNullTerminator) + { + (&payload->FirstCharInName)[typeNameLength] = WStr('\0'); + } + + return buffer; +} + +std::vector CreateStackWalkPayload(uint32_t frameCount, std::initializer_list frames) +{ + const size_t frameStorageSize = (frames.size() == 0) ? 0 : (frames.size() - 1) * sizeof(uintptr_t); + std::vector buffer(sizeof(StackWalkPayload) + frameStorageSize); + auto* payload = reinterpret_cast(buffer.data()); + payload->FrameCount = frameCount; + + size_t index = 0; + for (auto frame : frames) + { + payload->Stack[index++] = frame; + } + + return buffer; +} + +std::vector CreateClrEventsMessage(uint16_t declaredPayloadSize, size_t actualPayloadSize) +{ + const size_t clrEventsHeaderSize = sizeof(IpcHeader) + sizeof(EVENT_HEADER) + sizeof(uint16_t); + std::vector buffer(clrEventsHeaderSize + actualPayloadSize); + auto* message = reinterpret_cast(buffer.data()); + + memcpy(message->Magic, &DD_Ipc_Magic_V1, sizeof(DD_Ipc_Magic_V1)); + message->Size = static_cast(buffer.size()); + message->CommandId = Commands::ClrEvents; + message->EtwHeader.EventDescriptor.Keyword = KEYWORD_GC; + message->EtwHeader.EventDescriptor.Id = EVENT_ALLOCATION_TICK; + message->Payload.EtwUserDataLength = declaredPayloadSize; + + return buffer; +} + +void SendPipeMessage(const void* message, DWORD messageSize, TestEtwEventsReceiver& receiver) +{ + constexpr auto PipeTimeoutMs = 5000; + std::string pipeName = "\\\\.\\pipe\\DD_ETW_TEST_"; + pipeName += std::to_string(::GetCurrentProcessId()); + pipeName += "_"; + pipeName += std::to_string(::GetTickCount64()); + + HANDLE serverPipe = ::CreateNamedPipeA( + pipeName.c_str(), + PIPE_ACCESS_DUPLEX, + PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT | PIPE_REJECT_REMOTE_CLIENTS, + 1, + sizeof(SuccessResponse), + (1 << 16) + sizeof(IpcHeader), + PipeTimeoutMs, + nullptr); + ASSERT_NE(INVALID_HANDLE_VALUE, serverPipe); + + auto logger = std::make_shared(); + EtwEventsHandler handler(logger, &receiver, nullptr); + auto serverThread = std::thread([&handler, serverPipe]() { + if (::ConnectNamedPipe(serverPipe, nullptr) || ::GetLastError() == ERROR_PIPE_CONNECTED) + { + handler.OnConnect(serverPipe); + } + }); + + HANDLE clientPipe = ::CreateFileA( + pipeName.c_str(), + GENERIC_READ | GENERIC_WRITE, + 0, + nullptr, + OPEN_EXISTING, + SECURITY_SQOS_PRESENT | SECURITY_ANONYMOUS | FILE_FLAG_NO_BUFFERING | FILE_FLAG_WRITE_THROUGH, + nullptr); + ASSERT_NE(INVALID_HANDLE_VALUE, clientPipe); + + DWORD bytesWritten; + ASSERT_TRUE(::WriteFile(clientPipe, message, messageSize, &bytesWritten, nullptr)); + EXPECT_EQ(messageSize, bytesWritten); + + ::CloseHandle(clientPipe); + serverThread.join(); + ::CloseHandle(serverPipe); +} + +} + TEST(EtwEventsManagerTest, ContentionEventWithoutCallstack) { auto [configuration, mockConfiguration] = CreateConfiguration(); @@ -72,18 +224,8 @@ TEST(EtwEventsManagerTest, AllocationTickEventWithCallstack) auto manager = EtwEventsManager(allocationListener.get(), nullptr, nullptr, configuration.get()); - constexpr auto typeName = WStr("MyType"); - constexpr auto typeNbBytes = std::char_traits::length(typeName) * sizeof(WCHAR); - std::uint8_t buffer[sizeof(AllocationTickV3Payload) + typeNbBytes + 1] = {0}; - - auto* payload = (AllocationTickV3Payload*)&buffer; - - payload->AllocationAmount = 21; - payload->AllocationKind = 0x0; - payload->ClrInstanceId = 1; - payload->AllocationAmount64 = 21; - memcpy(&payload->FirstCharInName, typeName, typeNbBytes); - (&payload->FirstCharInName)[typeNbBytes] = WStr('\0'); + auto buffer = CreateAllocationTickPayload(WStr("MyType"), true); + auto* payload = reinterpret_cast(buffer.data()); EXPECT_CALL(mockAllocationListener, OnAllocation( @@ -95,7 +237,7 @@ TEST(EtwEventsManagerTest, AllocationTickEventWithCallstack) payload->AllocationAmount64, ElementsAre(FrameStore::FakeAllocationIP))); - manager.OnEvent(etw_timestamp(132463843855875757), 2, 1, KEYWORD_GC, 1, EVENT_ALLOCATION_TICK, 0, reinterpret_cast(&buffer)); + manager.OnEvent(etw_timestamp(132463843855875757), 2, 1, KEYWORD_GC, 1, EVENT_ALLOCATION_TICK, static_cast(buffer.size()), buffer.data()); manager.OnEvent(etw_timestamp(132463843855875800), 2, 1, KEYWORD_STACKWALK, 1, -1, 0, nullptr); } @@ -111,18 +253,8 @@ TEST(EtwEventsManagerTest, AllocationTickEventWithoutCallstack) auto manager = EtwEventsManager(allocationListener.get(), nullptr, nullptr, configuration.get()); - constexpr auto typeName = WStr("MyType"); - constexpr auto typeNbBytes = std::char_traits::length(typeName) * sizeof(WCHAR); - std::uint8_t buffer[sizeof(AllocationTickV3Payload) + typeNbBytes + 1] = {0}; - - auto* payload = reinterpret_cast(&buffer); - - payload->AllocationAmount = 21; - payload->AllocationKind = 0x0; - payload->ClrInstanceId = 1; - payload->AllocationAmount64 = 21; - memcpy(&payload->FirstCharInName, typeName, typeNbBytes); - (&payload->FirstCharInName)[typeNbBytes] = WStr('\0'); + auto buffer = CreateAllocationTickPayload(WStr("MyType"), true); + auto* payload = reinterpret_cast(buffer.data()); EXPECT_CALL(mockAllocationListener, OnAllocation( @@ -135,6 +267,92 @@ TEST(EtwEventsManagerTest, AllocationTickEventWithoutCallstack) ElementsAre(FrameStore::FakeAllocationIP))) .Times(0); - manager.OnEvent(etw_timestamp(132463843855875757), 2, 1, KEYWORD_GC, 1, EVENT_ALLOCATION_TICK, 0, reinterpret_cast(&buffer)); + manager.OnEvent(etw_timestamp(132463843855875757), 2, 1, KEYWORD_GC, 1, EVENT_ALLOCATION_TICK, static_cast(buffer.size()), buffer.data()); +} + +TEST(EtwEventsManagerTest, AllocationTickEventWithUnterminatedTypeNameIsIgnored) +{ + auto [configuration, mockConfiguration] = CreateConfiguration(); + + EXPECT_CALL(mockConfiguration, IsEtwLoggingEnabled()).WillRepeatedly(Return(false)); + std::string replayEndpoint = "not empty to simulate replay"; + EXPECT_CALL(mockConfiguration, GetEtwReplayEndpoint()).WillRepeatedly(ReturnRef(replayEndpoint)); + + auto [allocationListener, mockAllocationListener] = CreateMockForUniquePtr(); + auto manager = EtwEventsManager(allocationListener.get(), nullptr, nullptr, configuration.get()); + + auto buffer = CreateAllocationTickPayload(WStr("MyType"), false); + + EXPECT_CALL(mockAllocationListener, OnAllocation(_, _, _, _, _, _, _)).Times(0); + + manager.OnEvent(etw_timestamp(132463843855875757), 2, 1, KEYWORD_GC, 1, EVENT_ALLOCATION_TICK, static_cast(buffer.size()), buffer.data()); + manager.OnEvent(etw_timestamp(132463843855875800), 2, 1, KEYWORD_STACKWALK, 1, -1, 0, nullptr); +} + +TEST(EtwEventsManagerTest, AllocationTickEventWithTruncatedFixedFieldsIsIgnored) +{ + auto [configuration, mockConfiguration] = CreateConfiguration(); + + EXPECT_CALL(mockConfiguration, IsEtwLoggingEnabled()).WillRepeatedly(Return(false)); + std::string replayEndpoint = "not empty to simulate replay"; + EXPECT_CALL(mockConfiguration, GetEtwReplayEndpoint()).WillRepeatedly(ReturnRef(replayEndpoint)); + + auto [allocationListener, mockAllocationListener] = CreateMockForUniquePtr(); + auto manager = EtwEventsManager(allocationListener.get(), nullptr, nullptr, configuration.get()); + + std::vector buffer(AllocationTickTypeNameOffset - 1); + + EXPECT_CALL(mockAllocationListener, OnAllocation(_, _, _, _, _, _, _)).Times(0); + + manager.OnEvent(etw_timestamp(132463843855875757), 2, 1, KEYWORD_GC, 1, EVENT_ALLOCATION_TICK, static_cast(buffer.size()), buffer.data()); + manager.OnEvent(etw_timestamp(132463843855875800), 2, 1, KEYWORD_STACKWALK, 1, -1, 0, nullptr); +} + +TEST(EtwEventsManagerTest, StackWalkWithOversizedFrameCountIsIgnored) +{ + auto [configuration, mockConfiguration] = CreateConfiguration(); + + EXPECT_CALL(mockConfiguration, IsEtwLoggingEnabled()).WillRepeatedly(Return(false)); + std::string replayEndpoint; + EXPECT_CALL(mockConfiguration, GetEtwReplayEndpoint()).WillRepeatedly(ReturnRef(replayEndpoint)); + + auto [allocationListener, mockAllocationListener] = CreateMockForUniquePtr(); + auto manager = EtwEventsManager(allocationListener.get(), nullptr, nullptr, configuration.get()); + + auto allocationPayload = CreateAllocationTickPayload(WStr("MyType"), true); + auto stackWalkPayload = CreateStackWalkPayload(UINT32_MAX, {0x1234}); + + EXPECT_CALL(mockAllocationListener, + OnAllocation( + _, + _, + _, + _, + "MyType", + _, + IsEmpty())); + + manager.OnEvent(etw_timestamp(132463843855875757), 2, 1, KEYWORD_GC, 1, EVENT_ALLOCATION_TICK, static_cast(allocationPayload.size()), allocationPayload.data()); + manager.OnEvent(etw_timestamp(132463843855875800), 2, 1, KEYWORD_STACKWALK, 1, -1, static_cast(stackWalkPayload.size()), stackWalkPayload.data()); +} + +TEST(EtwEventsManagerTest, ClrEventsMessageWithOversizedPayloadLengthIsIgnored) +{ + TestEtwEventsReceiver receiver; + auto message = CreateClrEventsMessage(10, 1); + + SendPipeMessage(message.data(), static_cast(message.size()), receiver); + + EXPECT_EQ(0u, receiver.EventCount); +} + +TEST(EtwEventsManagerTest, TruncatedClrEventsMessageIsIgnored) +{ + TestEtwEventsReceiver receiver; + auto message = CreateClrEventsMessage(0, 0); + + SendPipeMessage(message.data(), static_cast(message.size() - 1), receiver); + + EXPECT_EQ(0u, receiver.EventCount); } #endif \ No newline at end of file From 3f7e6731943a0a0200f22546bb219f4078b6bea4 Mon Sep 17 00:00:00 2001 From: Christophe Nasarre Date: Wed, 24 Jun 2026 18:25:20 +0200 Subject: [PATCH 2/3] Update tests --- .../EtwEventsManagerTest.cpp | 127 ++++++++++++++++-- 1 file changed, 115 insertions(+), 12 deletions(-) diff --git a/profiler/test/Datadog.Profiler.Native.Tests/EtwEventsManagerTest.cpp b/profiler/test/Datadog.Profiler.Native.Tests/EtwEventsManagerTest.cpp index 9a64cddf4092..3cab948f0e58 100644 --- a/profiler/test/Datadog.Profiler.Native.Tests/EtwEventsManagerTest.cpp +++ b/profiler/test/Datadog.Profiler.Native.Tests/EtwEventsManagerTest.cpp @@ -146,15 +146,12 @@ void SendPipeMessage(const void* message, DWORD messageSize, TestEtwEventsReceiv nullptr); ASSERT_NE(INVALID_HANDLE_VALUE, serverPipe); - auto logger = std::make_shared(); - EtwEventsHandler handler(logger, &receiver, nullptr); - auto serverThread = std::thread([&handler, serverPipe]() { - if (::ConnectNamedPipe(serverPipe, nullptr) || ::GetLastError() == ERROR_PIPE_CONNECTED) - { - handler.OnConnect(serverPipe); - } - }); - + // Connect the client and write the message *before* starting the reader thread. This makes the + // test deterministic: the server-side ConnectNamedPipe() runs while the client handle is still + // open (so it reports the connection instead of racing into ERROR_NO_DATA when a client writes + // and disconnects too quickly), and the bytes are buffered in the pipe so they remain readable + // after the client handle is closed. Doing all the fatal ASSERT_* checks here, before the thread + // exists, also guarantees we never return while a joinable std::thread would call std::terminate(). HANDLE clientPipe = ::CreateFileA( pipeName.c_str(), GENERIC_READ | GENERIC_WRITE, @@ -165,13 +162,29 @@ void SendPipeMessage(const void* message, DWORD messageSize, TestEtwEventsReceiv nullptr); ASSERT_NE(INVALID_HANDLE_VALUE, clientPipe); - DWORD bytesWritten; - ASSERT_TRUE(::WriteFile(clientPipe, message, messageSize, &bytesWritten, nullptr)); - EXPECT_EQ(messageSize, bytesWritten); + const bool connected = ::ConnectNamedPipe(serverPipe, nullptr) || (::GetLastError() == ERROR_PIPE_CONNECTED); + + DWORD bytesWritten = 0; + const bool writeSucceeded = ::WriteFile(clientPipe, message, messageSize, &bytesWritten, nullptr) != FALSE; + + auto logger = std::make_shared(); + EtwEventsHandler handler(logger, &receiver, nullptr); + auto serverThread = std::thread([&handler, serverPipe, connected]() { + if (connected) + { + handler.OnConnect(serverPipe); + } + }); + // Closing the client end makes the reader loop see a broken pipe once the buffered message has + // been consumed, which is what lets OnConnect() return so the thread can be joined. ::CloseHandle(clientPipe); serverThread.join(); ::CloseHandle(serverPipe); + + EXPECT_TRUE(connected); + EXPECT_TRUE(writeSucceeded); + EXPECT_EQ(messageSize, bytesWritten); } } @@ -336,6 +349,68 @@ TEST(EtwEventsManagerTest, StackWalkWithOversizedFrameCountIsIgnored) manager.OnEvent(etw_timestamp(132463843855875800), 2, 1, KEYWORD_STACKWALK, 1, -1, static_cast(stackWalkPayload.size()), stackWalkPayload.data()); } +TEST(EtwEventsManagerTest, StackWalkWithFrameCountAtLimitIsAttached) +{ + auto [configuration, mockConfiguration] = CreateConfiguration(); + + EXPECT_CALL(mockConfiguration, IsEtwLoggingEnabled()).WillRepeatedly(Return(false)); + // empty replay endpoint so the real frames are parsed by AttachCallstack (the bounds check under test) + std::string replayEndpoint; + EXPECT_CALL(mockConfiguration, GetEtwReplayEndpoint()).WillRepeatedly(ReturnRef(replayEndpoint)); + + auto [allocationListener, mockAllocationListener] = CreateMockForUniquePtr(); + auto manager = EtwEventsManager(allocationListener.get(), nullptr, nullptr, configuration.get()); + + auto allocationPayload = CreateAllocationTickPayload(WStr("MyType"), true); + // the buffer holds exactly 3 frames, so maxFrames == 3: FrameCount == maxFrames must be accepted + auto stackWalkPayload = CreateStackWalkPayload(3, {0x1111, 0x2222, 0x3333}); + + EXPECT_CALL(mockAllocationListener, + OnAllocation( + _, + _, + _, + _, + "MyType", + _, + ElementsAre( + static_cast(0x1111), + static_cast(0x2222), + static_cast(0x3333)))); + + manager.OnEvent(etw_timestamp(132463843855875757), 2, 1, KEYWORD_GC, 1, EVENT_ALLOCATION_TICK, static_cast(allocationPayload.size()), allocationPayload.data()); + manager.OnEvent(etw_timestamp(132463843855875800), 2, 1, KEYWORD_STACKWALK, 1, -1, static_cast(stackWalkPayload.size()), stackWalkPayload.data()); +} + +TEST(EtwEventsManagerTest, StackWalkWithFrameCountAboveLimitIsIgnored) +{ + auto [configuration, mockConfiguration] = CreateConfiguration(); + + EXPECT_CALL(mockConfiguration, IsEtwLoggingEnabled()).WillRepeatedly(Return(false)); + std::string replayEndpoint; + EXPECT_CALL(mockConfiguration, GetEtwReplayEndpoint()).WillRepeatedly(ReturnRef(replayEndpoint)); + + auto [allocationListener, mockAllocationListener] = CreateMockForUniquePtr(); + auto manager = EtwEventsManager(allocationListener.get(), nullptr, nullptr, configuration.get()); + + auto allocationPayload = CreateAllocationTickPayload(WStr("MyType"), true); + // the buffer holds 3 frames (maxFrames == 3) but declares 4: one past the limit must be rejected + auto stackWalkPayload = CreateStackWalkPayload(4, {0x1111, 0x2222, 0x3333}); + + EXPECT_CALL(mockAllocationListener, + OnAllocation( + _, + _, + _, + _, + "MyType", + _, + IsEmpty())); + + manager.OnEvent(etw_timestamp(132463843855875757), 2, 1, KEYWORD_GC, 1, EVENT_ALLOCATION_TICK, static_cast(allocationPayload.size()), allocationPayload.data()); + manager.OnEvent(etw_timestamp(132463843855875800), 2, 1, KEYWORD_STACKWALK, 1, -1, static_cast(stackWalkPayload.size()), stackWalkPayload.data()); +} + TEST(EtwEventsManagerTest, ClrEventsMessageWithOversizedPayloadLengthIsIgnored) { TestEtwEventsReceiver receiver; @@ -355,4 +430,32 @@ TEST(EtwEventsManagerTest, TruncatedClrEventsMessageIsIgnored) EXPECT_EQ(0u, receiver.EventCount); } + +TEST(EtwEventsManagerTest, ClrEventsMessageWithExactPayloadLengthIsForwarded) +{ + TestEtwEventsReceiver receiver; + + // declared payload length == bytes actually available after the header (userDataLength == availablePayload): + // this exact-boundary case must be accepted and forwarded to the receiver. + constexpr uint16_t payloadSize = 16; + auto message = CreateClrEventsMessage(payloadSize, payloadSize); + + SendPipeMessage(message.data(), static_cast(message.size()), receiver); + + EXPECT_EQ(1u, receiver.EventCount); + EXPECT_EQ(static_cast(payloadSize), receiver.LastEventDataSize); +} + +TEST(EtwEventsManagerTest, MessageSmallerThanIpcHeaderIsIgnored) +{ + TestEtwEventsReceiver receiver; + + // a message that does not even contain a full IpcHeader must be rejected before any field is read. + std::array buffer{}; + memcpy(buffer.data(), &DD_Ipc_Magic_V1, sizeof(DD_Ipc_Magic_V1)); + + SendPipeMessage(buffer.data(), static_cast(buffer.size()), receiver); + + EXPECT_EQ(0u, receiver.EventCount); +} #endif \ No newline at end of file From b09cd32c8e5ca21fc393911be12ebc2259ad71d1 Mon Sep 17 00:00:00 2001 From: Christophe Nasarre Date: Thu, 25 Jun 2026 07:52:09 +0200 Subject: [PATCH 3/3] Protect allocation tick payload --- .../ETW/IpcServer.cpp | 20 +++++++++++++++++-- .../EtwEventsManager.cpp | 16 ++++++--------- .../AllocationsProvider.cpp | 10 +++++++--- 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/IpcServer.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/IpcServer.cpp index 13716df502f2..b21c6592f2b1 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/IpcServer.cpp +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/ETW/IpcServer.cpp @@ -168,7 +168,10 @@ IpcServer* IpcServer::StartAsync( pThis->_hNamedPipe = ::CreateNamedPipeA( pThis->_portName.c_str(), - PIPE_ACCESS_DUPLEX, + // FILE_FLAG_FIRST_PIPE_INSTANCE guarantees we are the creator of this pipe name. + // Without it, a malicious local process could pre-create the pipe and impersonate + // our endpoint so the Agent connects to it instead of us. + PIPE_ACCESS_DUPLEX | FILE_FLAG_FIRST_PIPE_INSTANCE, PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT | PIPE_REJECT_REMOTE_CLIENTS, pThis->_maxInstances, pThis->_outBufferSize, @@ -179,7 +182,20 @@ IpcServer* IpcServer::StartAsync( if (pThis->_hNamedPipe == INVALID_HANDLE_VALUE) { - pThis->ShowLastError("Failed to create named pipe..."); + DWORD lastError = ::GetLastError(); + + // With FILE_FLAG_FIRST_PIPE_INSTANCE, ERROR_ACCESS_DENIED means a pipe with the same + // name already exists: another (potentially malicious) process squatted our endpoint. + // Refuse to continue so we never end up attaching to an impersonated pipe. + if (lastError == ERROR_ACCESS_DENIED) + { + pThis->ShowLastError("Failed to create named pipe: the name is already in use (possible squatting); aborting...", lastError); + } + else + { + pThis->ShowLastError("Failed to create named pipe...", lastError); + } + if (pThis->_pLogger != nullptr) { std::stringstream builder; diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/EtwEventsManager.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/EtwEventsManager.cpp index 369d424a4f12..6d98fac234ae 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/EtwEventsManager.cpp +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/EtwEventsManager.cpp @@ -257,16 +257,12 @@ void EtwEventsManager::OnEvent( pThreadInfo->AllocationTickTimestamp = timestamp; pThreadInfo->AllocationKind = pPayload->AllocationKind; - // when events are replayed, no pointer value should be used - // --> ClassID is invalid and should not be used - if (!_agentReplayEndpoint.empty()) - { - pThreadInfo->AllocationClassId = 0; - } - else - { - pThreadInfo->AllocationClassId = pPayload->TypeId; - } + // The TypeId carried by the event is a raw CLR pointer that comes from an untrusted + // source (any local process can connect to the inbound pipe), and it may also be + // stale by the time we process the event. It must never be dereferenced as a ClassID, + // so we never propagate it: the type name string carried by the payload is used instead. + // This also covers the replay case where the ClassId is not valid anymore. + pThreadInfo->AllocationClassId = 0; pThreadInfo->AllocationAmount = pPayload->AllocationAmount64; diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/AllocationsProvider.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/AllocationsProvider.cpp index 39ba338bc662..eebc5a6d4958 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/AllocationsProvider.cpp +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/AllocationsProvider.cpp @@ -333,9 +333,13 @@ void AllocationsProvider::OnAllocation(std::chrono::nanoseconds timestamp, // rawSample.Address = address; rawSample.MethodTable = classId; - // The provided type name contains the metadata-based `xx syntax for generics instead of <> - // So rely on the frame store to get a C#-like representation like what is done for frames - if (!_pFrameStore->GetTypeName(classId, rawSample.AllocationClass)) + // classId is 0 for untrusted sources: for ETW on .NET Framework, any local process can connect + // to the inbound pipe and forge an AllocationTick with an arbitrary TypeId, so EtwEventsManager + // zeroes it and it must never be handed to ICorProfilerInfo here (FrameStore::GetTypeName -> + // IsArrayClass / GetClassIDInfo would dereference it as a CLR pointer). For trusted sources + // (in-process / EventPipe) it is a genuine ClassID and the frame store yields a nicer + // C#-like type name; otherwise we fall back to the transmitted, length-validated type name. + if ((classId == 0) || !_pFrameStore->GetTypeName(classId, rawSample.AllocationClass)) { rawSample.AllocationClass = typeName; }