Better fix to insecure password check (fixes #501)

Jay Berkenbilt
1 parent 63158cf5
Showing 5 changed files with 64 additions and 64 deletions
ChangeLog
manual/qpdf-manual.xml
qpdf/qpdf.cc
qpdf/qtest/qpdf.test
qpdf/qtest/qpdf/insecure-passwords.out
@@ -5,11 +5,11 @@
 	Fixes #499.
  
 	* 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.
+	with a 256-bit key, a non-empty user password, and an empty owner
+	password. Such files are insecure since they can be opened with no
+	password. To allow explicit creation of files like this, pass the
+	new --allow-insecure option. Thanks to github user RobK88 for a
+	detailed analysis and for reporting this issue. Fixes #501.
  
 2021-02-02  Jay Berkenbilt  <ejb@ql.org>
  
@@ -1239,10 +1239,11 @@ make
    <para>
     Either or both of the user password and the owner password may be
     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.
+    allowing creation of PDF files with a non-empty user password, an
+    empty owner password, and a 256-bit key since such files can be
+    opened with no password. If you want to create such files, specify
+    the encryption option <option>--allow-insecure</option>, as
+    described below.
    </para>
    <para>
     The value for
@@ -1252,25 +1253,6 @@ 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>
@@ -1466,6 +1448,21 @@ make
       </listitem>
      </varlistentry>
      <varlistentry>
+      <term><option>--allow-insecure</option></term>
+      <listitem>
+       <para>
+        From qpdf 10.2, qpdf defaults to not allowing creation of PDF
+        files where the user password is non-empty, the owner password
+        is empty, and a 256-bit key is in use. Files created in this
+        way are insecure since they can be opened without a password.
+        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>
+     <varlistentry>
       <term><option>--force-V4</option></term>
       <listitem>
        <para>
@@ -4877,15 +4874,16 @@ print &quot;\n&quot;;
         <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.
+          creation of encrypted PDF files whose user password is
+          non-empty and owner password is empty when a 256-bit key is
+          in use. The <option>--allow-insecure</option> 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>
@@ -1082,7 +1082,6 @@ 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.
@@ -1117,6 +1116,7 @@ ArgParser::initOptionTable()
  
     t = &this->encrypt256_option_table;
     (*t)["force-R5"] = oe_bare(&ArgParser::arg256ForceR5);
+    (*t)["allow-insecure"] = oe_bare(&ArgParser::argAllowInsecure);
  
     t = &this->under_overlay_option_table;
     (*t)[""] = oe_positional(&ArgParser::argUOpositional);
@@ -1333,10 +1333,6 @@ 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"
@@ -1362,6 +1358,9 @@ ArgParser::argHelp()
         << "    --force-V4               this option is not available with 256-bit keys\n"
         << "    --use-aes                this option is always on with 256-bit keys\n"
         << "    --force-R5               forces use of deprecated R=5 encryption\n"
+        << "    --allow-insecure         allow the owner password to be empty when the\n"
+        << "                             user password is not empty\n"
+        << "\n"
         << "\n"
         << "    print-opt may be:\n"
         << "\n"
@@ -3394,15 +3393,21 @@ ArgParser::doFinalChecks()
     }
  
     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.");
+        (o.owner_password.empty() &&
+         (! o.user_password.empty()) &&
+         (o.keylen == 256)))
+    {
+        // Note that empty owner passwords for R < 5 are copied from
+        // the user password, so this lack of security is not an issue
+        // for those files. Also we are consider only the ability to
+        // open the file without a password to be insecure. We are not
+        // concerned about whether the viwer enforces security
+        // settings when the user and owner password match.
+        usage("A PDF with a non-empty user password and an empty owner"
+              " password encrypted with a 256-bit key is insecure as it"
+              " can be opened without a password. 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 &&
@@ -3464,7 +3464,7 @@ my @encrypted_files =
       1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1],
     );
  
-$n_tests += 9 + (2 * (@encrypted_files)) + (7 * (@encrypted_files - 6)) + 9;
+$n_tests += 8 + (2 * (@encrypted_files)) + (7 * (@encrypted_files - 6)) + 9;
  
 $td->runtest("encrypted file",
 	     {$td->COMMAND => "test_driver 2 encrypted-with-images.pdf"},
@@ -3482,17 +3482,12 @@ $td-&gt;runtest(&quot;recheck encrypted file&quot;,
 	     $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->COMMAND => "qpdf --encrypt u '' 256 -- 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 --" .
+             {$td->COMMAND => "qpdf --encrypt u '' 256 --allow-insecure --" .
                   " minimal.pdf a.pdf"},
              {$td->STRING => "", $td->EXIT_STATUS => 0},
              $td->NORMALIZE_NEWLINES);
@@ -3630,7 +3625,7 @@ foreach my $d (@encrypted_files)
         $enc_json =~ s/---upm---/$upm/;
  
 	my $eflags = "-encrypt \"$upass\" \"$opass\" $bits $xeflags --";
-        if (($opass eq "") || ($opass eq $upass))
+        if (($opass eq "") && ($bits == 256))
         {
             $eflags =~ s/--$/--allow-insecure --/;
         }
@@ -3892,7 +3887,7 @@ foreach my $d ([&#39;--force-V4&#39;, &#39;V4&#39;],
     my ($args, $out) = @$d;
     $td->runtest("encrypt $args",
 		 {$td->COMMAND => "qpdf --static-aes-iv --static-id" .
-		      " --encrypt '' '' 128 $args --allow-insecure --" .
+		      " --encrypt '' '' 128 $args --" .
                       " enc-base.pdf a.pdf"},
 		 {$td->STRING => "", $td->EXIT_STATUS => 0});
     $td->runtest("check output",
 checking a.pdf
-PDF Version: 1.4
-R = 3
+PDF Version: 1.7 extension level 8
+R = 6
 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
@@ -14,6 +13,9 @@ modify forms: allowed
 modify annotations: allowed
 modify other: allowed
 modify anything: allowed
+stream encryption method: AESv3
+string encryption method: AESv3
+file encryption method: AESv3
 File is not linearized
 No syntax or stream encoding errors found; the file may still contain
 errors that qpdf cannot detect