CVE-2023-50245: Buffer Copy without Checking Size of Input ('Classic Buffer Overflow') (CWE-120)
value: OpenEXR-viewer is a viewer for OpenEXR files with detailed metadata probing. Versions prior to 0.6.1 have a memory overflow vulnerability. This issue is fixed in version 0.6.1.
CWE description: The product copies an input buffer to an output buffer without verifying that the size of the input buffer is less than the size of the output buffer, leading to a buffer overflow.
CVSS v3 base score: 9.8
Published: 2023-12-11T23:15Z
Repository: https://github.com/afichet/openexr-viewer
Language: C++
Files changed: src/model/framebuffer/FramebufferModel.cpp, src/model/framebuffer/FramebufferModel.h, src/model/framebuffer/RGBFramebufferModel.cpp, src/model/framebuffer/YFramebufferModel.cpp
Commit date: 2023-12-09 14:26:22 +0100
Commit message: Early catch potential overflow issue #43

`m_width` and `m_height` are of `int` type in the OpenEXR library. We
currently keep the same types in our class but this may case issue
when mapping 1D memory. In the most favorable case, they are
multiplied together (Y framebuffer). For RGB(A) case, the required
memory can also be 4 time larger. We check if resp. `m_width *
m_height` and `4 * m_width * m_heigh` stay within the `int` higher
limit. Thanks to @GAP-dev for bringing this issue.

This commit also cleans a bit raw memory allocation in favor of
`std::vector` container.

Diff: diff --git a/src/model/framebuffer/FramebufferModel.cpp b/src/model/framebuffer/FramebufferModel.cpp
index 4f21ba7..379b669 100644
--- a/src/model/framebuffer/FramebufferModel.cpp
+++ b/src/model/framebuffer/FramebufferModel.cpp
@@ -37,11 +37,10 @@
 #include <cstddef>
 #include <fstream>
 
 FramebufferModel::FramebufferModel(QObject* parent)
   : QObject(parent)
-  , m_pixelBuffer(nullptr)
   , m_width(0)
   , m_height(0)
   , m_isImageLoaded(false)
   , m_exposure(0)
   , m_imageLoadingWatcher(new QFutureWatcher<void>(this))
@@ -57,9 +56,6 @@ QRect FramebufferModel::getDisplayWindow() const
 QRect FramebufferModel::getDataWindow() const
 {
     return m_dataWindow;
 }
 
-FramebufferModel::~FramebufferModel()
-{
-    delete[] m_pixelBuffer;
-}
+FramebufferModel::~FramebufferModel() {}
diff --git a/src/model/framebuffer/FramebufferModel.h b/src/model/framebuffer/FramebufferModel.h
index f80085a..7993fb5 100644
--- a/src/model/framebuffer/FramebufferModel.h
+++ b/src/model/framebuffer/FramebufferModel.h
@@ -35,11 +35,11 @@
 #include <QFutureWatcher>
 #include <QImage>
 #include <QObject>
 #include <QRect>
 #include <QVector>
-#include <array>
+#include <vector>
 
 class FramebufferModel: public QObject
 {
     Q_OBJECT
 
@@ -65,12 +65,12 @@ class FramebufferModel: public QObject
     void imageLoaded();
     void exposureChanged(double exposure);
     void loadFailed(QString message);
 
   protected:
-    float* m_pixelBuffer;
-    QImage m_image;
+    std::vector<float> m_pixelBuffer;
+    QImage             m_image;
 
     // Right now, the width and height are defined as Vec2i in OpenEXR
     // i.e. int type.
     int m_width, m_height;
 
diff --git a/src/model/framebuffer/RGBFramebufferModel.cpp b/src/model/framebuffer/RGBFramebufferModel.cpp
index f345470..66fd78c 100644
--- a/src/model/framebuffer/RGBFramebufferModel.cpp
+++ b/src/model/framebuffer/RGBFramebufferModel.cpp
@@ -79,10 +79,27 @@ void RGBFramebufferModel::load(
             int dispW_height = dispW.max.y - dispW.min.y + 1;
 
             m_displayWindow
               = QRect(dispW.min.x, dispW.min.y, dispW_width, dispW_height);
 
+            // Check to avoid type overflow, width and height are 32bits int
+            // representing a 2 dimentional image. Can overflow the type when
+            // multiplied together.
+            // 0x1FFFFFFF is a save limit for 4 * 0x7FFFFFFF the max
+            // representable int since we need 4 channels.
+            // TODO: Use larger type when manipulating framebuffer
+            const uint64_t partial_size
+              = (uint64_t)m_width * (uint64_t)m_height;
+
+            if (partial_size > 0x1FFFFFFF) {
+                throw std::runtime_error(
+                  "The total image size is too large. May be supported in a "
+                  "future revision.");
+            }
+
+            m_pixelBuffer.resize(4 * m_width * m_height);
+
             // Check if there is specific chromaticities tied to the color
             // representation in this part.
             const Imf::ChromaticitiesAttribute* c
               = part.header().findTypedAttribute<Imf::ChromaticitiesAttribute>(
                 "chromaticities");
@@ -91,12 +108,10 @@ void RGBFramebufferModel::load(
 
             if (c != nullptr) {
                 chromaticities = c->value();
             }
 
-            m_pixelBuffer = new float[4 * m_width * m_height];
-
             // Check if there is alpha channel
             if (hasAlpha) {
                 std::string      aLayer = m_parentLayer + "A";
                 Imf::FrameBuffer framebuffer;
 
@@ -188,16 +203,16 @@ void RGBFramebufferModel::load(
                     std::string ryLayer = m_parentLayer + "RY";
                     std::string byLayer = m_parentLayer + "BY";
 
                     Imf::FrameBuffer framebuffer;
 
-                    Imf::Rgba* buff1 = new Imf::Rgba[m_width * m_height];
-                    Imf::Rgba* buff2 = new Imf::Rgba[m_width * m_height];
+                    std::vector<Imf::Rgba> buff1(m_width * m_height);
+                    std::vector<Imf::Rgba> buff2(m_width * m_height);
 
-                    float* yBuffer  = new float[m_width * m_height];
-                    float* ryBuffer = new float[m_width / 2 * m_height / 2];
-                    float* byBuffer = new float[m_width / 2 * m_height / 2];
+                    std::vector<float> yBuffer(m_width * m_height);
+                    std::vector<float> ryBuffer(m_width / 2 * m_height / 2);
+                    std::vector<float> byBuffer(m_width / 2 * m_height / 2);
 
                     Imf::Slice ySlice = Imf::Slice::Make(
                       Imf::PixelType::FLOAT,
                       &yBuffer[0],
                       datW,
@@ -333,16 +348,10 @@ void RGBFramebufferModel::load(
                             m_pixelBuffer[4 * (y * m_width + x) + 0] = rgb.x;
                             m_pixelBuffer[4 * (y * m_width + x) + 1] = rgb.y;
                             m_pixelBuffer[4 * (y * m_width + x) + 2] = rgb.z;
                         }
                     }
-
-                    delete[] yBuffer;
-                    delete[] ryBuffer;
-                    delete[] byBuffer;
-                    delete[] buff1;
-                    delete[] buff2;
                 }
 
                 break;
 
                 case Layer_Y: {
diff --git a/src/model/framebuffer/YFramebufferModel.cpp b/src/model/framebuffer/YFramebufferModel.cpp
index 06f388c..7310c6e 100644
--- a/src/model/framebuffer/YFramebufferModel.cpp
+++ b/src/model/framebuffer/YFramebufferModel.cpp
@@ -88,16 +88,29 @@ void YFramebufferModel::load(Imf::MultiPartInputFile& file, int partId)
                   dispW.min.x,
                   dispW.min.y,
                   dispW_width / 2,
                   dispW_height / 2);
 
-                m_pixelBuffer = new float[m_width * m_height];
+                // Check to avoid type overflow, width and height are 32bits int
+                // representing a 2 dimentional image. Can overflow the type when
+                // multiplied together
+                // TODO: Use larger type when manipulating framebuffer
+                const uint64_t partial_size
+                  = (uint64_t)m_width * (uint64_t)m_height;
+
+                if (partial_size > 0x7FFFFFFF) {
+                    throw std::runtime_error(
+                      "The total image size is too large. May be supported in "
+                      "a future revision.");
+                }
+
+                m_pixelBuffer.resize(m_width * m_height);
 
                 // Luminance Chroma channels
                 graySlice = Imf::Slice::Make(
                   Imf::PixelType::FLOAT,
-                  m_pixelBuffer,
+                  m_pixelBuffer.data(),
                   datW,
                   sizeof(float),
                   m_width * sizeof(float),
                   2,
                   2);
@@ -110,15 +123,15 @@ void YFramebufferModel::load(Imf::MultiPartInputFile& file, int partId)
                 int dispW_height = dispW.max.y - dispW.min.y + 1;
 
                 m_displayWindow
                   = QRect(dispW.min.x, dispW.min.y, dispW_width, dispW_height);
 
-                m_pixelBuffer = new float[m_width * m_height];
+                m_pixelBuffer.resize(m_width * m_height);
 
                 graySlice = Imf::Slice::Make(
                   Imf::PixelType::FLOAT,
-                  m_pixelBuffer,
+                  m_pixelBuffer.data(),
                   datW);
             }
 
             Imf::FrameBuffer framebuffer;
 

Vulnerable code:   , m_pixelBuffer(nullptr)
FramebufferModel::~FramebufferModel()
{
    delete[] m_pixelBuffer;
}
#include <array>
    float* m_pixelBuffer;
    QImage m_image;
            m_pixelBuffer = new float[4 * m_width * m_height];

                    Imf::Rgba* buff1 = new Imf::Rgba[m_width * m_height];
                    Imf::Rgba* buff2 = new Imf::Rgba[m_width * m_height];
                    float* yBuffer  = new float[m_width * m_height];
                    float* ryBuffer = new float[m_width / 2 * m_height / 2];
                    float* byBuffer = new float[m_width / 2 * m_height / 2];

                    delete[] yBuffer;
                    delete[] ryBuffer;
                    delete[] byBuffer;
                    delete[] buff1;
                    delete[] buff2;
                m_pixelBuffer = new float[m_width * m_height];
                  m_pixelBuffer,
                m_pixelBuffer = new float[m_width * m_height];
                  m_pixelBuffer,
Fixed code: FramebufferModel::~FramebufferModel() {}
#include <vector>
    std::vector<float> m_pixelBuffer;
    QImage             m_image;
            // Check to avoid type overflow, width and height are 32bits int
            // representing a 2 dimentional image. Can overflow the type when
            // multiplied together.
            // 0x1FFFFFFF is a save limit for 4 * 0x7FFFFFFF the max
            // representable int since we need 4 channels.
            // TODO: Use larger type when manipulating framebuffer
            const uint64_t partial_size
              = (uint64_t)m_width * (uint64_t)m_height;

            if (partial_size > 0x1FFFFFFF) {
                throw std::runtime_error(
                  "The total image size is too large. May be supported in a "
                  "future revision.");
            }

            m_pixelBuffer.resize(4 * m_width * m_height);

                    std::vector<Imf::Rgba> buff1(m_width * m_height);
                    std::vector<Imf::Rgba> buff2(m_width * m_height);
                    std::vector<float> yBuffer(m_width * m_height);
                    std::vector<float> ryBuffer(m_width / 2 * m_height / 2);
                    std::vector<float> byBuffer(m_width / 2 * m_height / 2);
                // Check to avoid type overflow, width and height are 32bits int
                // representing a 2 dimentional image. Can overflow the type when
                // multiplied together
                // TODO: Use larger type when manipulating framebuffer
                const uint64_t partial_size
                  = (uint64_t)m_width * (uint64_t)m_height;

                if (partial_size > 0x7FFFFFFF) {
                    throw std::runtime_error(
                      "The total image size is too large. May be supported in "
                      "a future revision.");
                }

                m_pixelBuffer.resize(m_width * m_height);
                  m_pixelBuffer.data(),
                m_pixelBuffer.resize(m_width * m_height);
                  m_pixelBuffer.data(),