From 8ffb087545734c025e740cbf1fded9394b8c4ae3 Mon Sep 17 00:00:00 2001 From: Thom Wiggers Date: Wed, 5 Feb 2020 12:38:08 +0100 Subject: [PATCH 1/2] Move keys in crypto_sign/functest.c to the heap Having the keys on the stack increases the stack space consumption by quite a bit, and this in turn results in the sanitizer tests failing for Rainbow. Moving the keys to the heap in the test seems like a harmless change. --- test/crypto_sign/functest.c | 83 ++++++++++++++++++++++++++----------- 1 file changed, 59 insertions(+), 24 deletions(-) diff --git a/test/crypto_sign/functest.c b/test/crypto_sign/functest.c index 23b1791b..aae36d90 100644 --- a/test/crypto_sign/functest.c +++ b/test/crypto_sign/functest.c @@ -3,6 +3,7 @@ #include #include #include +#include #include #ifndef NTESTS @@ -33,6 +34,16 @@ static int check_canary(const uint8_t *d) { return 0; } +/** Safe malloc */ +inline static void* malloc_s(size_t size) { + void *ptr = malloc(size); + if (ptr == NULL) { + perror("Malloc failed!"); + exit(1); + } + return ptr; +} + // https://stackoverflow.com/a/1489985/1711232 #define PASTER(x, y) x##_##y #define EVALUATOR(x, y) PASTER(x, y) @@ -52,7 +63,8 @@ static int check_canary(const uint8_t *d) { #define RETURNS_ZERO(f) \ if ((f) != 0) { \ puts("(f) returned non-zero returncode"); \ - return -1; \ + res = 1; \ + goto end; \ } // https://stackoverflow.com/a/55243651/248065 @@ -72,10 +84,10 @@ static int test_sign(void) { * 16 extra bytes for canary * 1 extra byte for unalignment */ - uint8_t pk_aligned[CRYPTO_PUBLICKEYBYTES + 16 + 1]; - uint8_t sk_aligned[CRYPTO_SECRETKEYBYTES + 16 + 1]; - uint8_t sm_aligned[MLEN + CRYPTO_BYTES + 16 + 1]; - uint8_t m_aligned[MLEN + 16 + 1]; + uint8_t *pk_aligned = malloc_s(CRYPTO_PUBLICKEYBYTES + 16 + 1); + uint8_t *sk_aligned = malloc_s(CRYPTO_SECRETKEYBYTES + 16 + 1); + uint8_t *sm_aligned = malloc_s(MLEN + CRYPTO_BYTES + 16 + 1); + uint8_t *m_aligned = malloc_s(MLEN + 16 + 1); /* * Make sure all pointers are odd. @@ -91,6 +103,7 @@ static int test_sign(void) { size_t mlen; size_t smlen; int returncode; + int res = 0; int i; /* @@ -122,7 +135,8 @@ static int test_sign(void) { if (returncode > 0) { fprintf(stderr, "ERROR return code should be < 0 on failure"); } - return 1; + res = 1; + goto end; } // Validate that the implementation did not touch the canary if (check_canary(pk) || check_canary(pk + CRYPTO_PUBLICKEYBYTES + 8) || @@ -130,11 +144,17 @@ static int test_sign(void) { check_canary(sm) || check_canary(sm + MLEN + CRYPTO_BYTES + 8) || check_canary(m) || check_canary(m + MLEN + 8)) { fprintf(stderr, "ERROR canary overwritten\n"); - return 1; + res = 1; + goto end; } } +end: + free(pk_aligned); + free(sk_aligned); + free(sm_aligned); + free(m_aligned); - return 0; + return res; } static int test_sign_detached(void) { @@ -143,10 +163,10 @@ static int test_sign_detached(void) { * 16 extra bytes for canary * 1 extra byte for unalignment */ - uint8_t pk_aligned[CRYPTO_PUBLICKEYBYTES + 16 + 1]; - uint8_t sk_aligned[CRYPTO_SECRETKEYBYTES + 16 + 1]; - uint8_t sig_aligned[CRYPTO_BYTES + 16 + 1]; - uint8_t m_aligned[MLEN + 16 + 1]; + uint8_t *pk_aligned = malloc_s(CRYPTO_PUBLICKEYBYTES + 16 + 1); + uint8_t *sk_aligned = malloc_s(CRYPTO_SECRETKEYBYTES + 16 + 1); + uint8_t *sig_aligned = malloc_s(CRYPTO_BYTES + 16 + 1); + uint8_t *m_aligned = malloc_s(MLEN + 16 + 1); /* * Make sure all pointers are odd. @@ -161,6 +181,7 @@ static int test_sign_detached(void) { size_t siglen; int returncode; + int res = 0; int i; /* @@ -190,7 +211,8 @@ static int test_sign_detached(void) { if (returncode > 0) { fprintf(stderr, "ERROR return code should be < 0 on failure"); } - return 1; + res = 1; + goto end; } // Validate that the implementation did not touch the canary if (check_canary(pk) || check_canary(pk + CRYPTO_PUBLICKEYBYTES + 8) || @@ -198,24 +220,31 @@ static int test_sign_detached(void) { check_canary(sig) || check_canary(sig + CRYPTO_BYTES + 8) || check_canary(m) || check_canary(m + MLEN + 8)) { fprintf(stderr, "ERROR canary overwritten\n"); - return 1; + res = 1; + goto end; } } - return 0; +end: + free(pk_aligned); + free(sk_aligned); + free(sig_aligned); + free(m_aligned); + + return res; } static int test_wrong_pk(void) { - uint8_t pk[CRYPTO_PUBLICKEYBYTES]; - uint8_t pk2[CRYPTO_PUBLICKEYBYTES]; - uint8_t sk[CRYPTO_SECRETKEYBYTES]; - uint8_t sm[MLEN + CRYPTO_BYTES]; - uint8_t m[MLEN]; + uint8_t *pk = malloc_s(CRYPTO_PUBLICKEYBYTES); + uint8_t *pk2 = malloc_s(CRYPTO_PUBLICKEYBYTES); + uint8_t *sk = malloc_s(CRYPTO_SECRETKEYBYTES); + uint8_t *sm = malloc_s(MLEN + CRYPTO_BYTES); + uint8_t *m = malloc_s(MLEN); size_t mlen; size_t smlen; - int returncode; + int returncode, res = 0; int i; @@ -235,11 +264,17 @@ static int test_wrong_pk(void) { if (returncode > 0) { fprintf(stderr, "ERROR return code should be < 0"); } - return 1; + res = 1; + goto end; } } - - return 0; +end: + free(pk); + free(pk2); + free(sk); + free(sm); + free(m); + return res; } int main(void) { From 1a4739e2bf5f89c278faa5e7adab0df7d647f1a0 Mon Sep 17 00:00:00 2001 From: Thom Wiggers Date: Tue, 11 Feb 2020 12:23:06 +0100 Subject: [PATCH 2/2] Move keys to heap for KEMs as well --- test/crypto_kem/functest.c | 89 ++++++++++++++++++++++++++++---------- 1 file changed, 66 insertions(+), 23 deletions(-) diff --git a/test/crypto_kem/functest.c b/test/crypto_kem/functest.c index 16650b21..ffd38ac8 100644 --- a/test/crypto_kem/functest.c +++ b/test/crypto_kem/functest.c @@ -1,7 +1,9 @@ #include "api.h" #include "randombytes.h" + #include #include +#include #include #ifndef NTESTS @@ -30,6 +32,15 @@ static int check_canary(const uint8_t *d) { return 0; } +inline static void* malloc_s(size_t size) { + void *ptr = malloc(size); + if (ptr == NULL) { + perror("Malloc failed!"); + exit(1); + } + return ptr; +} + // https://stackoverflow.com/a/1489985/1711232 #define PASTER(x, y) x##_##y #define EVALUATOR(x, y) PASTER(x, y) @@ -48,7 +59,8 @@ static int check_canary(const uint8_t *d) { #define RETURNS_ZERO(f) \ if ((f) != 0) { \ puts(#f " returned non-zero returncode"); \ - return -1; \ + res = 1; \ + goto end; \ } // https://stackoverflow.com/a/55243651/248065 @@ -67,12 +79,13 @@ static int test_keys(void) { * 16 extra bytes for canary * 1 extra byte for unalignment */ + int res = 0; - uint8_t key_a_aligned[CRYPTO_BYTES + 16 + 1]; - uint8_t key_b_aligned[CRYPTO_BYTES + 16 + 1]; - uint8_t pk_aligned[CRYPTO_PUBLICKEYBYTES + 16 + 1]; - uint8_t sendb_aligned[CRYPTO_CIPHERTEXTBYTES + 16 + 1]; - uint8_t sk_a_aligned[CRYPTO_SECRETKEYBYTES + 16 + 1]; + uint8_t *key_a_aligned = malloc_s(CRYPTO_BYTES + 16 + 1); + uint8_t *key_b_aligned = malloc_s(CRYPTO_BYTES + 16 + 1); + uint8_t *pk_aligned = malloc_s(CRYPTO_PUBLICKEYBYTES + 16 + 1); + uint8_t *sendb_aligned = malloc_s(CRYPTO_CIPHERTEXTBYTES + 16 + 1); + uint8_t *sk_a_aligned = malloc_s(CRYPTO_SECRETKEYBYTES + 16 + 1); /* * Make sure all pointers are odd. @@ -117,7 +130,8 @@ static int test_keys(void) { if (memcmp(key_a + 8, key_b + 8, CRYPTO_BYTES) != 0) { printf("ERROR KEYS\n"); - return -1; + res = 1; + goto end; } // Validate that the implementation did not touch the canary @@ -127,20 +141,30 @@ static int test_keys(void) { 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; + res = 1; + goto end; } } - return 0; +end: + free(key_a_aligned); + free(key_b_aligned); + free(pk_aligned); + free(sendb_aligned); + free(sk_a_aligned); + + return res; } static int test_invalid_sk_a(void) { - uint8_t sk_a[CRYPTO_SECRETKEYBYTES]; - uint8_t key_a[CRYPTO_BYTES], key_b[CRYPTO_BYTES]; - uint8_t pk[CRYPTO_PUBLICKEYBYTES]; - uint8_t sendb[CRYPTO_CIPHERTEXTBYTES]; + uint8_t *sk_a = malloc_s(CRYPTO_SECRETKEYBYTES); + uint8_t *key_a = malloc_s(CRYPTO_BYTES); + uint8_t *key_b = malloc_s(CRYPTO_BYTES); + uint8_t *pk = malloc_s(CRYPTO_PUBLICKEYBYTES); + uint8_t *sendb = malloc_s(CRYPTO_CIPHERTEXTBYTES); int i; int returncode; + int res = 0; for (i = 0; i < NTESTS; i++) { // Alice generates a public key @@ -157,25 +181,36 @@ static int test_invalid_sk_a(void) { printf("ERROR failing crypto_kem_dec returned %d instead of " "negative or zero code\n", returncode); - return -1; + res = 1; + goto end; } if (!memcmp(key_a, key_b, CRYPTO_BYTES)) { printf("ERROR invalid sk_a\n"); - return -1; + res = 1; + goto end; } } - return 0; +end: + free(sk_a); + free(key_a); + free(key_b); + free(pk); + free(sendb); + + return res; } static int test_invalid_ciphertext(void) { - uint8_t sk_a[CRYPTO_SECRETKEYBYTES]; - uint8_t key_a[CRYPTO_BYTES], key_b[CRYPTO_BYTES]; - uint8_t pk[CRYPTO_PUBLICKEYBYTES]; - uint8_t sendb[CRYPTO_CIPHERTEXTBYTES]; + uint8_t *sk_a = malloc_s(CRYPTO_SECRETKEYBYTES); + uint8_t *key_a = malloc_s(CRYPTO_BYTES); + uint8_t *key_b = malloc_s(CRYPTO_BYTES); + uint8_t *pk = malloc_s(CRYPTO_PUBLICKEYBYTES); + uint8_t *sendb = malloc_s(CRYPTO_CIPHERTEXTBYTES); int i; int returncode; + int res = 0; for (i = 0; i < NTESTS; i++) { // Alice generates a public key @@ -192,16 +227,24 @@ static int test_invalid_ciphertext(void) { printf("ERROR crypto_kem_dec should either fail (negative " "returncode) or succeed (return 0) but returned %d\n", returncode); - return -1; + res = 1; + goto end; } if (!memcmp(key_a, key_b, CRYPTO_BYTES)) { printf("ERROR invalid ciphertext\n"); - return -1; + res = 1; + goto end; } } +end: + free(sk_a); + free(key_a); + free(key_b); + free(pk); + free(sendb); - return 0; + return res; } int main(void) {