From 3de67173de1b162ad967f67dc23e4a2663b94f9b Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Thu, 4 Feb 2021 20:32:00 -0500 Subject: [PATCH] Better fix to insecure password check (fixes #501) --- ChangeLog | 10 ++--- manual/qpdf-manual.xml | 62 +++++++++++++------------- qpdf/qpdf.cc | 31 +++++++------ qpdf/qtest/qpdf.test | 15 +++---- qpdf/qtest/qpdf/insecure-passwords.out | 8 ++-- 5 files changed, 63 insertions(+), 63 deletions(-) diff --git a/ChangeLog b/ChangeLog index cbcfba7a..d5ceeea2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -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 diff --git a/manual/qpdf-manual.xml b/manual/qpdf-manual.xml index 5b4d2cee..09ba7408 100644 --- a/manual/qpdf-manual.xml +++ b/manual/qpdf-manual.xml @@ -1239,10 +1239,11 @@ make 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 - , 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 , as + described below. The value for @@ -1251,25 +1252,6 @@ make When no additional restrictions are given, the default is to be fully permissive. - - For all key lengths, the following options are available: - - - - - - 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. - - - - - If is 40, the following restriction options are available: @@ -1465,6 +1447,21 @@ make + + + + + 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. + + + @@ -4877,15 +4874,16 @@ print "\n"; By default, qpdf no longer allows - creation of encrypted PDF files whose owner password is - empty or matches the user password. The - , specified inside the - 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, + specified inside the 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. diff --git a/qpdf/qpdf.cc b/qpdf/qpdf.cc index 64badd72..0e64490c 100644 --- a/qpdf/qpdf.cc +++ b/qpdf/qpdf.cc @@ -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))) + (o.owner_password.empty() && + (! o.user_password.empty()) && + (o.keylen == 256))) { - 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."); + // 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 && diff --git a/qpdf/qtest/qpdf.test b/qpdf/qtest/qpdf.test index 5fede165..f3c452cf 100644 --- a/qpdf/qtest/qpdf.test +++ b/qpdf/qtest/qpdf.test @@ -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->runtest("recheck encrypted file", $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 (['--force-V4', 'V4'], 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", diff --git a/qpdf/qtest/qpdf/insecure-passwords.out b/qpdf/qtest/qpdf/insecure-passwords.out index f0ba33e8..001d64c0 100644 --- a/qpdf/qtest/qpdf/insecure-passwords.out +++ b/qpdf/qtest/qpdf/insecure-passwords.out @@ -1,10 +1,9 @@ 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