From d58ba66eeceb5a290ecd50f596606a7f77d68b4b Mon Sep 17 00:00:00 2001 From: Jay Satiro Date: Mon, 18 Jan 2016 03:48:10 -0500 Subject: [PATCH] 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 --- lib/vtls/mbedtls.c | 115 ++++++++++++++++++++++++++------------------- 1 file 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; -- 2.40.0