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>
diff --git a/crypto/cipher_extra/e_des.c b/crypto/cipher_extra/e_des.c
index 300ec00..a4a876e 100644
--- a/crypto/cipher_extra/e_des.c
+++ b/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 @@
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 @@
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 @@
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 @@
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 @@
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;
}
diff --git a/crypto/des/des.c b/crypto/des/des.c
index 9717fe5..07242f2 100644
--- a/crypto/des/des.c
+++ b/crypto/des/des.c
@@ -379,13 +379,17 @@
(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_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 @@
tout1 = tin[1];
l2c(tout1, out);
}
- iv = ivec->bytes;
+ iv = ivec;
l2c(tout0, iv);
l2c(tout1, iv);
} else {
@@ -712,7 +718,7 @@
xor0 = tin0;
xor1 = tin1;
}
- iv = ivec->bytes;
+ iv = ivec;
l2c(xor0, iv);
l2c(xor1, iv);
}
@@ -722,24 +728,23 @@
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,
@@ -747,12 +752,20 @@
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 @@
l2c(tout0, out);
l2c(tout1, out);
}
- iv = ivec->bytes;
+ iv = ivec;
l2c(tout0, iv);
l2c(tout1, iv);
} else {
@@ -834,7 +847,7 @@
xor1 = t1;
}
- iv = ivec->bytes;
+ iv = ivec;
l2c(xor0, iv);
l2c(xor1, iv);
}
diff --git a/crypto/des/internal.h b/crypto/des/internal.h
index 0d07a1b..ac6c719 100644
--- a/crypto/des/internal.h
+++ b/crypto/des/internal.h
@@ -67,6 +67,9 @@
#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 @@
} 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|.
