From 4229457068d6a28cc11b506f127a7bb650ab18c1 Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Sat, 5 Oct 2013 17:36:33 -0400 Subject: [PATCH] Security: use a secure random number generator If not available, give an error. The user may also configure qpdf to use an insecure random number generator. --- ChangeLog | 3 ++ README | 19 ++++++++ TODO | 11 ++--- configure.ac | 34 ++++++++++++++ copy_dlls | 2 +- include/qpdf/QUtil.hh | 12 +++-- libqpdf/QUtil.cc | 101 +++++++++++++++++++++++++++++++++++------ m4/ax_random_device.m4 | 31 +++++++++++++ 8 files changed, 190 insertions(+), 23 deletions(-) create mode 100644 m4/ax_random_device.m4 diff --git a/ChangeLog b/ChangeLog index fd15a753..6ef8d42f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,8 @@ 2013-10-05 Jay Berkenbilt + * Use cryptographically secure random number generation when + available. See additional notes in README. + * Replace some assert() calls with std::logic_error exceptions. Ideally there shouldn't be assert() calls outside of testing. This change may make a few more potential code errors in handling diff --git a/README b/README index e73d6694..445849e1 100644 --- a/README +++ b/README @@ -167,3 +167,22 @@ the test suite fails, test failure detail will be included in the build output. Otherwise, you will have to have access to the qtest.log file from the build to view test failures. The debian packages for qpdf enable this option, for example. + + +Random Number Generation +======================== + +When the qpdf detects either the Windows cryptography API or the +existence of /dev/urandom, /dev/arandom, or /dev/random, it uses them +to generate cryptography secure random numbers. If none of these +conditions are true, the build will fail with an error. It is +possible to configure qpdf with the --enable-insecure-random option, +in which case it will generate random numbers with stdlib's random() +or rand() calls instead. These random numbers are not cryptography +secure, but the qpdf library is fully functional using them. Using +non-secure random numbers means that it's easier in some cases to +guess encryption keys. If you're not generating encrypted files, +there's no advantage to using secure random numbers. + +If you are building qpdf on a platform that qpdf doesn't know how to +generate secure random numbers on, a patch would be welcome. diff --git a/TODO b/TODO index 1e4e309b..f7e5549a 100644 --- a/TODO +++ b/TODO @@ -76,12 +76,11 @@ General and replace the /Pages key of the root dictionary with the new tree. - * Improve the random number seed to make it more secure so that we - have stronger random numbers, particularly when multiple files are - generated in the same second. This code may need to be - OS-specific. Probably we should add a method in QUtil to seed with - a strong random number and call this automatically the first time - QUtil::random() is called. + * Secure random number generation could be made more efficient by + using a local static to ensure a single random device or crypt + provider as long as this can be done in a thread-safe fashion. In + the initial implementation, this is being skipped to avoid having + to add any dependencies on threading libraries. * Study what's required to support savable forms that can be saved by Adobe Reader. Does this require actually signing the document with diff --git a/configure.ac b/configure.ac index 22fffc24..14f4a848 100644 --- a/configure.ac +++ b/configure.ac @@ -16,6 +16,23 @@ AC_PROG_CXX AC_HEADER_STDC LT_INIT([win32-dll]) +AC_ARG_ENABLE(insecure-random, + AS_HELP_STRING([--enable-insecure-random], + [whether to use stdlib's random number generator (default is no)]), + [if test "$enableval" = "yes"; then + qpdf_INSECURE_RANDOM=1; + else + qpdf_INSECURE_RANDOM=0; + fi], [qpdf_INSECURE_RANDOM=0]) +if test "$qpdf_INSECURE_RANDOM" = "1"; then + AC_MSG_RESULT(yes) + AC_DEFINE([USE_INSECURE_RANDOM], [1], [Whether to use inscure random numbers]) +else + AC_MSG_RESULT(no) +fi + +AX_RANDOM_DEVICE + USE_EXTERNAL_LIBS=0 AC_MSG_CHECKING(for whether to use external libraries distribution) AC_ARG_ENABLE(external-libs, @@ -54,6 +71,23 @@ if test "$BUILD_INTERNAL_LIBS" = "0"; then AC_SEARCH_LIBS(pcre_compile,pcre,,[MISSING_PCRE=1; MISSING_ANY=1]) fi +if test "x$qpdf_INSECURE_RANDOM" != "x1"; then + OLIBS=$LIBS + LIBS="$LIBS Advapi32.lib" + AC_MSG_CHECKING(for Advapi32 library) + AC_LINK_IFELSE([AC_LANG_PROGRAM( + [[#pragma comment(lib, "crypt32.lib") + #include + #include + HCRYPTPROV cp;]], + [CryptAcquireContext(&cp, NULL, NULL, PROV_RSA_FULL, 0);] + )], + [AC_MSG_RESULT(yes) + LIBS="$OLIBS -lAdvapi32"], + [AC_MSG_RESULT(no) + LIBS=$OLIBS]) +fi + QPDF_LARGE_FILE_TEST_PATH= AC_SUBST(QPDF_LARGE_FILE_TEST_PATH) AC_ARG_WITH(large-file-test-path, diff --git a/copy_dlls b/copy_dlls index c85e44b9..ea7da779 100755 --- a/copy_dlls +++ b/copy_dlls @@ -20,7 +20,7 @@ while () { my $dll = $1; $dll =~ tr/A-Z/a-z/; - next if $dll =~ m/^(kernel32|user32|msvcrt)\.dll$/; + next if $dll =~ m/^(kernel32|user32|msvcrt|advapi32)\.dll$/; $dlls{$dll} = 1; } elsif (m/^Magic.*\((PE.+?)\)/) diff --git a/include/qpdf/QUtil.hh b/include/qpdf/QUtil.hh index 8bad535d..cbdc065c 100644 --- a/include/qpdf/QUtil.hh +++ b/include/qpdf/QUtil.hh @@ -108,12 +108,18 @@ namespace QUtil QPDF_DLL std::string toUTF8(unsigned long uval); - // Wrapper around random from stdlib. Calls srandom automatically - // the first time it is called. + // If secure random number generation is supported on your + // platform and qpdf was not compiled with insecure random number + // generation, this returns a crytographically secure random + // number. Otherwise it falls back to random from stdlib and + // calls srandom automatically the first time it is called. QPDF_DLL long random(); - // Wrapper around srandom from stdlib. + // Wrapper around srandom from stdlib. Seeds the standard library + // weak random number generator, which is not used if secure + // random number generation is being used. You never need to call + // this method as it is called automatically if needed. QPDF_DLL void srandom(unsigned int seed); diff --git a/libqpdf/QUtil.cc b/libqpdf/QUtil.cc index 84ff004b..de3099d2 100644 --- a/libqpdf/QUtil.cc +++ b/libqpdf/QUtil.cc @@ -17,6 +17,7 @@ #include #include #include +#include #else #include #endif @@ -377,6 +378,8 @@ QUtil::toUTF8(unsigned long uval) return result; } +#ifdef USE_INSECURE_RANDOM + long QUtil::random() { @@ -390,21 +393,11 @@ QUtil::random() seeded_random = true; } -#ifdef HAVE_RANDOM +# ifdef HAVE_RANDOM return ::random(); -#else +# else return rand(); -#endif -} - -void -QUtil::srandom(unsigned int seed) -{ -#ifdef HAVE_RANDOM - ::srandom(seed); -#else - srand(seed); -#endif +# endif } void @@ -415,3 +408,85 @@ QUtil::initializeWithRandomBytes(unsigned char* data, size_t len) data[i] = static_cast((QUtil::random() & 0xff0) >> 4); } } + +#else + +long +QUtil::random() +{ + long result = 0L; + initializeWithRandomBytes( + reinterpret_cast(&result), + sizeof(result)); + return result; +} + +#ifdef _WIN32 +class WindowsCryptProvider +{ + public: + WindowsCryptProvider() + { + if (! CryptAcquireContext(&crypt_prov, NULL, NULL, PROV_RSA_FULL, 0)) + { + throw std::runtime_error("unable to acquire crypt context"); + } + } + ~WindowsCryptProvider() + { + // Ignore error + CryptReleaseContext(crypt_prov, 0); + } + + HCRYPTPROV crypt_prov; +}; +#endif + +void +QUtil::initializeWithRandomBytes(unsigned char* data, size_t len) +{ +#if defined(_WIN32) + + // Optimization: make the WindowsCryptProvider static as long as + // it can be done in a thread-safe fashion. + WindowsCryptProvider c; + if (! CryptGenRandom(c.crypt_prov, len, reinterpret_cast(data))) + { + throw std::runtime_error("unable to generate secure random data"); + } + +#elif defined(RANDOM_DEVICE) + + // Optimization: wrap the file open and close in a class so that + // the file is closed in a destructor, then make this static to + // keep the file handle open. Only do this if it can be done in a + // thread-safe fashion. + FILE* f = QUtil::safe_fopen(RANDOM_DEVICE, "rb"); + size_t fr = fread(data, 1, len, f); + fclose(f); + if (fr != len) + { + throw std::runtime_error( + "unable to read " + + QUtil::int_to_string(len) + + " bytes from " + std::string(RANDOM_DEVICE)); + } + +#else + +# error "Don't know how to generate secure random numbers on this platform. See random number generation in the top-level README" + +#endif +} + +#endif + +void +QUtil::srandom(unsigned int seed) +{ +#ifdef HAVE_RANDOM + ::srandom(seed); +#else + srand(seed); +#endif +} diff --git a/m4/ax_random_device.m4 b/m4/ax_random_device.m4 new file mode 100644 index 00000000..aa8bf4c9 --- /dev/null +++ b/m4/ax_random_device.m4 @@ -0,0 +1,31 @@ +dnl @synopsis AX_RANDOM_DEVICE +dnl +dnl This macro will check for a random device, allowing the user to explicitly +dnl set the path. The user uses '--with-random=FILE' as an argument to +dnl configure. +dnl +dnl If A random device is found then HAVE_RANDOM_DEVICE is set to 1 and +dnl RANDOM_DEVICE contains the path. +dnl +dnl @category Miscellaneous +dnl @author Martin Ebourne +dnl @version 2005/07/01 +dnl @license AllPermissive + +AC_DEFUN([AX_RANDOM_DEVICE], [ + AC_ARG_WITH([random], + [AC_HELP_STRING([--with-random=FILE], [Use FILE as random number seed [auto-detected]])], + [RANDOM_DEVICE="$withval"], + [AC_CHECK_FILE("/dev/urandom", [RANDOM_DEVICE="/dev/urandom";], + [AC_CHECK_FILE("/dev/arandom", [RANDOM_DEVICE="/dev/arandom";], + [AC_CHECK_FILE("/dev/random", [RANDOM_DEVICE="/dev/random";])] + )]) + ]) + if test "x$RANDOM_DEVICE" != "x" ; then + AC_DEFINE([HAVE_RANDOM_DEVICE], 1, + [Define to 1 (and set RANDOM_DEVICE) if a random device is available]) + AC_SUBST([RANDOM_DEVICE]) + AC_DEFINE_UNQUOTED([RANDOM_DEVICE], ["$RANDOM_DEVICE"], + [Define to the filename of the random device (and set HAVE_RANDOM_DEVICE)]) + fi +])dnl