From fd8e64a66085d912ccc9aeb2ea201985fc333c06 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Wed, 30 Aug 2023 10:04:39 -0700 Subject: [PATCH] feat(api): ImageInput::open_check (#3967) Not unlike the ImageOutput::open_check we added a few months back, this creates an ImageInput utility function that centralizes a number of validation checks. Currently, this is primarily a check on allowed resolution and total size, aimed at making a guess at files that are corrupted or malicious. Also adds a new `supports("noimage")` query. --------- Signed-off-by: Larry Gritz --- src/fits.imageio/fits_pvt.h | 6 ++-- src/include/OpenImageIO/imageio.h | 59 ++++++++++++++++++++++++++++++- src/libOpenImageIO/imageinput.cpp | 59 +++++++++++++++++++++++++++++++ src/targa.imageio/targainput.cpp | 12 +------ src/tiff.imageio/tiffinput.cpp | 19 ++-------- 5 files changed, 124 insertions(+), 31 deletions(-) diff --git a/src/fits.imageio/fits_pvt.h b/src/fits.imageio/fits_pvt.h index b2bcb979ef..5db8dae0aa 100644 --- a/src/fits.imageio/fits_pvt.h +++ b/src/fits.imageio/fits_pvt.h @@ -44,8 +44,10 @@ class FitsInput final : public ImageInput { int supports(string_view feature) const override { return (feature == "arbitrary_metadata" - || feature == "exif" // Because of arbitrary_metadata - || feature == "iptc"); // Because of arbitrary_metadata + || feature == "exif" // Because of arbitrary_metadata + || feature == "iptc" // Because of arbitrary_metadata + || feature == "noimage" // allow metadata only, no pixels + ); } bool valid_file(const std::string& filename) const override; bool open(const std::string& name, ImageSpec& spec) override; diff --git a/src/include/OpenImageIO/imageio.h b/src/include/OpenImageIO/imageio.h index 117c5ac88c..1d9b07184b 100644 --- a/src/include/OpenImageIO/imageio.h +++ b/src/include/OpenImageIO/imageio.h @@ -1003,6 +1003,10 @@ class OIIO_API ImageInput { /// Does this format reader support retrieving a reduced /// resolution copy of the image via the `thumbnail()` method? /// + /// - `"noimage"` : + /// Does this format allow 0x0 sized images, i.e. an image file + /// with metadata only and no pixels? + /// /// This list of queries may be extended in future releases. Since this /// can be done simply by recognizing new query strings, and does not /// require any new API entry points, addition of support for new @@ -1818,6 +1822,53 @@ class OIIO_API ImageInput { /// Helper: retrieve the current position of the proxy, akin to ftell. int64_t iotell() const; + /// Helper: convenience boilerplate for several checks and operations that + /// every implementation of ImageInput::open() will need to do. Failure is + /// presumed to indicate a file that is corrupt (or perhaps maliciously + /// crafted) and no further reading should be attempted. + /// + /// @param spec + /// The ImageSpec that we are validating. + /// + /// @param range + /// An ROI that describes the allowable pixel coordinates and channel + /// indices as half-open intervals. For example, the default value + /// `{0, 65535, 0, 65535, 0, 1, 0, 4}` means that pixel coordinates + /// must be non-negative and the width and height be representable by + /// a uint16 value, up to 4 channels are allowed, and volumes are not + /// permitted (z coordinate may only be 0). File formats that can + /// handle larger resolutions, or volumes, or >4 channels must + /// override these limits! + /// + /// @param flags + /// A bitfield flag (bits defined by `enum OpenChecks`) that can + /// indicate additional checks to perform, or checks that should be + /// skipped. + /// + /// @returns + /// Return `true` if the spec is valid and passes all checks, + /// otherwise return `false` and make appropriate calls to + /// this->errorfmt() to record the errors. + /// + /// Checks performed include: + /// + /// * Whether the resolution and channel count are within the range + /// implied by `range`. + /// * Whether the channel count is within the `"limit:channels"` OIIO + /// attribute. + /// * The total uncompressed pixel data size is expected to be within the + /// `"limit:imagesize_MB"` OIIO attribute. + /// + bool check_open (const ImageSpec &spec, + ROI range = {0, 65535, 0, 65535, 0, 1, 0, 4}, + uint64_t flags = 0); + + /// Bit field definitions for the `flags` argument to `check_open()`. + enum class OpenChecks : uint64_t { + Defaults = 0, + // Reserved for future use + }; + /// @} private: @@ -2526,7 +2577,8 @@ class OIIO_API ImageOutput { /// is the request to write volumetric data but the format writer /// doesn't support it). /// - /// Returns true if ok, false if the open request can't be satisfied. + /// Returns true if ok, false if the open request can't be satisfied (and + /// makes appropriate calls to this->errorfmt() to record the errors). /// /// Having a central helper method for this is beneficial: /// @@ -2562,6 +2614,11 @@ class OIIO_API ImageOutput { /// indicate additional checks to perform, or checks that should be /// skipped. /// + /// @returns + /// Return `true` if the spec is valid and passes all checks, + /// otherwise return `false` and make appropriate calls to + /// this->errorfmt() to record the errors. + /// /// Checks performed include: /// /// * Whether the open `mode` is one allowed by `supports("multiimage")` diff --git a/src/libOpenImageIO/imageinput.cpp b/src/libOpenImageIO/imageinput.cpp index c56428c971..af96160995 100644 --- a/src/libOpenImageIO/imageinput.cpp +++ b/src/libOpenImageIO/imageinput.cpp @@ -1286,4 +1286,63 @@ ImageInput::iotell() const } + +bool +ImageInput::check_open(const ImageSpec& spec, ROI range, uint64_t /*flags*/) +{ + // Check for sensible resolutions, etc. + if ((m_spec.width <= 0 || m_spec.height <= 0 || m_spec.depth <= 0 + || m_spec.nchannels <= 0) + && !supports("noimage")) { + errorfmt( + "{} image resolution must be at least 1x1, but the file specified {}x{}. Possible corrupt input?", + format_name(), m_spec.width, m_spec.height); + return false; + } + if (m_spec.depth > 1) { + if (m_spec.width > range.width() || m_spec.height > range.height() + || m_spec.depth > range.depth()) { + errorfmt( + "{} image resolution may not exceed {}x{}x{}, but the file appears to be {}x{}x{}. Possible corrupt input?", + format_name(), range.width(), range.height(), range.depth(), + m_spec.width, m_spec.height, m_spec.depth); + return false; + } + } else { + if (m_spec.width > range.width() || m_spec.height > range.height()) { + errorfmt( + "{} image resolution may not exceed {}x{}, but the file appears to be {}x{}. Possible corrupt input?", + format_name(), range.width(), range.height(), m_spec.width, + m_spec.height); + return false; + } + } + if (spec.nchannels > range.nchannels()) { + errorfmt( + "{} does not support {}-channel images. Possible corrupt input?", + format_name(), spec.nchannels); + return false; + } + if (pvt::limit_channels && spec.nchannels > pvt::limit_channels) { + errorfmt( + "{} channels exceeds \"limits:channels\" = {}. Possible corrupt input?\nIf you're sure this is a valid file, raise the OIIO global attribute \"limits:channels\".", + spec.nchannels, pvt::limit_channels); + return false; + } + if (pvt::limit_imagesize_MB + && spec.image_bytes(true) + > pvt::limit_imagesize_MB * imagesize_t(1024 * 1024)) { + errorfmt( + "Uncompressed image size {:.1f} MB exceeds the {} MB limit.\n" + "Image claimed to be {}x{}, {}-channel {}. Possible corrupt input?\n" + "If this is a valid file, raise the OIIO attribute \"limits:imagesize_MB\".", + float(m_spec.image_bytes(true)) / float(1024 * 1024), + pvt::limit_imagesize_MB, m_spec.width, m_spec.height, + m_spec.nchannels, m_spec.format); + return false; + } + + return true; // all is ok +} + OIIO_NAMESPACE_END diff --git a/src/targa.imageio/targainput.cpp b/src/targa.imageio/targainput.cpp index 4a46d5ea0c..8c24fcf876 100644 --- a/src/targa.imageio/targainput.cpp +++ b/src/targa.imageio/targainput.cpp @@ -286,18 +286,8 @@ TGAInput::open(const std::string& name, ImageSpec& newspec) } m_spec.attribute("targa:version", int(m_tga_version)); - if (pvt::limit_imagesize_MB - && m_spec.image_bytes(true) - > pvt::limit_imagesize_MB * imagesize_t(1024 * 1024)) { - errorfmt( - "Uncompressed image size {:.1f} MB exceeds the {} MB limit.\n" - "Image claimed to be {}x{}, {}-channel {}. Possible corrupt input?\n" - "If this is a valid file, raise the OIIO attribute \"limits:imagesize_MB\".", - float(m_spec.image_bytes(true)) / float(1024 * 1024), - pvt::limit_imagesize_MB, m_spec.width, m_spec.height, - m_spec.nchannels, m_spec.format); + if (!check_open(m_spec)) return false; - } if (m_spec.alpha_channel != -1 && m_alpha_type == TGA_ALPHA_USEFUL && m_keep_unassociated_alpha) diff --git a/src/tiff.imageio/tiffinput.cpp b/src/tiff.imageio/tiffinput.cpp index 4e9ad50f22..2c53d7e274 100644 --- a/src/tiff.imageio/tiffinput.cpp +++ b/src/tiff.imageio/tiffinput.cpp @@ -839,24 +839,9 @@ TIFFInput::seek_subimage(int subimage, int miplevel) errorfmt("No support for data format of \"{}\"", m_filename); return false; } - if (pvt::limit_channels && m_spec.nchannels > pvt::limit_channels) { - errorfmt( - "{} channels exceeds \"limits:channels\" = {}. Possible corrupt input?\nIf you're sure this is a valid file, raise the OIIO global attribute \"limits:channels\".", - m_spec.nchannels, pvt::limit_channels); + if (!check_open(m_spec, + { 0, 1 << 20, 0, 1 << 20, 0, 1 << 16, 0, 1 << 16 })) return false; - } - if (pvt::limit_imagesize_MB - && m_spec.image_bytes(true) - > pvt::limit_imagesize_MB * imagesize_t(1024 * 1024)) { - errorfmt( - "Uncompressed image size {:.1f} MB exceeds the {} MB limit.\n" - "Image claimed to be {}x{}, {}-channel {}. Possible corrupt input?\n" - "If this is a valid file, raise the OIIO attribute \"limits:imagesize_MB\".", - float(m_spec.image_bytes(true)) / float(1024 * 1024), - pvt::limit_imagesize_MB, m_spec.width, m_spec.height, - m_spec.nchannels, m_spec.format); - return false; - } return true; } else { std::string e = oiio_tiff_last_error();