Re: [Gdcm2] GDCM <2.6.1 two vulnerabilites

[Mathieu M. <mat...@gm...>]

Hi Stelios,

Thanks for your time studying GDCM code base. Both issues were fixed with:

commit e547b1ded3fd21e0b0ad149f13045aa12d4b9b7c
Author: Mathieu Malaterre <mat...@gm...>
Date:   Fri Dec 11 14:35:08 2015 +0100

    Avoid a buffer over run with properly crafted JPEG-LS

    This was reported on the mailing as CVE-2015-8397


and


commit 9cbca25ff7f20c432b61eb9f4cae43a946502b66
Author: Mathieu Malaterre <mat...@gm...>
Date:   Fri Dec 11 14:23:59 2015 +0100

    Fix a case when Region was never initialized

    This was reported on the mailing list under CVE-2015-8396


regards

On Fri, Dec 4, 2015 at 6:08 PM, Stelios Tsampas <st...@ce...> wrote:
> 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.
>
>
> ------------------------------------------------------------------------------
> Go from Idea to Many App Stores Faster with Intel(R) XDK
> Give your users amazing mobile app experiences with Intel(R) XDK.
> Use one codebase in this all-in-one HTML5 development environment.
> Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs.
> http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
> _______________________________________________
> Gdcm-developers mailing list
> Gdc...@li...
> https://lists.sourceforge.net/lists/listinfo/gdcm-developers



-- 
Mathieu