commit dc2554dc76a94e599fa8f2b2d5bb9aae4982411b
author Max Bires <jbires@google.com> Sun Jun 27 19:32:40 2021 -0700
committer Max Bires <jbires@google.com> Mon Jun 28 01:44:47 2021 -0700
tree 99ceffbe247ae7ae017fb8a07a85fa2e9262f09b
parent a7b61e5ff3cced698dc02e7d0b7962c1b7e356d5

Fixing passing pointer to uninitialized object.

There was a bug in the code for extracting the raw EC public key x and y
coordinates from the certificate retrieved from keymaster. Two boringssl
BIGNUM objects were declared on the stack with pointers passed in
directly to a function without the objects first being initialized. This
went undetected in Cuttlefish, but predictably caused KM on Trusty to
attempt to access a memory page it didn't own.

This patch also fixes a pair of memory leaks and cleans up the memory
management.

Bug: 192199094
Test: atest VtsHalRemotelyProvisionedComponentTargetTests
Change-Id: Ic513dc9fc2146ef0c131f04088f1d25c9963bc2a
Merged-In: Ic513dc9fc2146ef0c131f04088f1d25c9963bc2a

km_openssl/openssl_utils.cpp

diff --git a/km_openssl/openssl_utils.cpp b/km_openssl/openssl_utils.cpp
index b27c434..a0c0300 100644
--- a/km_openssl/openssl_utils.cpp
+++ b/km_openssl/openssl_utils.cpp
@@ -129,31 +129,30 @@
         return KM_ERROR_INVALID_ARGUMENT;
     }
     const uint8_t* temp = km_cert->data;
-    X509* cert = d2i_X509(NULL, &temp, km_cert->data_length);
-    if (cert == nullptr) return TranslateLastOpenSslError();
-    EVP_PKEY* pubKey = X509_get_pubkey(cert);
-    if (pubKey == nullptr) return TranslateLastOpenSslError();
-    EC_KEY* ecKey = EVP_PKEY_get0_EC_KEY(pubKey);
-    if (ecKey == nullptr) return TranslateLastOpenSslError();
+    X509_Ptr cert(d2i_X509(NULL, &temp, km_cert->data_length));
+    if (!cert.get()) return TranslateLastOpenSslError();
+    EVP_PKEY_Ptr pubKey(X509_get_pubkey(cert.get()));
+    if (!pubKey.get()) return TranslateLastOpenSslError();
+    EC_KEY* ecKey = EVP_PKEY_get0_EC_KEY(pubKey.get());
+    if (!ecKey) return TranslateLastOpenSslError();
     const EC_POINT* jacobian_coords = EC_KEY_get0_public_key(ecKey);
-    if (jacobian_coords == nullptr) return TranslateLastOpenSslError();
-    BIGNUM x;
-    BIGNUM y;
-    BN_CTX* ctx = BN_CTX_new();
-    if (ctx == nullptr) return TranslateLastOpenSslError();
-    if (!EC_POINT_get_affine_coordinates_GFp(EC_KEY_get0_group(ecKey), jacobian_coords, &x, &y,
-                                             ctx)) {
+    if (!jacobian_coords) return TranslateLastOpenSslError();
+    bssl::UniquePtr<BIGNUM> x(BN_new());
+    bssl::UniquePtr<BIGNUM> y(BN_new());
+    BN_CTX_Ptr ctx(BN_CTX_new());
+    if (!ctx.get()) return TranslateLastOpenSslError();
+    if (!EC_POINT_get_affine_coordinates_GFp(EC_KEY_get0_group(ecKey), jacobian_coords, x.get(),
+                                             y.get(), ctx.get())) {
         return TranslateLastOpenSslError();
     }
     uint8_t* tmp_x = x_coord;
-    if (BN_bn2binpad(&x, tmp_x, kAffinePointLength) != kAffinePointLength) {
+    if (BN_bn2binpad(x.get(), tmp_x, kAffinePointLength) != kAffinePointLength) {
         return TranslateLastOpenSslError();
     }
     uint8_t* tmp_y = y_coord;
-    if (BN_bn2binpad(&y, tmp_y, kAffinePointLength) != kAffinePointLength) {
+    if (BN_bn2binpad(y.get(), tmp_y, kAffinePointLength) != kAffinePointLength) {
         return TranslateLastOpenSslError();
     }
-    BN_CTX_free(ctx);
     return KM_ERROR_OK;
 }