Fix mbedtls_pk_get_bitlen() for RSA with non-byte-aligned sizes

Add non-regression tests. Update some test functions to not assume that byte_length == bit_length / 8. Signed-off-by: Gilles Peskine <[email protected]>
Mbed-TLS · Feb 12, 2024 · 0196f48 · 0196f48
1 parent ede909f
commit 0196f48
Show file tree

Hide file tree

Showing 6 changed files with 65 additions and 6 deletions.
diff --git a/ChangeLog.d/rsa-bitlen.txt b/ChangeLog.d/rsa-bitlen.txt
@@ -0,0 +1,3 @@
+Bugfix
+   * Fix mbedtls_pk_get_bitlen() for RSA keys whose size is not a
+     multiple of 8. Fixes #868.
diff --git a/library/pk_wrap.c b/library/pk_wrap.c
@@ -53,7 +53,23 @@ static int rsa_can_do(mbedtls_pk_type_t type)
 static size_t rsa_get_bitlen(const void *ctx)
 {
     const mbedtls_rsa_context *rsa = (const mbedtls_rsa_context *) ctx;
-    return 8 * mbedtls_rsa_get_len(rsa);
+    /* Unfortunately, the rsa.h interface does not have a direct way
+     * to access the bit-length that works with MBEDTLS_RSA_ALT.
+     * So we have to do a little work here.
+     */
+    mbedtls_mpi N;
+    mbedtls_mpi_init(&N);
+    int ret = mbedtls_rsa_export(rsa, &N, NULL, NULL, NULL, NULL);
+    /* If the export fails for some reason (e.g. the RSA_ALT implementation
+     * does not support export, or there is not enough memory),
+     * we have no way of returning an error from this function.
+     * As a fallback, return the byte-length converted in bits, which is
+     * the correct  value if the modulus size is a multiple of 8 bits, which
+     * is very often the case in practice. */
+    size_t bitlen = (ret == 0 ? mbedtls_mpi_bitlen(&N) :
+                     8 * mbedtls_rsa_get_len(rsa));
+    mbedtls_mpi_free(&N);
+    return bitlen;
 }
 
 static int rsa_verify_wrap(void *ctx, mbedtls_md_type_t md_alg,

diff --git a/tests/suites/test_suite_pk.data b/tests/suites/test_suite_pk.data
@@ -12,6 +12,19 @@ PK utils: RSA 512-bit
 depends_on:MBEDTLS_RSA_C:MBEDTLS_GENPRIME
 pk_utils:MBEDTLS_PK_RSA:512:512:64:"RSA"
 
+# mbedtls_rsa_gen_key() only supports even sizes, so we don't test 513 etc.
+PK utils: RSA 514-bit
+depends_on:MBEDTLS_RSA_C:MBEDTLS_GENPRIME
+pk_utils:MBEDTLS_PK_RSA:514:514:65:"RSA"
+
+PK utils: RSA 516-bit
+depends_on:MBEDTLS_RSA_C:MBEDTLS_GENPRIME
+pk_utils:MBEDTLS_PK_RSA:516:516:65:"RSA"
+
+PK utils: RSA 518-bit
+depends_on:MBEDTLS_RSA_C:MBEDTLS_GENPRIME
+pk_utils:MBEDTLS_PK_RSA:518:518:65:"RSA"
+
 PK utils: ECKEY SECP192R1
 depends_on:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP192R1_ENABLED
 pk_utils:MBEDTLS_PK_ECKEY:MBEDTLS_ECP_DP_SECP192R1:192:24:"EC"

diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function
@@ -155,7 +155,7 @@ void pk_psa_utils()
     TEST_ASSERT(strcmp(mbedtls_pk_get_name(&pk), name) == 0);
 
     TEST_ASSERT(mbedtls_pk_get_bitlen(&pk) == bitlen);
-    TEST_ASSERT(mbedtls_pk_get_len(&pk) == bitlen / 8);
+    TEST_ASSERT(mbedtls_pk_get_len(&pk) == (bitlen + 7) / 8);
 
     TEST_ASSERT(mbedtls_pk_can_do(&pk, MBEDTLS_PK_ECKEY) == 1);
     TEST_ASSERT(mbedtls_pk_can_do(&pk, MBEDTLS_PK_ECDSA) == 1);
@@ -683,7 +683,7 @@ void pk_rsa_verify_test_vec(data_t *message_str, int digest, int mod,
     TEST_ASSERT(mbedtls_pk_setup(&pk, mbedtls_pk_info_from_type(MBEDTLS_PK_RSA)) == 0);
     rsa = mbedtls_pk_rsa(pk);
 
-    rsa->len = mod / 8;
+    rsa->len = (mod + 7) / 8;
     TEST_ASSERT(mbedtls_test_read_mpi(&rsa->N, input_N) == 0);
     TEST_ASSERT(mbedtls_test_read_mpi(&rsa->E, input_E) == 0);
 
@@ -731,7 +731,7 @@ void pk_rsa_verify_ext_test_vec(data_t *message_str, int digest,
     TEST_ASSERT(mbedtls_pk_setup(&pk, mbedtls_pk_info_from_type(MBEDTLS_PK_RSA)) == 0);
     rsa = mbedtls_pk_rsa(pk);
 
-    rsa->len = mod / 8;
+    rsa->len = (mod + 7) / 8;
     TEST_ASSERT(mbedtls_test_read_mpi(&rsa->N, input_N) == 0);
     TEST_ASSERT(mbedtls_test_read_mpi(&rsa->E, input_E) == 0);
 
@@ -1004,7 +1004,7 @@ void pk_rsa_encrypt_test_vec(data_t *message, int mod,
     TEST_ASSERT(mbedtls_pk_setup(&pk, mbedtls_pk_info_from_type(MBEDTLS_PK_RSA)) == 0);
     rsa = mbedtls_pk_rsa(pk);
 
-    rsa->len = mod / 8;
+    rsa->len = (mod + 7) / 8;
     TEST_ASSERT(mbedtls_test_read_mpi(&rsa->N, input_N) == 0);
     TEST_ASSERT(mbedtls_test_read_mpi(&rsa->E, input_E) == 0);
 
@@ -1053,9 +1053,12 @@ void pk_rsa_decrypt_test_vec(data_t *cipher, int mod,
     TEST_ASSERT(mbedtls_test_read_mpi(&P, input_P) == 0);
     TEST_ASSERT(mbedtls_test_read_mpi(&Q, input_Q) == 0);
     TEST_ASSERT(mbedtls_rsa_import(rsa, &N, &P, &Q, NULL, &E) == 0);
-    TEST_ASSERT(mbedtls_rsa_get_len(rsa) == (size_t) (mod / 8));
+    TEST_EQUAL(mbedtls_rsa_get_len(rsa), (mod + 7) / 8);
     TEST_ASSERT(mbedtls_rsa_complete(rsa) == 0);
 
+    TEST_EQUAL(mbedtls_pk_get_bitlen(&pk), mod);
+    TEST_EQUAL(mbedtls_pk_get_len(&pk), (mod + 7) / 8);
+
     /* decryption test */
     memset(output, 0, sizeof(output));
     olen = 0;

diff --git a/tests/suites/test_suite_pkparse.data b/tests/suites/test_suite_pkparse.data
@@ -938,6 +938,22 @@ Parse RSA Key #99.2 (PKCS#8 encrypted v2 PBKDF2 DES hmacWithSHA512 DER, 4096-bit
 depends_on:MBEDTLS_DES_C:MBEDTLS_SHA512_C:MBEDTLS_PKCS5_C
 pk_parse_keyfile_rsa:"data_files/rsa_pkcs8_pbes2_pbkdf2_4096_des_sha512.der":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
 
+Parse RSA Key #100.1 (512-bit)
+depends_on:MBEDTLS_PEM_C
+pk_parse_keyfile_rsa:"data_files/rsa512.key":"":0
+
+Parse RSA Key #100.1 (521-bit)
+depends_on:MBEDTLS_PEM_C
+pk_parse_keyfile_rsa:"data_files/rsa521.key":"":0
+
+Parse RSA Key #100.1 (522-bit)
+depends_on:MBEDTLS_PEM_C
+pk_parse_keyfile_rsa:"data_files/rsa522.key":"":0
+
+Parse RSA Key #100.1 (528-bit)
+depends_on:MBEDTLS_PEM_C
+pk_parse_keyfile_rsa:"data_files/rsa528.key":"":0
+
 Parse Public RSA Key #1 (PKCS#8 wrapped)
 depends_on:MBEDTLS_PEM_PARSE_C
 pk_parse_public_keyfile_rsa:"data_files/rsa_pkcs8_2048_public.pem":0

diff --git a/tests/suites/test_suite_pkparse.function b/tests/suites/test_suite_pkparse.function
@@ -32,6 +32,10 @@ void pk_parse_keyfile_rsa(char *key_file, char *password, int result)
         TEST_ASSERT(mbedtls_pk_can_do(&ctx, MBEDTLS_PK_RSA));
         rsa = mbedtls_pk_rsa(ctx);
         TEST_ASSERT(mbedtls_rsa_check_privkey(rsa) == 0);
+
+        /* Test consistency between get_len and get_bitlen */
+        size_t bitlen = mbedtls_pk_get_bitlen(&ctx);
+        TEST_EQUAL(mbedtls_pk_get_len(&ctx), (bitlen + 7) / 8);
     }
 
 exit:
@@ -58,6 +62,10 @@ void pk_parse_public_keyfile_rsa(char *key_file, int result)
         TEST_ASSERT(mbedtls_pk_can_do(&ctx, MBEDTLS_PK_RSA));
         rsa = mbedtls_pk_rsa(ctx);
         TEST_ASSERT(mbedtls_rsa_check_pubkey(rsa) == 0);
+
+        /* Test consistency between get_len and get_bitlen */
+        size_t bitlen = mbedtls_pk_get_bitlen(&ctx);
+        TEST_EQUAL(mbedtls_pk_get_len(&ctx), (bitlen + 7) / 8);
     }
 
 exit: