Commit 21bdf9c9 authored by Adam Langley's avatar Adam Langley Committed by Commit Bot

device/fido: check ECDH_compute_key return value correctly.

Due to its OpenSSL-based origins, ECDH_compute_key returns the output
length or -1 for error, unlike standard BoringSSL functions which are
0/1. This CL fixes the pattern of checking the return value.

(Doesn't matter in practice because it doesn't fail unless the private
key is on the wrong group.)

Change-Id: I87ceb4afa419ae578eed9cbf38edebe7e061570a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2159749
Commit-Queue: Adam Langley <agl@chromium.org>
Auto-Submit: Adam Langley <agl@chromium.org>
Reviewed-by: default avatarMartin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#761162}
parent b5a72eb7
......@@ -219,9 +219,10 @@ std::vector<uint8_t> HandshakeInitiator::BuildInitialMessage() {
uint8_t es_key[32];
CHECK(EC_POINT_oct2point(group, peer_identity_point.get(),
peer_identity_->data(), peer_identity_->size(),
/*ctx=*/nullptr) &&
ECDH_compute_key(es_key, sizeof(es_key), peer_identity_point.get(),
ephemeral_key_.get(), /*kdf=*/nullptr));
/*ctx=*/nullptr));
CHECK(ECDH_compute_key(es_key, sizeof(es_key), peer_identity_point.get(),
ephemeral_key_.get(),
/*kdf=*/nullptr) == sizeof(es_key));
noise_.MixKey(es_key);
}
......@@ -256,8 +257,9 @@ HandshakeInitiator::ProcessResponse(base::span<const uint8_t> response) {
const EC_GROUP* group = EC_KEY_get0_group(ephemeral_key_.get());
if (!EC_POINT_oct2point(group, peer_point.get(), peer_point_bytes.data(),
peer_point_bytes.size(), /*ctx=*/nullptr) ||
!ECDH_compute_key(shared_key_ee, sizeof(shared_key_ee), peer_point.get(),
ephemeral_key_.get(), /*kdf=*/nullptr)) {
ECDH_compute_key(shared_key_ee, sizeof(shared_key_ee), peer_point.get(),
ephemeral_key_.get(),
/*kdf=*/nullptr) != sizeof(shared_key_ee)) {
FIDO_LOG(DEBUG) << "Peer's P-256 point not on curve.";
return base::nullopt;
}
......@@ -270,9 +272,9 @@ HandshakeInitiator::ProcessResponse(base::span<const uint8_t> response) {
uint8_t shared_key_se[32];
bssl::UniquePtr<EC_KEY> identity_key(EC_KEY_derive_from_secret(
group, local_seed_->data(), local_seed_->size()));
if (!ECDH_compute_key(shared_key_se, sizeof(shared_key_se),
peer_point.get(), identity_key.get(),
/*kdf=*/nullptr)) {
if (ECDH_compute_key(shared_key_se, sizeof(shared_key_se), peer_point.get(),
identity_key.get(),
/*kdf=*/nullptr) != sizeof(shared_key_se)) {
return base::nullopt;
}
noise_.MixKey(shared_key_se);
......@@ -396,8 +398,8 @@ base::Optional<std::unique_ptr<Crypter>> RespondToHandshake(
if (identity) {
uint8_t es_key[32];
if (!ECDH_compute_key(es_key, sizeof(es_key), peer_point.get(), identity,
/*kdf=*/nullptr)) {
if (ECDH_compute_key(es_key, sizeof(es_key), peer_point.get(), identity,
/*kdf=*/nullptr) != sizeof(es_key)) {
return base::nullopt;
}
noise.MixKey(es_key);
......@@ -419,16 +421,18 @@ base::Optional<std::unique_ptr<Crypter>> RespondToHandshake(
noise.MixKey(ephemeral_key_public_bytes);
uint8_t shared_key_ee[32];
if (!ECDH_compute_key(shared_key_ee, sizeof(shared_key_ee), peer_point.get(),
ephemeral_key.get(), /*kdf=*/nullptr)) {
if (ECDH_compute_key(shared_key_ee, sizeof(shared_key_ee), peer_point.get(),
ephemeral_key.get(),
/*kdf=*/nullptr) != sizeof(shared_key_ee)) {
return base::nullopt;
}
noise.MixKey(shared_key_ee);
if (peer_identity) {
uint8_t shared_key_se[32];
if (!ECDH_compute_key(shared_key_se, sizeof(shared_key_se), peer_identity,
ephemeral_key.get(), /*kdf=*/nullptr)) {
if (ECDH_compute_key(shared_key_se, sizeof(shared_key_se), peer_identity,
ephemeral_key.get(),
/*kdf=*/nullptr) != sizeof(shared_key_se)) {
return base::nullopt;
}
noise.MixKey(shared_key_se);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment