dotnet / dotnet/diagnostics

Document Diagnostic IPC Header security considerations

Open
#1,893 3 comments 0 reactions 0 assignees View on GitHub
documentation
Dominant language
C++
Stars
1.3k
Forks
404
Avg merge
2d 5h
Merged PRs (30d)
35

Description

The specification for the [Diagnostic IPC Protocol](https://github.com/dotnet/diagnostics/blob/master/documentation/design-docs/ipc-protocol.md) 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?

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.