From d022c8905360e96a0360b35169e45471e22f0cb5 Mon Sep 17 00:00:00 2001 From: Michael Onken Date: Fri, 15 Jun 2018 17:46:06 +0200 Subject: [PATCH] Fix possible pixel data size int overflows. Fix possible overflows that can happen when computing number of bytes required for pixel data by making sure data types are large in enough for computations and by checking whether related multiplications are safe. Also make sure that the dcmdata API is not accepting values that are larger than 4 GB. Thanks to Sahab Zanjanizadeh and Thomas Dement for the report. --- dcmdata/libsrc/dcvrpobw.cc | 16 ++++++ dcmseg/include/dcmtk/dcmseg/segdoc.h | 38 ++++++-------- dcmseg/libsrc/segdoc.cc | 78 ++++++++++++++++++---------- dcmseg/libsrc/segutils.cc | 9 +++- ofstd/include/dcmtk/ofstd/ofstd.h | 47 ++++++++++++----- ofstd/tests/tests.cc | 2 +- ofstd/tests/tofstd.cc | 16 +++++- 7 files changed, 141 insertions(+), 65 deletions(-) diff --git a/dcmdata/libsrc/dcvrpobw.cc b/dcmdata/libsrc/dcvrpobw.cc index 6ccce57ab8..151b9d7713 100644 --- a/dcmdata/libsrc/dcvrpobw.cc +++ b/dcmdata/libsrc/dcvrpobw.cc @@ -211,6 +211,14 @@ DcmPolymorphOBOW::putUint8Array( { if (byteValue) { + // Check if more than 4 GB is requested, which is the maximum + // length DICOM can handle. Take into account that the alignValue() + // call adds a byte if an odd length is provided, thus, 4294967295 + // would not work. + if (numBytes > 4294967294) + { + return EC_TooManyBytesRequested; + } errorFlag = putValue(byteValue, OFstatic_cast(Uint32, sizeof(Uint8) * OFstatic_cast(size_t, numBytes))); if (errorFlag == EC_Normal) { @@ -241,6 +249,14 @@ DcmPolymorphOBOW::putUint16Array( { if (wordValue) { + // Check whether input would lead to a buffer allocation of more than + // 4 GB for a value, which is not possible in DICOM. The biggest input + // parameter value permitted is 2147483647, since 2147483647*2 is still + // < 2^32-1 (4 GB). + if (numWords > 2147483647) + { + return EC_TooManyBytesRequested; + } errorFlag = putValue(wordValue, OFstatic_cast(Uint32, sizeof(Uint16) * OFstatic_cast(size_t, numWords))); if (errorFlag == EC_Normal && getTag().getEVR() == EVR_OB && getByteOrder() == EBO_BigEndian) diff --git a/dcmseg/include/dcmtk/dcmseg/segdoc.h b/dcmseg/include/dcmtk/dcmseg/segdoc.h index 64d176e04c..608f0d76f9 100644 --- a/dcmseg/include/dcmtk/dcmseg/segdoc.h +++ b/dcmseg/include/dcmtk/dcmseg/segdoc.h @@ -503,11 +503,12 @@ class DCMTK_DCMSEG_EXPORT DcmSegmentation * @param bitsPerFrame The number of bits per frame (usually rows * columns) * @param results The resulting frames. Memory for the frames is allocated * by the method, so the Vector can/should be empty before calling. + * @result Return EC_Normal on success, error otherwise */ - virtual void extractFrames(Uint8* pixData, - const size_t numFrames, - const size_t bitsPerFrame, - OFVector< DcmIODTypes::Frame* >& results); + virtual OFCondition extractFrames(Uint8* pixData, + const size_t numFrames, + const size_t bitsPerFrame, + OFVector< DcmIODTypes::Frame* >& results); /** This is the counterpart to the extractFrames() function. It takes a number * of frames that are in binary segmentation format (i.e. "bit-packed") and @@ -619,27 +620,22 @@ class DCMTK_DCMSEG_EXPORT DcmSegmentation const OFString& filename, DcmDataset*& dset); - /** Returns the number of bits per frame, taking into account binary versus - * fractional segmentation (member variables) and the dimensions of the - * image (parameters) - * @param rows The number of rows returned - * @param cols The number of columns returned - * @return Bits used by a single frame of the segmentation - */ - size_t getBitsPerFrame(const Uint16& rows, - const Uint16& cols); - - /** Returns the number of total bytes required for the frame data of this + /** Computes the number of total bytes required for the frame data of this * segmentation object. Takes into account dimensions and number of frames, - * as well as the type of segmentation (member variables). + * as well as the type of segmentation (member variables). May file if + * size_t type is not able to hold the result of intermediate computations. * @param rows Number of rows of a frame * @param cols Number of cols of a frame * @param numberOfFrames The number of frames of the object - * @return Number of bytes used by all frames of this segmentation - */ - size_t getTotalBytesRequired(const Uint16& rows, - const Uint16& cols, - const Uint16& numberOfFrames); + * @param bytesRequired Will hold the result of the computation, + * if successful. Does not any padding into account. + * @return EC_Normal if computation was possible, EC_TooManyBytesRequested + * otherwise. + */ + OFCondition getTotalBytesRequired(const Uint16& rows, + const Uint16& cols, + const Uint16& numberOfFrames, + size_t& bytesRequired); /** Read Fractional Type of segmentation. * @param item The item to read from diff --git a/dcmseg/libsrc/segdoc.cc b/dcmseg/libsrc/segdoc.cc index 00f7d55a0e..3871ca4104 100644 --- a/dcmseg/libsrc/segdoc.cc +++ b/dcmseg/libsrc/segdoc.cc @@ -753,18 +753,28 @@ OFCondition DcmSegmentation::readFrames(DcmItem& dataset) } /* Read all frames into dedicated data structure */ - size_t pixelsPerFrame = rows * cols; + size_t pixelsPerFrame = OFstatic_cast(size_t, rows) * cols; if (m_SegmentationType == DcmSegTypes::ST_BINARY) { - extractFrames(pixels, numberOfFrames, pixelsPerFrame, m_Frames); + result = extractFrames(pixels, numberOfFrames, pixelsPerFrame, m_Frames); + if (result.bad()) + { + return result; + } } else if (m_SegmentationType == DcmSegTypes::ST_FRACTIONAL) { for (size_t count = 0; count < numberOfFrames; count++) { DcmIODTypes::Frame *frame = new DcmIODTypes::Frame(); + if (!frame) return EC_MemoryExhausted; frame->length = pixelsPerFrame; frame->pixData= new Uint8[pixelsPerFrame]; + if (!frame->pixData) + { + delete frame; + return EC_MemoryExhausted; + } memcpy(frame->pixData, pixels + count* pixelsPerFrame, pixelsPerFrame); m_Frames.push_back(frame); } @@ -915,7 +925,14 @@ OFCondition DcmSegmentation::writeFractionalFrames(DcmItem& dataset) rows = cols = 0; getImagePixel().getRows(rows); getImagePixel().getColumns(cols); - size_t numBytes = getTotalBytesRequired(rows, cols, numFrames); + size_t numBytes = 0; + result = getTotalBytesRequired(rows, cols, numFrames, numBytes); + if (result.bad()) return result; + if (numBytes >= 4294967295) + { + DCMSEG_ERROR("Cannot store Segmentation objects with more than 4 GB pixel data (compression for writing not supported)"); + return EC_TooManyBytesRequested; + } Uint8* pixdata = new Uint8[numBytes]; OFVector::iterator it = m_Frames.begin(); // Just copy bytes for each frame as is @@ -938,7 +955,14 @@ OFCondition DcmSegmentation::writeBinaryFrames(DcmItem& dataset) OFCondition result; getImagePixel().getRows(rows); getImagePixel().getColumns(cols); - size_t numBytes = getTotalBytesRequired(rows, cols, numFrames); + size_t numBytes = 0; + result = getTotalBytesRequired(rows, cols, numFrames, numBytes); + if (result.bad()) return result; + if (numBytes >= 4294967295) + { + DCMSEG_ERROR("Cannot store Segmentation objects with more than 4 GB pixel data (compression for writing not supported)"); + return EC_TooManyBytesRequested; + } // Holds the pixels for all frames. Each bit represents a pixel which is either // 1 (part of segment) or 0 (not part of segment. All frames are directly // concatenated, i.e. there are no unused bits between the frames. @@ -1068,7 +1092,9 @@ OFBool DcmSegmentation::checkPixDataLength(DcmElement* pixelData, size_t length = pixelData->getLengthField(); // Find out how many bytes are needed - size_t bytesRequired = getTotalBytesRequired(rows, cols, numberOfFrames); + size_t bytesRequired = 0; + OFCondition result = getTotalBytesRequired(rows, cols, numberOfFrames, bytesRequired); + if (result.bad()) return OFFalse; // Length found in Pixel Data element is always even if (bytesRequired % 2 == 1) bytesRequired++; /* Compare expected and actual length */ @@ -1090,31 +1116,25 @@ OFBool DcmSegmentation::checkPixDataLength(DcmElement* pixelData, } -size_t DcmSegmentation::getBitsPerFrame(const Uint16& rows, - const Uint16& cols) +OFCondition DcmSegmentation::getTotalBytesRequired(const Uint16& rows, + const Uint16& cols, + const Uint16& numberOfFrames, + size_t& bytesRequired) { - size_t bitsRequired = 0; - bitsRequired = rows * cols; - /* For fractional segmentations we need 1 byte instead of 1 bit for a single pixel */ - if (m_SegmentationType == DcmSegTypes::ST_FRACTIONAL) + OFBool ok = OFStandard::safeMult(OFstatic_cast(size_t, rows), OFstatic_cast(size_t, cols), bytesRequired); + if (ok) OFStandard::safeMult(bytesRequired, OFstatic_cast(size_t, numberOfFrames), bytesRequired); + if (!ok) { - bitsRequired *= 8; + DCMSEG_ERROR("Cannot compute number of bytes required for Pixel Data since size_t type is too small"); + return EC_TooManyBytesRequested; } - return bitsRequired; -} - -size_t DcmSegmentation::getTotalBytesRequired(const Uint16& rows, - const Uint16& cols, - const Uint16& numberOfFrames) -{ - size_t bytesRequired = rows * cols * numberOfFrames; /* for binary, we only need one bit per pixel */ size_t remainder = 0; if (m_SegmentationType == DcmSegTypes::ST_BINARY) { // check whether the 1-bit pixels exactly fit into bytes - remainder = (rows * cols) % 8; + remainder = (OFstatic_cast(size_t, rows) * cols) % 8; // number of bytes that work on an exact fit bytesRequired = bytesRequired / 8; // add one byte if we have a remainder @@ -1122,9 +1142,8 @@ size_t DcmSegmentation::getTotalBytesRequired(const Uint16& rows, { bytesRequired++; } - } - return bytesRequired; + return EC_Normal; } @@ -1324,10 +1343,10 @@ OFCondition DcmSegmentation::decompress(DcmDataset& dset) } -void DcmSegmentation::extractFrames(Uint8* pixData, - const size_t numFrames, - const size_t bitsPerFrame, - OFVector< DcmIODTypes::Frame* >& results) +OFCondition DcmSegmentation::extractFrames(Uint8* pixData, + const size_t numFrames, + const size_t bitsPerFrame, + OFVector< DcmIODTypes::Frame* >& results) { // Will hold the bit position (0-7) that the current frame starts from. The // first frame will always start at bit 0. @@ -1349,6 +1368,10 @@ void DcmSegmentation::extractFrames(Uint8* pixData, DcmIODTypes::Frame* frame = new DcmIODTypes::Frame(); frame->length = frameLengthBytes; frame->pixData = new Uint8[frameLengthBytes]; + if (!frame->pixData) + { + return EC_MemoryExhausted; + } memcpy(frame->pixData, readPos, frame->length); // If we have been copying too much, i.e the first bits of the frame // actually belong to the former frame, shift the whole frame this amount @@ -1377,6 +1400,7 @@ void DcmSegmentation::extractFrames(Uint8* pixData, readPos = readPos + frame->length; } } + return EC_Normal; } diff --git a/dcmseg/libsrc/segutils.cc b/dcmseg/libsrc/segutils.cc index 8a35591964..b2b647c480 100644 --- a/dcmseg/libsrc/segutils.cc +++ b/dcmseg/libsrc/segutils.cc @@ -29,7 +29,7 @@ DcmIODTypes::Frame* DcmSegUtils::packBinaryFrame(const Uint8* pixelData, const Uint16 columns) { // Sanity checking - const size_t numPixels = rows*columns; + const size_t numPixels = OFstatic_cast(size_t, rows) * columns; if (numPixels == 0) { DCMSEG_ERROR("Unable to pack binary segmentation frame: Rows or Columns is 0"); @@ -78,11 +78,16 @@ DcmIODTypes::Frame* DcmSegUtils::unpackBinaryFrame(const DcmIODTypes::Frame* fra } // Create result frame in memory - size_t numBits = rows * cols; + size_t numBits = OFstatic_cast(size_t, rows) * cols; DcmIODTypes::Frame* result = new DcmIODTypes::Frame(); if (result) { result->pixData = new Uint8[numBits]; + if (!result->pixData) + { + delete result; + return NULL; + } result->length = numBits; } if ( !result || !(result->pixData) ) diff --git a/ofstd/include/dcmtk/ofstd/ofstd.h b/ofstd/include/dcmtk/ofstd/ofstd.h index 369d7eca26..3a6b08804a 100644 --- a/ofstd/include/dcmtk/ofstd/ofstd.h +++ b/ofstd/include/dcmtk/ofstd/ofstd.h @@ -899,13 +899,13 @@ class DCMTK_OFSTD_EXPORT OFStandard static OFBool safeSubtract(T minuend, T subtrahend, T& difference) { - assert(!OFnumeric_limits::is_signed); - if (minuend < subtrahend) { - return OFFalse; - } else { - difference = minuend - subtrahend; - return OFTrue; - } + assert(!OFnumeric_limits::is_signed); + if (minuend < subtrahend) { + return OFFalse; + } else { + difference = minuend - subtrahend; + return OFTrue; + } } /** check whether addition is safe (i.e.\ no overflow occurs) and if so, @@ -921,13 +921,34 @@ class DCMTK_OFSTD_EXPORT OFStandard static OFBool safeAdd(T a, T b, T& sum) { - assert(!OFnumeric_limits::is_signed); - if ((OFnumeric_limits::max)() - a < b) { - return OFFalse; - } else { - sum = a + b; + assert(!OFnumeric_limits::is_signed); + if ((OFnumeric_limits::max)() - a < b) { + return OFFalse; + } else { + sum = a + b; + return OFTrue; + } + } + + /** check whether multiplication is safe (i.e.\ no overflow occurs) and if so, + * perform it (i.e.\ compute a*b=product). Only works for unsigned types. + * @param a first number to multiply + * @param b second number to multiply + * @param product resulting product of both numbers, if multiplication is + * safe, otherwise parameter value is not touched by the function + * @return OFTrue if multiplication is safe and could be performed, OFFalse + * otherwise + */ + template + static OFBool safeMult(T a, T b, T& product) + { + assert(!OFnumeric_limits::is_signed); + T x = a * b; + if (a != 0 && x / a != b) { + return OFFalse; + } + product = x; return OFTrue; - } } #ifdef DOXYGEN diff --git a/ofstd/tests/tests.cc b/ofstd/tests/tests.cc index d6671b8e12..a2e8f9d8cc 100644 --- a/ofstd/tests/tests.cc +++ b/ofstd/tests/tests.cc @@ -77,7 +77,7 @@ OFTEST_REGISTER(ofstd_memory); OFTEST_REGISTER(ofstd_optional); OFTEST_REGISTER(ofstd_tuple); OFTEST_REGISTER(ofstd_limits); -OFTEST_REGISTER(ofstd_safeSubtractAndAdd); +OFTEST_REGISTER(ofstd_safeSubtractAddMult); OFTEST_REGISTER(ofstd_variant); OFTEST_REGISTER(ofstd_error); OFTEST_REGISTER(ofstd_filesystem); diff --git a/ofstd/tests/tofstd.cc b/ofstd/tests/tofstd.cc index 33eef3fa6d..96ef044560 100644 --- a/ofstd/tests/tofstd.cc +++ b/ofstd/tests/tofstd.cc @@ -257,7 +257,7 @@ OFTEST(ofstd_OFStandard_removeRootDirFromPathname) OFCHECK(OFStandard::removeRootDirFromPathname(result, nullPtr, nullPtr).good()); } -OFTEST(ofstd_safeSubtractAndAdd) +OFTEST(ofstd_safeSubtractAddMult) { // --------------- Subtraction ---------------- @@ -291,6 +291,20 @@ OFTEST(ofstd_safeSubtractAndAdd) // dividing and then multiplying by 2 is required since max may be an // odd number so that max/2 is rounded to the floor number. OFCHECK_EQUAL(a, OFnumeric_limits::max()); + + // --------------- Multiplication ---------------- + a = OFnumeric_limits::max() / 2; + // check whether overflow occurred (it should) + OFCHECK( OFStandard::safeMult(a, OFstatic_cast(unsigned int, 3), a) == OFFalse); + // check whether no overflow occurred (it shouldn't) + OFCHECK_EQUAL(a, OFnumeric_limits::max() / 2); + + b = 2; // a still equals max / 2 + OFCHECK( OFStandard::safeMult(a, b, a) == OFTrue); + if ( (OFnumeric_limits::max() %2 == 1) ) + OFCHECK_EQUAL(a+1, OFnumeric_limits::max()); + else + OFCHECK_EQUAL(a, OFnumeric_limits::max()); } OFTEST(ofstd_snprintf)