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.

8 files changed, 187 insertions, 20 deletions

diff --git a/ChangeLog b/ChangeLog
index fd15a753..6ef8d42f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,8 @@
 2013-10-05  Jay Berkenbilt  <ejb@ql.org>
 
+	* 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 <windows.h>
+     #include <Wincrypt.h>
+     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 (<O>)
     {
 	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 <Windows.h>
 #include <direct.h>
 #include <io.h>
+#include <Wincrypt.h>
 #else
 #include <unistd.h>
 #endif
@@ -377,6 +378,8 @@ QUtil::toUTF8(unsigned long uval)
     return result;
 }
 
+#ifdef USE_INSECURE_RANDOM
+
 long
 QUtil::random()
 {
@@ -390,28 +393,100 @@ QUtil::random()
 	seeded_random = true;
     }
 
-#ifdef HAVE_RANDOM
+#  ifdef HAVE_RANDOM
     return ::random();
-#else
+#  else
     return rand();
-#endif
+#  endif
 }
 
 void
-QUtil::srandom(unsigned int seed)
+QUtil::initializeWithRandomBytes(unsigned char* data, size_t len)
 {
-#ifdef HAVE_RANDOM
-    ::srandom(seed);
+    for (size_t i = 0; i < len; ++i)
+    {
+        data[i] = static_cast<unsigned char>((QUtil::random() & 0xff0) >> 4);
+    }
+}
+
 #else
-    srand(seed);
-#endif
+
+long
+QUtil::random()
+{
+    long result = 0L;
+    initializeWithRandomBytes(
+        reinterpret_cast<unsigned char*>(&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)
 {
-    for (size_t i = 0; i < len; ++i)
+#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<BYTE*>(data)))
     {
-        data[i] = static_cast<unsigned char>((QUtil::random() & 0xff0) >> 4);
+        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