Merge branch 'fix/esp_crt_bundle_init_api' into 'master'

esp_crt_bundle: Add bounds checking for the "esp_crt_bundle_init" API.

Closes IDFGH-6769

See merge request espressif/esp-idf!17646

components/mbedtls/Kconfig

@@ -327,6 +327,12 @@ menu "mbedTLS"
             help
                 Name of the custom certificate directory or file. This path is evaluated
                 relative to the project root directory.
+
+        config MBEDTLS_CERTIFICATE_BUNDLE_MAX_CERTS
+            int "Maximum no of certificates allowed in certificate bundle"
+            default 200
+            depends on MBEDTLS_CERTIFICATE_BUNDLE
+
     endmenu
 
     config MBEDTLS_ECP_RESTARTABLE

components/mbedtls/esp_crt_bundle/esp_crt_bundle.c

@@ -146,27 +146,56 @@ int esp_crt_verify_callback(void *buf, mbedtls_x509_crt *crt, int depth, uint32_
 /* Initialize the bundle into an array so we can do binary search for certs,
    the bundle generated by the python utility is already presorted by subject name
  */
-static esp_err_t esp_crt_bundle_init(const uint8_t *x509_bundle)
+static esp_err_t esp_crt_bundle_init(const uint8_t *x509_bundle, size_t bundle_size)
 {
-    s_crt_bundle.num_certs = (x509_bundle[0] << 8) | x509_bundle[1];
-    s_crt_bundle.crts = calloc(s_crt_bundle.num_certs, sizeof(x509_bundle));
+    if (bundle_size < BUNDLE_HEADER_OFFSET + CRT_HEADER_OFFSET) {
+        ESP_LOGE(TAG, "Invalid certificate bundle");
+        return ESP_ERR_INVALID_ARG;
+    }
 
-    if (s_crt_bundle.crts == NULL) {
+    uint16_t num_certs = (x509_bundle[0] << 8) | x509_bundle[1];
+    if (num_certs > CONFIG_MBEDTLS_CERTIFICATE_BUNDLE_MAX_CERTS) {
+        ESP_LOGE(TAG, "No. of certs in the certificate bundle = %d exceeds\n"
+                      "Max allowed certificates in the certificate bundle = %d\n"
+                      "Please update the menuconfig option with appropriate value", num_certs, CONFIG_MBEDTLS_CERTIFICATE_BUNDLE_MAX_CERTS);
+        return ESP_ERR_INVALID_ARG;
+    }
+
+    const uint8_t **crts = calloc(num_certs, sizeof(x509_bundle));
+    if (crts == NULL) {
         ESP_LOGE(TAG, "Unable to allocate memory for bundle");
         return ESP_ERR_NO_MEM;
     }
 
     const uint8_t *cur_crt;
+    /* This is the maximum region that is allowed to access */
+    const uint8_t *bundle_end = x509_bundle + bundle_size;
     cur_crt = x509_bundle + BUNDLE_HEADER_OFFSET;
 
-    for (int i = 0; i < s_crt_bundle.num_certs; i++) {
-        s_crt_bundle.crts[i] = cur_crt;
-
+    for (int i = 0; i < num_certs; i++) {
+        crts[i] = cur_crt;
+        if (cur_crt + CRT_HEADER_OFFSET > bundle_end) {
+            ESP_LOGE(TAG, "Invalid certificate bundle");
+            free(crts);
+            return ESP_ERR_INVALID_ARG;
+        }
         size_t name_len = cur_crt[0] << 8 | cur_crt[1];
         size_t key_len = cur_crt[2] << 8 | cur_crt[3];
         cur_crt = cur_crt + CRT_HEADER_OFFSET + name_len + key_len;
     }
 
+    if (cur_crt > bundle_end) {
+        ESP_LOGE(TAG, "Invalid certificate bundle");
+        free(crts);
+        return ESP_ERR_INVALID_ARG;
+    }
+
+    /* The previous crt bundle is only updated when initialization of the
+     * current crt_bundle is successful */
+    /* Free previous crt_bundle */
+    free(s_crt_bundle.crts);
+    s_crt_bundle.num_certs = num_certs;
+    s_crt_bundle.crts = crts;
     return ESP_OK;
 }
 
@@ -175,7 +204,7 @@ esp_err_t esp_crt_bundle_attach(void *conf)
     esp_err_t ret = ESP_OK;
     // If no bundle has been set by the user then use the bundle embedded in the binary
     if (s_crt_bundle.crts == NULL) {
-        ret = esp_crt_bundle_init(x509_crt_imported_bundle_bin_start);
+        ret = esp_crt_bundle_init(x509_crt_imported_bundle_bin_start, x509_crt_imported_bundle_bin_end - x509_crt_imported_bundle_bin_start);
     }
 
     if (ret != ESP_OK) {
@@ -206,9 +235,7 @@ void esp_crt_bundle_detach(mbedtls_ssl_config *conf)
     }
 }
 
-void esp_crt_bundle_set(const uint8_t *x509_bundle)
+esp_err_t esp_crt_bundle_set(const uint8_t *x509_bundle, size_t bundle_size)
 {
-    // Free any previously used bundle
-    free(s_crt_bundle.crts);
-    esp_crt_bundle_init(x509_bundle);
+    return esp_crt_bundle_init(x509_bundle, bundle_size);
 }

components/mbedtls/esp_crt_bundle/include/esp_crt_bundle.h

@@ -45,13 +45,19 @@ void esp_crt_bundle_detach(mbedtls_ssl_config *conf);
 /**
  * @brief      Set the default certificate bundle used for verification
  *
- * Overrides the default certificate bundle. In most use cases the bundle should be
+ * Overrides the default certificate bundle only in case of successful initialization. In most use cases the bundle should be
  * set through menuconfig. The bundle needs to be sorted by subject name since binary search is
  * used to find certificates.
  *
  * @param[in]  x509_bundle     A pointer to the certificate bundle.
+ *
+ * @param[in]  bundle_size     Size of the certificate bundle in bytes.
+ *
+ * @return
+ *             - ESP_OK  if adding certificates was successful.
+ *             - Other   if an error occured or an action must be taken by the calling process.
  */
-void esp_crt_bundle_set(const uint8_t *x509_bundle);
+esp_err_t esp_crt_bundle_set(const uint8_t *x509_bundle, size_t bundle_size);
 
 
 #ifdef __cplusplus

components/mbedtls/test/test_esp_crt_bundle.c

@@ -25,6 +25,7 @@
 #include "mbedtls/debug.h"
 
 #include "esp_crt_bundle.h"
+#include "esp_random.h"
 
 #include "unity.h"
 #include "test_utils.h"
@@ -252,7 +253,7 @@ esp_err_t client_setup(mbedtls_endpoint_t *client)
     return ESP_OK;
 }
 
-int client_task(const uint8_t *bundle, esp_crt_validate_res_t *res)
+int client_task(const uint8_t *bundle, size_t bundle_size, esp_crt_validate_res_t *res)
 {
     int ret = ESP_FAIL;
 
@@ -268,7 +269,7 @@ int client_task(const uint8_t *bundle, esp_crt_validate_res_t *res)
     esp_crt_bundle_attach(&client.conf);
     if (bundle) {
         /* Set a bundle different from the menuconfig bundle */
-        esp_crt_bundle_set(bundle);
+        esp_crt_bundle_set(bundle, bundle_size);
     }
 
     ESP_LOGI(TAG, "Connecting to %s:%s...", SERVER_ADDRESS, SERVER_PORT);
@@ -328,11 +329,11 @@ TEST_CASE("custom certificate bundle", "[mbedtls]")
    }
 
    /* Test with default crt bundle that doesnt contain the ca crt */
-   client_task(NULL, &validate_res);
+   client_task(NULL, 0, &validate_res);
    TEST_ASSERT(validate_res == ESP_CRT_VALIDATE_FAIL);
 
    /* Test with bundle that does contain the CA crt */
-   client_task(server_cert_bundle_start, &validate_res);
+   client_task(server_cert_bundle_start, server_cert_bundle_end - server_cert_bundle_start, &validate_res);
    TEST_ASSERT(validate_res == ESP_CRT_VALIDATE_OK);
 
    exit_flag = true;
@@ -391,3 +392,50 @@ TEST_CASE("custom certificate bundle - wrong signature", "[mbedtls]")
 
     esp_crt_bundle_detach(NULL);
 }
+
+TEST_CASE("custom certificate bundle init API - bound checking", "[mbedtls]")
+{
+
+    uint8_t test_bundle[256] = {0};
+    esp_err_t esp_ret;
+    /* The API should fail with bundle size given as 1 */
+    esp_ret = esp_crt_bundle_set(test_bundle, 1);
+    TEST_ASSERT( esp_ret == ESP_ERR_INVALID_ARG);
+
+    /* Check that the esp_crt_bundle_set API will not accept a bundle
+     * which has more no. of certs than configured in
+     * CONFIG_MBEDTLS_CERTIFICATE_BUNDLE_MAX_CERTS */
+
+    uint8_t rand;
+    esp_fill_random(&rand, 1);
+    test_bundle[0] = rand;
+
+    /* Make sure that the number of certs will always be greater than
+     * CONFIG_MBEDTLS_CERTIFICATE_BUNDLE_MAX_CERTS */
+    test_bundle[1] = rand + CONFIG_MBEDTLS_CERTIFICATE_BUNDLE_MAX_CERTS;
+
+    esp_ret = esp_crt_bundle_set(test_bundle, sizeof(test_bundle));
+    TEST_ASSERT( esp_ret == ESP_ERR_INVALID_ARG);
+
+    /* The API should fail with bundle_size < BUNDLE_HEADER_OFFSET (2) + CRT_HEADER_OFFSET (4) */
+    test_bundle[0] = 0;
+    test_bundle[1] = 1; /* set num_certs = 1 */
+    esp_ret = esp_crt_bundle_set(test_bundle, 5);
+    TEST_ASSERT(esp_ret == ESP_ERR_INVALID_ARG);
+
+    /* Cert number is greater than actual certs present, The API should fail */
+    /* Actual No. of certs present in bundle = 1, setting num_certs to 5 */
+    test_bundle[1] = 5; /* num_certs */
+    test_bundle[3] = 5; /* cert_1_name_len */
+    test_bundle[5] = 10; /* cert_1_pub_key_len */
+    /* Actual bundle size becomes BUNDLE_HEADER_OFFSET (2) + CRT_HEADER_OFFSET (4) + cert_1_name_len(5) + cert_1_pub_key_len(10)
+     * i.e. 21 bytes */
+    esp_ret = esp_crt_bundle_set(test_bundle, 21);
+    TEST_ASSERT(esp_ret == ESP_ERR_INVALID_ARG);
+
+    /* The API should fail if bundle_size < BUNDLE_HEADER_OFFSET (2) + CRT_HEADER_OFFSET (4) + cert_1_name_len(5) + cert_1_pub_key_len(10) */
+    esp_ret = esp_crt_bundle_set(test_bundle, 20);
+    TEST_ASSERT(esp_ret == ESP_ERR_INVALID_ARG);
+
+    esp_crt_bundle_detach(NULL);
+}

docs/en/migration-guides/protocols.rst

@@ -64,6 +64,10 @@ Remove certs module from X509 library
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 `mbedtls/certs.h` header is no longer available in mbedtls 3.1, most applications can safely remove it from the list of includes.
 
+Breaking change for "esp_crt_bundle_set" API
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+:cpp:func:`esp_crt_bundle_set()` API now requires one additional argument named ``bundle_size``. The return type of the API has also been changed to :cpp:type:`esp_err_t` from ``void``.
+
 ESP HTTPS SERVER
 -----------------