commit 418f3d6298beb27e050618e2f59e01d6d3b2f45b Author: Nick Mathewson <nickm@torproject.org> Date: Wed Aug 9 09:24:16 2017 -0400 Make sure we always wind up checking i2d_*'s output. The biggest offender here was sometimes not checking the output of crypto_pk_get_digest. Fixes bug 19418. Reported by Guido Vranken. --- changes/bug19418 | 7 +++++++ src/common/tortls.c | 21 ++++++++++++++------- src/or/connection_or.c | 4 +++- src/or/hibernate.c | 5 ++++- src/or/rendclient.c | 5 +++++ src/or/rendcommon.c | 5 ++++- src/or/rendservice.c | 9 ++++++++- src/or/router.c | 14 +++++++++++--- src/or/routerkeys.c | 4 +++- src/or/routerparse.c | 5 ++++- 10 files changed, 63 insertions(+), 16 deletions(-) diff --git a/changes/bug19418 b/changes/bug19418 new file mode 100644 index 000000000..fb5f6ad5d --- /dev/null +++ b/changes/bug19418 @@ -0,0 +1,7 @@ + o Minor bugfixes (robustness, error handling): + - Improve our handling of the cases where OpenSSL encounters a + memory error while encoding keys and certificates. We haven't + observed these happening in the wild, but if they do happen, + we now detect and respond better. Fixes bug 19418; bugfix + on all versions of Tor. Reported by Guido Vranken. + diff --git a/src/common/tortls.c b/src/common/tortls.c index d61cc2e58..bd41e0b7f 100644 --- a/src/common/tortls.c +++ b/src/common/tortls.c @@ -648,12 +648,7 @@ MOCK_IMPL(STATIC tor_x509_cert_t *, length = i2d_X509(x509_cert, &buf); cert = tor_malloc_zero(sizeof(tor_x509_cert_t)); if (length <= 0 || buf == NULL) { - /* LCOV_EXCL_START for the same reason as the exclusion above */ - tor_free(cert); - log_err(LD_CRYPTO, "Couldn't get length of encoded x509 certificate"); - X509_free(x509_cert); - return NULL; - /* LCOV_EXCL_STOP */ + goto err; } cert->encoded_len = (size_t) length; cert->encoded = tor_malloc(length); @@ -668,13 +663,25 @@ MOCK_IMPL(STATIC tor_x509_cert_t *, if ((pkey = X509_get_pubkey(x509_cert)) && (rsa = EVP_PKEY_get1_RSA(pkey))) { crypto_pk_t *pk = crypto_new_pk_from_rsa_(rsa); - crypto_pk_get_common_digests(pk, &cert->pkey_digests); + if (crypto_pk_get_common_digests(pk, &cert->pkey_digests) < 0) { + crypto_pk_free(pk); + EVP_PKEY_free(pkey); + goto err; + } + cert->pkey_digests_set = 1; crypto_pk_free(pk); EVP_PKEY_free(pkey); } return cert; + err: + /* LCOV_EXCL_START for the same reason as the exclusion above */ + tor_free(cert); + log_err(LD_CRYPTO, "Couldn't wrap encoded X509 certificate."); + X509_free(x509_cert); + return NULL; + /* LCOV_EXCL_STOP */ } /** Return a new copy of <b>cert</b>. */ diff --git a/src/or/connection_or.c b/src/or/connection_or.c index dadfdc438..49d48d72e 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -1552,7 +1552,9 @@ connection_or_check_valid_tls_handshake(or_connection_t *conn, } if (identity_rcvd) { - crypto_pk_get_digest(identity_rcvd, digest_rcvd_out); + if (crypto_pk_get_digest(identity_rcvd, digest_rcvd_out) < 0) { + return -1; + } } else { memset(digest_rcvd_out, 0, DIGEST_LEN); } diff --git a/src/or/hibernate.c b/src/or/hibernate.c index e3c80b5f1..82196ef2c 100644 --- a/src/or/hibernate.c +++ b/src/or/hibernate.c @@ -587,7 +587,10 @@ accounting_set_wakeup_time(void) char buf[ISO_TIME_LEN+1]; format_iso_time(buf, interval_start_time); - crypto_pk_get_digest(get_server_identity_key(), digest); + if (crypto_pk_get_digest(get_server_identity_key(), digest) < 0) { + log_err(LD_BUG, "Error getting our key's digest."); + tor_assert(0); + } d_env = crypto_digest_new(); crypto_digest_add_bytes(d_env, buf, ISO_TIME_LEN); diff --git a/src/or/rendclient.c b/src/or/rendclient.c index a93bc94a9..e54818c54 100644 --- a/src/or/rendclient.c +++ b/src/or/rendclient.c @@ -263,6 +263,11 @@ rend_client_send_introduction(origin_circuit_t *introcirc, klen = crypto_pk_asn1_encode(extend_info->onion_key, tmp+v3_shift+7+DIGEST_LEN+2, sizeof(tmp)-(v3_shift+7+DIGEST_LEN+2)); + if (klen < 0) { + log_warn(LD_BUG,"Internal error: can't encode public key."); + status = -2; + goto perm_err; + } set_uint16(tmp+v3_shift+7+DIGEST_LEN, htons(klen)); memcpy(tmp+v3_shift+7+DIGEST_LEN+2+klen, rendcirc->rend_data->rend_cookie, REND_COOKIE_LEN); diff --git a/src/or/rendcommon.c b/src/or/rendcommon.c index d9d39b1f1..c01ce8f1d 100644 --- a/src/or/rendcommon.c +++ b/src/or/rendcommon.c @@ -475,7 +475,10 @@ rend_encode_v2_descriptors(smartlist_t *descs_out, tor_assert(descriptor_cookie); } /* Obtain service_id from public key. */ - crypto_pk_get_digest(service_key, service_id); + if (crypto_pk_get_digest(service_key, service_id) < 0) { + log_warn(LD_BUG, "Couldn't compute service key digest."); + return -1; + } /* Calculate current time-period. */ time_period = get_time_period(now, period, service_id); /* Determine how many seconds the descriptor will be valid. */ diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 4d04da02a..d4441b63c 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -2689,7 +2689,14 @@ rend_service_decrypt_intro( /* Check that this cell actually matches this service key */ /* first DIGEST_LEN bytes of request is intro or service pk digest */ - crypto_pk_get_digest(key, (char *)key_digest); + if (crypto_pk_get_digest(key, (char *)key_digest) < 0) { + if (err_msg_out) + *err_msg_out = tor_strdup("Couldn't compute RSA digest."); + log_warn(LD_BUG, "Couldn't compute key digest."); + status = -7; + goto err; + } + if (tor_memneq(key_digest, intro->pk, DIGEST_LEN)) { if (err_msg_out) { base32_encode(service_id, REND_SERVICE_ID_LEN_BASE32 + 1, diff --git a/src/or/router.c b/src/or/router.c index 6d3a32a60..f7528c22d 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -212,7 +212,11 @@ set_server_identity_key(crypto_pk_t *k) { crypto_pk_free(server_identitykey); server_identitykey = k; - crypto_pk_get_digest(server_identitykey, server_identitykey_digest); + if (crypto_pk_get_digest(server_identitykey, + server_identitykey_digest) < 0) { + log_err(LD_BUG, "Couldn't compute our own identity key digest."); + tor_assert(0); + } } /** Make sure that we have set up our identity keys to match or not match as @@ -871,8 +875,12 @@ init_keys(void) } cert = get_my_v3_authority_cert(); if (cert) { - crypto_pk_get_digest(get_my_v3_authority_cert()->identity_key, - v3_digest); + if (crypto_pk_get_digest(get_my_v3_authority_cert()->identity_key, + v3_digest) < 0) { + log_err(LD_BUG, "Couldn't compute my v3 authority identity key " + "digest."); + return -1; + } v3_digest_set = 1; } } diff --git a/src/or/routerkeys.c b/src/or/routerkeys.c index ca32228fc..5a41d6261 100644 --- a/src/or/routerkeys.c +++ b/src/or/routerkeys.c @@ -1079,7 +1079,9 @@ make_tap_onion_key_crosscert(const crypto_pk_t *onion_key, uint8_t signed_data[DIGEST_LEN + ED25519_PUBKEY_LEN]; *len_out = 0; - crypto_pk_get_digest(rsa_id_key, (char*)signed_data); + if (crypto_pk_get_digest(rsa_id_key, (char*)signed_data) < 0) { + return NULL; + } memcpy(signed_data + DIGEST_LEN, master_id_key->pubkey, ED25519_PUBKEY_LEN); int r = crypto_pk_private_sign(onion_key, diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 521e237be..bfd027043 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -5967,7 +5967,10 @@ rend_parse_v2_service_descriptor(rend_service_descriptor_t **parsed_out, "v2 rendezvous service descriptor") < 0) goto err; /* Verify that descriptor ID belongs to public key and secret ID part. */ - crypto_pk_get_digest(result->pk, public_key_hash); + if (crypto_pk_get_digest(result->pk, public_key_hash) < 0) { + log_warn(LD_REND, "Unable to compute rend descriptor public key digest"); + goto err; + } rend_get_descriptor_id_bytes(test_desc_id, public_key_hash, secret_id_part); if (tor_memneq(desc_id_out, test_desc_id, DIGEST_LEN)) {