From 27344bd7ca442e5ce8a3493e8a8c5e3cf067bcb0 Mon Sep 17 00:00:00 2001 From: "Matthias J. Kannwischer" Date: Wed, 27 Feb 2019 16:28:20 +0100 Subject: [PATCH] un-align pointers. Resolves #24 --- test/crypto_kem/functest.c | 53 +++++++++++++++++++++++++++---------- test/crypto_sign/functest.c | 47 +++++++++++++++++++++++--------- 2 files changed, 74 insertions(+), 26 deletions(-) diff --git a/test/crypto_kem/functest.c b/test/crypto_kem/functest.c index 92f2d7f1..2ca72da6 100644 --- a/test/crypto_kem/functest.c +++ b/test/crypto_kem/functest.c @@ -42,21 +42,45 @@ static int check_canary(const unsigned char *d) { } static int test_keys(void) { - unsigned char key_a[CRYPTO_BYTES + 16], key_b[CRYPTO_BYTES + 16]; - unsigned char pk[CRYPTO_PUBLICKEYBYTES + 16]; - unsigned char sendb[CRYPTO_CIPHERTEXTBYTES + 16]; - unsigned char sk_a[CRYPTO_SECRETKEYBYTES + 16]; + /* + * This is most likely going to be aligned by the compiler. + * 16 extra bytes for canary + * 1 extra byte for unalignment + */ + unsigned char key_a_aligned[CRYPTO_BYTES + 16 + 1]; + unsigned char key_b_aligned[CRYPTO_BYTES + 16 + 1]; + unsigned char pk_aligned[CRYPTO_PUBLICKEYBYTES + 16 + 1]; + unsigned char sendb_aligned[CRYPTO_CIPHERTEXTBYTES + 16 + 1]; + unsigned char sk_a_aligned[CRYPTO_SECRETKEYBYTES + 16 + 1]; + /* + * Make sure all pointers are odd. + * This ensures that the implementation does not assume anything about the + * data alignment. For example this would catch if an implementation + * directly uses these pointers to load into vector registers using movdqa. + */ + unsigned char *key_a = (unsigned char *) ((uintptr_t) key_a_aligned|(uintptr_t) 1); + unsigned char *key_b = (unsigned char *) ((uintptr_t) key_b_aligned|(uintptr_t) 1); + unsigned char *pk = (unsigned char *) ((uintptr_t) pk_aligned|(uintptr_t) 1); + unsigned char *sendb = (unsigned char *) ((uintptr_t) sendb_aligned|(uintptr_t) 1); + unsigned char *sk_a = (unsigned char *) ((uintptr_t) sk_a_aligned|(uintptr_t) 1); + + /* + * Write 8 byte canary before and after the actual memory regions. + * This is used to validate that the implementation does not assume + * anything about the placement of data in memory + * (e.g., assuming that the pk is always behind the sk) + */ write_canary(key_a); - write_canary(key_a + sizeof(key_a) - 8); + write_canary(key_a + CRYPTO_BYTES + 8); write_canary(key_b); - write_canary(key_b + sizeof(key_b) - 8); + write_canary(key_b + CRYPTO_BYTES + 8); write_canary(pk); - write_canary(pk + sizeof(pk) - 8); + write_canary(pk + CRYPTO_PUBLICKEYBYTES + 8); write_canary(sendb); - write_canary(sendb + sizeof(sendb) - 8); + write_canary(sendb + CRYPTO_CIPHERTEXTBYTES + 8); write_canary(sk_a); - write_canary(sk_a + sizeof(sk_a) - 8); + write_canary(sk_a + CRYPTO_SECRETKEYBYTES + 8); int i; @@ -75,11 +99,12 @@ static int test_keys(void) { return -1; } - if (check_canary(key_a) || check_canary(key_a + sizeof(key_a) - 8) || - check_canary(key_b) || check_canary(key_b + sizeof(key_b) - 8) || - check_canary(pk) || check_canary(pk + sizeof(pk) - 8) || - check_canary(sendb) || check_canary(sendb + sizeof(sendb) - 8) || - check_canary(sk_a) || check_canary(sk_a + sizeof(sk_a) - 8)) { + // Validate that the implementation did not touch the canary + if (check_canary(key_a) || check_canary(key_a + CRYPTO_BYTES + 8) || + check_canary(key_b) || check_canary(key_b + CRYPTO_BYTES + 8 ) || + check_canary(pk) || check_canary(pk + CRYPTO_PUBLICKEYBYTES + 8 ) || + check_canary(sendb) || check_canary(sendb + CRYPTO_CIPHERTEXTBYTES + 8 ) || + check_canary(sk_a) || check_canary(sk_a + CRYPTO_SECRETKEYBYTES + 8 )) { printf("ERROR canary overwritten\n"); return -1; } diff --git a/test/crypto_sign/functest.c b/test/crypto_sign/functest.c index 014b0542..e0bd31ab 100644 --- a/test/crypto_sign/functest.c +++ b/test/crypto_sign/functest.c @@ -43,24 +43,46 @@ static int check_canary(const unsigned char *d) { } static int test_sign(void) { - unsigned char pk[CRYPTO_PUBLICKEYBYTES + 16]; - unsigned char sk[CRYPTO_SECRETKEYBYTES + 16]; - unsigned char sm[MLEN + CRYPTO_BYTES + 16]; - unsigned char m[MLEN + 16]; + /* + * This is most likely going to be aligned by the compiler. + * 16 extra bytes for canary + * 1 extra byte for unalignment + */ + unsigned char pk_aligned[CRYPTO_PUBLICKEYBYTES + 16 + 1]; + unsigned char sk_aligned[CRYPTO_SECRETKEYBYTES + 16 + 1]; + unsigned char sm_aligned[MLEN + CRYPTO_BYTES + 16 + 1]; + unsigned char m_aligned[MLEN + 16 + 1]; + + /* + * Make sure all pointers are odd. + * This ensures that the implementation does not assume anything about the + * data alignment. For example this would catch if an implementation + * directly uses these pointers to load into vector registers using movdqa. + */ + unsigned char *pk = (unsigned char *) ((uintptr_t) pk_aligned|(uintptr_t) 1); + unsigned char *sk = (unsigned char *) ((uintptr_t) sk_aligned|(uintptr_t) 1); + unsigned char *sm = (unsigned char *) ((uintptr_t) sm_aligned|(uintptr_t) 1); + unsigned char *m = (unsigned char *) ((uintptr_t) m_aligned|(uintptr_t) 1); unsigned long long mlen; unsigned long long smlen; int returncode; int i; + /* + * Write 8 byte canary before and after the actual memory regions. + * This is used to validate that the implementation does not assume + * anything about the placement of data in memory + * (e.g., assuming that the pk is always behind the sk) + */ write_canary(pk); - write_canary(pk + sizeof(pk) - 8); + write_canary(pk + CRYPTO_PUBLICKEYBYTES + 8); write_canary(sk); - write_canary(sk + sizeof(sk) - 8); + write_canary(sk + CRYPTO_SECRETKEYBYTES + 8); write_canary(sm); - write_canary(sm + sizeof(sm) - 8); + write_canary(sm + MLEN + CRYPTO_BYTES + 8); write_canary(m); - write_canary(m + sizeof(m) - 8); + write_canary(m + MLEN + 8); for (i = 0; i < NTESTS; i++) { RETURNS_ZERO(crypto_sign_keypair(pk + 8, sk + 8)); @@ -78,10 +100,11 @@ static int test_sign(void) { } return 1; } - if (check_canary(pk) || check_canary(pk + sizeof(pk) - 8) || - check_canary(sk) || check_canary(sk + sizeof(sk) - 8) || - check_canary(sm) || check_canary(sm + sizeof(sm) - 8) || - check_canary(m) || check_canary(m + sizeof(m) - 8)) { + // Validate that the implementation did not touch the canary + if (check_canary(pk) || check_canary(pk + CRYPTO_PUBLICKEYBYTES + 8) || + check_canary(sk) || check_canary(sk + CRYPTO_SECRETKEYBYTES + 8) || + check_canary(sm) || check_canary(sm + MLEN + CRYPTO_BYTES + 8) || + check_canary(m) || check_canary(m + MLEN + 8)) { printf("ERROR canary overwritten\n"); return 1; }