From 9aa5ec52494add5ce63c4f8217e4a303fac7ebfb Mon Sep 17 00:00:00 2001 From: Simon Josefsson Date: Wed, 23 Aug 2023 09:16:34 +0200 Subject: [PATCH 1/2] crypto: Add ssh_crypto_free(). The intention is that this releases memory allocated by the crypto library, for functions like bignum_bn2hex() and bignum_bn2dec(). Consequently, ssh_gcry_bn2dec and ssh_mbedcry_bn2num should use gcry_malloc() and mbedtls_calloc() respectively to allocate memory since it will/should be released by ssh_crypto_free() so that the internal APIs are consistent between crypto libraries. Signed-off-by: Simon Josefsson --- include/libssh/libcrypto.h | 7 +++++++ include/libssh/libgcrypt.h | 2 ++ include/libssh/libmbedcrypto.h | 3 +++ src/bignum.c | 8 +------- src/gcrypt_missing.c | 2 +- src/mbedcrypto_missing.c | 2 +- 6 files changed, 15 insertions(+), 9 deletions(-) diff --git a/include/libssh/libcrypto.h b/include/libssh/libcrypto.h index 2a7343fc5..79a5fd5cf 100644 --- a/include/libssh/libcrypto.h +++ b/include/libssh/libcrypto.h @@ -59,8 +59,15 @@ typedef void *EVPCTX; #define EVP_DIGEST_LEN EVP_MAX_MD_SIZE #endif +/* Use ssh_crypto_free() to release memory allocated by bignum_bn2dec(), + bignum_bn2hex() and other functions that use crypto-library functions that + are documented to allocate memory that needs to be de-allocate with + OPENSSL_free. */ +#define ssh_crypto_free(x) OPENSSL_free(x) + #include #include + typedef BIGNUM* bignum; typedef const BIGNUM* const_bignum; typedef BN_CTX* bignum_CTX; diff --git a/include/libssh/libgcrypt.h b/include/libssh/libgcrypt.h index e4087fd22..966fb0447 100644 --- a/include/libssh/libgcrypt.h +++ b/include/libssh/libgcrypt.h @@ -49,6 +49,8 @@ typedef gcry_md_hd_t EVPCTX; #define EVP_DIGEST_LEN EVP_MAX_MD_SIZE +#define ssh_crypto_free(x) gcry_free(x) + typedef gcry_mpi_t bignum; typedef const struct gcry_mpi *const_bignum; typedef void* bignum_CTX; diff --git a/include/libssh/libmbedcrypto.h b/include/libssh/libmbedcrypto.h index 6cf186261..a4ee010b7 100644 --- a/include/libssh/libmbedcrypto.h +++ b/include/libssh/libmbedcrypto.h @@ -34,6 +34,7 @@ #include #include #include +#include typedef mbedtls_md_context_t *SHACTX; typedef mbedtls_md_context_t *SHA256CTX; @@ -59,6 +60,8 @@ typedef mbedtls_md_context_t *EVPCTX; #define EVP_DIGEST_LEN EVP_MAX_MD_SIZE +#define ssh_crypto_free(x) mbedtls_free(x) + typedef mbedtls_mpi *bignum; typedef const mbedtls_mpi *const_bignum; typedef void* bignum_CTX; diff --git a/src/bignum.c b/src/bignum.c index d812b4127..bee55d671 100644 --- a/src/bignum.c +++ b/src/bignum.c @@ -88,11 +88,5 @@ void ssh_print_bignum(const char *name, const_bignum num) } SSH_LOG(SSH_LOG_DEBUG, "%s value: %s", name, (hex == NULL) ? "(null)" : (char *)hex); -#ifdef HAVE_LIBGCRYPT - SAFE_FREE(hex); -#elif defined HAVE_LIBCRYPTO - OPENSSL_free(hex); -#elif defined HAVE_LIBMBEDCRYPTO - SAFE_FREE(hex); -#endif + ssh_crypto_free(hex); } diff --git a/src/gcrypt_missing.c b/src/gcrypt_missing.c index e931ec5bb..21a63a9b2 100644 --- a/src/gcrypt_missing.c +++ b/src/gcrypt_missing.c @@ -55,7 +55,7 @@ char *ssh_gcry_bn2dec(bignum bn) { size = gcry_mpi_get_nbits(bn) * 3; rsize = size / 10 + size / 1000 + 2; - ret = malloc(rsize + 1); + ret = gcry_malloc(rsize + 1); if (ret == NULL) { return NULL; } diff --git a/src/mbedcrypto_missing.c b/src/mbedcrypto_missing.c index fb35ca473..2c1a8d7ad 100644 --- a/src/mbedcrypto_missing.c +++ b/src/mbedcrypto_missing.c @@ -56,7 +56,7 @@ char *ssh_mbedcry_bn2num(const_bignum num, int radix) return NULL; } - buf = malloc(olen); + buf = mbedtls_calloc(1, olen); if (buf == NULL) { return NULL; } -- GitLab From f8ca4a79e202d25f23cb1ad888197d161787d73b Mon Sep 17 00:00:00 2001 From: Simon Josefsson Date: Sun, 20 Aug 2023 18:31:32 +0200 Subject: [PATCH 2/2] tests: Regression check src/bignum.c. Signed-off-by: Simon Josefsson --- tests/unittests/CMakeLists.txt | 1 + tests/unittests/torture_bignum.c | 106 +++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 tests/unittests/torture_bignum.c diff --git a/tests/unittests/CMakeLists.txt b/tests/unittests/CMakeLists.txt index f14598653..04fcba112 100644 --- a/tests/unittests/CMakeLists.txt +++ b/tests/unittests/CMakeLists.txt @@ -3,6 +3,7 @@ project(unittests C) include_directories(${OPENSSL_INCLUDE_DIR}) set(LIBSSH_UNIT_TESTS + torture_bignum torture_buffer torture_bytearray torture_callbacks diff --git a/tests/unittests/torture_bignum.c b/tests/unittests/torture_bignum.c new file mode 100644 index 000000000..c36b81f85 --- /dev/null +++ b/tests/unittests/torture_bignum.c @@ -0,0 +1,106 @@ +#include "config.h" + +#define LIBSSH_STATIC + +#include "torture.h" +#include "libssh/bignum.h" +#include "libssh/string.h" + +static void check_str (int n, ssh_string str) +{ + if (n > 0 && n <= 127) { + assert_int_equal(1, ntohl (str->size)); + assert_int_equal(n, str->data[0]); + } else if (n > 127 && n <= 255) { + assert_int_equal(2, ntohl (str->size)); + assert_int_equal(0, str->data[0]); + assert_int_equal(n, str->data[1]); + } else if (n > 255 && n <= 32767) { + assert_int_equal(2, ntohl (str->size)); + assert_int_equal(n >> 8, str->data[0]); + assert_int_equal(n & 0xFF, str->data[1]); + } else { + assert_int_equal(3, ntohl (str->size)); + assert_int_equal(n >> 16, str->data[0]); + assert_int_equal((n >> 8) & 0xFF, str->data[1]); + assert_int_equal(n & 0xFF, str->data[2]); + } +} + +static void check_bignum(int n, const char *nstr) { + bignum num, num2; + ssh_string str; + char *dec; + + num = bignum_new(); + assert_non_null(num); + + assert_int_equal (1, bignum_set_word (num, n)); + + ssh_print_bignum("num", num); + + dec = bignum_bn2dec (num); + assert_non_null (dec); + assert_string_equal (nstr, dec); + ssh_crypto_free(dec); + + /* ssh_make_bignum_string */ + + str = ssh_make_bignum_string(num); + assert_non_null(str); + + check_str (n, str); + + /* ssh_make_string_bn */ + + num2 = ssh_make_string_bn(str); + ssh_string_free (str); + assert_non_null(num2); + + ssh_print_bignum("num2", num2); + + assert_int_equal (0, bignum_cmp (num, num2)); + + dec = bignum_bn2dec (num2); + assert_non_null (dec); + assert_string_equal (nstr, dec); + ssh_crypto_free(dec); + + bignum_safe_free(num); + bignum_safe_free(num2); +} + + +static void torture_bignum(void **state) { + (void) state; /* unused */ + + ssh_set_log_level(SSH_LOG_TRACE); + + check_bignum (1, "1"); + check_bignum (17, "17"); + check_bignum (42, "42"); + check_bignum (127, "127"); + check_bignum (128, "128"); + check_bignum (254, "254"); + check_bignum (255, "255"); + check_bignum (256, "256"); + check_bignum (257, "257"); + check_bignum (300, "300"); + check_bignum (32767, "32767"); + check_bignum (32768, "32768"); + check_bignum (65535, "65535"); + check_bignum (65536, "65536"); +} + +int torture_run_tests(void) { + int rc; + struct CMUnitTest tests[] = { + cmocka_unit_test(torture_bignum), + }; + + ssh_init(); + torture_filter_tests(tests); + rc = cmocka_run_group_tests(tests, NULL, NULL); + ssh_finalize(); + return rc; +} -- GitLab