Test for broken counter-mode at runtime

To solve bug 4779, we want to avoid OpenSSL 1.0.0's counter mode.
But Fedora (and maybe others) lie about the actual OpenSSL version,
so we can't trust the header to tell us if it's safe.

Instead, let's do a run-time test to see whether it's safe, and if
not, use our built-in version.

fermenthor contributed a pretty essential fixup to this patch. Thanks!

changes/aes_ctr_test

@@ -0,0 +1,5 @@
+  o Minor bugfixes
+    - Test for the OpenSSL 1.0.0 counter mode bug at runtime, not compile
+      time. This is necessary because OpenSSL has been hacked to mis-report
+      its version on a few distributions.
+      Bugfix on Tor 0.2.3.11-alpha.

src/common/aes.c

@@ -18,10 +18,10 @@
 #include <openssl/evp.h>
 #include <openssl/engine.h>
 #include "crypto.h"
-#if OPENSSL_VERSION_NUMBER >= OPENSSL_V(1,0,0,'a')
+#if OPENSSL_VERSION_NUMBER >= OPENSSL_V_SERIES(1,0,0)
 /* See comments about which counter mode implementation to use below. */
 #include <openssl/modes.h>
-#define USE_OPENSSL_CTR
+#define CAN_USE_OPENSSL_CTR
 #endif
 #include "compat.h"
 #include "aes.h"
@@ -62,7 +62,7 @@ struct aes_cnt_cipher {
     AES_KEY aes;
   } key;
 
-#if !defined(WORDS_BIGENDIAN) && !defined(USE_OPENSSL_CTR)
+#if !defined(WORDS_BIGENDIAN)
 #define USING_COUNTER_VARS
   /** These four values, together, implement a 128-bit counter, with
    * counter0 as the low-order word and counter3 as the high-order word. */
@@ -84,11 +84,7 @@ struct aes_cnt_cipher {
   /** The encrypted value of ctr_buf. */
   uint8_t buf[16];
   /** Our current stream position within buf. */
-#ifdef USE_OPENSSL_CTR
   unsigned int pos;
-#else
-  uint8_t pos;
-#endif
 
   /** True iff we're using the evp implementation of this cipher. */
   uint8_t using_evp;
@@ -98,6 +94,11 @@ struct aes_cnt_cipher {
  * we're testing it or because we have hardware acceleration configured */
 static int should_use_EVP = 0;
 
+#ifdef CAN_USE_OPENSSL_CTR
+/**DOCDOC*/
+static int should_use_openssl_CTR = 0;
+#endif
+
 /** Check whether we should use the EVP interface for AES. If <b>force_val</b>
  * is nonnegative, we use use EVP iff it is true.  Otherwise, we use EVP
  * if there is an engine enabled for aes-ecb. */
@@ -128,7 +129,50 @@ evaluate_evp_for_aes(int force_val)
   return 0;
 }
 
-#ifndef USE_OPENSSL_CTR
+/**DOCDOC*/
+int
+evaluate_ctr_for_aes(void)
+{
+#ifdef CAN_USE_OPENSSL_CTR
+  /* Result of encrypting an all-zero block with an all-zero 128-bit AES key.
+   * This should be the same as encrypting an all-zero block with an all-zero
+   * 128-bit AES key in counter mode, starting at position 0 of the stream.
+   */
+  static const unsigned char encrypt_zero[] =
+    "\x66\xe9\x4b\xd4\xef\x8a\x2c\x3b\x88\x4c\xfa\x59\xca\x34\x2b\x2e";
+  unsigned char zero[16];
+  unsigned char output[16];
+  unsigned char ivec[16];
+  unsigned char ivec_tmp[16];
+  unsigned int pos, i;
+  AES_KEY key;
+  memset(zero, 0, sizeof(zero));
+  memset(ivec, 0, sizeof(ivec));
+  AES_set_encrypt_key(zero, 128, &key);
+
+  pos = 0;
+  /* Encrypting a block one byte at a time should make the error manifest
+   * itself for known bogus openssl versions. */
+  for (i=0; i<16; ++i)
+    AES_ctr128_encrypt(&zero[i], &output[i], 1, &key, ivec, ivec_tmp, &pos);
+
+  if (memcmp(output, encrypt_zero, 16)) {
+    /* Counter mode is buggy */
+    log_notice(LD_CRYPTO, "This OpenSSL has a buggy version of counter mode; "
+               "not using it.");
+  } else {
+    /* Counter mode is okay */
+    log_notice(LD_CRYPTO, "This OpenSSL has a good implementation of counter "
+               "mode; using it.");
+    should_use_openssl_CTR = 1;
+  }
+#else
+  log_notice(LD_CRYPTO, "This version of OpenSSL has a slow implementation of "
+             "counter mode; not using it.");
+#endif
+  return 0;
+}
+
 #if !defined(USING_COUNTER_VARS)
 #define COUNTER(c, n) ((c)->ctr_buf.buf32[3-(n)])
 #else
@@ -157,7 +201,6 @@ _aes_fill_buf(aes_cnt_cipher_t *cipher)
     AES_encrypt(cipher->ctr_buf.buf, cipher->buf, &cipher->key.aes);
   }
 }
-#endif
 
 /**
  * Return a newly allocated counter-mode AES128 cipher implementation.
@@ -203,11 +246,12 @@ aes_set_key(aes_cnt_cipher_t *cipher, const char *key, int key_bits)
 
   cipher->pos = 0;
 
-#ifdef USE_OPENSSL_CTR
-  memset(cipher->buf, 0, sizeof(cipher->buf));
-#else
-  _aes_fill_buf(cipher);
+#ifdef CAN_USE_OPENSSL_CTR
+  if (should_use_openssl_CTR)
+    memset(cipher->buf, 0, sizeof(cipher->buf));
+  else
 #endif
+    _aes_fill_buf(cipher);
 }
 
 /** Release storage held by <b>cipher</b>
@@ -232,7 +276,7 @@ aes_free_cipher(aes_cnt_cipher_t *cipher)
 #define UPDATE_CTR_BUF(c, n)
 #endif
 
-#ifdef USE_OPENSSL_CTR
+#ifdef CAN_USE_OPENSSL_CTR
 /* Helper function to use EVP with openssl's counter-mode wrapper. */
 static void evp_block128_fn(const uint8_t in[16],
                             uint8_t out[16],
@@ -252,29 +296,34 @@ void
 aes_crypt(aes_cnt_cipher_t *cipher, const char *input, size_t len,
           char *output)
 {
-#ifdef USE_OPENSSL_CTR
-  if (cipher->using_evp) {
-    /* In openssl 1.0.0, there's an if'd out EVP_aes_128_ctr in evp.h.  If
-     * it weren't disabled, it might be better just to use that.
-     */
-    CRYPTO_ctr128_encrypt((const unsigned char *)input,
-                          (unsigned char *)output,
-                          len,
-                          &cipher->key.evp,
-                          cipher->ctr_buf.buf,
-                          cipher->buf,
-                          &cipher->pos,
-                          evp_block128_fn);
-  } else {
-    AES_ctr128_encrypt((const unsigned char *)input,
-                       (unsigned char *)output,
-                       len,
-                       &cipher->key.aes,
-                       cipher->ctr_buf.buf,
-                       cipher->buf,
-                       &cipher->pos);
+#ifdef CAN_USE_OPENSSL_CTR
+  if (should_use_openssl_CTR) {
+    if (cipher->using_evp) {
+      /* In openssl 1.0.0, there's an if'd out EVP_aes_128_ctr in evp.h.  If
+       * it weren't disabled, it might be better just to use that.
+       */
+      CRYPTO_ctr128_encrypt((const unsigned char *)input,
+                            (unsigned char *)output,
+                            len,
+                            &cipher->key.evp,
+                            cipher->ctr_buf.buf,
+                            cipher->buf,
+                            &cipher->pos,
+                            evp_block128_fn);
+    } else {
+      AES_ctr128_encrypt((const unsigned char *)input,
+                         (unsigned char *)output,
+                         len,
+                         &cipher->key.aes,
+                         cipher->ctr_buf.buf,
+                         cipher->buf,
+                         &cipher->pos);
+    }
+    return;
   }
-#else
+  else
+#endif
+  {
   int c = cipher->pos;
   if (PREDICT_UNLIKELY(!len)) return;
 
@@ -297,7 +346,7 @@ aes_crypt(aes_cnt_cipher_t *cipher, const char *input, size_t len,
     UPDATE_CTR_BUF(cipher, 0);
     _aes_fill_buf(cipher);
   }
-#endif
+  }
 }
 
 /** Encrypt <b>len</b> bytes from <b>input</b>, storing the results in place.
@@ -307,9 +356,14 @@ aes_crypt(aes_cnt_cipher_t *cipher, const char *input, size_t len,
 void
 aes_crypt_inplace(aes_cnt_cipher_t *cipher, char *data, size_t len)
 {
-#ifdef USE_OPENSSL_CTR
-  aes_crypt(cipher, data, len, data);
-#else
+#ifdef CAN_USE_OPENSSL_CTR
+  if (should_use_openssl_CTR) {
+    aes_crypt(cipher, data, len, data);
+    return;
+  }
+  else
+#endif
+  {
   int c = cipher->pos;
   if (PREDICT_UNLIKELY(!len)) return;
 
@@ -332,7 +386,7 @@ aes_crypt_inplace(aes_cnt_cipher_t *cipher, char *data, size_t len)
     UPDATE_CTR_BUF(cipher, 0);
     _aes_fill_buf(cipher);
   }
-#endif
+  }
 }
 
 /** Reset the 128-bit counter of <b>cipher</b> to the 16-bit big-endian value
@@ -349,8 +403,9 @@ aes_set_iv(aes_cnt_cipher_t *cipher, const char *iv)
   cipher->pos = 0;
   memcpy(cipher->ctr_buf.buf, iv, 16);
 
-#ifndef USE_OPENSSL_CTR
-  _aes_fill_buf(cipher);
+#ifdef CAN_USE_OPENSSL_CTR
+  if (!should_use_openssl_CTR)
 #endif
+    _aes_fill_buf(cipher);
 }
 

src/common/aes.h

@@ -25,6 +25,7 @@ void aes_crypt_inplace(aes_cnt_cipher_t *cipher, char *data, size_t len);
 void aes_set_iv(aes_cnt_cipher_t *cipher, const char *iv);
 
 int evaluate_evp_for_aes(int force_value);
+int evaluate_ctr_for_aes(void);
 
 #endif
 

src/common/crypto.c

@@ -281,6 +281,7 @@ crypto_global_init(int useAccel, const char *accelName, const char *accelDir)
     }
 
     evaluate_evp_for_aes(-1);
+    evaluate_ctr_for_aes();
 
     return crypto_seed_rng(1);
   }

src/test/test_crypto.c

@@ -105,6 +105,7 @@ test_crypto_aes(void *arg)
 
   int use_evp = !strcmp(arg,"evp");
   evaluate_evp_for_aes(use_evp);
+  evaluate_ctr_for_aes();
 
   data1 = tor_malloc(1024);
   data2 = tor_malloc(1024);