Clean up |ECDH_compute_key|.
1. Check for the presence of the private key before allocating or
computing anything.
2. Check the return value of |BN_CTX_get|.
3. Don't bother computing the Y coordinate since it is not used.
4. Remove conditional logic in cleanup section.
Change-Id: I4d8611603363c7e5d16a8e9f1d6c3a56809f27ae
Reviewed-on: https://boringssl-review.googlesource.com/6171
Reviewed-by: Adam Langley <alangley@gmail.com>
diff --git a/crypto/ecdh/ecdh.c b/crypto/ecdh/ecdh.c
index 14856db..4a1964a 100644
--- a/crypto/ecdh/ecdh.c
+++ b/crypto/ecdh/ecdh.c
@@ -75,33 +75,27 @@
int ECDH_compute_key(void *out, size_t outlen, const EC_POINT *pub_key,
- EC_KEY *priv_key, void *(*KDF)(const void *in, size_t inlen,
- void *out, size_t *outlen)) {
- BN_CTX *ctx;
- EC_POINT *tmp = NULL;
- BIGNUM *x = NULL, *y = NULL;
- const BIGNUM *priv;
- const EC_GROUP *group;
- int ret = -1;
- size_t buflen;
- uint8_t *buf = NULL;
-
- if ((ctx = BN_CTX_new()) == NULL) {
- goto err;
- }
- BN_CTX_start(ctx);
- x = BN_CTX_get(ctx);
- y = BN_CTX_get(ctx);
-
- priv = EC_KEY_get0_private_key(priv_key);
+ EC_KEY *priv_key,
+ void *(*kdf)(const void *in, size_t inlen, void *out,
+ size_t *outlen)) {
+ const BIGNUM *const priv = EC_KEY_get0_private_key(priv_key);
if (priv == NULL) {
OPENSSL_PUT_ERROR(ECDH, ECDH_R_NO_PRIVATE_VALUE);
- goto err;
+ return -1;
}
- group = EC_KEY_get0_group(priv_key);
+ BN_CTX *ctx = BN_CTX_new();
+ if (ctx == NULL) {
+ return -1;
+ }
+ BN_CTX_start(ctx);
- tmp = EC_POINT_new(group);
+ int ret = -1;
+ size_t buflen = 0;
+ uint8_t *buf = NULL;
+
+ const EC_GROUP *const group = EC_KEY_get0_group(priv_key);
+ EC_POINT *tmp = EC_POINT_new(group);
if (tmp == NULL) {
OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE);
goto err;
@@ -112,7 +106,13 @@
goto err;
}
- if (!EC_POINT_get_affine_coordinates_GFp(group, tmp, x, y, ctx)) {
+ BIGNUM *x = BN_CTX_get(ctx);
+ if (!x) {
+ OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE);
+ goto err;
+ }
+
+ if (!EC_POINT_get_affine_coordinates_GFp(group, tmp, x, NULL, ctx)) {
OPENSSL_PUT_ERROR(ECDH, ECDH_R_POINT_ARITHMETIC_FAILURE);
goto err;
}
@@ -129,33 +129,25 @@
goto err;
}
- if (KDF != 0) {
- if (KDF(buf, buflen, out, &outlen) == NULL) {
+ if (kdf != NULL) {
+ if (kdf(buf, buflen, out, &outlen) == NULL) {
OPENSSL_PUT_ERROR(ECDH, ECDH_R_KDF_FAILED);
goto err;
}
- ret = outlen;
} else {
/* no KDF, just copy as much as we can */
- if (outlen > buflen) {
+ if (buflen < outlen) {
outlen = buflen;
}
memcpy(out, buf, outlen);
- ret = outlen;
}
+ ret = outlen;
+
err:
- if (tmp) {
- EC_POINT_free(tmp);
- }
- if (ctx) {
- BN_CTX_end(ctx);
- }
- if (ctx) {
- BN_CTX_free(ctx);
- }
- if (buf) {
- OPENSSL_free(buf);
- }
+ OPENSSL_free(buf);
+ EC_POINT_free(tmp);
+ BN_CTX_end(ctx);
+ BN_CTX_free(ctx);
return ret;
}
diff --git a/include/openssl/ecdh.h b/include/openssl/ecdh.h
index 878cbeb..5fe3ae9 100644
--- a/include/openssl/ecdh.h
+++ b/include/openssl/ecdh.h
@@ -80,14 +80,14 @@
/* ECDH_compute_key calculates the shared key between |pub_key| and |priv_key|.
- * If |KDF| is not NULL, then it is called with the bytes of the shared key and
- * the parameter |out|. When |KDF| returns, the value of |*outlen| becomes the
+ * If |kdf| is not NULL, then it is called with the bytes of the shared key and
+ * the parameter |out|. When |kdf| returns, the value of |*outlen| becomes the
* return value. Otherwise, as many bytes of the shared key as will fit are
* copied directly to, at most, |outlen| bytes at |out|. It returns the number
* of bytes written to |out|, or -1 on error. */
OPENSSL_EXPORT int ECDH_compute_key(void *out, size_t outlen,
const EC_POINT *pub_key, EC_KEY *priv_key,
- void *(*KDF)(const void *in, size_t inlen,
+ void *(*kdf)(const void *in, size_t inlen,
void *out, size_t *outlen));
