Heap buffer overflow in itkImportImageContainer

Community
, , ,
1

Hello!

During an internal security assessment of the medical ML pipeline based on Simple-itk we found heap-buffer-overflow in DicomReader.

In the attached file you can find an example of a file that triggers the exception.

example.tar.gz (269.5 KB)

1 Like
2

The image has
(0028,1053) Rescale Slope -1024 and no (0028,1052) Rescale Intercept attribute, is it wrong, should be
(0028,1053) Rescale Slope 1
(0028,1052) Rescale Intercept -1024

Edit:
and, BTW, Pixel Padding Value 65536 is wrong too (left as is)

Edit 2:
There is Pixel Representation 1 (2’s complement, so -1024 may be not required at all or it is wrong too), wait a minute…

Edit 3:
Sorry, there are too many things broken to speak about, this version will open so far HU consistent, i hope

2 Likes
3

@mathieu.malaterre Any insight as to what might be that problem here?

4

IMHO we should add a check in itkGDCMImageIO when copying GDCM buffer to ITK and visa versa that the size of the buffers are exactly the same. Yes, this is an intentional not compliant image trying to exploit the code, but we should add some checks of assumptions.

5

I confirm the issue is deep down in GDCM. I’ll try to provide a complete analysis ASAP, and possible a fix. Thanks for the sample dataset.

1 Like
6

@mihail.isakov I like your analysis ! Here is mine. The following attribute does not make much sense indeed:

(0028,1053) DS [-1024 ] # 6,1 Rescale Slope

If you look carefully a second attribute is actually send in the DataSet:

(0028,1053) DS [1 ] # 6,1 Rescale Slope

Since both GDCM & DCMTK uses the same convention: preserve the first one, the second one is actually never seen. So indeed the bug is indeed coming from the vendor machine, which send a Rescale Slope attribute in place of a Rescale Intercept.

As a side note, in this particular DataSet, the PixelData is already in the correct unit. So I believe the correct Attributes are:

 (0028,1052) DS [0 ]                                           # 6,1 Rescale Intercept
 (0028,1053) DS [1 ]                                           # 6,1 Rescale Slope

For reference here is a truncated output of dcdump (dicom3tools):

[...]
|(0x0028,0x1050) DS Window Center | VR=<DS>   VL=<0x0004>  <-400> |
|(0x0028,0x1051) DS Window Width | VR=<DS>   VL=<0x0004>  <1500> |
|(0x0028,0x1053) DS Rescale Slope | VR=<DS>   VL=<0x0002>  <1 > |
|(0x0028,0x1053) DS Rescale Slope | VR=<DS>   VL=<0x0006>  <-1024 > |
|(0x7fe0,0x0010) OX Pixel Data | VR=<OW>   VL=<0x80000>|
[...]

(I do not know why the attributes are being printed in opposite order of the file order)

In any cases, GDCM codebase should not trigger a heap buffer overflow. I’ll add a guard against this case.

3 Likes
7

(0x0028,0x1053) DS Rescale Slope | VR= VL=<0x0002> <1 >
(0x0028,0x1053) DS Rescale Slope | VR= VL=<0x0006> <-1024 >

Great! BTW, i wish the same behavior in my GUI metadata viewer too.
Tried to test with

-typedef std::set<DataElement> DataElementSet;
+typedef std::multiset<DataElement> DataElementSet;

metadata
metadata787×107 32.9 KB

seems to work (it is not a request, just playing and testing). May be a warning in release mode were great, it happens here.

I do not know why the attributes are being printed in opposite order of the file order

Some kind of sorting, probably, multimap or whatever sorting things around, just guessing.

Thank you.

1 Like
8

A fix was commit via this PR:

2 Likes