Mercurial > hg > orthanc

changeset 6658:d0a3f15d2793 machine-spirits

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
integration mainline->machine-spirits
author Sebastien Jodogne <s.jodogne@gmail.com>
date Fri, 20 Mar 2026 17:09:51 +0100
parents db756d9176cc (diff) ab12547ac3df (current diff)
children 81d6130cf13a
files
diffstat 10 files changed, 236 insertions(+), 74 deletions(-) [+]
line wrap: on
line diff
--- a/NEWS	Fri Mar 20 17:05:49 2026 +0100
+++ b/NEWS	Fri Mar 20 17:09:51 2026 +0100
@@ -56,6 +56,13 @@
 * Save the jobs registry in DB only if it has changed.
   https://discourse.orthanc-server.org/t/frequent-idle-messages-between-postgres-and-orthanc/6406
 * New CMake option: "USE_SYSTEM_MINIZIP" to use the system-wide version of minizip
+* Security fixes courtesy of Machine Spirits UG:
+  - Fix possible out-of-bound access when calling /tools/create-dicom with a PAM file.
+  - Fix possible out-of-bound access when rows/columns DICOM tags exceed the maximum value for a US (65535).
+  - Fix possible out-of-bound access in PMSCT_RLE1 encoded images.
+  - Fix possible out-of-bound access in palette images.
+  - Fix possible out-of-bound access when reading a DICOM file with invalid group length tag.
+  - Fix possible memory exhaustion via very large "Content-Length"
 * Upgraded dependencies for static builds:
   - boost 1.89.0
   - dcmtk 3.7.0
--- a/OrthancFramework/Sources/DicomFormat/DicomImageInformation.cpp	Fri Mar 20 17:05:49 2026 +0100
+++ b/OrthancFramework/Sources/DicomFormat/DicomImageInformation.cpp	Fri Mar 20 17:09:51 2026 +0100
@@ -42,6 +42,8 @@
 #include <stdio.h>
 #include <memory>
 
+static const uint64_t MAX_FRAME_SIZE = 4ul * 1024ul * 1024ul * 1024ul;  // defensive approach: set a reasonable max size for a frame
+
 namespace Orthanc
 {
   DicomImageInformation::DicomImageInformation(const DicomMap& values)
@@ -130,6 +132,11 @@
       values.GetValue(DICOM_TAG_COLUMNS).ParseFirstUnsignedInteger(width_); // in some US images, we've seen tag values of "800\0"; that's why we parse the 'first' value
       values.GetValue(DICOM_TAG_ROWS).ParseFirstUnsignedInteger(height_);
 
+      if (height_ > 65535 || width_ > 65535)
+      {
+        throw OrthancException(ErrorCode_BadFileFormat, "Image width or height exceed DICOM VR US range (65535)");
+      }
+
       if (!values.ParseUnsignedInteger32(bitsAllocated_, DICOM_TAG_BITS_ALLOCATED))
       {
         throw OrthancException(ErrorCode_BadFileFormat);
@@ -454,13 +461,15 @@
 
   size_t DicomImageInformation::GetFrameSize() const
   {
+    uint64_t totalFrameSize;
+
     if (bitsStored_ == 1)
     {
       assert(GetWidth() % 8 == 0);
       
       if (GetChannelCount() == 1)
       {
-        return GetHeight() * GetWidth() / 8;
+        totalFrameSize = static_cast<uint64_t>(GetHeight()) * static_cast<uint64_t>(GetWidth()) / 8;
       }
       else
       {
@@ -470,10 +479,20 @@
     }
     else
     {
-      return (GetHeight() *
-              GetWidth() *
-              GetBytesPerValue() *
-              GetChannelCount());
+      totalFrameSize = (static_cast<uint64_t>(GetHeight()) *
+                        static_cast<uint64_t>(GetWidth()) *
+                        static_cast<uint64_t>(GetBytesPerValue()) *
+                        static_cast<uint64_t>(GetChannelCount()));
+    }
+
+    if (totalFrameSize > MAX_FRAME_SIZE ||
+        static_cast<uint64_t>(static_cast<size_t>(totalFrameSize)) != totalFrameSize)
+    {
+      throw OrthancException(ErrorCode_BadFileFormat, "DICOM Frame size overflow");
+    }
+    else
+    {
+      return static_cast<size_t>(totalFrameSize);
     }
   }
 
--- a/OrthancFramework/Sources/DicomFormat/DicomIntegerPixelAccessor.cpp	Fri Mar 20 17:05:49 2026 +0100
+++ b/OrthancFramework/Sources/DicomFormat/DicomIntegerPixelAccessor.cpp	Fri Mar 20 17:09:51 2026 +0100
@@ -49,7 +49,7 @@
         information_.GetBitsStored() >= 32)
     {
       // Not available, as the accessor internally uses int32_t values
-      throw OrthancException(ErrorCode_NotImplemented);
+      throw OrthancException(ErrorCode_NotImplemented, "No support for BitsAllocated > 32 or BitsStored >= 32");
     }
 
     frame_ = 0;
@@ -57,7 +57,7 @@
 
     if (information_.GetNumberOfFrames() * frameOffset_ > size)
     {
-      throw OrthancException(ErrorCode_BadFileFormat);
+      throw OrthancException(ErrorCode_BadFileFormat, "Invalid size");
     }
 
     if (information_.IsSigned())
--- a/OrthancFramework/Sources/DicomFormat/DicomStreamReader.cpp	Fri Mar 20 17:05:49 2026 +0100
+++ b/OrthancFramework/Sources/DicomFormat/DicomStreamReader.cpp	Fri Mar 20 17:09:51 2026 +0100
@@ -204,6 +204,11 @@
       {
         uint16_t length = ReadUnsignedInteger16(p + pos + 6, true);
 
+        if (pos + 8 + length > block.size())
+        {
+          throw OrthancException(ErrorCode_BadFileFormat, "DICOM meta-header tag length exceeds available data");
+        }
+
         std::string value;
         value.assign(p + pos + 8, length);
         NormalizeValue(value, vr);
@@ -237,6 +242,11 @@
           
         uint32_t length = ReadUnsignedInteger32(p + pos + 8, true);
 
+        if (pos + 12 + static_cast<size_t>(length) > block.size())
+        {
+          throw OrthancException(ErrorCode_BadFileFormat, "DICOM meta-header tag length exceeds available data");
+        }
+
         if (tag.GetGroup() == 0x0002)
         {
           std::string value;
--- a/OrthancFramework/Sources/DicomParsing/Internals/DicomImageDecoder.cpp	Fri Mar 20 17:05:49 2026 +0100
+++ b/OrthancFramework/Sources/DicomParsing/Internals/DicomImageDecoder.cpp	Fri Mar 20 17:09:51 2026 +0100
@@ -192,6 +192,11 @@
     {
       if (inbuffer[i] == 0xa5)
       {
+        if (i + 2 >= length)
+        {
+          throw OrthancException(ErrorCode_BadFileFormat, "Truncated PMSCT_RLE1 escape sequence");
+        }
+
         temp.push_back(inbuffer[i+2]);
         for (uint8_t repeat = inbuffer[i + 1]; repeat != 0; repeat--)
         {
@@ -215,6 +220,11 @@
 
       if (temp[i] == 0x5a)
       {
+        if (i + 2 >= temp.size())
+        {
+          throw OrthancException(ErrorCode_BadFileFormat, "Truncated PMSCT_RLE1 delta sequence");
+        }
+
         uint16_t v1 = temp[i + 1];
         uint16_t v2 = temp[i + 2];
         value = (v2 << 8) + v1;
@@ -460,9 +470,12 @@
           throw OrthancException(ErrorCode_NotImplemented, std::string("Palette Color Lookup Table Descriptor invalid palette size: '") + r.c_str() + "'");
         }
 
-        if (pixelLength != target->GetWidth() * target->GetHeight())
+        const uint64_t expectedSize = (static_cast<uint64_t>(target->GetWidth()) *
+                                       static_cast<uint64_t>(target->GetHeight()));
+        
+        if (pixelLength != expectedSize)
         {
-          DcmElement *elem;
+          DcmElement *elem = NULL;
           Uint16 bitsAllocated = 0;
 
           if (!dataset.findAndGetUint16(DCM_BitsAllocated, bitsAllocated).good())
@@ -470,7 +483,8 @@
             throw OrthancException(ErrorCode_NotImplemented);  
           }
 
-          if (!dataset.findAndGetElement(DCM_PixelData, elem).good())
+          if (!dataset.findAndGetElement(DCM_PixelData, elem).good() ||
+              elem == NULL)
           {
             throw OrthancException(ErrorCode_NotImplemented);  
           }
@@ -478,9 +492,11 @@
           // In implicit VR files, pixelLength is expressed in words (OW) although pixels can actually be 8 bits
           // -> pixelLength is wrong by a factor of two and the image can still be decoded!
           // seen in some Philips ClearVue 650 images (using 8 bits LUT)
-          if (!(elem->getVR() == EVR_OW && bitsAllocated == 8 && (2*pixelLength == target->GetWidth() * target->GetHeight())))  
+          if (elem->getVR() != EVR_OW ||
+              bitsAllocated != 8 ||
+              2 * pixelLength != expectedSize)
           {
-            throw OrthancException(ErrorCode_NotImplemented);
+            throw OrthancException(ErrorCode_BadFileFormat, "Invalid size");
           }
         }
 
@@ -494,6 +510,11 @@
 
           for (unsigned int x = 0; x < width; x++)
           {
+            if (*source >= paletteSize)
+            {
+              throw OrthancException(ErrorCode_BadFileFormat, "Pixel value exceeds palette");
+            }
+
             p[0] = lutRed[*source] >> offsetBits;
             p[1] = lutGreen[*source] >> offsetBits;
             p[2] = lutBlue[*source] >> offsetBits;
--- a/OrthancFramework/Sources/HttpServer/HttpServer.cpp	Fri Mar 20 17:05:49 2026 +0100
+++ b/OrthancFramework/Sources/HttpServer/HttpServer.cpp	Fri Mar 20 17:09:51 2026 +0100
@@ -185,7 +185,8 @@
       PostDataStatus_Success,
       PostDataStatus_NoLength,
       PostDataStatus_Pending,
-      PostDataStatus_Failure
+      PostDataStatus_Failure,
+      PostDataStatus_RequestEntityTooLarge  // New in Orthanc 1.12.11
     };
   }
 
@@ -491,57 +492,19 @@
     reader.AddChunk(body);        
     reader.CloseStream();
   }
-  
 
-  static PostDataStatus ReadBodyWithContentLength(std::string& body,
-                                                  struct mg_connection *connection,
-                                                  const std::string& contentLength)
-  {
-    size_t length;
-    try
-    {
-      int64_t tmp = boost::lexical_cast<int64_t>(contentLength);
-      if (tmp < 0)
-      {
-        return PostDataStatus_NoLength;
-      }
-
-      length = static_cast<size_t>(tmp);
-    }
-    catch (boost::bad_lexical_cast&)
-    {
-      return PostDataStatus_NoLength;
-    }
 
-    body.resize(length);
-
-    size_t pos = 0;
-    while (length > 0)
-    {
-      int r = mg_read(connection, &body[pos], length);
-      if (r <= 0)
-      {
-        return PostDataStatus_Failure;
-      }
-
-      assert(static_cast<size_t>(r) <= length);
-      length -= r;
-      pos += r;
-    }
-
-    return PostDataStatus_Success;
-  }
-                                                  
-
-  static PostDataStatus ReadBodyWithoutContentLength(std::string& body,
-                                                     struct mg_connection *connection)
+  static PostDataStatus ReadBodyUsingFile(std::string& body,
+                                          struct mg_connection *connection,
+                                          size_t maxSize /* "0" means no limit */)
   {
     // Store the individual chunks in a temporary file, then read it
     // back into the memory buffer "body"
     FileBuffer buffer;
 
+    uint64_t readSoFar = 0;
     std::string tmp(1024 * 1024, 0);
-      
+
     for (;;)
     {
       int r = mg_read(connection, &tmp[0], tmp.size());
@@ -555,6 +518,15 @@
       }
       else
       {
+        readSoFar += r;
+
+        if (readSoFar > std::numeric_limits<size_t>::max() ||
+            (maxSize != 0 &&
+             readSoFar > maxSize))
+        {
+          return PostDataStatus_RequestEntityTooLarge;
+        }
+
         buffer.Append(tmp.c_str(), r);
       }
     }
@@ -563,6 +535,88 @@
 
     return PostDataStatus_Success;
   }
+
+
+  static PostDataStatus ReadBodyWithContentLength(std::string& body,
+                                                  struct mg_connection *connection,
+                                                  const std::string& contentLength)
+  {
+    static const size_t MAXIMUM_BODY_SIZE_IN_MEMORY = 10 * 1024 * 1024;  // 10MB
+
+    size_t length;
+    try
+    {
+      int64_t tmp = boost::lexical_cast<int64_t>(contentLength);
+      if (tmp < 0)
+      {
+        return PostDataStatus_NoLength;
+      }
+
+      length = static_cast<size_t>(tmp);
+      if (static_cast<int64_t>(length) != tmp)
+      {
+        return PostDataStatus_Failure;
+      }
+    }
+    catch (boost::bad_lexical_cast&)
+    {
+      return PostDataStatus_NoLength;
+    }
+
+    if (length < MAXIMUM_BODY_SIZE_IN_MEMORY)
+    {
+      /**
+       * Small POST bodies should land into RAM to avoid creating
+       * temporary files, which would result in bad performance.
+       **/
+      body.resize(length);
+
+      size_t pos = 0;
+      while (length > 0)
+      {
+        int r = mg_read(connection, &body[pos], length);
+        if (r <= 0)
+        {
+          return PostDataStatus_Failure;
+        }
+
+        assert(static_cast<size_t>(r) <= length);
+        length -= r;
+        pos += r;
+      }
+
+      return PostDataStatus_Success;
+    }
+    else
+    {
+      /**
+       * Deal with CWE-770 (Machine Spirits UG). If the client wants
+       * to send a large body, use a temporary file to prevent memory
+       * exhaustion by a malicious client that would set a large
+       * "Content-Length" without sending any actual data.
+       **/
+
+      PostDataStatus status = ReadBodyUsingFile(body, connection, length);
+
+      if (status == PostDataStatus_Success)
+      {
+        return (body.size() == length ?
+                PostDataStatus_Success :
+                PostDataStatus_Failure);
+      }
+      else
+      {
+        return status;
+      }
+    }
+  }
+
+
+  static PostDataStatus ReadBodyWithoutContentLength(std::string& body,
+                                                     struct mg_connection *connection)
+  {
+    return ReadBodyUsingFile(body, connection, 0 /* TODO - no bound */);
+  }
                                                   
 
   static PostDataStatus ReadBodyToString(std::string& body,
@@ -1492,6 +1546,10 @@
           output.SendStatus(HttpStatus_411_LengthRequired);
           return;
 
+        case PostDataStatus_RequestEntityTooLarge:  // New in Orthanc 1.12.11
+          output.SendStatus(HttpStatus_413_RequestEntityTooLarge);
+          return;
+
         case PostDataStatus_Failure:
           output.SendStatus(HttpStatus_400_BadRequest);
           return;
--- a/OrthancFramework/Sources/Images/ImageAccessor.cpp	Fri Mar 20 17:05:49 2026 +0100
+++ b/OrthancFramework/Sources/Images/ImageAccessor.cpp	Fri Mar 20 17:09:51 2026 +0100
@@ -165,7 +165,14 @@
   {
     if (buffer_ != NULL)
     {
-      return buffer_ + static_cast<size_t>(y) * static_cast<size_t>(pitch_);
+      if (y < height_)
+      {
+        return buffer_ + static_cast<size_t>(y) * static_cast<size_t>(pitch_);
+      }
+      else
+      {
+        throw OrthancException(ErrorCode_ParameterOutOfRange);
+      }
     }
     else
     {
@@ -184,7 +191,14 @@
 
     if (buffer_ != NULL)
     {
-      return buffer_ + static_cast<size_t>(y) * static_cast<size_t>(pitch_);
+      if (y < height_)
+      {
+        return buffer_ + static_cast<size_t>(y) * static_cast<size_t>(pitch_);
+      }
+      else
+      {
+        throw OrthancException(ErrorCode_ParameterOutOfRange);
+      }
     }
     else
     {
@@ -210,17 +224,20 @@
                                      unsigned int pitch,
                                      const void *buffer)
   {
+    const uint64_t size = static_cast<uint64_t>(height) * static_cast<uint64_t>(pitch);
+
+    if (static_cast<uint64_t>(GetBytesPerPixel() * width) > static_cast<uint64_t>(pitch) ||
+        static_cast<uint64_t>(static_cast<size_t>(size)) != size)
+    {
+      throw OrthancException(ErrorCode_ParameterOutOfRange);
+    }
+
     readOnly_ = true;
     format_ = format;
     width_ = width;
     height_ = height;
     pitch_ = pitch;
     buffer_ = reinterpret_cast<uint8_t*>(const_cast<void*>(buffer));
-
-    if (GetBytesPerPixel() * width_ > pitch_)
-    {
-      throw OrthancException(ErrorCode_ParameterOutOfRange);
-    }
   }
 
   void ImageAccessor::GetReadOnlyAccessor(ImageAccessor &target) const
@@ -235,17 +252,20 @@
                                      unsigned int pitch,
                                      void *buffer)
   {
+    const uint64_t size = static_cast<uint64_t>(height) * static_cast<uint64_t>(pitch);
+
+    if (static_cast<uint64_t>(GetBytesPerPixel() * width) > static_cast<uint64_t>(pitch) ||
+        static_cast<uint64_t>(static_cast<size_t>(size)) != size)
+    {
+      throw OrthancException(ErrorCode_ParameterOutOfRange);
+    }
+
     readOnly_ = false;
     format_ = format;
     width_ = width;
     height_ = height;
     pitch_ = pitch;
     buffer_ = reinterpret_cast<uint8_t*>(buffer);
-
-    if (GetBytesPerPixel() * width_ > pitch_)
-    {
-      throw OrthancException(ErrorCode_ParameterOutOfRange);
-    }
   }
 
 
--- a/OrthancFramework/Sources/Images/ImageBuffer.cpp	Fri Mar 20 17:05:49 2026 +0100
+++ b/OrthancFramework/Sources/Images/ImageBuffer.cpp	Fri Mar 20 17:09:51 2026 +0100
@@ -46,8 +46,16 @@
         }
       */
 
-      pitch_ = GetBytesPerPixel() * width_;
-      size_t size = static_cast<size_t>(pitch_) * static_cast<size_t>(height_);
+      const uint64_t tmpPitch = static_cast<uint64_t>(GetBytesPerPixel()) * static_cast<uint64_t>(width_);
+      const uint64_t size = tmpPitch * static_cast<uint64_t>(height_);
+
+      if (static_cast<uint64_t>(static_cast<unsigned int>(tmpPitch)) != tmpPitch ||
+          static_cast<uint64_t>(static_cast<size_t>(size)) != size)
+      {
+        throw OrthancException(ErrorCode_NotEnoughMemory);
+      }
+
+      pitch_ = static_cast<unsigned int>(tmpPitch);
 
       if (size == 0)
       {
@@ -55,7 +63,7 @@
       }
       else
       {
-        buffer_ = malloc(size);
+        buffer_ = malloc(static_cast<size_t>(size));
         if (buffer_ == NULL)
         {
           throw OrthancException(ErrorCode_NotEnoughMemory,
--- a/OrthancFramework/Sources/Images/PamReader.cpp	Fri Mar 20 17:05:49 2026 +0100
+++ b/OrthancFramework/Sources/Images/PamReader.cpp	Fri Mar 20 17:09:51 2026 +0100
@@ -38,6 +38,7 @@
 #include <boost/algorithm/string/find.hpp>
 #include <boost/lexical_cast.hpp>
 
+static const uint64_t MAX_PAM_IMAGE_BUFFER_SIZE = 4ul * 1024ul * 1024ul * 1024ul;  // defensive approach: set a reasonable max size for a PAM image
 
 namespace Orthanc
 {
@@ -181,11 +182,29 @@
     const unsigned int maxValue = LookupIntegerParameter(parameters, "MAXVAL");
     const std::string tupleType = LookupStringParameter(parameters, "TUPLTYPE");
 
+    if (width > 65535 || height > 65535 || channelCount > 4 || maxValue > 65535)
+    {
+      throw OrthancException(ErrorCode_BadFileFormat, "PAM header values exceed reasonable limits");
+    }
+
     unsigned int bytesPerChannel;
     PixelFormat format;
     GetPixelFormat(format, bytesPerChannel, maxValue, channelCount, tupleType);
 
-    unsigned int pitch = width * channelCount * bytesPerChannel;
+    // unsigned int pitch = width * channelCount * bytesPerChannel;
+    uint64_t pitch = static_cast<uint64_t>(width) * channelCount * bytesPerChannel;
+
+    if (pitch > std::numeric_limits<unsigned int>::max())
+    {
+      throw OrthancException(ErrorCode_BadFileFormat, "PAM dimensions exceed limits");
+    }
+
+    uint64_t totalSize = pitch * height;
+    if (totalSize > MAX_PAM_IMAGE_BUFFER_SIZE ||
+        static_cast<uint64_t>(static_cast<size_t>(totalSize)) != totalSize)
+    {
+      throw OrthancException(ErrorCode_BadFileFormat, "PAM image too large");
+    }
 
     if (content_.size() != header.size() + headerDelimiter.size() + pitch * height)
     {
@@ -196,7 +215,7 @@
 
     {
       intptr_t bufferAddr = reinterpret_cast<intptr_t>(&content_[offset]);
-      if((bufferAddr % 8) == 0)
+      if ((bufferAddr % 8) == 0)
         LOG(TRACE) << "PamReader::ParseContent() image address = " << bufferAddr;
       else
         LOG(TRACE) << "PamReader::ParseContent() image address = " << bufferAddr << " (not a multiple of 8!)";
--- a/OrthancServer/Sources/ServerContext.cpp	Fri Mar 20 17:05:49 2026 +0100
+++ b/OrthancServer/Sources/ServerContext.cpp	Fri Mar 20 17:09:51 2026 +0100
@@ -1950,7 +1950,7 @@
       }
       catch (OrthancException& e)
       {
-        LOG(INFO) << e.GetDetails();
+        LOG(INFO) << "Failed to decode a DICOM frame: " << e.GetDetails();
       }
     }