[PATCH] Adding diversity for security

[Tom Roeder]

  I'm glad you were able to set up the OpenSSL locking. I have a few comments on the code; see below. My high-level comment about the algorithm is that it looks like this is not an exact implementation of of SP 800-90A; see my comments for some (not all) of the differences.

  Also, someone familiar with the details of the X86 Target and other backend code should look at this, too, since I don't know much about the backend details.


================
Comment at: include/llvm/Support/RandomNumberGenerator.h:17
@@ +16,3 @@
+
+#ifndef RANDOMNUMBERGENERATOR_H_
+#define RANDOMNUMBERGENERATOR_H_
----------------
This should follow the style LLVM_SUPPORT_RANDOMNUMBERGENERATOR_H (see other files in include/llvm)

================
Comment at: include/llvm/Support/RandomNumberGenerator.h:69
@@ +68,3 @@
+    uint64_t r;
+    while ((r = Random()) >= t) { /* NOOP */ }
+
----------------
Looks like the style here should be a semi-colon alone on a line. See, e.g., lib/CodeGen/MachineBasicBlock.cpp around the changes in this diff.

================
Comment at: include/llvm/Target/TargetOptions.h:53-55
@@ -52,4 +52,5 @@
           EnableFastISel(false), PositionIndependentExecutable(false),
-          EnableSegmentedStacks(false), UseInitArray(false), TrapFuncName(""),
-          FloatABIType(FloatABI::Default), AllowFPOpFusion(FPOpFusion::Standard)
+          EnableSegmentedStacks(false), NOPInsertion(false), UseInitArray(false),
+          TrapFuncName(""), FloatABIType(FloatABI::Default),
+          AllowFPOpFusion(FPOpFusion::Standard)
     {}
----------------
It looks like this diff is missing at least one place that uses TargetOptions: in tools/lto/lto.cpp, there is a function lto_set_target_options that sets values in TargetOptions based on the command-line flags. Please change that too.

================
Comment at: lib/Support/RandomNumberGenerator.cpp:77-80
@@ +76,6 @@
+/// CTR_DRBG random number generator, with AES128 as the block
+/// cipher. In addition, the initial seed and additional entropy from
+/// the list of command-line arguments are ran through a standard
+/// PBKDF2 key stretching algorithm to produce enough bits to
+/// initialize the RNG.
+class OpenSSLRandomNumberGenerator : public RandomNumberGenerator {
----------------
You should probably note, then, that the entropy here depends almost entirely on the entropy in the seed, since the command-line arguments aren't going to have much, I assume. Do you have a sense of how many bits of entropy are needed here to make this construction secure in a given context?

>From Table 3 in SP 800-90A, it looks like the minimum entropy required for the construction that uses a key derivation function is the required security strength.

================
Comment at: lib/Support/RandomNumberGenerator.cpp:91-92
@@ +90,4 @@
+  // Noncopyable.
+  OpenSSLRandomNumberGenerator(const OpenSSLRandomNumberGenerator &other) LLVM_DELETED_FUNCTION;
+  OpenSSLRandomNumberGenerator &operator=(const OpenSSLRandomNumberGenerator &other) LLVM_DELETED_FUNCTION;
+
----------------
80-char limit (here and in a couple other places). Maybe reformat with clang-format --style=LLVM ?

================
Comment at: lib/Target/X86/NOPInsertion.cpp:103
@@ +102,3 @@
+        MachineInstr *NewMI = NULL;
+        unsigned reg = nopRegs[NOPCode][!!is64Bit];
+        switch (NOPCode) {
----------------
!! on bool is unnecessary

================
Comment at: tools/opt/opt.cpp:600-609
@@ -597,2 +599,12 @@
 
+  // Seed the RNG
+  std::string SeedData;
+  // If entropy already set (for predictable testing), ignore
+  if (RandomNumberGenerator::EntropyData.empty()) {
+    for (int i = 0; i < argc; ++i) {
+      SeedData += argv[i];
+    }
+    RandomNumberGenerator::EntropyData = SeedData;
+  }
+
   if (AnalyzeOnly && NoOutput) {
----------------
I think something similar to this will have to be added to llvm-lto to support nop code generation there, too. Maybe instead of repeating the same snippet everywhere, you could make this a function in RandomNumberGenerator. Something like AddSeedData?

================
Comment at: lib/Support/RandomNumberGenerator.cpp:153-154
@@ +152,4 @@
+    unsigned char Output[AES_BLOCK_SIZE];
+    AES_ctr128_encrypt(Plaintext, Output, AES_BLOCK_SIZE, &AESKey, IV,
+                       EcountBuffer, &Num);
+
----------------
See the description of the CTR_DRBG algorithm on page 58-59 of NIST SP 800-90A: the randomness generation process also needs to call an update function to change the secret state, including the key, each time. This is step 6 of CTR_DRBG Generate Process.

Also, it looks like there is supposed to be a reseed count that forces reseeding after a certain interval. Table 3 in SP 800-90A gives the recommended number of iterations for a given security strength.

================
Comment at: lib/Support/RandomNumberGenerator.cpp:112
@@ +111,3 @@
+    unsigned KeyLen = AES_KEY_LENGTH + 2*AES_BLOCK_SIZE;
+    unsigned char *SeedBuffer = new unsigned char[KeyLen];
+
----------------
AFAICT from SP 800-90A, the initial key is supposed to be 0, and subsequent reseeding  operations are supposed to use the existing key to generate a new one.


http://llvm-reviews.chandlerc.com/D1802