mbedtls: Avoid using a large buffer on the stack. by MAntoniak · Pull Request #7586 · curl/curl

MAntoniak · 2021-08-17T18:22:07Z

Use dynamic memory allocation for the buffer used in checking "pinned public key". The PUB_DER_MAX_BYTES parameter with default settings is set to a value greater than 2kB.

danielgustafsson · 2021-08-17T18:37:29Z

lib/vtls/mbedtls.c

Is PUB_DER_MAX_BYTES always needed or can it be smaller and grown when needed?

So I analyzed this question more. It seems possible, but it will not be easy.
From the mbedtls code you can deduce what size buffer is needed.

For RSA:

/ * * RSA public keys: * SubjectPublicKeyInfo :: = SEQUENCE {1 + 3 * algorithm AlgorithmIdentifier, 1 + 1 (sequence) * + 1 + 1 + 9 (rsa oid) * + 1 + 1 (params null) * subjectPublicKey BIT STRING} 1 + 3 + (1 + below) * RSAPublicKey :: = SEQUENCE {1 + 3 * modulus INTEGER, - n 1 + 3 + MPI_MAX + 1 * publicExponent INTEGER - e 1 + 3 + MPI_MAX + 1 *} * /

For EC:

/ * * EC public keys: * SubjectPublicKeyInfo :: = SEQUENCE {1 + 2 * algorithm AlgorithmIdentifier, 1 + 1 (sequence) * + 1 + 1 + 7 (ec oid) * + 1 + 1 + 9 (namedCurve oid) * subjectPublicKey BIT STRING 1 + 2 + 1 [1] * + 1 (point format) [1] * + 2 * ECP_MAX (coords) [1] *} * /

The problem for RSA is determining the size of the modulus parameter and the publicExponent (instead of taking the value of MPI_MAX). For the EC, the problem is to determine the size of the Q parameter (instead of taking the value ECP_MAX).

If the public key is RSA, you can get the N (modulus) and E (exponent) parameters using the mbedtls_rsa_import function, read their size by the mbedtls_mpi_size function, sum the values and add an additional 38 bytes.

Similarly for EC: the mbedtls_pk_ec function returns the mbedtls_ecp_keypair structure that contains the Q parameter. Then read the X and Y coordinates, sum and add an additional 30 bytes.

Disadvantages:

adding new asymmetric keys will require changes in curl as well.

RSA and EC key parameters have been marked as private in the mbedtls 3.0.0 version - it is uncertain whether they will not be removed in the future for the appropriate functions in the API.

The best solution would of course be to write such a function on the mbedtls side. However, I have not found any premises for the mbedtls team to implement such a function.

Thanks for the exhaustive answer! With this in mind I agree that just allocating the known upper limit is clearly the best option.

bagder · 2021-08-18T12:23:29Z

How about trying to do all the frees at a single spot in case of errors? Like this:

diff --git a/lib/vtls/mbedtls.c b/lib/vtls/mbedtls.c
index 576b20a59..822fc18ef 100644
--- a/lib/vtls/mbedtls.c
+++ b/lib/vtls/mbedtls.c
@@ -666,12 +666,12 @@ mbed_connect_step2(struct Curl_easy *data, struct connectdata *conn,
   }
 
   if(pinnedpubkey) {
     int size;
     CURLcode result;
-    mbedtls_x509_crt *p;
-    unsigned char *pubkey;
+    mbedtls_x509_crt *p = NULL;
+    unsigned char *pubkey = NULL;
 
 #if MBEDTLS_VERSION_NUMBER >= 0x03000000
     if(!peercert || !peercert->MBEDTLS_PRIVATE(raw).MBEDTLS_PRIVATE(p) ||
        !peercert->MBEDTLS_PRIVATE(raw).MBEDTLS_PRIVATE(len)) {
 #else
@@ -680,19 +680,17 @@ mbed_connect_step2(struct Curl_easy *data, struct connectdata *conn,
       failf(data, "Failed due to missing peer certificate");
       return CURLE_SSL_PINNEDPUBKEYNOTMATCH;
     }
 
     p = calloc(1, sizeof(*p));
-
     if(!p)
       return CURLE_OUT_OF_MEMORY;
 
     pubkey = malloc(PUB_DER_MAX_BYTES);
-
     if(!pubkey) {
-      free(p);
-      return CURLE_OUT_OF_MEMORY;
+      result = CURLE_OUT_OF_MEMORY;
+      goto error;
     }
 
     mbedtls_x509_crt_init(p);
 
     /* Make a copy of our const peercert because mbedtls_pk_write_pubkey_der
@@ -704,14 +702,12 @@ mbed_connect_step2(struct Curl_easy *data, struct connectdata *conn,
                         peercert->MBEDTLS_PRIVATE(raw).MBEDTLS_PRIVATE(len))) {
 #else
     if(mbedtls_x509_crt_parse_der(p, peercert->raw.p, peercert->raw.len)) {
 #endif
       failf(data, "Failed copying peer certificate");
-      mbedtls_x509_crt_free(p);
-      free(p);
-      free(pubkey);
-      return CURLE_SSL_PINNEDPUBKEYNOTMATCH;
+      result = CURLE_SSL_PINNEDPUBKEYNOTMATCH;
+      goto error;
     }
 
 #if MBEDTLS_VERSION_NUMBER >= 0x03000000
     size = mbedtls_pk_write_pubkey_der(&p->MBEDTLS_PRIVATE(pk), pubkey,
                                        PUB_DER_MAX_BYTES);
@@ -719,20 +715,19 @@ mbed_connect_step2(struct Curl_easy *data, struct connectdata *:
conn,
     size = mbedtls_pk_write_pubkey_der(&p->pk, pubkey, PUB_DER_MAX_BYTES);
 #endif
 
     if(size <= 0) {
       failf(data, "Failed copying public key from peer certificate");
-      mbedtls_x509_crt_free(p);
-      free(p);
-      free(pubkey);
-      return CURLE_SSL_PINNEDPUBKEYNOTMATCH;
+      result = CURLE_SSL_PINNEDPUBKEYNOTMATCH;
+      goto error;
     }
 
     /* mbedtls_pk_write_pubkey_der writes data at the end of the buffer. */
     result = Curl_pin_peer_pubkey(data,
                                   pinnedpubkey,
                                   &pubkey[PUB_DER_MAX_BYTES - size], size);
+      error:
     if(result) {
       mbedtls_x509_crt_free(p);
       free(p);
       free(pubkey);
       return result;

MAntoniak · 2021-08-18T13:28:59Z

How about trying to do all the frees at a single spot in case of errors? Like this:

diff --git a/lib/vtls/mbedtls.c b/lib/vtls/mbedtls.c
index 576b20a59..822fc18ef 100644
--- a/lib/vtls/mbedtls.c
+++ b/lib/vtls/mbedtls.c
@@ -666,12 +666,12 @@ mbed_connect_step2(struct Curl_easy *data, struct connectdata *conn,
   }
 
   if(pinnedpubkey) {
     int size;
     CURLcode result;
-    mbedtls_x509_crt *p;
-    unsigned char *pubkey;
+    mbedtls_x509_crt *p = NULL;
+    unsigned char *pubkey = NULL;
 
 #if MBEDTLS_VERSION_NUMBER >= 0x03000000
     if(!peercert || !peercert->MBEDTLS_PRIVATE(raw).MBEDTLS_PRIVATE(p) ||
        !peercert->MBEDTLS_PRIVATE(raw).MBEDTLS_PRIVATE(len)) {
 #else
@@ -680,19 +680,17 @@ mbed_connect_step2(struct Curl_easy *data, struct connectdata *conn,
       failf(data, "Failed due to missing peer certificate");
       return CURLE_SSL_PINNEDPUBKEYNOTMATCH;
     }
 
     p = calloc(1, sizeof(*p));
-
     if(!p)
       return CURLE_OUT_OF_MEMORY;
 
     pubkey = malloc(PUB_DER_MAX_BYTES);
-
     if(!pubkey) {
-      free(p);
-      return CURLE_OUT_OF_MEMORY;
+      result = CURLE_OUT_OF_MEMORY;
+      goto error;
     }
 
     mbedtls_x509_crt_init(p);
 
     /* Make a copy of our const peercert because mbedtls_pk_write_pubkey_der
@@ -704,14 +702,12 @@ mbed_connect_step2(struct Curl_easy *data, struct connectdata *conn,
                         peercert->MBEDTLS_PRIVATE(raw).MBEDTLS_PRIVATE(len))) {
 #else
     if(mbedtls_x509_crt_parse_der(p, peercert->raw.p, peercert->raw.len)) {
 #endif
       failf(data, "Failed copying peer certificate");
-      mbedtls_x509_crt_free(p);
-      free(p);
-      free(pubkey);
-      return CURLE_SSL_PINNEDPUBKEYNOTMATCH;
+      result = CURLE_SSL_PINNEDPUBKEYNOTMATCH;
+      goto error;
     }
 
 #if MBEDTLS_VERSION_NUMBER >= 0x03000000
     size = mbedtls_pk_write_pubkey_der(&p->MBEDTLS_PRIVATE(pk), pubkey,
                                        PUB_DER_MAX_BYTES);
@@ -719,20 +715,19 @@ mbed_connect_step2(struct Curl_easy *data, struct connectdata *:
conn,
     size = mbedtls_pk_write_pubkey_der(&p->pk, pubkey, PUB_DER_MAX_BYTES);
 #endif
 
     if(size <= 0) {
       failf(data, "Failed copying public key from peer certificate");
-      mbedtls_x509_crt_free(p);
-      free(p);
-      free(pubkey);
-      return CURLE_SSL_PINNEDPUBKEYNOTMATCH;
+      result = CURLE_SSL_PINNEDPUBKEYNOTMATCH;
+      goto error;
     }
 
     /* mbedtls_pk_write_pubkey_der writes data at the end of the buffer. */
     result = Curl_pin_peer_pubkey(data,
                                   pinnedpubkey,
                                   &pubkey[PUB_DER_MAX_BYTES - size], size);
+      error:
     if(result) {
       mbedtls_x509_crt_free(p);
       free(p);
       free(pubkey);
       return result;

Code that does not repeat looks much better. I will modify and update the PR.

Use dynamic memory allocation for the buffer used in checking "pinned public key". The PUB_DER_MAX_BYTES parameter with default settings is set to a value greater than 2kB. (co-authored by Daniel Stenberg)

bagder · 2021-09-06T08:02:55Z

Thanks!

danielgustafsson reviewed Aug 17, 2021

View reviewed changes

mbedtls: Avoid using a large buffer on the stack.

6306611

Use dynamic memory allocation for the buffer used in checking "pinned public key". The PUB_DER_MAX_BYTES parameter with default settings is set to a value greater than 2kB. (co-authored by Daniel Stenberg)

MAntoniak force-pushed the master branch from 23681b4 to 6306611 Compare August 19, 2021 16:29

jay added the TLS label Aug 21, 2021

bagder approved these changes Sep 6, 2021

View reviewed changes

bagder closed this in 37fb213 Sep 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

mbedtls: Avoid using a large buffer on the stack.#7586

mbedtls: Avoid using a large buffer on the stack.#7586
MAntoniak wants to merge 1 commit intocurl:masterfrom
MAntoniak:master

MAntoniak commented Aug 17, 2021

Uh oh!

danielgustafsson Aug 17, 2021

Uh oh!

MAntoniak Aug 18, 2021

Uh oh!

danielgustafsson Aug 18, 2021

Uh oh!

bagder commented Aug 18, 2021

Uh oh!

MAntoniak commented Aug 18, 2021

Uh oh!

bagder commented Sep 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

MAntoniak commented Aug 17, 2021

Uh oh!

danielgustafsson Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

MAntoniak Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

danielgustafsson Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

bagder commented Aug 18, 2021

Uh oh!

MAntoniak commented Aug 18, 2021

Uh oh!

bagder commented Sep 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants