Document Diagnostic IPC Header security considerations

[dmex]

The specification for the Diagnostic IPC Protocol doesn't mention any security considerations for the IpcHeader structure:

// size = 14 + 2 + 1 + 1 + 2 = 20 bytes
struct IpcHeader
{
    uint8_t[14]  magic = "DOTNET_IPC_V1";
    uint16_t     size;        // size of packet = size of header + payload
    uint8_t      command_set; // combined with command_id is the Command to invoke
    uint8_t      command_id;  // combined with command_set is the Command to invoke
    uint16_t     reserved;    // for potential future use
};

The main issue is with the Size field. For example: https://github.com/dotnet/diagnostics/blob/047b623f5a3b9bc59b5dcd0c05b5ea7f972ca8f3/src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsIpc/IpcMessage.cs#L108-L117

line 114 in particular: reader.ReadBytes(message.Header.Size - IpcHeader.HeaderSizeInBytes);

This is compiled into what is essentially:

ReadFile(
    pipeHandle,
    message,
    sizeof(HDR),
    NULL,
    NULL
    );

payload = malloc(message.Header.Size - sizeof(HDR)); // security issue

ReadFile(
    pipeHandle,
    payload,
    message.Header.Size - sizeof(HDR),
    NULL,
    NULL
    );

Passing the size via the header is unnecessary? sockets return the message size via int size = recv() while windows pipes return the message size when using the PIPE_READMODE_MESSAGE flag but dotnet-diagnostics doesn't use either of these options and attempts to return the size using the structure instead which violates security and creates other problems when parsing IPC messages.

A specially crafted application could exploit the message size and potentially create security issues in any application implementing the IPC protocol - This is especially problematic when the client is running elevated and the dotnet process isn't elevated.

How can we avoid trusting a user supplied variable/untrusted input for IPC message sizes/memory allocation?

[josalem]

Thanks for reaching out about this! I don't believe that the size field poses an inherent security issue. Typical issues that a size field can introduce in any protocol implementation include causing an OOM exception or inducing a form of denial of service attack by forcing the client to allocated too many resources per exchange. The max size for the Diagnostic IPC protocol is ~64kb, so I don't believe we necessarily run the risk of over requesting resources. Client writers that are severely resource constrained can follow similar strategies to HTTP clients and the content-length header, e.g., error on user specified lengths above some threshold below the max. Providing a size in the protocol is not uncommon, e.g., HTTP's content-length or IPv4's total length field. Client creators are allowed to handle this situation however they see fit. If you need it in the future, here is a link to Microsoft's FAQ for security vulnerabilities: https://www.microsoft.com/en-us/msrc/faqs-report-an-issue

[dmex]

Client creators are allowed to handle this situation however they see fit

I was mainly looking for notes in the documentation about how clients can 'clamp' or limit the data. Documentation for protocols generally include a 'security considerations' section somewhere.

For example; Values 1-100 are reserved for future expansion. Values 100-65535 should be considered invalid etc... it's implied in some places but not mentioned.

The max size for the Diagnostic IPC protocol is ~64kb

That's the type of notes that should be included. The type might be ushort with a maximum 0xffff but I wasn't sure if that's what the diagnostics implementation also uses or if its a lower value?

If you need it in the future, here is a link to Microsoft's FAQ for security vulnerabilities

They've never done anything about attacks targeting our project... I've already seen games and some other 'products' from companies doing thing like this where they get the diagnostics pipe handle using reflection and just kill anything opening the pipe:

if (GetNamedPipeClientProcessId(pipeReflectedHandle, &processId))
{
    Process process = Process.GetProcessById(processId);
    process.Kill();
}

I think those types of issues should also be mentioned in a 'security considerations' section of the documentation? The only way I can implement the diagnostics pipe is by using a child process otherwise it's too problematic.

[josalem]

We're looking at the situation you mention in the latter half of your comment already. Thanks for giving us some more evidence of it.

As for adding security concerns to the document, we can add some clarifying phrasing about valid ranges. In general, we used field sizes to imply that, e.g., uint8_t equates to only 256 valid values and we've documented the currently used ones while the rest will error until use them. I'll reclassify this issue as a documentation issue and track that work from here.