From cff26040d8e4019e2c9db950d7986f6422c6711b Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Sat, 30 Apr 2022 15:41:14 -0400 Subject: [PATCH] Using insecure crytpo from the CLI is now an error by default --- ChangeLog | 6 ++++++ TODO | 10 ---------- job.sums | 2 +- libqpdf/QPDFJob.cc | 11 +++++------ qpdf/qpdf.testcov | 2 +- qpdf/qtest/qpdf.test | 13 ++++++------- 6 files changed, 19 insertions(+), 25 deletions(-) diff --git a/ChangeLog b/ChangeLog index 2f06936f..6826703b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2022-04-30 Jay Berkenbilt + + * Make attempting to write encrypted files that use RC4 (40-bit or + 128-bit without AES) an error rather than a warning when + --allow-weak-crypto is not specified. + 2022-04-29 Jay Berkenbilt * QPDFObjectHandle: for the methods insertItem, appendItem, diff --git a/TODO b/TODO index a3394989..96ee2455 100644 --- a/TODO +++ b/TODO @@ -486,16 +486,6 @@ in source and header files to find items not listed here. developer and user experience. We don't want to create a situation where exactly the same code fails to work in 11 but worked on 10. See #576 for latest notes. - * Change deterministic id to use something other than MD5 but allow - the old way for compatibility -- maybe rename the method to force - the developer to make a choice - * Find other uses of MD5 and find the ones that are discretionary, - if any - * Have QPDFWriter raise an exception if it's about to write using - weak crypto and hasn't been given permission - * Search for --allow-weak-crypto in the manual and in qpdf.cc's help - information - * Update the ref.weak-crypto section of the manual Page splitting/merging ====================== diff --git a/job.sums b/job.sums index e1fce3af..0368fc48 100644 --- a/job.sums +++ b/job.sums @@ -14,4 +14,4 @@ libqpdf/qpdf/auto_job_json_decl.hh 06caa46eaf71db8a50c046f91866baa8087745a947431 libqpdf/qpdf/auto_job_json_init.hh 06d51f11c117011256e175386eee9946441f3c22b49dd91fc591bbc1fa3bbeec libqpdf/qpdf/auto_job_schema.hh 43273b9edfc48b1f4cccbff1d2b31916a9057c474ef97d2936b2f1f14170885b manual/_ext/qpdf.py e9ac9d6c70642a3d29281ee5ad92ae2422dee8be9306fb8a0bc9dba0ed5e28f3 -manual/cli.rst aa44cbe7b6281ee05dc8b19ee1b12ca770503681ffc8ba90e795fc3c3b55153d +manual/cli.rst 6a2d99acedbd207370a8dc2807f6657323c42bccbe51ebdc6bc2d00f6851219c diff --git a/libqpdf/QPDFJob.cc b/libqpdf/QPDFJob.cc index 4d136223..2459394f 100644 --- a/libqpdf/QPDFJob.cc +++ b/libqpdf/QPDFJob.cc @@ -2812,18 +2812,17 @@ QPDFJob::setEncryptionOptions(QPDF& pdf, QPDFWriter& w) maybeFixWritePassword(R, m->owner_password); if ((R < 4) || ((R == 4) && (!m->use_aes))) { if (!m->allow_weak_crypto) { - // Do not set warnings = true for this case as this does - // not reflect a potential problem with the input file. - QTC::TC("qpdf", "QPDFJob weak crypto warning"); + QTC::TC("qpdf", "QPDFJob weak crypto error"); *(this->m->cerr) << this->m->message_prefix - << ": writing a file with RC4, a weak cryptographic algorithm" + << ": refusing to write a file with RC4, a weak cryptographic algorithm" << std::endl << "Please use 256-bit keys for better security." << std::endl - << "Pass --allow-weak-crypto to suppress this warning." + << "Pass --allow-weak-crypto to enable writing insecure files." << std::endl - << "This will become an error in a future version of qpdf." + << "See also https://qpdf.readthedocs.io/en/stable/weak-crypto.html" << std::endl; + throw std::runtime_error("refusing to write a file with weak crypto"); } } switch (R) { diff --git a/qpdf/qpdf.testcov b/qpdf/qpdf.testcov index d679fa25..92ac66f8 100644 --- a/qpdf/qpdf.testcov +++ b/qpdf/qpdf.testcov @@ -605,7 +605,7 @@ QPDFWriter exclude from object stream 0 QPDF_pages findPage not found 0 QPDFJob overlay page with no resources 0 QPDFObjectHandle check ownership 0 -QPDFJob weak crypto warning 0 +QPDFJob weak crypto error 0 qpdf-c called qpdf_oh_is_initialized 0 qpdf-c registered progress reporter 0 qpdf-c called qpdf_oh_new_uninitialized 0 diff --git a/qpdf/qtest/qpdf.test b/qpdf/qtest/qpdf.test index 4dcf7384..d8359f75 100644 --- a/qpdf/qtest/qpdf.test +++ b/qpdf/qtest/qpdf.test @@ -3983,16 +3983,15 @@ $td->runtest("128-bit with AES: no warning", ' minimal.pdf a.pdf'}, {$td->STRING => "", $td->EXIT_STATUS => 0}, $td->NORMALIZE_NEWLINES); -# Note: we intentionally have exit status 0 for this warning. -$td->runtest("128-bit without AES: warning", +$td->runtest("128-bit without AES: error", {$td->COMMAND => 'qpdf --encrypt "" "" 128 -- minimal.pdf a.pdf'}, - {$td->REGEXP => "Pass --allow-weak-crypto to suppress", - $td->EXIT_STATUS => 0}, + {$td->REGEXP => "Pass --allow-weak-crypto to enable", + $td->EXIT_STATUS => 2}, $td->NORMALIZE_NEWLINES); -$td->runtest("40-bit: warning", +$td->runtest("40-bit: error", {$td->COMMAND => 'qpdf --encrypt "" "" 40 -- minimal.pdf a.pdf'}, - {$td->REGEXP => "Pass --allow-weak-crypto to suppress", - $td->EXIT_STATUS => 0}, + {$td->REGEXP => "Pass --allow-weak-crypto to enable", + $td->EXIT_STATUS => 2}, $td->NORMALIZE_NEWLINES); show_ntests();