Fix memory leaks in BoringSSL secure socket implementation

runtime/bin/secure_socket_boringssl.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #if !defined(DART_IO_DISABLED) && !defined(DART_IO_SECURE_SOCKET_DISABLED)
 
 #include "platform/globals.h"
 #if defined(TARGET_OS_ANDROID) || \
     defined(TARGET_OS_LINUX)   || \
     defined(TARGET_OS_WINDOWS)
@@ skipping 76 matching lines @@
   }
 }
 
 
 /* Handle an error reported from the BoringSSL library. */
 static void ThrowIOException(int status,
                              const char* exception_type,
                              const char* message) {
   char error_string[SSL_ERROR_MESSAGE_BUFFER_SIZE];
   FetchErrorString(error_string, SSL_ERROR_MESSAGE_BUFFER_SIZE);
-  OSError os_error_struct(status, error_string, OSError::kBoringSSL);
-  Dart_Handle os_error = DartUtils::NewDartOSError(&os_error_struct);
-  Dart_Handle exception =
-      DartUtils::NewDartIOException(exception_type, message, os_error);
-  ASSERT(!Dart_IsError(exception));
+  Dart_Handle exception;
+  {
+    OSError os_error_struct(status, error_string, OSError::kBoringSSL);
+    Dart_Handle os_error = DartUtils::NewDartOSError(&os_error_struct);
+    exception =
+        DartUtils::NewDartIOException(exception_type, message, os_error);
+    ASSERT(!Dart_IsError(exception));
+  }
   Dart_ThrowException(exception);
   UNREACHABLE();
 }
 
 
 static SSLFilter* GetFilter(Dart_NativeArguments args) {
   SSLFilter* filter;
   Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
   ASSERT(Dart_IsInstance(dart_this));
   ThrowIfError(Dart_GetNativeInstanceField(
@@ skipping 24 matching lines @@
       reinterpret_cast<intptr_t>(filter));
   RETURN_IF_ERROR(err);
   Dart_NewWeakPersistentHandle(dart_this,
                                reinterpret_cast<void*>(filter),
                                sizeof(*filter),
                                DeleteFilter);
   return Dart_Null();
 }
 
 
-static SSL_CTX* GetSecurityContext(Dart_NativeArguments args) {
-  SSL_CTX* context;
+static SSLContext* GetSecurityContext(Dart_NativeArguments args) {
+  SSLContext* context;
   Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
   ASSERT(Dart_IsInstance(dart_this));
   ThrowIfError(Dart_GetNativeInstanceField(
       dart_this,
       kSecurityContextNativeFieldIndex,
       reinterpret_cast<intptr_t*>(&context)));
   return context;
 }
 
 
-static void FreeSecurityContext(
+static void DeleteSecurityContext(
     void* isolate_data,
     Dart_WeakPersistentHandle handle,
     void* context_pointer) {
-  SSL_CTX* context = static_cast<SSL_CTX*>(context_pointer);
-  SSL_CTX_free(context);
+  SSLContext* context = static_cast<SSLContext*>(context_pointer);
+  delete context;
 }
 
 
 static Dart_Handle SetSecurityContext(Dart_NativeArguments args,
-                                      SSL_CTX* context) {
+                                      SSLContext* context) {
   const int approximate_size_of_context = 1500;
   Dart_Handle dart_this = Dart_GetNativeArgument(args, 0);
   RETURN_IF_ERROR(dart_this);
   ASSERT(Dart_IsInstance(dart_this));
   Dart_Handle err = Dart_SetNativeInstanceField(
       dart_this,
       kSecurityContextNativeFieldIndex,
       reinterpret_cast<intptr_t>(context));
   RETURN_IF_ERROR(err);
   Dart_NewWeakPersistentHandle(dart_this,
                                context,
                                approximate_size_of_context,
-                               FreeSecurityContext);
+                               DeleteSecurityContext);
   return Dart_Null();
 }
 
 
 static X509* GetX509Certificate(Dart_NativeArguments args) {
   X509* certificate;
   Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
   ASSERT(Dart_IsInstance(dart_this));
   ThrowIfError(Dart_GetNativeInstanceField(
       dart_this,
       kX509NativeFieldIndex,
       reinterpret_cast<intptr_t*>(&certificate)));
   return certificate;
 }
 
 
 // Forward declaration.
 static void SetAlpnProtocolList(Dart_Handle protocols_handle,
                                 SSL* ssl,
-                                SSL_CTX* context,
+                                SSLContext* context,
                                 bool is_server);
 
 
 void FUNCTION_NAME(SecureSocket_Init)(Dart_NativeArguments args) {
   Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
   SSLFilter* filter = new SSLFilter();
   Dart_Handle err = SetFilter(args, filter);
   if (Dart_IsError(err)) {
     filter->Release();
     Dart_PropagateError(err);
@@ skipping 16 matching lines @@
       DartUtils::GetBooleanValue(Dart_GetNativeArgument(args, 4));
   bool require_client_certificate =
       DartUtils::GetBooleanValue(Dart_GetNativeArgument(args, 5));
   Dart_Handle protocols_handle =
       ThrowIfError(Dart_GetNativeArgument(args, 6));
 
   const char* host_name = NULL;
   // TODO(whesse): Is truncating a Dart string containing \0 what we want?
   ThrowIfError(Dart_StringToCString(host_name_object, &host_name));
 
-  SSL_CTX* context = NULL;
+  SSLContext* context = NULL;
   if (!Dart_IsNull(context_object)) {
     ThrowIfError(Dart_GetNativeInstanceField(
         context_object,
         kSecurityContextNativeFieldIndex,
         reinterpret_cast<intptr_t*>(&context)));
   }
 
   // The protocols_handle is guaranteed to be a valid Uint8List.
   // It will have the correct length encoding of the protocols array.
   ASSERT(!Dart_IsNull(protocols_handle));
 
   GetFilter(args)->Connect(host_name,
-                           context,
+                           context->context(),
                            is_server,
                            request_client_certificate,
                            require_client_certificate,
                            protocols_handle);
 }
 
 
 void FUNCTION_NAME(SecureSocket_Destroy)(Dart_NativeArguments args) {
   SSLFilter* filter = GetFilter(args);
   // The SSLFilter is deleted in the finalizer for the Dart object created by
@@ skipping 157 matching lines @@
   if (Dart_IsError(result)) {
     filter->callback_error = result;
     return 0;
   }
   return DartUtils::GetBooleanValue(result);
 }
 
 
 void FUNCTION_NAME(SecurityContext_Allocate)(Dart_NativeArguments args) {
   SSLFilter::InitializeLibrary();
-  SSL_CTX* context = SSL_CTX_new(TLS_method());
-  SSL_CTX_set_verify(context, SSL_VERIFY_PEER, CertificateCallback);
-  SSL_CTX_set_min_version(context, TLS1_VERSION);
-  SSL_CTX_set_cipher_list(context, "HIGH:MEDIUM");
-  SSL_CTX_set_cipher_list_tls11(context, "HIGH:MEDIUM");
+  SSL_CTX* ctx = SSL_CTX_new(TLS_method());
+  SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, CertificateCallback);
+  SSL_CTX_set_min_version(ctx, TLS1_VERSION);
+  SSL_CTX_set_cipher_list(ctx, "HIGH:MEDIUM");
+  SSL_CTX_set_cipher_list_tls11(ctx, "HIGH:MEDIUM");
+  SSLContext* context = new SSLContext(ctx);
   Dart_Handle err = SetSecurityContext(args, context);
   if (Dart_IsError(err)) {
-    SSL_CTX_free(context);
+    delete context;
     Dart_PropagateError(err);
   }
 }
 
 
 int PasswordCallback(char* buf, int size, int rwflag, void* userdata) {
   char* password = static_cast<char*>(userdata);
   ASSERT(size == PEM_BUFSIZE);
   strncpy(buf, password, size);
   return strlen(password);
@@ skipping 209 matching lines @@
   } else {
     Dart_ThrowException(DartUtils::NewDartArgumentError(
         "Password is not a String or null"));
   }
   return password;
 }
 
 
 void FUNCTION_NAME(SecurityContext_UsePrivateKeyBytes)(
     Dart_NativeArguments args) {
-  SSL_CTX* context = GetSecurityContext(args);
+  SSLContext* context = GetSecurityContext(args);
   const char* password = GetPasswordArgument(args, 2);
 
   int status;
   {
     ScopedMemBIO bio(ThrowIfError(Dart_GetNativeArgument(args, 1)));
     EVP_PKEY *key = GetPrivateKey(bio.bio(), password);
-    status = SSL_CTX_use_PrivateKey(context, key);
+    status = SSL_CTX_use_PrivateKey(context->context(), key);
+    EVP_PKEY_free(key);
   }
 
   // TODO(24184): Handle different expected errors here - file missing,
   // incorrect password, file not a PEM, and throw exceptions.
   // CheckStatus should also throw an exception in uncaught cases.
   CheckStatus(status, "TlsException", "Failure in usePrivateKeyBytes");
 }
 
 
 static int SetTrustedCertificatesBytesPKCS12(SSL_CTX* context,
                                              BIO* bio,
                                              const char* password) {
   ScopedPKCS12 p12(d2i_PKCS12_bio(bio, NULL));
   if (p12.get() == NULL) {
     return 0;
   }
 
   EVP_PKEY* key = NULL;
   X509 *cert = NULL;
   STACK_OF(X509) *ca_certs = NULL;
   int status = PKCS12_parse(p12.get(), password, &key, &cert, &ca_certs);
   if (status == 0) {
     return status;
   }
 
   ScopedX509Stack cert_stack(ca_certs);
   X509_STORE* store = SSL_CTX_get_cert_store(context);
   status = X509_STORE_add_cert(store, cert);
+  X509_free(cert);
   if (status == 0) {
-    X509_free(cert);
     return status;
   }
 
   X509* ca;
   while ((ca = sk_X509_shift(cert_stack.get())) != NULL) {
     status = X509_STORE_add_cert(store, ca);
+    X509_free(ca);
     if (status == 0) {
-      X509_free(ca);
       return status;
     }
   }
 
   return status;
 }
 
 
 static int SetTrustedCertificatesBytesPEM(SSL_CTX* context, BIO* bio) {
   X509_STORE* store = SSL_CTX_get_cert_store(context);
 
   int status = 0;
   X509* cert = NULL;
   while ((cert = PEM_read_bio_X509(bio, NULL, NULL, NULL)) != NULL) {
     status = X509_STORE_add_cert(store, cert);
+    X509_free(cert);
     if (status == 0) {
-      X509_free(cert);
       return status;
     }
   }
 
   // If no PEM start line is found, it means that we read to the end of the
   // file, or that the file isn't PEM. In the first case, status will be
   // non-zero indicating success. In the second case, status will be 0,
   // indicating that we should try to read as PKCS12. If there is some other
   // error, we return it up to the caller.
   return NoPEMStartLine() ? status : 0;
@@ skipping 13 matching lines @@
   } else {
     // The PEM file was successfully parsed.
     ERR_clear_error();
   }
   return status;
 }
 
 
 void FUNCTION_NAME(SecurityContext_SetTrustedCertificatesBytes)(
     Dart_NativeArguments args) {
-  SSL_CTX* context = GetSecurityContext(args);
+  SSLContext* context = GetSecurityContext(args);
   const char* password = GetPasswordArgument(args, 2);
   int status;
   {
     ScopedMemBIO bio(ThrowIfError(Dart_GetNativeArgument(args, 1)));
-    status = SetTrustedCertificatesBytes(context, bio.bio(), password);
+    status = SetTrustedCertificatesBytes(
+        context->context(), bio.bio(), password);
   }
   CheckStatus(status,
               "TlsException",
               "Failure in setTrustedCertificatesBytes");
 }
 
 
 void FUNCTION_NAME(SecurityContext_AlpnSupported)(Dart_NativeArguments args) {
   Dart_SetReturnValue(args, Dart_NewBoolean(true));
 }
 
 
 void FUNCTION_NAME(SecurityContext_TrustBuiltinRoots)(
     Dart_NativeArguments args) {
-  SSL_CTX* context = GetSecurityContext(args);
+  SSLContext* context = GetSecurityContext(args);
 #if defined(TARGET_OS_ANDROID)
   // On Android, we don't compile in the trusted root certificates. Insead,
   // we use the directory of trusted certificates already present on the device.
   // This saves ~240KB from the size of the binary. This has the drawback that
   // SSL_do_handshake will synchronously hit the filesystem looking for root
   // certs during its trust evaluation. We call SSL_do_handshake directly from
   // the Dart thread so that Dart code can be invoked from the "bad certificate"
   // callback called by SSL_do_handshake.
   const char* android_cacerts = "/system/etc/security/cacerts";
-  int status = SSL_CTX_load_verify_locations(context, NULL, android_cacerts);
+  int status = SSL_CTX_load_verify_locations(
+      context->context(), NULL, android_cacerts);
   CheckStatus(status, "TlsException", "Failure trusting builtint roots");
 #else
-  X509_STORE* store = SSL_CTX_get_cert_store(context);
+  X509_STORE* store = SSL_CTX_get_cert_store(context->context());
   BIO* roots_bio =
       BIO_new_mem_buf(const_cast<unsigned char*>(root_certificates_pem),
                       root_certificates_pem_length);
   X509* root_cert;
   // PEM_read_bio_X509 reads PEM-encoded certificates from a bio (in our case,
   // backed by a memory buffer), and returns X509 objects, one by one.
   // When the end of the bio is reached, it returns null.
   while ((root_cert = PEM_read_bio_X509(roots_bio, NULL, NULL, NULL)) != NULL) {
-    X509_STORE_add_cert(store, root_cert);
+    int status = X509_STORE_add_cert(store, root_cert);
+    X509_free(root_cert);
+    if (status == 0) {
+      break;
+    }
   }
   BIO_free(roots_bio);
   // If there is an error here, it must be the error indicating that we are done
   // reading PEM certificates.
   ASSERT((ERR_peek_error() == 0) || NoPEMStartLine());
   ERR_clear_error();
 #endif  // defined(TARGET_OS_ANDROID)
 }
 
 
@@ skipping 22 matching lines @@
   }
   if (status == 0) {
     return status;
   }
 
   SSL_CTX_clear_chain_certs(context);
 
   X509* ca;
   while ((ca = sk_X509_shift(certs.get())) != NULL) {
     status = SSL_CTX_add0_chain_cert(context, ca);
+    // SSL_CTX_add0_chain_cert does not inc ref count, so don't free unless the
+    // call fails.
     if (status == 0) {
       X509_free(ca);
       return status;
     }
   }
 
   return status;
 }
 
 
@@ skipping 11 matching lines @@
   }
   if (status == 0) {
     return status;
   }
 
   SSL_CTX_clear_chain_certs(context);
 
   X509* ca;
   while ((ca = PEM_read_bio_X509(bio, NULL, NULL, NULL)) != NULL) {
     status = SSL_CTX_add0_chain_cert(context, ca);
+    // SSL_CTX_add0_chain_cert does not inc ref count, so don't free unless the
+    // call fails.
     if (status == 0) {
       X509_free(ca);
       return status;
     }
     // Note that we must not free `ca` if it was successfully added to the
     // chain. We must free the main certificate x509, though since its reference
     // count is increased by SSL_CTX_use_certificate.
   }
 
   return NoPEMStartLine() ? status : 0;
@@ skipping 11 matching lines @@
   } else {
     // The PEM file was successfully read.
     ERR_clear_error();
   }
   return status;
 }
 
 
 void FUNCTION_NAME(SecurityContext_UseCertificateChainBytes)(
     Dart_NativeArguments args) {
-  SSL_CTX* context = GetSecurityContext(args);
+  SSLContext* context = GetSecurityContext(args);
   const char* password = GetPasswordArgument(args, 2);
   int status;
   {
     ScopedMemBIO bio(ThrowIfError(Dart_GetNativeArgument(args, 1)));
-    status = UseChainBytes(context, bio.bio(), password);
+    status = UseChainBytes(context->context(), bio.bio(), password);
   }
   CheckStatus(status,
               "TlsException",
               "Failure in useCertificateChainBytes");
 }
 
 
 static int SetClientAuthoritiesPKCS12(SSL_CTX* context,
                                       BIO* bio,
                                       const char* password) {
   ScopedPKCS12 p12(d2i_PKCS12_bio(bio, NULL));
   if (p12.get() == NULL) {
     return 0;
   }
 
   EVP_PKEY* key = NULL;
   X509 *cert = NULL;
   STACK_OF(X509) *ca_certs = NULL;
   int status = PKCS12_parse(p12.get(), password, &key, &cert, &ca_certs);
   if (status == 0) {
     return status;
   }
 
   ScopedX509Stack cert_stack(ca_certs);
   status = SSL_CTX_add_client_CA(context, cert);
+  X509_free(cert);

[siva, 2016/08/04 17:25:03]

Can you add a comment that SSL_CTX_add_client_CA increments the ref count on the
cert objects and so we unconditionally decrement the ref count here.

(Maybe a ditto comment on all the spots above where you decrement the ref count
unconditionally).

[zra, 2016/08/04 17:49:34]

Done.

   if (status == 0) {
-    X509_free(cert);
     return status;
   }
 
   X509* ca;
   while ((ca = sk_X509_shift(cert_stack.get())) != NULL) {
     status = SSL_CTX_add_client_CA(context, ca);
     X509_free(ca);  // The name has been extracted.
     if (status == 0) {
       return status;
     }
@@ skipping 30 matching lines @@
   } else {
     // The PEM file was successfully parsed.
     ERR_clear_error();
   }
   return status;
 }
 
 
 void FUNCTION_NAME(SecurityContext_SetClientAuthoritiesBytes)(
     Dart_NativeArguments args) {
-  SSL_CTX* context = GetSecurityContext(args);
+  SSLContext* context = GetSecurityContext(args);
   const char* password = GetPasswordArgument(args, 2);
 
   int status;
   {
     ScopedMemBIO bio(ThrowIfError(Dart_GetNativeArgument(args, 1)));
-    status = SetClientAuthorities(context, bio.bio(), password);
+    status = SetClientAuthorities(context->context(), bio.bio(), password);
   }
 
   CheckStatus(status,
       "TlsException",
       "Failure in setClientAuthoritiesBytes");
 }
 
 
 void FUNCTION_NAME(SecurityContext_SetAlpnProtocols)(
     Dart_NativeArguments args) {
-  SSL_CTX* context = GetSecurityContext(args);
+  SSLContext* context = GetSecurityContext(args);
   Dart_Handle protocols_handle =
       ThrowIfError(Dart_GetNativeArgument(args, 1));
   Dart_Handle is_server_handle =
       ThrowIfError(Dart_GetNativeArgument(args, 2));
   if (Dart_IsBoolean(is_server_handle)) {
     bool is_server = DartUtils::GetBooleanValue(is_server_handle);
     SetAlpnProtocolList(protocols_handle, NULL, context, is_server);
   } else {
     Dart_ThrowException(DartUtils::NewDartArgumentError(
         "Non-boolean is_server argument passed to SetAlpnProtocols"));
@@ skipping 336 matching lines @@
     server_list += protocol_length;
   }
   // TODO(23580): Make failure send a fatal alert instead of ignoring ALPN.
   return SSL_TLSEXT_ERR_NOACK;
 }
 
 
 // Sets the protocol list for ALPN on a SSL object or a context.
 static void SetAlpnProtocolList(Dart_Handle protocols_handle,
                                 SSL* ssl,
-                                SSL_CTX* context,
+                                SSLContext* context,
                                 bool is_server) {
   // Enable ALPN (application layer protocol negotiation) if the caller provides
   // a valid list of supported protocols.
   Dart_TypedData_Type protocols_type;
   uint8_t* protocol_string = NULL;
   uint8_t* protocol_string_copy = NULL;
   intptr_t protocol_string_len = 0;
   int status;
 
   Dart_Handle result = Dart_TypedDataAcquireData(
@@ skipping 16 matching lines @@
       // ALPN on server connections must be set on an SSL_CTX object,
       // not on the SSL object of the individual connection.
       ASSERT(context != NULL);
       ASSERT(ssl == NULL);
       // Because it must be passed as a single void*, terminate
       // the list of (length, data) strings with a length 0 string.
       protocol_string_copy =
           static_cast<uint8_t*>(malloc(protocol_string_len + 1));
       memmove(protocol_string_copy, protocol_string, protocol_string_len);
       protocol_string_copy[protocol_string_len] = '\0';
-      SSL_CTX_set_alpn_select_cb(context, AlpnCallback, protocol_string_copy);
-      // TODO(whesse): If this function is called again, free the previous
-      // protocol_string_copy.  It may be better to keep this as a native
-      // field on the Dart object, since fetching it from the structure is
-      // not in the public api.
-      // Also free protocol_string_copy when the context is destroyed,
-      // in FreeSecurityContext()
+      SSL_CTX_set_alpn_select_cb(
+          context->context(), AlpnCallback, protocol_string_copy);
+      context->set_alpn_protocol_string(protocol_string_copy);
     } else {
       // The function makes a local copy of protocol_string, which it owns.
       if (ssl != NULL) {
         ASSERT(context == NULL);
         status = SSL_set_alpn_protos(ssl, protocol_string, protocol_string_len);
       } else {
         ASSERT(context != NULL);
         ASSERT(ssl == NULL);
         status = SSL_CTX_set_alpn_protos(
-            context, protocol_string, protocol_string_len);
+            context->context(), protocol_string, protocol_string_len);
       }
       ASSERT(status == 0);  // The function returns a non-standard status.
     }
   }
   Dart_TypedDataReleaseData(protocols_handle);
 }
 
 
 void SSLFilter::Connect(const char* hostname,
                         SSL_CTX* context,
@@ skipping 258 matching lines @@
   return bytes_processed;
 }
 
 }  // namespace bin
 }  // namespace dart
 
 #endif  // defined(TARGET_OS_LINUX)
 
 #endif  // !defined(DART_IO_DISABLED) &&
         // !defined(DART_IO_SECURE_SOCKET_DISABLED)