Strong memory use surge on HTTP request to DicomService about fo-dicom

[amoerie]

So, I looked at this in detail, and I think the problem is clear.
You see, when a DICOM client connects to a DICOM server, the following things happen at the TCP level:

1. A TCP connection is opened
2. A DICOM association request is sent
3. This DICOM association looks like this:

Every DICOM message is encoded as a PDU, a Protocol Data Unit. A PDU always begins with 6 bytes, the first 2 contain the type of PDU ( 0x1 = Association Request, 0x2 = Association Accept, 0x3 = Association Reject, etc.) and the last 4 bytes contain the length of the PDU.
Especially of note is this Length field:

These 4 bytes indicate how long the ensuing PDU will be, and thus how many bytes the DICOM server should expect to read.

You can find the code that reads the first 6 bytes, and then the remaining [PDU Length] bytes into a MemoryStream here in DicomService:

fo-dicom/FO-DICOM.Core/Network/DicomService.cs

Lines 481 to 542 in 5524ebc

	var stream = _network.AsStream();
	// Read common fields of the PDU header. The first 6 bytes contain the type and the length
	_bytesToRead = RawPDU.CommonFieldsLength;
	// This is the (extremely small) buffer we use to read the raw PDU header
	using var rawPduCommonFieldsBuffer = _memoryProvider.Provide(RawPDU.CommonFieldsLength);
	var count = await stream.ReadAsync(rawPduCommonFieldsBuffer.Bytes, 0, rawPduCommonFieldsBuffer.Length).ConfigureAwait(false);
	do
	{
	if (count == 0)
	{
	// disconnected
	Logger.LogDebug("Read 0 bytes from network stream while reading PDU header, connection will be marked as closed");
	await TryCloseConnectionAsync(force: true).ConfigureAwait(false);
	return;
	}
	_bytesToRead -= count;
	if (_bytesToRead > 0)
	{
	count = await stream.ReadAsync(rawPduCommonFieldsBuffer.Bytes, rawPduCommonFieldsBuffer.Length - _bytesToRead, _bytesToRead).ConfigureAwait(false);
	}
	}
	while (_bytesToRead > 0);
	var length = BitConverter.ToInt32(rawPduCommonFieldsBuffer.Bytes, 2);
	length = Endian.Swap(length);
	_bytesToRead = length;
	// Read PDU
	var rawPduLength = length + RawPDU.CommonFieldsLength;
	// This is the buffer that will hold the entire Raw PDU at once
	using var rawPduBuffer = _memoryProvider.Provide(rawPduLength);
	Array.Copy(rawPduCommonFieldsBuffer.Bytes, 0, rawPduBuffer.Bytes, 0, RawPDU.CommonFieldsLength);
	int rawPduOffset = RawPDU.CommonFieldsLength;
	while (_bytesToRead > 0)
	{
	int bytesToRead = Math.Min(_bytesToRead, _maxBytesToRead);
	count = await stream.ReadAsync(rawPduBuffer.Bytes, rawPduOffset, bytesToRead).ConfigureAwait(false);
	if (count == 0)
	{
	// disconnected
	Logger.LogDebug("Read 0 bytes from network stream while reading PDU, connection will be marked as closed");
	await TryCloseConnectionAsync(force: true).ConfigureAwait(false);
	return;
	}
	rawPduOffset += count;
	_bytesToRead -= count;
	}
	using var rawPduStream = new MemoryStream(rawPduBuffer.Bytes, 0, rawPduLength);
	using var raw = new RawPDU(rawPduStream, _memoryProvider);

As you can see, we first read into a 6 bytes sized buffer (the header) and then into a rawPduLength sized byte buffer.

Now, coming back to your issue, you're sending an HTTP GET request to this DICOM server.
Sending an HTTP GET request involves sending the text "GET / HTTP/1.1" (in ASCII encoding if I recall correctly) to an HTTP server.

At the byte level, this is what that HTTP request looks like:

When the DICOM server reads the first 6 bytes of that request, we get the following (hexadecimal) buffer:
[ 0x47, 0x45, 0x54, 0x20, 0x2F, 0x20 ]

Now, the DICOM service reads the PDU type from the first two bytes and the PDU length from the remaining four bytes.
The PDU length is determined as follows:

1. Take the four bytes
   0x54, 0x20, 0x2F, 0x20
2. Interpret them as a C# integer:

01010100 00100000 00101111 00100000
-------  -------  -------   -------
  (0x54)   (0x20)   (0x2F)   (0x20)

The result of this is 1 411 395 360, or 1.4 million. Looks familiar?

When the following line executes, we will request a buffer of size 1.4 million bytes, which amounts to about 1.4GB. :-)

rawPduBuffer = _memoryProvider.Provide(rawPduLength);

So, now that we know what the issue is, what can we do about it. The fact is that we obviously don't support HTTP requests to our DICOM server. So don't do that! :-)
But there are probably things we can do to improve our resilience against random network traffic that does not adhere to the DICOM standard.

First of all, we could exit early if the first 2 bytes of the PDU header do not resolve to a known DICOM PDU type. In this case we probably want to close the connection too.
Secondly, we could add some sort of timeout to the Networkstream reading code that bails out if the promised number of bytes don't arrive in a timely fashion. However, this would be tricky, as we don't know how many applications out there in the wild rely on us not closing a connection if it does not have any activity.

@gofal @mrbean-bremen Any input welcome here. Do you know how other DICOM libraries in other languages handle this?

[amoerie]

I will look into this. Please give me some time.

[danzhik]

Just wow😅 Thank you so much @amoerie for digging out the truth, it was so kind of you to provide such a thorough explanation. I was definitely not expecting an answer, moreover a pull request fixing the problem in less than a day, huge kudos for all this👍 I had a gut feeling myself that the problem occurs due to an incompatible protocol and request data not conforming to PDU format, but ,of course, without in-depth knowledge of the library it would take me X-folds of the time you spent to find the root. Many thanks from our team, you folks are the MVPs of this week for us

[amoerie]

Glad to help, this was fun to figure out!
The fix was rather straight forward too, but it might take some time before it gets merged and released.

And thank you for the kind words, that's always nice to hear 👍

[gofal]

An very interesting issue. I was surprised that the DicomServer is so simply vulnerable to an DOS attack. But the explanation is more than clear. I would have guessed that because the DicomServer will not be able to parse a http request as DICOM Dataset, the connection will fail fast. It is bad luck that it happens that the length value is such a high value and the the DicomServer just waits until this amout of data arrived.

Orthanc handles http request and closes the association fast. DicomObjects on the other hand results in a timeout.
I would vote for the fix to exit early if the first 2 bytes of the PDU header do not resolve to a known DICOM PDU type.

And maybe we could also hardcode a maximum value for the length of an Association Request message? This would also lead to an early exit of an invalid request. And while a timeout solves the problem that the association stays open for long, it would not solve the memory consumption issue. but a max length for the rented array would.

[amoerie]

This is exactly what I thought while viewing your PR. You can always make a hand-crafted DOS attack that way, so I think what you did here is good enough against accidental incorrect traffic.

Securing against too much memory load from legitimate requests is a bit more complicated - restricting the size of data and the number of connections, and allocating memory in chunks as you wrote are certainly ways to go here.

Actually the best way to add security is probably to make use of our recently added TLS support. That way, you basically eliminate bad actors from being able to even contact your DICOM services at all, since they wouldn't get past the TLS handshake.

[amoerie]

It's not just association requests, it's any kind of PDU message.
As for a max size, since the DICOM standard provides 4 bytes for an unsigned number, I think it can go up to 4 GB and still be possibly valid. But I agree, such large values would only be expected with P-DATA-TF messages, so we could maybe enforce some maximum on other types.

I think a more solid approach could be to not allocate (rent) the 1.4GB immediately, but only allocate it in chunks when we actually receive the data.
This is also how MemoryStream works under the hood, even if you specify a large capacity. But the PDU early bailout will already work wonders.

If you talk about security, anyone intimately familiar with the DICOM network stack will always be able to hand craft a network message that allocates a lot. But I do agree that Fellow Oak DICOM should be more resilient against accidental network traffic such as HTTP requests.

[mrbean-bremen]

If you talk about security, anyone intimately familiar with the DICOM network stack will always be able to hand craft a network message that allocates a lot.

This is exactly what I thought while viewing your PR. You can always make a hand-crafted DOS attack that way, so I think what you did here is good enough against accidental incorrect traffic.

Securing against too much memory load from legitimate requests is a bit more complicated - restricting the size of data and the number of connections, and allocating memory in chunks as you wrote are certainly ways to go here.