diff mbox

OpenSSL: Fix keystore-backed keys

Message ID CAH7ZN-xk3LE6oQntn8dmnVuGbEnF9w6hJ+sJnzU4YjwM5=KoZg@mail.gmail.com
State Accepted
Headers show

Commit Message

Dmitry Shmidt Oct. 6, 2015, 6:11 p.m. UTC 
The switch to BoringSSL broke keystore-backed keys because
wpa_supplicant was using the dynamic ENGINE loading to load
the keystore module.
The ENGINE-like functionality in BoringSSL is much simpler
and this change should enable it.

Signed-off-by: Dmitry Shmidt <dimitrysh@google.com>
---
 src/crypto/tls_openssl.c  | 29 ++++++++++++++++++++++++++---
 wpa_supplicant/Android.mk |  7 +++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

Comments

Jouni Malinen Oct. 6, 2015, 8:15 p.m. UTC | #1 
On Tue, Oct 06, 2015 at 11:11:11AM -0700, Dmitry Shmidt wrote:
> The switch to BoringSSL broke keystore-backed keys because
> wpa_supplicant was using the dynamic ENGINE loading to load
> the keystore module.
> The ENGINE-like functionality in BoringSSL is much simpler
> and this change should enable it.

Thanks, applied with some cleanup and a small fix:

>  static int tls_engine_init(struct tls_connection *conn, const char *engine_id,

> + conn->engine = NULL;
> + conn->private_key = EVP_PKEY_from_keystore(key_id);

tls_engine_init() can be called with key_id == NULL depending on
configuration, so I added a check for that to avoid a NULL pointer
dereference within BoringSSL. I'd assume this does not happen with the
configuration used on Android, but anyway, better have this more robust
should the configuration ever change.
diff mbox

Patch

diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c
index 69f51a8..4183741 100644
--- a/src/crypto/tls_openssl.c
+++ b/src/crypto/tls_openssl.c
@@ -64,6 +64,7 @@  static BIO * BIO_from_keystore(const char *key)
  free(value);
  return bio;
 }
+
 #endif /* ANDROID */

 static int tls_openssl_ref_count = 0;
@@ -84,7 +85,7 @@  struct tls_connection {
  SSL_CTX *ssl_ctx;
  SSL *ssl;
  BIO *ssl_in, *ssl_out;
-#ifndef OPENSSL_NO_ENGINE
+#if defined(ANDROID) || !defined(OPENSSL_NO_ENGINE)
  ENGINE *engine;        /* functional reference to the engine */
  EVP_PKEY *private_key; /* the private key if using engine */
 #endif /* OPENSSL_NO_ENGINE */
@@ -890,11 +891,31 @@  static int tls_is_pin_error(unsigned int err)

 #endif /* OPENSSL_NO_ENGINE */

+#ifdef ANDROID
+/* EVP_PKEY_from_keystore comes from system/security/keystore-engine. */
+EVP_PKEY* EVP_PKEY_from_keystore(const char* key_id);
+#endif

 static int tls_engine_init(struct tls_connection *conn, const char *engine_id,
    const char *pin, const char *key_id,
    const char *cert_id, const char *ca_cert_id)
 {
+#if defined(ANDROID) && defined(OPENSSL_IS_BORINGSSL)
+#if !defined(OPENSSL_NO_ENGINE)
+#error "This code depends on OPENSSL_NO_ENGINE being defined by BoringSSL."
+#endif
+
+ conn->engine = NULL;
+ conn->private_key = EVP_PKEY_from_keystore(key_id);
+ if (!conn->private_key) {
+ wpa_printf(MSG_ERROR,
+   "ENGINE: cannot load private key with id '%s' [%s]",
+   key_id,
+   ERR_error_string(ERR_get_error(), NULL));
+ return TLS_SET_PARAMS_ENGINE_PRV_INIT_FAILED;
+ }
+#endif
+
 #ifndef OPENSSL_NO_ENGINE
  int ret = -1;
  if (engine_id == NULL) {
@@ -992,14 +1013,16 @@  err:

 static void tls_engine_deinit(struct tls_connection *conn)
 {
-#ifndef OPENSSL_NO_ENGINE
+#if defined(ANDROID) || !defined(OPENSSL_NO_ENGINE)
  wpa_printf(MSG_DEBUG, "ENGINE: engine deinit");
  if (conn->private_key) {
  EVP_PKEY_free(conn->private_key);
  conn->private_key = NULL;
  }
  if (conn->engine) {
+#if !defined(OPENSSL_IS_BORINGSSL)
  ENGINE_finish(conn->engine);
+#endif
  conn->engine = NULL;
  }
 #endif /* OPENSSL_NO_ENGINE */
@@ -2289,7 +2312,7 @@  static int tls_connection_engine_ca_cert(void *_ssl_ctx,

 static int tls_connection_engine_private_key(struct tls_connection *conn)
 {
-#ifndef OPENSSL_NO_ENGINE
+#if defined(ANDROID) || !defined(OPENSSL_NO_ENGINE)
  if (SSL_use_PrivateKey(conn->ssl, conn->private_key) != 1) {
  tls_show_errors(MSG_ERROR, __func__,
  "ENGINE: cannot use private key for TLS");
diff --git a/wpa_supplicant/Android.mk b/wpa_supplicant/Android.mk
index 84a0d58..b205eaf 100644
--- a/wpa_supplicant/Android.mk
+++ b/wpa_supplicant/Android.mk
@@ -1566,6 +1566,13 @@  endif
 ifeq ($(CONFIG_TLS), openssl)
 LOCAL_SHARED_LIBRARIES += libcrypto libssl libkeystore_binder
 endif
+
+# With BoringSSL we need libkeystore-engine in order to provide access to
+# keystore keys.
+ifneq (,$(wildcard external/boringssl/flavor.mk))
+LOCAL_SHARED_LIBRARIES += libkeystore-engine
+endif
+
 ifdef CONFIG_DRIVER_NL80211
 ifneq ($(wildcard external/libnl),)
 LOCAL_SHARED_LIBRARIES += libnl