commit 5f1c8a720fdcc62cb9854da7b3a64a7708f5f88c
author Valerio Setti <valerio.setti@nordicsemi.no> Fri Oct 11 14:55:24 2024 +0200
committer Gilles Peskine <Gilles.Peskine@arm.com> Mon Oct 14 10:37:00 2024 +0200
tree 764dd800bce790a8c3d6b6c9b55ef06573cddb1e
parent 71c569d44bf3a8bd53d874c81ee8ac644dd6e9e3

pkwrite: fix buffer overrun

This commit fixes potential buffer overrun in:
- pk_write_rsa_der
- pk_write_ec_pubkey

In both functions, when dealing with opaque keys, there was no
check that the provided buffer was large enough to contain the
key being exported. This commit fixes this problem and it also
adds some testing in test_suite_pkwrite to trigger these checks.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

tests/suites/test_suite_pkwrite.function

diff --git a/tests/suites/test_suite_pkwrite.function b/tests/suites/test_suite_pkwrite.function
index 735c125..3392528 100644
--- a/tests/suites/test_suite_pkwrite.function
+++ b/tests/suites/test_suite_pkwrite.function
@@ -2,6 +2,7 @@
 #include "pk_internal.h"
 #include "mbedtls/pem.h"
 #include "mbedtls/oid.h"
+#include "mbedtls/base64.h"
 #include "psa/crypto_sizes.h"
 
 typedef enum {
@@ -72,7 +73,8 @@
     unsigned char *buf = NULL;
     unsigned char *check_buf = NULL;
     unsigned char *start_buf;
-    size_t buf_len, check_buf_len;
+    size_t buf_len, check_buf_len, wrong_buf_len = 1;
+    int expected_result;
 #if defined(MBEDTLS_USE_PSA_CRYPTO)
     mbedtls_svc_key_id_t opaque_id = MBEDTLS_SVC_KEY_ID_INIT;
     psa_key_attributes_t key_attr = PSA_KEY_ATTRIBUTES_INIT;
@@ -109,6 +111,17 @@
 
     start_buf = buf;
     buf_len = check_buf_len;
+    if (is_der) {
+        expected_result = MBEDTLS_ERR_ASN1_BUF_TOO_SMALL;
+    } else {
+        expected_result = MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL;
+    }
+    /* Intentionally pass a wrong size for the provided output buffer and check
+     * that the writing functions fails as expected. */
+    TEST_EQUAL(pk_write_any_key(&key, &start_buf, &wrong_buf_len, is_public_key,
+                                is_der), expected_result);
+    TEST_EQUAL(pk_write_any_key(&key, &start_buf, &buf_len, is_public_key,
+                                is_der), 0);
     TEST_EQUAL(pk_write_any_key(&key, &start_buf, &buf_len, is_public_key,
                                 is_der), 0);
 
@@ -127,6 +140,10 @@
         TEST_EQUAL(mbedtls_pk_setup_opaque(&key, opaque_id), 0);
         start_buf = buf;
         buf_len = check_buf_len;
+        /* Intentionally pass a wrong size for the provided output buffer and check
+         * that the writing functions fails as expected. */
+        TEST_EQUAL(pk_write_any_key(&key, &start_buf, &wrong_buf_len, is_public_key,
+                                    is_der), expected_result);
         TEST_EQUAL(pk_write_any_key(&key, &start_buf, &buf_len, is_public_key,
                                     is_der), 0);