[Profiler] Fix possible namepipe-related issues

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...");

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;

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)
         {
@@ -166,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,
@@ -177,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;

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 <cstddef>
 #include <memory>
 #include <sstream>
 #include <string>
@@ -218,27 +220,54 @@ void EtwEventsManager::OnEvent(
             //    <data name = "Address" inType = "win:Pointer" />
             // 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
-            auto* pPayload = reinterpret_cast<AllocationTickV3Payload const*>(pEventData);
-            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())
+            //
+            // 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<uint32_t>(offsetof(AllocationTickV3Payload, FirstCharInName));
+            if (cbEventData < fixedFieldsSize)
             {
-                pThreadInfo->AllocationClassId = 0;
+                if (_isDebugLogEnabled)
+                {
+                    std::cout << "AllocationTick payload too small (" << cbEventData << " bytes)" << std::endl;
+                }
+
+                pThreadInfo->ClearLastEventId();
+                return;
             }
-            else
+
+            ULONG nameOffset = fixedFieldsSize;
+            const WCHAR* pTypeName = EventsParserHelper::ReadWideString(pEventData, cbEventData, &nameOffset);
+            if (pTypeName == nullptr)
             {
-                pThreadInfo->AllocationClassId = pPayload->TypeId;
+                if (_isDebugLogEnabled)
+                {
+                    std::cout << "AllocationTick type name is not properly terminated" << std::endl;
+                }
+
+                pThreadInfo->ClearLastEventId();
+                return;
             }
 
+            auto* pPayload = reinterpret_cast<AllocationTickV3Payload const*>(pEventData);
+            pThreadInfo->AllocationTickTimestamp = timestamp;
+            pThreadInfo->AllocationKind = pPayload->AllocationKind;
+
+            // 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;
 
             // 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 +331,14 @@ void EtwEventsManager::AttachCallstack(std::vector<uintptr_t>& 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<size_t>(userDataLength) - headerSize) / sizeof(uintptr_t);
+    if (pPayload->FrameCount > maxFrames)
     {
         return;
     }

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;
     }