Regularize some errors thrown by SecureContext.

runtime/bin/secure_socket.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.
 
 #include "bin/secure_socket.h"
 
 #include <errno.h>
 #include <fcntl.h>
 #include <sys/stat.h>
 #include <stdio.h>
@@ skipping 506 matching lines @@
   Dart_Handle object_;
   uint8_t* bytes_;
   intptr_t bytes_len_;
   BIO* bio_;
   bool is_typed_data_;
 
   DISALLOW_ALLOCATION();
   DISALLOW_COPY_AND_ASSIGN(ScopedMemBIO);
 };
 
-
 template<typename T, void (*free_func)(T*)>
 class ScopedSSLType {
  public:
   explicit ScopedSSLType(T* obj) : obj_(obj) {}
 
   ~ScopedSSLType() {
     if (obj_ != NULL) {
       free_func(obj_);
     }
   }
@@ skipping 37 matching lines @@
 
  private:
   T* obj_;
 
   DISALLOW_ALLOCATION();
   DISALLOW_COPY_AND_ASSIGN(ScopedSSLStackType);
 };
 
 typedef ScopedSSLType<PKCS12, PKCS12_free> ScopedPKCS12;
 typedef ScopedSSLType<X509, X509_free> ScopedX509;
-
 typedef ScopedSSLStackType<STACK_OF(X509), X509, X509_free> ScopedX509Stack;
-typedef ScopedSSLStackType<STACK_OF(X509_NAME), X509_NAME, X509_NAME_free>
-    ScopedX509NAMEStack;
-
 
 // We try reading data as PKCS12 only if reading as PEM was unsuccessful and
 // if there is no indication that the data is malformed PEM. We assume the data
 // is malformed PEM if it contains the start line, i.e. a line with ----- BEGIN.
-static bool TryPKCS12(bool pem_success) {
+static bool TryPKCS12() {
   uint32_t last_error = ERR_peek_last_error();
-  return !pem_success &&
-      (ERR_GET_LIB(last_error) == ERR_LIB_PEM) &&
-      (ERR_GET_REASON(last_error) == PEM_R_NO_START_LINE);
+  return (ERR_GET_LIB(last_error) == ERR_LIB_PEM) &&
+         (ERR_GET_REASON(last_error) == PEM_R_NO_START_LINE);
 }
 
 
 static EVP_PKEY* GetPrivateKeyPKCS12(BIO* bio, const char* password) {
   ScopedPKCS12 p12(d2i_PKCS12_bio(bio, NULL));
   if (p12.get() == NULL) {
     return NULL;
   }
 
   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 NULL;
   }
 
   // We only care about the private key.
   ScopedX509 delete_cert(cert);
   ScopedX509Stack delete_ca_certs(ca_certs);
   return key;
 }
 
 
 static EVP_PKEY* GetPrivateKey(BIO* bio, const char* password) {
   EVP_PKEY *key = PEM_read_bio_PrivateKey(
       bio, NULL, PasswordCallback, const_cast<char*>(password));
-  if (TryPKCS12(key != NULL)) {
+  if ((key == NULL) && TryPKCS12()) {
     // Reset the bio, and clear the error from trying to read as PEM.
     ERR_clear_error();
     BIO_reset(bio);
 
     // Try to decode as PKCS12
     key = GetPrivateKeyPKCS12(bio, password);
   }
   return key;
 }
 
@@ skipping 99 matching lines @@
   }
 
   return status;
 }
 
 
 static int SetTrustedCertificatesBytes(SSL_CTX* context,
                                        BIO* bio,
                                        const char* password) {
   int status = SetTrustedCertificatesBytesPEM(context, bio);
-  if (TryPKCS12(status != 0)) {
+  if ((status == 0) && TryPKCS12()) {

[Ivan Posva, 2016/03/02 23:22:15]

This would read even better (at least for me) and avoids the extra elsif.

if (status == 0) {
  if (TryPKCS12()) {
    ...
  }
} else {
  ERR_clear_error();
}

[zra, 2016/03/03 00:06:24]

Done.

     ERR_clear_error();
     BIO_reset(bio);
     status = SetTrustedCertificatesBytesPKCS12(context, bio, password);
   } else if (status != 0) {
     // The PEM file was successfully parsed.
     ERR_clear_error();
   }
   return status;
 }
 
@@ skipping 112 matching lines @@
     // this case will be triggered.
     status = 0;
   }
 
   return status;
 }
 
 
 static int UseChainBytes(SSL_CTX* context, BIO* bio, const char* password) {
   int status = UseChainBytesPEM(context, bio);
-  if (TryPKCS12(status != 0)) {
+  if ((status == 0) && TryPKCS12()) {
     ERR_clear_error();
     BIO_reset(bio);
     status = UseChainBytesPKCS12(context, bio, password);
   } else if (status != 0) {
     // The PEM file was successfully read.
     ERR_clear_error();
   }
   return status;
 }
 
 
 void FUNCTION_NAME(SecurityContext_UseCertificateChainBytes)(
     Dart_NativeArguments args) {
   SSL_CTX* context = GetSecurityContext(args);
   const char* password = GetPasswordArgument(args, 2);
   int status;
   {
     ScopedMemBIO bio(ThrowIfError(Dart_GetNativeArgument(args, 1)));
     status = UseChainBytes(context, bio.bio(), password);
   }
   CheckStatus(status,
               "TlsException",
               "Failure in useCertificateChainBytes");
 }
 
 
-static STACK_OF(X509_NAME)* GetCertificateNamesPKCS12(BIO* bio,
-                                                      const char* password) {
+static int SetClientAuthoritiesPKCS12(SSL_CTX* context,
+                                      BIO* bio,
+                                      const char* password) {
   ScopedPKCS12 p12(d2i_PKCS12_bio(bio, NULL));
   if (p12.get() == NULL) {
     return NULL;

[Ivan Posva, 2016/03/02 23:22:15]

Returning NULL in a function typed as returning an int.

[zra, 2016/03/03 00:06:24]

Done.

   }
 
-  ScopedX509NAMEStack result(sk_X509_NAME_new_null());
-  if (result.get() == NULL) {
-    return NULL;
-  }
-
   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 NULL;
+    return status;
   }
 
-  ScopedX509 x509(cert);
-  ScopedX509Stack certs(ca_certs);
-  X509_NAME* x509_name = X509_get_subject_name(x509.get());
-  if (x509_name == NULL) {
-    return NULL;
+  ScopedX509Stack cert_stack(ca_certs);
+  status = SSL_CTX_add_client_CA(context, cert);
+  if (status == 0) {
+    X509_free(cert);
+    return status;
   }
 
-  x509_name = X509_NAME_dup(x509_name);
-  if (x509_name == NULL) {
-    return NULL;
+  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;
+    }
   }
 
-  sk_X509_NAME_push(result.get(), x509_name);
-
-  while (true) {
-    ScopedX509 ca(sk_X509_shift(certs.get()));
-    if (ca.get() == NULL) {
-      break;
-    }
-
-    X509_NAME* x509_name = X509_get_subject_name(ca.get());
-    if (x509_name == NULL) {
-      return NULL;
-    }
-
-    x509_name = X509_NAME_dup(x509_name);
-    if (x509_name == NULL) {
-      return NULL;
-    }
-
-    sk_X509_NAME_push(result.get(), x509_name);
-  }
-
-  return result.release();
+  return status;
 }
 
 
-static STACK_OF(X509_NAME)* GetCertificateNamesPEM(BIO* bio) {
-  ScopedX509NAMEStack result(sk_X509_NAME_new_null());
-  if (result.get() == NULL) {
-    return NULL;
+static int SetClientAuthoritiesPEM(SSL_CTX* context, BIO* bio) {
+  int status = 0;
+  X509* cert = NULL;
+  while ((cert = PEM_read_bio_X509(bio, NULL, NULL, NULL)) != NULL) {
+    status = SSL_CTX_add_client_CA(context, cert);
+    X509_free(cert);  // The name has been extracted.
+    if (status == 0) {
+      return status;
+    }
   }
 
-  while (true) {
-    ScopedX509 x509(PEM_read_bio_X509(bio, NULL, NULL, NULL));
-    if (x509.get() == NULL) {
-      break;
-    }
-
-    X509_NAME* x509_name = X509_get_subject_name(x509.get());
-    if (x509_name == NULL) {
-      return NULL;
-    }
-
-    // Duplicate the name to put it on the stack.
-    x509_name = X509_NAME_dup(x509_name);
-    if (x509_name == NULL) {
-      return NULL;
-    }
-    sk_X509_NAME_push(result.get(), x509_name);
-  }
-
-  if (sk_X509_NAME_num(result.get()) == 0) {
-    // The data was not PEM.
-    return NULL;
-  }
-
+  // If bio does not contain PEM data, the first call to PEM_read_bio_X509 will
+  // return NULL, and the while-loop will exit while status is still 0.
   uint32_t err = ERR_peek_last_error();
   if ((ERR_GET_LIB(err) != ERR_LIB_PEM) ||

[Ivan Posva, 2016/03/02 23:22:15]

Isn't this essemtially TryPKCS12()?

[zra, 2016/03/03 00:06:24]

Renamed TryPKCS12 to NoPEMStartLine, and used it instead.

       (ERR_GET_REASON(err) != PEM_R_NO_START_LINE)) {
-    // The data was trying to be PEM, but was malformed.
-    return NULL;
+    // If bio contains data that is trying to be PEM but is malformed, then
+    // this case will be triggered.
+    status = 0;
   }
 
-  return result.release();
+  return status;
 }
 
 
-static STACK_OF(X509_NAME)* GetCertificateNames(BIO* bio,
-                                                const char* password) {
-  STACK_OF(X509_NAME)* result = GetCertificateNamesPEM(bio);
-  if (TryPKCS12(result != NULL)) {
+static int SetClientAuthorities(SSL_CTX* context,
+                                BIO* bio,
+                                const char* password) {
+  int status = SetClientAuthoritiesPEM(context, bio);
+  if ((status == 0) && TryPKCS12()) {
     ERR_clear_error();
     BIO_reset(bio);
-    result = GetCertificateNamesPKCS12(bio, password);
-  } else if (result != NULL) {
+    status = SetClientAuthoritiesPKCS12(context, bio, password);
+  } else if (status != 0) {
     // The PEM file was successfully parsed.
     ERR_clear_error();
   }
-  return result;
+  return status;
 }
 
 
 void FUNCTION_NAME(SecurityContext_SetClientAuthoritiesBytes)(
     Dart_NativeArguments args) {
   SSL_CTX* context = GetSecurityContext(args);
   const char* password = GetPasswordArgument(args, 2);
-  STACK_OF(X509_NAME)* certificate_names;
 
+  int status;
   {
     ScopedMemBIO bio(ThrowIfError(Dart_GetNativeArgument(args, 1)));
-    certificate_names = GetCertificateNames(bio.bio(), password);
+    status = SetClientAuthorities(context, bio.bio(), password);
   }
 
-  if (certificate_names != NULL) {
-    SSL_CTX_set_client_CA_list(context, certificate_names);
-  } else {
-    Dart_ThrowException(DartUtils::NewDartArgumentError(
-        "Could not load certificate names from file in SetClientAuthorities"));
-  }
+  CheckStatus(status,
+      "TlsException",
+      "Failure in setClientAuthoritiesBytes");
 }
 
 
 void FUNCTION_NAME(SecurityContext_SetAlpnProtocols)(
     Dart_NativeArguments args) {
   SSL_CTX* context = GetSecurityContext(args);
   Dart_Handle protocols_handle =
       ThrowIfError(Dart_GetNativeArgument(args, 1));
   Dart_Handle is_server_handle =
       ThrowIfError(Dart_GetNativeArgument(args, 2));
@@ skipping 667 matching lines @@
     } else {
       if (SSL_LOG_DATA) Log::Print(
           "WriteEncrypted  BIO_read wrote %d bytes\n", bytes_processed);
     }
   }
   return bytes_processed;
 }
 
 }  // namespace bin
 }  // namespace dart