fix(pnm,jxl): Prevent PNM/JXL readers from loading arbitrarily large non-image files (#5203)

##### PNM

The PNM reader did not provide an optimized `valid_file` implementation.
While `PNMInput::open` would perform basic validation of the PNM header,
it would only do this after having read the entire contents of the file
into memory. Besides the obvious risks/issues with having `open`
unconditionally read arbitrarily large files into memory, this also
meant that any call to `valid_file` would necessarily do the same (as
the default implementation delegates to `open` when ioproxy support is
present).

To fix these issues, the following changes were made:
* Provide an efficient implementation of `valid_file` for `PNMInput`
that only loads the header, and then validates magic number/dimensions.
* In `PNMInput::open`, first load only the header to memory, and
subsequently, only load the rest of the image data if the header is
parsed successfully, and the data contained within is valid.
* Added a rough limit to header read size of 1KiB.
* Added a rough limit to full file read size of 1GiB.

Note that, for now, if the file size exceeds the 1GiB limit, the read is
simply truncated to 1GiB, rather than failing altogether.

##### JPEGXL

The JXL reader already provided an efficient `valid_file` override,
implemented by validating the 128 byte JXL signature. The signature,
however, was not being validated in `JxlInput::open` before attempting
to decode the file.

The process of JXL decoding appears to read the entire contents of the
file to memory (at least for some non-JXL inputs). Taken in combination,
this means that when `JXLInput::open` was called on arbitrarily large
non-JXL inputs, the entire file was being read into memory before `open`
would fail.

As a simple fix for this, `JxlInput::open` now checks the result of
`valid_file` before attempting any decoding.

### Tests

I did not add a new testsuite case for this, but did test on my end that
for both PNM and JPEGXL, large invalid files are no longer being read
into memory, while valid images (of their respective types) are still
read/opened successfully.

---------

Signed-off-by: maxwelliverson <maxwelliverson@gmail.com>

Author: maxwelliverson

src/jpegxl.imageio/jxlinput.cpp

@@ -164,6 +164,13 @@ JxlInput::open(const std::string& name, ImageSpec& newspec)
         return false;
     }
 
+    if (!valid_file(m_io)) {
+        DBG std::cout << "JxlInput::valid_file() return false\n";
+        errorfmt("Possible corrupt file, "
+                 "JPEG XL signature verification failed\n");
+        return false;
+    }
+
     m_decoder = JxlDecoderMake(nullptr);
     if (m_decoder == nullptr) {
         DBG std::cout << "JxlDecoderMake failed\n";

src/pnm.imageio/pnminput.cpp

@@ -5,6 +5,7 @@
 #include <cstdio>
 #include <cstdlib>
 #include <fstream>
+#include <optional>
 #include <string>
 
 #include <OpenImageIO/filesystem.h>
@@ -23,6 +24,17 @@ OIIO_PLUGIN_NAMESPACE_BEGIN
 // http://netpbm.sourceforge.net/doc/pam.html  (base format)
 //
 
+enum PNMType { P1, P2, P3, P4, P5, P6, Pf, PF };
+
+
+
+struct PNMBasicInfo {
+    PNMType type;
+    int width;
+    int height;
+};
+
+
 
 class PNMInput final : public ImageInput {
 public:
@@ -40,10 +52,9 @@ class PNMInput final : public ImageInput {
     int current_subimage(void) const override { return 0; }
     bool read_native_scanline(int subimage, int miplevel, int y, int z,
                               void* data) override;
+    bool valid_file(Filesystem::IOProxy* ioproxy) const override;
 
 private:
-    enum PNMType { P1, P2, P3, P4, P5, P6, Pf, PF };
-
     PNMType m_pnm_type;
     int m_max_val;
     float m_scaling_factor;
@@ -63,17 +74,13 @@ class PNMInput final : public ImageInput {
     bool read_file_scanline(void* data, int y);
     bool read_file_header();
 
-    void skipComments()
-    {
-        while (m_remaining.size() && Strutil::parse_char(m_remaining, '#'))
-            Strutil::parse_line(m_remaining);
-    }
+    static string_view read_header_to_buffer(std::vector<char>& buffer,
+                                             Filesystem::IOProxy* io);
+    static string_view append_remainder_to_buffer(std::vector<char>& buffer,
+                                                  Filesystem::IOProxy* io,
+                                                  string_view remaining);
 
-    template<typename T> bool nextVal(T& val)
-    {
-        skipComments();
-        return Strutil::parse_value(m_remaining, val);
-    }
+    template<typename T> bool nextVal(T& val);
 
     template<class T>
     bool ascii_to_raw(T* write, imagesize_t nvals, T max, bool invert = false);
@@ -104,6 +111,34 @@ OIIO_EXPORT const char* pnm_input_extensions[] = { "ppm", "pgm", "pbm",
 OIIO_PLUGIN_EXPORTS_END
 
 
+// 1KiB approximate limit on header size
+static const imagesize_t max_pnm_header_size = 1024;
+
+// 1GiB rough limit on file size to avoid loading arbitrarily
+// large files into memory
+static const imagesize_t max_pnm_file_size = 1024 * 1024 * 1024;
+
+
+
+inline static void
+skip_header_comments(string_view& header)
+{
+    while (header.size() && Strutil::parse_char(header, '#'))
+        Strutil::parse_line(header);
+}
+
+
+
+template<typename T>
+inline static bool
+parse_next_header_value(string_view& header, T& val)
+{
+    skip_header_comments(header);
+    return Strutil::parse_value(header, val);
+}
+
+
+
 template<class T>
 inline void
 invert(const T* read, T* write, imagesize_t nvals)
@@ -114,6 +149,15 @@ invert(const T* read, T* write, imagesize_t nvals)
 
 
 
+template<typename T>
+bool
+PNMInput::nextVal(T& val)
+{
+    return parse_next_header_value(m_remaining, val);
+}
+
+
+
 template<class T>
 bool
 PNMInput::ascii_to_raw(T* write, imagesize_t nvals, T max, bool invert)
@@ -189,6 +233,39 @@ unpack_floats(const unsigned char* read, float* write, imagesize_t numsamples,
 
 
 
+static std::optional<PNMBasicInfo>
+read_type_and_resolution(string_view& header)
+{
+    PNMType type;
+
+    if (!Strutil::parse_char(header, 'P') || header.empty())
+        return std::nullopt;
+
+    switch (header.front()) {
+    case '1': type = P1; break;
+    case '2': type = P2; break;
+    case '3': type = P3; break;
+    case '4': type = P4; break;
+    case '5': type = P5; break;
+    case '6': type = P6; break;
+    case 'f': type = Pf; break;
+    case 'F': type = PF; break;
+    default: return std::nullopt;
+    }
+    header.remove_prefix(1);
+
+    //Size
+    int width, height;
+    if (!parse_next_header_value(header, width))
+        return std::nullopt;
+    if (!parse_next_header_value(header, height))
+        return std::nullopt;
+
+    return PNMBasicInfo { type, width, height };
+}
+
+
+
 bool
 PNMInput::read_file_scanline(void* data, int y)
 {
@@ -277,28 +354,15 @@ PNMInput::read_file_scanline(void* data, int y)
 bool
 PNMInput::read_file_header()
 {
-    // MagicNumber
-    if (!Strutil::parse_char(m_remaining, 'P') || m_remaining.empty())
-        return false;
-    switch (m_remaining.front()) {
-    case '1': m_pnm_type = P1; break;
-    case '2': m_pnm_type = P2; break;
-    case '3': m_pnm_type = P3; break;
-    case '4': m_pnm_type = P4; break;
-    case '5': m_pnm_type = P5; break;
-    case '6': m_pnm_type = P6; break;
-    case 'f': m_pnm_type = Pf; break;
-    case 'F': m_pnm_type = PF; break;
-    default: return false;
-    }
-    m_remaining.remove_prefix(1);
-
-    //Size
     int width, height;
-    if (!nextVal(width))
-        return false;
-    if (!nextVal(height))
+
+    if (auto basic_info = read_type_and_resolution(m_remaining)) {
+        m_pnm_type = basic_info->type;
+        width      = basic_info->width;
+        height     = basic_info->height;
+    } else {
         return false;
+    }
 
     if (m_pnm_type != PF && m_pnm_type != Pf) {
         // Max Val
@@ -312,7 +376,6 @@ PNMInput::read_file_header()
         if (!(m_remaining.size() && Strutil::isspace(m_remaining.front())))
             return false;
         m_remaining.remove_prefix(1);
-        m_after_header = m_remaining;
 
         m_spec = ImageSpec(width, height,
                            (m_pnm_type == P3 || m_pnm_type == P6) ? 3 : 1,
@@ -332,7 +395,6 @@ PNMInput::read_file_header()
         if (!(m_remaining.size() && Strutil::isspace(m_remaining.front())))
             return false;
         m_remaining.remove_prefix(1);
-        m_after_header = m_remaining;
 
         m_spec = ImageSpec(width, height, m_pnm_type == PF ? 3 : 1,
                            TypeDesc::FLOAT);
@@ -345,6 +407,79 @@ PNMInput::read_file_header()
 
 
 
+// Read only enough of the file to contain the header (at momst 1KB) into buffer
+string_view
+PNMInput::read_header_to_buffer(std::vector<char>& buffer,
+                                Filesystem::IOProxy* io)
+{
+    imagesize_t header_size = std::min(static_cast<imagesize_t>(io->size()),
+                                       max_pnm_header_size);
+    buffer.resize(header_size);
+    io->pread(buffer.data(), header_size, 0);
+    return string_view(buffer.data(), buffer.size());
+}
+
+
+
+// buffer contains at most the first 1K of the file. At this point, we know
+// the file seems valid. Read the rest in, appending to what we have, and
+// return the adjusted string_view of the contents.
+string_view
+PNMInput::append_remainder_to_buffer(std::vector<char>& buffer,
+                                     Filesystem::IOProxy* io,
+                                     string_view remaining)
+{
+    // Assume we've already read the header into buffer
+    imagesize_t header_size    = buffer.size();
+    imagesize_t full_size      = std::min(static_cast<imagesize_t>(io->size()),
+                                          max_pnm_file_size);
+    ptrdiff_t remaining_offset = remaining.data() - buffer.data();
+
+    buffer.resize(full_size);
+    io->pread(buffer.data() + header_size, full_size - header_size,
+              header_size);
+
+    string_view result { buffer.data(), buffer.size() };
+    result.remove_prefix(remaining_offset);
+    return result;
+}
+
+
+
+bool
+PNMInput::valid_file(Filesystem::IOProxy* ioproxy) const
+{
+    DBG std::cout << "PNMInput::valid_file()\n";
+
+    if (!ioproxy || ioproxy->mode() != Filesystem::IOProxy::Mode::Read)
+        return false;
+
+    std::vector<char> buffer;
+    string_view header = read_header_to_buffer(buffer, ioproxy);
+
+    int width, height;
+    if (auto basic_info = read_type_and_resolution(header)) {
+        width  = basic_info->width;
+        height = basic_info->height;
+    } else {
+        return false;
+    }
+
+    // Per spec, width and height must both be positive integers.
+    // No formal upper limit is placed on width/height, but for sanity,
+    // assume dimensions should be no greater than 2^12
+    if (width < 0 || 4096 < width)
+        return false;
+    if (height < 0 || 4096 < height)
+        return false;
+
+    DBG std::cout << "PNMInput::valid_file returned true\n";
+
+    return true;
+}
+
+
+
 bool
 PNMInput::open(const std::string& name, ImageSpec& newspec,
                const ImageSpec& config)
@@ -371,18 +506,20 @@ PNMInput::open(const std::string& name, ImageSpec& newspec)
 
     // Read the whole file's contents into m_file_contents
     Filesystem::IOProxy* m_io = ioproxy();
-    m_file_contents.resize(m_io->size());
-    m_io->pread(m_file_contents.data(), m_file_contents.size(), 0);
-    m_remaining = string_view(m_file_contents.data(), m_file_contents.size());
-    m_pfm_flip  = false;
+    m_remaining               = read_header_to_buffer(m_file_contents, m_io);
+    m_pfm_flip                = false;
 
     if (!read_file_header())
         return false;
 
     if (!check_open(m_spec))  // check for apparently invalid values
         return false;
 
-    newspec = m_spec;
+    m_remaining    = append_remainder_to_buffer(m_file_contents, m_io,
+                                                m_remaining);
+    m_after_header = m_remaining;
+    newspec        = m_spec;
+
     return true;
 }
 
@@ -412,4 +549,5 @@ PNMInput::read_native_scanline(int subimage, int miplevel, int y, int z,
     return true;
 }
 
+
 OIIO_PLUGIN_NAMESPACE_END