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 <[email protected]> and Thomas Dement <[email protected]> for the report.
medxchange · Jun 15, 2018 · d022c89 · d022c89
1 parent 29f9de1
commit d022c89
Show file tree

Hide file tree

Showing 7 changed files with 141 additions and 65 deletions.
diff --git 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
@@ -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
@@ -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<DcmIODTypes::Frame*>::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,41 +1116,34 @@ 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
     if (remainder > 0)
     {
       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
@@ -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
@@ -899,13 +899,13 @@ class DCMTK_OFSTD_EXPORT OFStandard
     static OFBool
     safeSubtract(T minuend, T subtrahend, T& difference)
     {
-      assert(!OFnumeric_limits<T>::is_signed);
-      if (minuend < subtrahend) {
-        return OFFalse;
-      } else {
-        difference = minuend - subtrahend;
-        return OFTrue;
-      }
+        assert(!OFnumeric_limits<T>::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<T>::is_signed);
-      if ((OFnumeric_limits<T>::max)() - a < b) {
-        return OFFalse;
-      } else {
-        sum = a + b;
+        assert(!OFnumeric_limits<T>::is_signed);
+        if ((OFnumeric_limits<T>::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 <typename T>
+    static OFBool safeMult(T a, T b, T& product)
+    {
+        assert(!OFnumeric_limits<T>::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
@@ -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
@@ -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<unsigned int>::max());
+
+  // --------------- Multiplication ----------------
+  a = OFnumeric_limits<unsigned int>::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<unsigned int>::max() / 2);
+
+  b = 2; // a still equals max / 2
+  OFCHECK( OFStandard::safeMult(a, b, a) == OFTrue);
+  if ( (OFnumeric_limits<unsigned int>::max() %2 == 1) )
+      OFCHECK_EQUAL(a+1, OFnumeric_limits<unsigned int>::max());
+  else
+      OFCHECK_EQUAL(a, OFnumeric_limits<unsigned int>::max());
 }
 
 OFTEST(ofstd_snprintf)