Require --allow-insecure to create certain encrypted files (fixes #501)

For now, --allow-insecure allows creation of files with the owner
passwords empty or matching the user password.

5 files changed, 137 insertions, 11 deletions

diff --git a/ChangeLog b/ChangeLog
index 2670dd07..32410df9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2021-02-04  Jay Berkenbilt  <ejb@ql.org>
+
+	* By default, give an error if a user attempts to encrypt a file
+	with an empty owner password or an owner password that is the same
+	as the user password. Such files are insecure. Most viewers either
+	won't open such files or will not enforce security settings. To
+	allow explicit creation of files like this, pass the new
+	--allow-insecure option. Fixes #501.
+
 2021-02-02  Jay Berkenbilt  <ejb@ql.org>
 
 	* Bug fix: if a form XObject lacks a resources dictionary,
diff --git a/manual/qpdf-manual.xml b/manual/qpdf-manual.xml
index fda9c1fd..0d9fd489 100644
--- a/manual/qpdf-manual.xml
+++ b/manual/qpdf-manual.xml
@@ -1214,7 +1214,11 @@ make
    </para>
    <para>
     Either or both of the user password and the owner password may be
-    empty strings.
+    empty strings. Starting in qpdf 10.2, qpdf defaults to not
+    allowing creation of PDF files with an empty owner password or an
+    owner password that matches the user password. If you want to
+    create such files, specify the encryption option
+    <option>--allow-insecure</option>, as described below.
    </para>
    <para>
     The value for
@@ -1224,6 +1228,25 @@ make
     fully permissive.
    </para>
    <para>
+    For all key lengths, the following options are available:
+    <variablelist>
+     <varlistentry>
+      <term><option>--allow-insecure</option></term>
+      <listitem>
+       <para>
+        From qpdf 10.2, qpdf defaults to not allowing creation of PDF
+        files where the owner password is blank or matches the user
+        password. Files created in this way are insecure and can't be
+        opened by some viewers. Users would ordinarily never want to
+        create such files. If you are using qpdf to intentionally
+        created strange files for testing (a definite valid use of
+        qpdf!), this option allows you to create such insecure files.
+       </para>
+      </listitem>
+     </varlistentry>
+    </variablelist>
+   </para>
+   <para>
     If <option><replaceable>key-length</replaceable></option> is 40,
     the following restriction options are available:
     <variablelist>
@@ -4824,7 +4847,28 @@ print "\n";
      <itemizedlist>
       <listitem>
        <para>
-        Behavior Changes
+        CLI Behavior Changes
+       </para>
+       <itemizedlist>
+        <listitem>
+         <para>
+          By default, <command>qpdf</command> no longer allows
+          creation of encrypted PDF files whose owner password is
+          empty or matches the user password. The
+          <option>--allow-insecure</option>, specified inside the
+          <option>--encrypt</option> options, allows creation of such
+          files. Behavior changes in the CLI are avoided when
+          possible, but an exception was made here because this is
+          security-related. qpdf must always allow creation of weird
+          files for testing purposes, but it should not default to
+          letting users unknowingly create insecure files.
+         </para>
+        </listitem>
+       </itemizedlist>
+      </listitem>
+      <listitem>
+       <para>
+        Library Behavior Changes
        </para>
        <itemizedlist>
         <listitem>
diff --git a/qpdf/qpdf.cc b/qpdf/qpdf.cc
index 0405a45d..702a6b9e 100644
--- a/qpdf/qpdf.cc
+++ b/qpdf/qpdf.cc
@@ -113,6 +113,7 @@ struct Options
         password_is_hex_key(false),
         suppress_password_recovery(false),
         password_mode(pm_auto),
+        allow_insecure(false),
         keylen(0),
         r2_print(true),
         r2_modify(true),
@@ -211,6 +212,7 @@ struct Options
     bool password_is_hex_key;
     bool suppress_password_recovery;
     password_mode_e password_mode;
+    bool allow_insecure;
     std::string user_password;
     std::string owner_password;
     int keylen;
@@ -742,6 +744,7 @@ class ArgParser
     void argEncrypt();
     void argDecrypt();
     void argPasswordIsHexKey();
+    void argAllowInsecure();
     void argPasswordMode(char* parameter);
     void argSuppressPasswordRecovery();
     void argCopyEncryption(char* parameter);
@@ -1074,13 +1077,17 @@ ArgParser::initOptionTable()
 
     t = &this->encrypt40_option_table;
     (*t)["--"] = oe_bare(&ArgParser::argEndEncrypt);
+    (*t)["allow-insecure"] = oe_bare(&ArgParser::argAllowInsecure);
+    // The above 40-bit options are also 128-bit and 256-bit options,
+    // so copy what we have so far to 128. Then continue separately
+    // with 128. We later copy 128 to 256.
+    this->encrypt128_option_table = this->encrypt40_option_table;
     (*t)["print"] = oe_requiredChoices(&ArgParser::arg40Print, yn);
     (*t)["modify"] = oe_requiredChoices(&ArgParser::arg40Modify, yn);
     (*t)["extract"] = oe_requiredChoices(&ArgParser::arg40Extract, yn);
     (*t)["annotate"] = oe_requiredChoices(&ArgParser::arg40Annotate, yn);
 
     t = &this->encrypt128_option_table;
-    (*t)["--"] = oe_bare(&ArgParser::argEndEncrypt);
     (*t)["accessibility"] = oe_requiredChoices(
         &ArgParser::arg128Accessibility, yn);
     (*t)["extract"] = oe_requiredChoices(&ArgParser::arg128Extract, yn);
@@ -1317,6 +1324,10 @@ ArgParser::argHelp()
         << "\n"
         << "Additional flags are dependent upon key length.\n"
         << "\n"
+        << "  For all key lengths:\n"
+        << "    --allow-insecure         allow the owner password to be empty or the\n"
+        << "                             same as the user password\n"
+        << "\n"
         << "  If 40:\n"
         << "\n"
         << "    --print=[yn]             allow printing\n"
@@ -1850,6 +1861,12 @@ ArgParser::argPasswordMode(char* parameter)
 }
 
 void
+ArgParser::argAllowInsecure()
+{
+    o.allow_insecure = true;
+}
+
+void
 ArgParser::argCopyEncryption(char* parameter)
 {
     o.encryption_file = parameter;
@@ -3337,6 +3354,18 @@ ArgParser::doFinalChecks()
               " together");
     }
 
+    if (o.encrypt && (! o.allow_insecure) &&
+        (o.owner_password.empty() ||
+         (o.owner_password == o.user_password)))
+    {
+        usage("An encrypted PDF with an empty owner password or an"
+              " owner password that is the same as a user password"
+              " is insecure and can't be opened by some viewers. If you"
+              " really want to do this, you must also give the"
+              " --allow-insecure option before the -- that follows"
+              " --encrypt.");
+    }
+
     if (o.require_outfile && o.outfilename &&
         (strcmp(o.outfilename, "-") == 0))
     {
diff --git a/qpdf/qtest/qpdf.test b/qpdf/qtest/qpdf.test
index 0ec1b834..f16210e7 100644
--- a/qpdf/qtest/qpdf.test
+++ b/qpdf/qtest/qpdf.test
@@ -3189,19 +3189,19 @@ foreach my $f (qw(compressed-metadata.pdf enc-base.pdf))
 	check_metadata("a.pdf", 0, 1);
 	$td->runtest("encrypt normally",
 		     {$td->COMMAND =>
-			  "qpdf --encrypt '' '' 128 -- a.pdf b.pdf"},
+			  "qpdf --encrypt '' o 128 -- a.pdf b.pdf"},
 		     {$td->STRING => "", $td->EXIT_STATUS => 0});
 	check_metadata("b.pdf", 1, 0);
 	unlink "b.pdf";
 	$td->runtest("encrypt V4",
 		     {$td->COMMAND =>
-			  "qpdf --encrypt '' '' 128 --force-V4 -- a.pdf b.pdf"},
+			  "qpdf --encrypt '' o 128 --force-V4 -- a.pdf b.pdf"},
 		     {$td->STRING => "", $td->EXIT_STATUS => 0});
 	check_metadata("b.pdf", 1, 0);
 	unlink "b.pdf";
 	$td->runtest("encrypt with cleartext metadata",
 		     {$td->COMMAND =>
-			  "qpdf --encrypt '' '' 128 --cleartext-metadata --" .
+			  "qpdf --encrypt '' o 128 --cleartext-metadata --" .
 			  " a.pdf b.pdf"},
 		     {$td->STRING => "", $td->EXIT_STATUS => 0});
 	check_metadata("b.pdf", 1, 1);
@@ -3212,7 +3212,7 @@ foreach my $f (qw(compressed-metadata.pdf enc-base.pdf))
 	unlink "b.pdf", "c.pdf";
 	$td->runtest("encrypt with aes and cleartext metadata",
 		     {$td->COMMAND =>
-			  "qpdf --encrypt '' '' 128" .
+			  "qpdf --encrypt '' o 128" .
 			  " --cleartext-metadata --use-aes=y -- a.pdf b.pdf"},
 		     {$td->STRING => "", $td->EXIT_STATUS => 0});
 	check_metadata("b.pdf", 1, 1);
@@ -3441,7 +3441,7 @@ my @encrypted_files =
       1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1],
     );
 
-$n_tests += 5 + (2 * (@encrypted_files)) + (7 * (@encrypted_files - 6)) + 9;
+$n_tests += 9 + (2 * (@encrypted_files)) + (7 * (@encrypted_files - 6)) + 9;
 
 $td->runtest("encrypted file",
 	     {$td->COMMAND => "test_driver 2 encrypted-with-images.pdf"},
@@ -3458,6 +3458,26 @@ $td->runtest("recheck encrypted file",
 	      $td->EXIT_STATUS => 0},
 	     $td->NORMALIZE_NEWLINES);
 
+$td->runtest("empty owner password",
+             {$td->COMMAND => "qpdf --encrypt '' '' 128 -- minimal.pdf a.pdf"},
+             {$td->REGEXP => ".*is insecure.*--allow-insecure.*",
+                  $td->EXIT_STATUS => 2},
+             $td->NORMALIZE_NEWLINES);
+$td->runtest("matching user/owner password",
+             {$td->COMMAND => "qpdf --encrypt q q 128 -- minimal.pdf a.pdf"},
+             {$td->REGEXP => ".*is insecure.*--allow-insecure.*",
+                  $td->EXIT_STATUS => 2},
+             $td->NORMALIZE_NEWLINES);
+$td->runtest("allow insecure",
+             {$td->COMMAND => "qpdf --encrypt '' '' 128 --allow-insecure --" .
+                  " minimal.pdf a.pdf"},
+             {$td->STRING => "", $td->EXIT_STATUS => 0},
+             $td->NORMALIZE_NEWLINES);
+$td->runtest("check insecure",
+             {$td->COMMAND => "qpdf --check a.pdf"},
+             {$td->FILE => "insecure-passwords.out", $td->EXIT_STATUS => 0},
+             $td->NORMALIZE_NEWLINES);
+
 # Test that long passwords that are one character too short fail.  We
 # test the truncation cases in the loop below by using passwords
 # longer than the supported length.
@@ -3587,6 +3607,10 @@ foreach my $d (@encrypted_files)
         $enc_json =~ s/---upm---/$upm/;
 
 	my $eflags = "-encrypt \"$upass\" \"$opass\" $bits $xeflags --";
+        if (($opass eq "") || ($opass eq $upass))
+        {
+            $eflags =~ s/--$/--allow-insecure --/;
+        }
         if (($pass ne $upass) && ($V >= 5))
         {
             # V >= 5 can no longer recover user password with owner
@@ -3758,7 +3782,7 @@ $td->runtest("check linearization",
 # Test AES encryption in various ways.
 $n_tests += 18;
 $td->runtest("encrypt with AES",
-	     {$td->COMMAND => "qpdf --encrypt '' '' 128 --use-aes=y --" .
+	     {$td->COMMAND => "qpdf --encrypt '' o 128 --use-aes=y --" .
 		  " enc-base.pdf a.pdf"},
 	     {$td->STRING => "", $td->EXIT_STATUS => 0});
 $td->runtest("check encryption",
@@ -3779,7 +3803,7 @@ $td->runtest("compare files",
 	     {$td->FILE => 'a.qdf'},
 	     {$td->FILE => 'b.qdf'});
 $td->runtest("linearize with AES and object streams",
-	     {$td->COMMAND => "qpdf --encrypt '' '' 128 --use-aes=y --" .
+	     {$td->COMMAND => "qpdf --encrypt '' o 128 --use-aes=y --" .
 		  " --linearize --object-streams=generate enc-base.pdf a.pdf"},
 	     {$td->STRING => "", $td->EXIT_STATUS => 0});
 $td->runtest("check encryption",
@@ -3845,7 +3869,8 @@ foreach my $d (['--force-V4', 'V4'],
     my ($args, $out) = @$d;
     $td->runtest("encrypt $args",
 		 {$td->COMMAND => "qpdf --static-aes-iv --static-id" .
-		      " --encrypt '' '' 128 $args -- enc-base.pdf a.pdf"},
+		      " --encrypt '' '' 128 $args --allow-insecure --" .
+                      " enc-base.pdf a.pdf"},
 		 {$td->STRING => "", $td->EXIT_STATUS => 0});
     $td->runtest("check output",
 		 {$td->FILE => "a.pdf"},
diff --git a/qpdf/qtest/qpdf/insecure-passwords.out b/qpdf/qtest/qpdf/insecure-passwords.out
new file mode 100644
index 00000000..f0ba33e8
--- /dev/null
+++ b/qpdf/qtest/qpdf/insecure-passwords.out
@@ -0,0 +1,19 @@
+checking a.pdf
+PDF Version: 1.4
+R = 3
+P = -4
+User password = 
+Supplied password is owner password
+Supplied password is user password
+extract for accessibility: allowed
+extract for any purpose: allowed
+print low resolution: allowed
+print high resolution: allowed
+modify document assembly: allowed
+modify forms: allowed
+modify annotations: allowed
+modify other: allowed
+modify anything: allowed
+File is not linearized
+No syntax or stream encoding errors found; the file may still contain
+errors that qpdf cannot detect