Document Diagnostic IPC Header security considerations
- 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.