Merge pull request #1350 from m-holger/fuzz

Apply sanity checks on SF_FlateLzwDecode parameters

Merge pull request #1350 from m-holger/fuzz
Apply sanity checks on SF_FlateLzwDecode parameters
m-holger · GitHub
2 parents f5cac93a 1db68172
Showing 15 changed files with 78 additions and 67 deletions
.git-blame-ignore-revs
fuzz/CMakeLists.txt
fuzz/qpdf_crypt_fuzzer.cc
fuzz/qpdf_crypt_insecure_fuzzer.cc
fuzz/qpdf_extra/394463491.fuzz
fuzz/qpdf_fuzzer.cc
fuzz/qpdf_lin_fuzzer.cc
fuzz/qpdf_outlines_fuzzer.cc
fuzz/qpdf_pages_fuzzer.cc
fuzz/qtest/fuzz.test
include/qpdf/Pl_Flate.hh
libqpdf/Pl_Flate.cc
libqpdf/QPDFJob_argv.cc
libqpdf/SF_FlateLzwDecode.cc
libqpdf/qpdf/SF_FlateLzwDecode.hh
@@ -18,3 +18,5 @@ d740c6ccced02147f84a39d5e5f0984d12bac6cb
 9ae7bdea966102f9621b22192747a891078e7470
 # Normalize white space in ChangeLog
 d7b909f97d3effc9540c35b0251bdf1c9abf187c
+# Remove 'this->'
+f5cac93ac63559ddc0aec1b1c61c9e2503c7b8e0
@@ -154,6 +154,7 @@ set(CORPUS_OTHER
   389974979.fuzz
   391974927.fuzz
   394129398.fuzz
+  394463491.fuzz
 )
 set(CORPUS_DIR ${CMAKE_CURRENT_BINARY_DIR}/qpdf_corpus)
@@ -111,7 +111,7 @@ FuzzHelper::doChecks()
     Pl_PNGFilter::setMemoryLimit(1'000'000);
     Pl_RunLength::setMemoryLimit(1'000'000);
     Pl_TIFFPredictor::setMemoryLimit(1'000'000);
-    Pl_Flate::setMemoryLimit(200'000);
+    Pl_Flate::memory_limit(200'000);
     // Do not decompress corrupt data. This may cause extended runtime within jpeglib without
     // exercising additional code paths in qpdf, and potentially causing counterproductive timeouts.
@@ -111,7 +111,7 @@ FuzzHelper::doChecks()
     Pl_PNGFilter::setMemoryLimit(1'000'000);
     Pl_RunLength::setMemoryLimit(1'000'000);
     Pl_TIFFPredictor::setMemoryLimit(1'000'000);
-    Pl_Flate::setMemoryLimit(200'000);
+    Pl_Flate::memory_limit(200'000);
     // Do not decompress corrupt data. This may cause extended runtime within jpeglib without
     // exercising additional code paths in qpdf, and potentially causing counterproductive timeouts.
@@ -109,7 +109,7 @@ FuzzHelper::doChecks()
     Pl_PNGFilter::setMemoryLimit(1'000'000);
     Pl_RunLength::setMemoryLimit(1'000'000);
     Pl_TIFFPredictor::setMemoryLimit(1'000'000);
-    Pl_Flate::setMemoryLimit(200'000);
+    Pl_Flate::memory_limit(200'000);
     // Do not decompress corrupt data. This may cause extended runtime within jpeglib without
     // exercising additional code paths in qpdf, and potentially causing counterproductive timeouts.
@@ -110,7 +110,7 @@ FuzzHelper::doChecks()
     Pl_PNGFilter::setMemoryLimit(1'000'000);
     Pl_RunLength::setMemoryLimit(1'000'000);
     Pl_TIFFPredictor::setMemoryLimit(1'000'000);
-    Pl_Flate::setMemoryLimit(200'000);
+    Pl_Flate::memory_limit(200'000);
     // Do not decompress corrupt data. This may cause extended runtime within jpeglib without
     // exercising additional code paths in qpdf, and potentially causing counterproductive timeouts.
@@ -87,7 +87,7 @@ FuzzHelper::doChecks()
     Pl_PNGFilter::setMemoryLimit(1'000'000);
     Pl_RunLength::setMemoryLimit(1'000'000);
     Pl_TIFFPredictor::setMemoryLimit(1'000'000);
-    Pl_Flate::setMemoryLimit(200'000);
+    Pl_Flate::memory_limit(200'000);
     // Do not decompress corrupt data. This may cause extended runtime within jpeglib without
     // exercising additional code paths in qpdf, and potentially causing counterproductive timeouts.
@@ -108,7 +108,7 @@ FuzzHelper::doChecks()
     Pl_PNGFilter::setMemoryLimit(1'000'000);
     Pl_RunLength::setMemoryLimit(1'000'000);
     Pl_TIFFPredictor::setMemoryLimit(1'000'000);
-    Pl_Flate::setMemoryLimit(200'000);
+    Pl_Flate::memory_limit(200'000);
     // Do not decompress corrupt data. This may cause extended runtime within jpeglib without
     // exercising additional code paths in qpdf, and potentially causing counterproductive timeouts.
@@ -11,7 +11,7 @@ my $td = new TestDriver(&#39;fuzz&#39;);
 my $qpdf_corpus = $ENV{'QPDF_FUZZ_CORPUS'} || die "must set QPDF_FUZZ_CORPUS";
-my $n_qpdf_files = 91;       # increment when adding new files
+my $n_qpdf_files = 92;       # increment when adding new files
 my @fuzzers = (
     ['ascii85' => 1],
@@ -48,7 +48,9 @@ class QPDF_DLL_CLASS Pl_Flate: public Pipeline
     // Limit the memory used.
     // NB This is a static option affecting all Pl_Flate instances.
     QPDF_DLL
-    static void setMemoryLimit(unsigned long long limit);
+    static unsigned long long memory_limit();
+    QPDF_DLL
+    static void memory_limit(unsigned long long limit);
     QPDF_DLL
     void write(unsigned char const* data, size_t len) override;
@@ -14,7 +14,7 @@
 namespace
 {
-    unsigned long long memory_limit{0};
+    unsigned long long memory_limit_{0};
 } // namespace
 int Pl_Flate::compression_level = Z_DEFAULT_COMPRESSION;
@@ -80,10 +80,16 @@ Pl_Flate::~Pl_Flate() // NOLINT (modernize-use-equals-default)
     // Must be explicit and not inline -- see QPDF_DLL_CLASS in README-maintainer
 }
+unsigned long long
+Pl_Flate::memory_limit()
+{
+    return memory_limit_;
+}
+
 void
-Pl_Flate::setMemoryLimit(unsigned long long limit)
+Pl_Flate::memory_limit(unsigned long long limit)
 {
-    memory_limit = limit;
+    memory_limit_ = limit;
 }
 void
@@ -197,9 +203,9 @@ Pl_Flate::handleData(unsigned char const* data, size_t len, int flush)
                 }
                 uLong ready = QIntC::to_ulong(m->out_bufsize - zstream.avail_out);
                 if (ready > 0) {
-                    if (memory_limit && m->action != a_deflate) {
+                    if (memory_limit_ && m->action != a_deflate) {
                         m->written += ready;
-                        if (m->written > memory_limit) {
+                        if (m->written > memory_limit_) {
                             throw std::runtime_error("PL_Flate memory limit exceeded");
                         }
                     }
@@ -220,7 +226,7 @@ Pl_Flate::handleData(unsigned char const* data, size_t len, int flush)
 void
 Pl_Flate::finish()
 {
-    if (m->written > memory_limit) {
+    if (m->written > memory_limit_) {
         throw std::runtime_error("PL_Flate memory limit exceeded");
     }
     try {
@@ -117,8 +117,10 @@ ArgParser::argZopfli()
             logger->info("Set the environment variable QPDF_ZOPFLI to activate.\n");
             logger->info("* QPDF_ZOPFLI=disabled or QPDF_ZOPFLI not set: don't use zopfli.\n");
             logger->info("* QPDF_ZOPFLI=force: use zopfli, and fail if not available.\n");
-            logger->info("* QPDF_ZOPFLI=silent: use zopfli if available and silently fall back if not.\n");
-            logger->info("* QPDF_ZOPFLI= any other value: use zopfli if available, and warn if not.\n");
+            logger->info(
+                "* QPDF_ZOPFLI=silent: use zopfli if available and silently fall back if not.\n");
+            logger->info(
+                "* QPDF_ZOPFLI= any other value: use zopfli if available, and warn if not.\n");
         }
     } else {
         logger->error("zopfli support is not enabled\n");
@@ -7,17 +7,6 @@
 #include <qpdf/QIntC.hh>
 #include <qpdf/QTC.hh>
-SF_FlateLzwDecode::SF_FlateLzwDecode(bool lzw) :
-    lzw(lzw),
-    // Initialize values to their defaults as per the PDF spec
-    predictor(1),
-    columns(1),
-    colors(1),
-    bits_per_component(8),
-    early_code_change(true)
-{
-}
-
 bool
 SF_FlateLzwDecode::setDecodeParms(QPDFObjectHandle decode_parms)
 {
@@ -25,78 +14,83 @@ SF_FlateLzwDecode::setDecodeParms(QPDFObjectHandle decode_parms)
         return true;
     }
-    bool filterable = true;
+    auto memory_limit = Pl_Flate::memory_limit();
+
     std::set<std::string> keys = decode_parms.getKeys();
     for (auto const& key: keys) {
         QPDFObjectHandle value = decode_parms.getKey(key);
         if (key == "/Predictor") {
             if (value.isInteger()) {
-                this->predictor = value.getIntValueAsInt();
-                if (!((this->predictor == 1) || (this->predictor == 2) ||
-                      ((this->predictor >= 10) && (this->predictor <= 15)))) {
-                    filterable = false;
+                predictor = value.getIntValueAsInt();
+                if (!(predictor == 1 || predictor == 2 || (predictor >= 10 && predictor <= 15))) {
+                    return false;
                 }
             } else {
-                filterable = false;
+                return false;
             }
-        } else if ((key == "/Columns") || (key == "/Colors") || (key == "/BitsPerComponent")) {
+        } else if (key == "/Columns" || key == "/Colors" || key == "/BitsPerComponent") {
             if (value.isInteger()) {
                 int val = value.getIntValueAsInt();
+                if (memory_limit && static_cast<unsigned int>(val) > memory_limit) {
+                    QPDFLogger::defaultLogger()->warn(
+                        "SF_FlateLzwDecode parameter exceeds PL_Flate memory limit\n");
+                    return false;
+                }
                 if (key == "/Columns") {
-                    this->columns = val;
+                    columns = val;
                 } else if (key == "/Colors") {
-                    this->colors = val;
+                    colors = val;
                 } else if (key == "/BitsPerComponent") {
-                    this->bits_per_component = val;
+                    bits_per_component = val;
                 }
             } else {
-                filterable = false;
+                return false;
             }
         } else if (lzw && (key == "/EarlyChange")) {
             if (value.isInteger()) {
                 int earlychange = value.getIntValueAsInt();
-                this->early_code_change = (earlychange == 1);
-                if (!((earlychange == 0) || (earlychange == 1))) {
-                    filterable = false;
+                early_code_change = (earlychange == 1);
+                if (!(earlychange == 0 || earlychange == 1)) {
+                    return false;
                 }
             } else {
-                filterable = false;
+                return false;
             }
         }
     }
-    if ((this->predictor > 1) && (this->columns == 0)) {
-        filterable = false;
+    if (predictor > 1 && columns == 0) {
+        return false;
     }
-    return filterable;
+    return true;
 }
 Pipeline*
 SF_FlateLzwDecode::getDecodePipeline(Pipeline* next)
 {
     std::shared_ptr<Pipeline> pipeline;
-    if ((this->predictor >= 10) && (this->predictor <= 15)) {
+    if (predictor >= 10 && predictor <= 15) {
         QTC::TC("qpdf", "SF_FlateLzwDecode PNG filter");
         pipeline = std::make_shared<Pl_PNGFilter>(
             "png decode",
             next,
             Pl_PNGFilter::a_decode,
-            QIntC::to_uint(this->columns),
-            QIntC::to_uint(this->colors),
-            QIntC::to_uint(this->bits_per_component));
-        this->pipelines.push_back(pipeline);
+            QIntC::to_uint(columns),
+            QIntC::to_uint(colors),
+            QIntC::to_uint(bits_per_component));
+        pipelines.push_back(pipeline);
         next = pipeline.get();
-    } else if (this->predictor == 2) {
+    } else if (predictor == 2) {
         QTC::TC("qpdf", "SF_FlateLzwDecode TIFF predictor");
         pipeline = std::make_shared<Pl_TIFFPredictor>(
             "tiff decode",
             next,
             Pl_TIFFPredictor::a_decode,
-            QIntC::to_uint(this->columns),
-            QIntC::to_uint(this->colors),
-            QIntC::to_uint(this->bits_per_component));
-        this->pipelines.push_back(pipeline);
+            QIntC::to_uint(columns),
+            QIntC::to_uint(colors),
+            QIntC::to_uint(bits_per_component));
+        pipelines.push_back(pipeline);
         next = pipeline.get();
     }
@@ -105,7 +99,7 @@ SF_FlateLzwDecode::getDecodePipeline(Pipeline* next)
     } else {
         pipeline = std::make_shared<Pl_Flate>("stream inflate", next, Pl_Flate::a_inflate);
     }
-    this->pipelines.push_back(pipeline);
+    pipelines.push_back(pipeline);
     return pipeline.get();
 }
@@ -5,25 +5,29 @@
 #ifndef SF_FLATELZWDECODE_HH
 # define SF_FLATELZWDECODE_HH
-class SF_FlateLzwDecode: public QPDFStreamFilter
+class SF_FlateLzwDecode final: public QPDFStreamFilter
 {
   public:
-    SF_FlateLzwDecode(bool lzw);
-    ~SF_FlateLzwDecode() override = default;
+    SF_FlateLzwDecode(bool lzw) :
+        lzw(lzw)
+    {
+    }
+    ~SF_FlateLzwDecode() final = default;
-    bool setDecodeParms(QPDFObjectHandle decode_parms) override;
-    Pipeline* getDecodePipeline(Pipeline* next) override;
+    bool setDecodeParms(QPDFObjectHandle decode_parms) final;
+    Pipeline* getDecodePipeline(Pipeline* next) final;
     static std::shared_ptr<QPDFStreamFilter> flate_factory();
     static std::shared_ptr<QPDFStreamFilter> lzw_factory();
   private:
-    bool lzw;
-    int predictor;
-    int columns;
-    int colors;
-    int bits_per_component;
-    bool early_code_change;
+    bool lzw{};
+    // Defaults as per the PDF spec
+    int predictor{1};
+    int columns{1};
+    int colors{1};
+    int bits_per_component{8};
+    bool early_code_change{true};
     std::vector<std::shared_ptr<Pipeline>> pipelines;
 };