[Gdcm2] GDCM <2.6.1 two vulnerabilites

[Stelios T. <st...@ce...>]

Hello again,

Here are the two issues:


i) GDCM integer overflow

GDCM versions 2.6.0 and  2.6.1 (and possibly previous versions) are
prone to an
integer overflow vulnerability which leads to a buffer overflow and
potentially
to remote code execution. The vulnerability is triggered by the exposed
function
gdcm::ImageRegionReader::ReadIntoBuffer(char *buffer, size_t buflen), which
processes a DICOM file and dumps its contents in the output buffer,
`buffer',
if its length, `buflen', is sufficient. In file gdcmImageRegionReader.cxx:

445  bool ImageRegionReader::ReadIntoBuffer(char *buffer, size_t buflen)
446  {
447    size_t thelen = ComputeBufferLength();
448    if( buflen < thelen )
449      {
450      gdcmDebugMacro( "buffer cannot be smaller than computed buffer
length" );
451      return false;
451      }
452    assert( Internals->GetFileOffset() != std::streampos(-1) );
453    gdcmDebugMacro( "Using FileOffset: " << Internals->GetFileOffset() );
454    std::istream* theStream = GetStreamPtr();
455    theStream->seekg( Internals->GetFileOffset() );
456
457    bool success = false;
458    if( !success ) success = ReadRAWIntoBuffer(buffer, buflen);
459    if( !success ) success = ReadRLEIntoBuffer(buffer, buflen);
460    if( !success ) success = ReadJPEGIntoBuffer(buffer, buflen);
461    if( !success ) success = ReadJPEGLSIntoBuffer(buffer, buflen);
462    if( !success ) success = ReadJPEG2000IntoBuffer(buffer, buflen);
463
464    return success;
465  }

ReadIntoBuffer() first computes the required buffer length at line 447 by
calling ComputeBufferLength() and then compares the return value to the
supplied
length. In the case where the supplied buffer is deemed large enough,
ReadIntoBuffer() will sequentially try to decode the image using every
supported
codec.

ComputeBufferLength() will call BoxRegion::Area() and this is where the
integer
overflow takes place. File gdcmBoxRegion.cxx, line 85:

85  size_t BoxRegion::Area() const
86  {
87    return (Internals->YMax - Internals->YMin + 1)*
88           (Internals->XMax - Internals->XMin + 1)*
89           (Internals->ZMax - Internals->ZMin + 1);
90  }

The variables represent the dimensions of the image's region to be
dumped, as
set via a call to gdcm::ImageRegionReader::SetRegion(). In the most
common case
the region covers the entire image and is therefore controlled by the input
file's DICOM headers, where the dimensions are specified. Specially crafted
dimensions can cause the multiplication to wrap around zero resulting into
a return value that does not reflect the real size requirements.

The return value is eventually saved in variable `thelen' and then used
in the
length check. As long as the check is passed, all of the decoding functions
will assume that the input buffer is large enough so they will copy the
image's
data into it without further checks, resulting in a buffer overflow.

It is advisable to implement a length calculation mechanism that allows the
caller to know whether an overflow has occurred so that the caller
(ReadIntoBuffer() in our case) may exit immediately with an error.

CVE-2015-8396 has been assigned to this bug.

It is advisable to implement a length calculation mechanism
that allows the caller to know whether an overflow has occured
so that the caller (ReadIntoBuffer() in our case) may exit
immediately with an error.


ii) GDCM out-of-bounds read

GDCM versions 2.6.0 and 2.6.1 (and possibly previous versions) are prone
ta a
read vulnerability due to missing checks. The vulnerability occurs
during the
decoding of JPEG-LS images when the dimensions of the embedded JPEG-LS
image
according to its headers are smaller than the ones specified by
gdcm::ImageRegionReader::SetRegion(), which are based on DICOM header
values.

The faulty operation can be found twice in function
JPEGLSCodec::DecodeExtent(),
file gdcmJPEGLSCodec.cxx, lines 463 (for two-dimensional DICOM images)
and 518
(for three-dimensional DICOM images). Both cases are similar.

[448]    std::vector <unsigned char> outv;
[449]    bool b = DecodeByStreamsCommon(dummy_buffer, buf_size, outv);
[450]    if( !b ) return false;
[451]
[452]    unsigned char *raw = &outv[0];
[453]    const unsigned int rowsize = xmax - xmin + 1;
[454]    const unsigned int colsize = ymax - ymin + 1;
[455]    const unsigned int bytesPerPixel = pf.GetPixelSize();
[456]
[457]    const unsigned char *tmpBuffer1 = raw;
[458]    unsigned int z = 0;
[459]    for (unsigned int y = ymin; y <= ymax; ++y)
[460]      {
[461]      size_t theOffset = 0 + (z*dimensions[1]*dimensions[0] +
y*dimensions[0] + xmin)*bytesPerPixel;
[462]      tmpBuffer1 = raw + theOffset;
[463]      memcpy(&(buffer[((z-zmin)*rowsize*colsize +
[464]            (y-ymin)*rowsize)*bytesPerPixel]),
[465]        tmpBuffer1, rowsize*bytesPerPixel);
[466]      }

For each iteration in the "for" loop, memcpy() will copy
`rowsize*bytesPerPixel'
bytes to `buffer' to `raw + theOffset', with `theOffset' increasing each
time.
The problem is that the number of bytes to be copied is based solely on the
size of the output buffer, not taking into account the fact that the source
buffer `raw' (assigned to `outv' at line 452) may be made smaller by an
attacker
via the image's JPEG-LS headers.

This is an information disclosure issue. An attacker could potentially leak
parts of the application's memory. It is advised that the copying operation
addresses the case where there is a mismatch between the DICOM headers
and the
JPEG-LS image's.

CVE-2015-8397 has been assigned to this bug.


We will not proceed to public advisories until a security update is made
available. Let us know if you need any further information regarding the two
findings.


Stelios Tsampas
IT Security Researcher
CENSUS S.A.