mbedtls: Fix pinned key return value on fail

- Switch from verifying a pinned public key in a callback during the
certificate verification to inline after the certificate verification.

The callback method had three problems:

1. If a pinned public key didn't match, CURLE_SSL_PINNEDPUBKEYNOTMATCH
was not returned.

2. If peer certificate verification was disabled the pinned key
verification did not take place as it should.

3. (related to #2) If there was no certificate of depth 0 the callback
would not have checked the pinned public key.

Though all those problems could have been fixed it would have made the
code more complex. Instead we now verify inline after the certificate
verification in mbedtls_connect_step2.

Ref: http://curl.haxx.se/mail/lib-2016-01/0047.html
Ref: https://github.com/bagder/curl/pull/601

1 files changed, 66 insertions, 49 deletions

diff --git a/lib/vtls/mbedtls.c b/lib/vtls/mbedtls.c
index 05af46212..26959e041 100644
--- a/lib/vtls/mbedtls.c
+++ b/lib/vtls/mbedtls.c
@@ -148,45 +148,7 @@ const mbedtls_x509_crt_profile mbedtls_x509_crt_profile_fr =
 #define ECP_PUB_DER_MAX_BYTES   (30 + 2 * MBEDTLS_ECP_MAX_BYTES)
 
 #define PUB_DER_MAX_BYTES   (RSA_PUB_DER_MAX_BYTES > ECP_PUB_DER_MAX_BYTES ? \
-                            RSA_PUB_DER_MAX_BYTES : ECP_PUB_DER_MAX_BYTES)
-
-static int
-mbedtls_verify_pinned_crt(void *p, mbedtls_x509_crt *crt,
-                          int depth, unsigned int *flags)
-{
-  struct SessionHandle *data = p;
-  unsigned char pubkey[PUB_DER_MAX_BYTES];
-  int ret;
-  int size;
-  char *pinned_cert = data->set.str[STRING_SSL_PINNEDPUBLICKEY];
-
-  /* Skip intermediate and root certificates */
-  if(depth) {
-    return 0;
-  }
-
-  if(pinned_cert == NULL || crt == NULL) {
-    *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED;
-    return 1;
-  }
-
-  /* Extract pubkey */
-  size = mbedtls_pk_write_pubkey_der(&crt->pk, pubkey, PUB_DER_MAX_BYTES);
-  if(size <= 0) {
-    *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED;
-    return 1;
-  }
-
-  /* mbedtls_pk_write_pubkey_der writes data at the end of the buffer. */
-  ret = Curl_pin_peer_pubkey(data, pinned_cert,
-                             &pubkey[PUB_DER_MAX_BYTES - size], size);
-  if(ret == CURLE_OK) {
-    return 0;
-  }
-
-  *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED;
-  return 1;
-}
+                             RSA_PUB_DER_MAX_BYTES : ECP_PUB_DER_MAX_BYTES)
 
 static Curl_recv mbedtls_recv;
 static Curl_send mbedtls_send;
@@ -455,7 +417,7 @@ mbedtls_connect_step2(struct connectdata *conn,
   int ret;
   struct SessionHandle *data = conn->data;
   struct ssl_connect_data* connssl = &conn->ssl[sockindex];
-  char buffer[1024];
+  const mbedtls_x509_crt *peercert;
 
 #ifdef HAS_ALPN
   const char* next_protocol;
@@ -510,13 +472,72 @@ mbedtls_connect_step2(struct connectdata *conn,
     return CURLE_PEER_FAILED_VERIFICATION;
   }
 
-  if(mbedtls_ssl_get_peer_cert(&(connssl->ssl))) {
-    /* If the session was resumed, there will be no peer certs */
-    memset(buffer, 0, sizeof(buffer));
+  peercert = mbedtls_ssl_get_peer_cert(&connssl->ssl);
 
-    if(mbedtls_x509_crt_info(buffer, sizeof(buffer), (char *)"* ",
-                     mbedtls_ssl_get_peer_cert(&(connssl->ssl))) != -1)
+  if(peercert && data->set.verbose) {
+    const size_t bufsize = 16384;
+    char *buffer = malloc(bufsize);
+
+    if(!buffer)
+      return CURLE_OUT_OF_MEMORY;
+
+    if(mbedtls_x509_crt_info(buffer, bufsize, "* ", peercert) > 0)
       infof(data, "Dumping cert info:\n%s\n", buffer);
+    else
+      infof(data, "Unable to dump certificate information.\n");
+
+    free(buffer);
+  }
+
+  if(data->set.str[STRING_SSL_PINNEDPUBLICKEY]) {
+    int size;
+    CURLcode result;
+    mbedtls_x509_crt *p;
+    unsigned char pubkey[PUB_DER_MAX_BYTES];
+
+    if(!peercert || !peercert->raw.p || !peercert->raw.len) {
+      failf(data, "Failed due to missing peer certificate");
+      return CURLE_SSL_PINNEDPUBKEYNOTMATCH;
+    }
+
+    p = calloc(1, sizeof(*p));
+
+    if(!p)
+      return CURLE_OUT_OF_MEMORY;
+
+    mbedtls_x509_crt_init(p);
+
+    /* Make a copy of our const peercert because mbedtls_pk_write_pubkey_der
+       needs a non-const key, for now.
+       https://github.com/ARMmbed/mbedtls/issues/396 */
+    if(mbedtls_x509_crt_parse_der(p, peercert->raw.p, peercert->raw.len)) {
+      failf(data, "Failed copying peer certificate");
+      mbedtls_x509_crt_free(p);
+      free(p);
+      return CURLE_SSL_PINNEDPUBKEYNOTMATCH;
+    }
+
+    size = mbedtls_pk_write_pubkey_der(&p->pk, pubkey, PUB_DER_MAX_BYTES);
+
+    if(size <= 0) {
+      failf(data, "Failed copying public key from peer certificate");
+      mbedtls_x509_crt_free(p);
+      free(p);
+      return CURLE_SSL_PINNEDPUBKEYNOTMATCH;
+    }
+
+    /* mbedtls_pk_write_pubkey_der writes data at the end of the buffer. */
+    result = Curl_pin_peer_pubkey(data,
+                                  data->set.str[STRING_SSL_PINNEDPUBLICKEY],
+                                  &pubkey[PUB_DER_MAX_BYTES - size], size);
+    if(result) {
+      mbedtls_x509_crt_free(p);
+      free(p);
+      return result;
+    }
+
+    mbedtls_x509_crt_free(p);
+    free(p);
   }
 
 #ifdef HAS_ALPN
@@ -683,10 +704,6 @@ mbedtls_connect_common(struct connectdata *conn,
   long timeout_ms;
   int what;
 
-  if(data->set.str[STRING_SSL_PINNEDPUBLICKEY]) {
-    mbedtls_ssl_conf_verify(&connssl->config, mbedtls_verify_pinned_crt, data);
-  }
-
   /* check if the connection has already been established */
   if(ssl_connection_complete == connssl->state) {
     *done = TRUE;