Fix strict aliasing issues with DES_cblock

After all, we have to keep this robust, modern cipher conforming to C
well-definedness expectations!

These functions should have simply taken uint8_t* pointers. Make
internal _ex variants that fix this. I've not bothered updating the
public APIs because it will cause a ton of downstream churn and make
those APIs even more OpenSSL-incompatible.  (OpenSSL's APIs take a
const-incorrect uint8_t (*in)[8]. Both our struct and their pointers
expect callers to call with &foo.) This does not seem worth the trouble.

Also since the underlying functions now access as uint8_t*, I suspect
this broadly fixes strict aliasing issues with callers that cast from a
byte array. (Though perhaps in->bytes should be (const uint8_t*)in?)

Ideally c2l and l2c would be replaced with CRYPTO_load_u32_le and
CRYPTO_store_u32_le. (It's a little rude for a header to squat those
names, especially when those name often vary in endianness.) I did that
in a couple places where we'd otherwise increment a pointer declared
with the funny array parameter syntax.  Otherwise I left it alone for
now.

Fixed: 683
Change-Id: I7b0d8b2a16697095ebf42a71482c4ba805a193e4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65690
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

crypto/cipher_extra/e_des.c

@@ -58,6 +58,7 @@
 #include <openssl/des.h>
 #include <openssl/nid.h>
 
+#include "../des/internal.h"
 #include "../fipsmodule/cipher/internal.h"
 #include "internal.h"
 
@@ -71,20 +72,15 @@ typedef struct {
 
 static int des_init_key(EVP_CIPHER_CTX *ctx, const uint8_t *key,
                         const uint8_t *iv, int enc) {
-  DES_cblock *deskey = (DES_cblock *)key;
   EVP_DES_KEY *dat = (EVP_DES_KEY *)ctx->cipher_data;
-
-  DES_set_key(deskey, &dat->ks.ks);
+  DES_set_key_ex(key, &dat->ks.ks);
   return 1;
 }
 
 static int des_cbc_cipher(EVP_CIPHER_CTX *ctx, uint8_t *out, const uint8_t *in,
                           size_t in_len) {
   EVP_DES_KEY *dat = (EVP_DES_KEY *)ctx->cipher_data;
-
-  DES_ncbc_encrypt(in, out, in_len, &dat->ks.ks, (DES_cblock *)ctx->iv,
-                   ctx->encrypt);
-
+  DES_ncbc_encrypt_ex(in, out, in_len, &dat->ks.ks, ctx->iv, ctx->encrypt);
   return 1;
 }
 
@@ -113,8 +109,7 @@ static int des_ecb_cipher(EVP_CIPHER_CTX *ctx, uint8_t *out, const uint8_t *in,
 
   EVP_DES_KEY *dat = (EVP_DES_KEY *)ctx->cipher_data;
   for (size_t i = 0; i <= in_len; i += ctx->cipher->block_size) {
-    DES_ecb_encrypt((DES_cblock *)(in + i), (DES_cblock *)(out + i),
-                    &dat->ks.ks, ctx->encrypt);
+    DES_ecb_encrypt_ex(in + i, out + i, &dat->ks.ks, ctx->encrypt);
   }
   return 1;
 }
@@ -144,23 +139,18 @@ typedef struct {
 
 static int des_ede3_init_key(EVP_CIPHER_CTX *ctx, const uint8_t *key,
                              const uint8_t *iv, int enc) {
-  DES_cblock *deskey = (DES_cblock *)key;
   DES_EDE_KEY *dat = (DES_EDE_KEY *)ctx->cipher_data;
-
-  DES_set_key(&deskey[0], &dat->ks.ks[0]);
-  DES_set_key(&deskey[1], &dat->ks.ks[1]);
-  DES_set_key(&deskey[2], &dat->ks.ks[2]);
-
+  DES_set_key_ex(key, &dat->ks.ks[0]);
+  DES_set_key_ex(key + 8, &dat->ks.ks[1]);
+  DES_set_key_ex(key + 16, &dat->ks.ks[2]);
   return 1;
 }
 
 static int des_ede3_cbc_cipher(EVP_CIPHER_CTX *ctx, uint8_t *out,
                                const uint8_t *in, size_t in_len) {
   DES_EDE_KEY *dat = (DES_EDE_KEY *)ctx->cipher_data;
-
-  DES_ede3_cbc_encrypt(in, out, in_len, &dat->ks.ks[0], &dat->ks.ks[1],
-                       &dat->ks.ks[2], (DES_cblock *)ctx->iv, ctx->encrypt);
-
+  DES_ede3_cbc_encrypt_ex(in, out, in_len, &dat->ks.ks[0], &dat->ks.ks[1],
+                          &dat->ks.ks[2], ctx->iv, ctx->encrypt);
   return 1;
 }
 
@@ -182,13 +172,11 @@ const EVP_CIPHER *EVP_des_ede3_cbc(void) { return &evp_des_ede3_cbc; }
 
 static int des_ede_init_key(EVP_CIPHER_CTX *ctx, const uint8_t *key,
                             const uint8_t *iv, int enc) {
-  DES_cblock *deskey = (DES_cblock *)key;
   DES_EDE_KEY *dat = (DES_EDE_KEY *)ctx->cipher_data;
-
-  DES_set_key(&deskey[0], &dat->ks.ks[0]);
-  DES_set_key(&deskey[1], &dat->ks.ks[1]);
-  DES_set_key(&deskey[0], &dat->ks.ks[2]);
-
+  // 2-DES is 3-DES with the first key used twice.
+  DES_set_key_ex(key, &dat->ks.ks[0]);
+  DES_set_key_ex(key + 8, &dat->ks.ks[1]);
+  DES_set_key_ex(key, &dat->ks.ks[2]);
   return 1;
 }
 
@@ -217,9 +205,8 @@ static int des_ede_ecb_cipher(EVP_CIPHER_CTX *ctx, uint8_t *out,
 
   DES_EDE_KEY *dat = (DES_EDE_KEY *) ctx->cipher_data;
   for (size_t i = 0; i <= in_len; i += ctx->cipher->block_size) {
-    DES_ecb3_encrypt((DES_cblock *) (in + i), (DES_cblock *) (out + i),
-                     &dat->ks.ks[0], &dat->ks.ks[1], &dat->ks.ks[2],
-                     ctx->encrypt);
+    DES_ecb3_encrypt_ex(in + i, out + i, &dat->ks.ks[0], &dat->ks.ks[1],
+                        &dat->ks.ks[2], ctx->encrypt);
   }
   return 1;
 }

crypto/des/des.c

@@ -379,13 +379,17 @@ static const uint32_t DES_SPtrans[8][64] = {
    (a) = (a) ^ (t) ^ ((t) >> (16 - (n))))
 
 void DES_set_key(const DES_cblock *key, DES_key_schedule *schedule) {
+  DES_set_key_ex(key->bytes, schedule);
+}
+
+void DES_set_key_ex(const uint8_t key[8], DES_key_schedule *schedule) {
   static const int shifts2[16] = {0, 0, 1, 1, 1, 1, 1, 1,
                                   0, 1, 1, 1, 1, 1, 1, 0};
   uint32_t c, d, t, s, t2;
   const uint8_t *in;
   int i;
 
-  in = key->bytes;
+  in = key;
 
   c2l(in, c);
   c2l(in, d);
@@ -626,32 +630,34 @@ void DES_decrypt3(uint32_t data[2], const DES_key_schedule *ks1,
 
 void DES_ecb_encrypt(const DES_cblock *in_block, DES_cblock *out_block,
                      const DES_key_schedule *schedule, int is_encrypt) {
-  uint32_t l;
-  uint32_t ll[2];
-  const uint8_t *in = in_block->bytes;
-  uint8_t *out = out_block->bytes;
+  DES_ecb_encrypt_ex(in_block->bytes, out_block->bytes, schedule, is_encrypt);
+}
 
-  c2l(in, l);
-  ll[0] = l;
-  c2l(in, l);
-  ll[1] = l;
+void DES_ecb_encrypt_ex(const uint8_t in[8], uint8_t out[8],
+                        const DES_key_schedule *schedule, int is_encrypt) {
+  uint32_t ll[2];
+  ll[0] = CRYPTO_load_u32_le(in);
+  ll[1] = CRYPTO_load_u32_le(in + 4);
   DES_encrypt1(ll, schedule, is_encrypt);
-  l = ll[0];
-  l2c(l, out);
-  l = ll[1];
-  l2c(l, out);
-  ll[0] = ll[1] = 0;
+  CRYPTO_store_u32_le(out, ll[0]);
+  CRYPTO_store_u32_le(out + 4, ll[1]);
 }
 
 void DES_ncbc_encrypt(const uint8_t *in, uint8_t *out, size_t len,
                       const DES_key_schedule *schedule, DES_cblock *ivec,
                       int enc) {
+  DES_ncbc_encrypt_ex(in, out, len, schedule, ivec->bytes, enc);
+}
+
+void DES_ncbc_encrypt_ex(const uint8_t *in, uint8_t *out, size_t len,
+                         const DES_key_schedule *schedule, uint8_t ivec[8],
+                         int enc) {
   uint32_t tin0, tin1;
   uint32_t tout0, tout1, xor0, xor1;
   uint32_t tin[2];
   unsigned char *iv;
 
-  iv = ivec->bytes;
+  iv = ivec;
 
   if (enc) {
     c2l(iv, tout0);
@@ -681,7 +687,7 @@ void DES_ncbc_encrypt(const uint8_t *in, uint8_t *out, size_t len,
       tout1 = tin[1];
       l2c(tout1, out);
     }
-    iv = ivec->bytes;
+    iv = ivec;
     l2c(tout0, iv);
     l2c(tout1, iv);
   } else {
@@ -712,7 +718,7 @@ void DES_ncbc_encrypt(const uint8_t *in, uint8_t *out, size_t len,
       xor0 = tin0;
       xor1 = tin1;
     }
-    iv = ivec->bytes;
+    iv = ivec;
     l2c(xor0, iv);
     l2c(xor1, iv);
   }
@@ -722,37 +728,44 @@ void DES_ncbc_encrypt(const uint8_t *in, uint8_t *out, size_t len,
 void DES_ecb3_encrypt(const DES_cblock *input, DES_cblock *output,
                       const DES_key_schedule *ks1, const DES_key_schedule *ks2,
                       const DES_key_schedule *ks3, int enc) {
-  uint32_t l0, l1;
-  uint32_t ll[2];
-  const uint8_t *in = input->bytes;
-  uint8_t *out = output->bytes;
+  DES_ecb3_encrypt_ex(input->bytes, output->bytes, ks1, ks2, ks3, enc);
+}
 
-  c2l(in, l0);
-  c2l(in, l1);
-  ll[0] = l0;
-  ll[1] = l1;
+void DES_ecb3_encrypt_ex(const uint8_t in[8], uint8_t out[8],
+                         const DES_key_schedule *ks1,
+                         const DES_key_schedule *ks2,
+                         const DES_key_schedule *ks3, int enc) {
+  uint32_t ll[2];
+  ll[0] = CRYPTO_load_u32_le(in);
+  ll[1] = CRYPTO_load_u32_le(in + 4);
   if (enc) {
     DES_encrypt3(ll, ks1, ks2, ks3);
   } else {
     DES_decrypt3(ll, ks1, ks2, ks3);
   }
-  l0 = ll[0];
-  l1 = ll[1];
-  l2c(l0, out);
-  l2c(l1, out);
+  CRYPTO_store_u32_le(out, ll[0]);
+  CRYPTO_store_u32_le(out + 4, ll[1]);
 }
 
 void DES_ede3_cbc_encrypt(const uint8_t *in, uint8_t *out, size_t len,
                           const DES_key_schedule *ks1,
                           const DES_key_schedule *ks2,
                           const DES_key_schedule *ks3, DES_cblock *ivec,
                           int enc) {
+  DES_ede3_cbc_encrypt_ex(in, out, len, ks1, ks2, ks3, ivec->bytes, enc);
+}
+
+void DES_ede3_cbc_encrypt_ex(const uint8_t *in, uint8_t *out, size_t len,
+                             const DES_key_schedule *ks1,
+                             const DES_key_schedule *ks2,
+                             const DES_key_schedule *ks3, uint8_t ivec[8],
+                             int enc) {
   uint32_t tin0, tin1;
   uint32_t tout0, tout1, xor0, xor1;
   uint32_t tin[2];
   uint8_t *iv;
 
-  iv = ivec->bytes;
+  iv = ivec;
 
   if (enc) {
     c2l(iv, tout0);
@@ -786,7 +799,7 @@ void DES_ede3_cbc_encrypt(const uint8_t *in, uint8_t *out, size_t len,
       l2c(tout0, out);
       l2c(tout1, out);
     }
-    iv = ivec->bytes;
+    iv = ivec;
     l2c(tout0, iv);
     l2c(tout1, iv);
   } else {
@@ -834,7 +847,7 @@ void DES_ede3_cbc_encrypt(const uint8_t *in, uint8_t *out, size_t len,
       xor1 = t1;
     }
 
-    iv = ivec->bytes;
+    iv = ivec;
     l2c(xor0, iv);
     l2c(xor1, iv);
   }

crypto/des/internal.h

@@ -67,6 +67,9 @@ extern "C" {
 #endif
 
 
+// TODO(davidben): Ideally these macros would be replaced with
+// |CRYPTO_load_u32_le| and |CRYPTO_store_u32_le|.
+
 #define c2l(c, l)                         \
   do {                                    \
     (l) = ((uint32_t)(*((c)++)));         \
@@ -147,6 +150,27 @@ extern "C" {
   } while (0)
 
 
+// Correctly-typed versions of DES functions.
+//
+// See https://crbug.com/boringssl/683.
+
+void DES_set_key_ex(const uint8_t key[8], DES_key_schedule *schedule);
+void DES_ecb_encrypt_ex(const uint8_t in[8], uint8_t out[8],
+                        const DES_key_schedule *schedule, int is_encrypt);
+void DES_ncbc_encrypt_ex(const uint8_t *in, uint8_t *out, size_t len,
+                         const DES_key_schedule *schedule, uint8_t ivec[8],
+                         int enc);
+void DES_ecb3_encrypt_ex(const uint8_t input[8], uint8_t output[8],
+                         const DES_key_schedule *ks1,
+                         const DES_key_schedule *ks2,
+                         const DES_key_schedule *ks3, int enc);
+void DES_ede3_cbc_encrypt_ex(const uint8_t *in, uint8_t *out, size_t len,
+                             const DES_key_schedule *ks1,
+                             const DES_key_schedule *ks2,
+                             const DES_key_schedule *ks3, uint8_t ivec[8],
+                             int enc);
+
+
 // Private functions.
 //
 // These functions are only exported for use in |decrepit|.