Add check for password length in SecurityContext. Check return values from BoringSSL functions, thr…

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 28 matching lines @@
 static const int kSSLFilterNativeFieldIndex = 0;
 static const int kSecurityContextNativeFieldIndex = 0;
 static const int kX509NativeFieldIndex = 0;
 
 static const bool SSL_LOG_STATUS = false;
 static const bool SSL_LOG_DATA = false;
 
 static const int SSL_ERROR_MESSAGE_BUFFER_SIZE = 200;
 
 /* Handle an error reported from the BoringSSL library. */
-static void ThrowIOException(const char* exception_type,
+static void ThrowIOException(int status,
+                             const char* exception_type,
                              const char* message,
                              bool free_message = false) {
   // TODO(24068): Get the error code and message from the error stack.
   // There may be more than one error on the stack - should we
   // concatenate the error messages?
-  int error_code = 0;
-  const char* error_message = "Unknown error from BoringSSL library";
+  int error_code = status;
+  const char* error_message = "Error from BoringSSL library";
   OSError os_error_struct(error_code, error_message, OSError::kBoringSSL);
   Dart_Handle os_error = DartUtils::NewDartOSError(&os_error_struct);
   Dart_Handle exception =
       DartUtils::NewDartIOException(exception_type, message, os_error);
   if (free_message) {
     free(const_cast<char*>(message));
   }
   Dart_ThrowException(exception);
   UNREACHABLE();
 }
@@ skipping 258 matching lines @@
   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");
   SetSecurityContext(args, context);
 }
 
 
 int PasswordCallback(char* buf, int size, int rwflag, void* userdata) {
   char* password = static_cast<char*>(userdata);
-  if (static_cast<size_t>(size) < strlen(password) + 1) {
-    Log::PrintErr("Password buffer too small.\n");
-    exit(1);
-    // TODO(24182): Find the actual value of size passed in here, and
-    // check for password length longer than this in the Dart function
-    // that passes in the password, so we never have this problem.
-  }
-  strncpy(buf, static_cast<char*>(userdata), size);
-  return strlen(static_cast<char*>(userdata));
+  assert(size == PEM_BUFSIZE);
+  strncpy(buf, password, size);
+  return strlen(password);

[Søren Gjesse, 2015/10/05 09:13:40]

Always add a terminating \0 to password.

[Bill Hesse, 2015/10/05 11:28:31]

WE check in the caller that strlen(password) is < PEM_BUFSIZE, so there will
always be a \0 written.  We can do it again, but it is redundant.

 }
 
 
-void CheckStatus(int status, const char* message, int line) {
+void CheckStatus(int status,
+                 const char* type,
+                 const char* message) {
   // TODO(24183): Take appropriate action on failed calls,
   // throw exception that includes all messages from the error stack.
-  if (status != 1 && SSL_LOG_STATUS) {
+  if (status == 1) return;
+  if (SSL_LOG_STATUS) {
     int error = ERR_get_error();
-    Log::PrintErr("Failed: %s line %d\n", message, line);
+    Log::PrintErr("Failed: %s status %d", message, status);
     char error_string[SSL_ERROR_MESSAGE_BUFFER_SIZE];
     ERR_error_string_n(error, error_string, SSL_ERROR_MESSAGE_BUFFER_SIZE);
     Log::PrintErr("ERROR: %d %s\n", error, error_string);
   }
+  ThrowIOException(status, type, message);
 }
 
 
 void FUNCTION_NAME(SecurityContext_UsePrivateKey)(Dart_NativeArguments args) {
   SSL_CTX* context = GetSecurityContext(args);
   Dart_Handle filename_object = ThrowIfError(Dart_GetNativeArgument(args, 1));
   const char* filename = NULL;
   if (Dart_IsString(filename_object)) {
     ThrowIfError(Dart_StringToCString(filename_object, &filename));
   } else {
     Dart_ThrowException(DartUtils::NewDartArgumentError(
         "File argument to SecurityContext.usePrivateKey is not a String"));
   }
   Dart_Handle password_object = ThrowIfError(Dart_GetNativeArgument(args, 2));
   const char* password = NULL;
   if (Dart_IsString(password_object)) {
     ThrowIfError(Dart_StringToCString(password_object, &password));
+    assert(PEM_BUFSIZE == 1024);

[Søren Gjesse, 2015/10/05 09:13:40]

Why this assert - is it for the error message? If so use snprintf to generate
the message.

Also use the ASSERT macro.

[Bill Hesse, 2015/10/05 11:28:31]

Yes, it is for the error message. Removed it.

+    if (strlen(password) > PEM_BUFSIZE - 1) {
+      Dart_ThrowException(DartUtils::NewDartArgumentError(
+        "SecurityContext.usePrivateKey password length is greater than 1023"));
+    }
   } else if (Dart_IsNull(password_object)) {
     password = "";
   } else {
     Dart_ThrowException(DartUtils::NewDartArgumentError(
-        "Password argument to SecurityContext.usePrivateKey is not "
-        "a String or null"));
+        "SecurityContext.usePrivateKey password is not a String or null"));
   }
 
   SSL_CTX_set_default_passwd_cb(context, PasswordCallback);
   SSL_CTX_set_default_passwd_cb_userdata(context, const_cast<char*>(password));
   int status = SSL_CTX_use_PrivateKey_file(context,
                                            filename,
                                            SSL_FILETYPE_PEM);
   // 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, "SSL_CTX_use_PrivateKey_file", __LINE__);
+  CheckStatus(status, "TlsException", "Failure in usePrivateKey");
   SSL_CTX_set_default_passwd_cb_userdata(context, NULL);
 }
 
 
 void FUNCTION_NAME(SecurityContext_SetTrustedCertificates)(
     Dart_NativeArguments args) {
   SSL_CTX* context = GetSecurityContext(args);
   Dart_Handle filename_object = ThrowIfError(Dart_GetNativeArgument(args, 1));
   const char* filename = NULL;
   if (Dart_IsString(filename_object)) {
     ThrowIfError(Dart_StringToCString(filename_object, &filename));
   }
   Dart_Handle directory_object = ThrowIfError(Dart_GetNativeArgument(args, 2));
   const char* directory = NULL;
   if (Dart_IsString(directory_object)) {
     ThrowIfError(Dart_StringToCString(directory_object, &directory));
   } else if (Dart_IsNull(directory_object)) {
     directory = NULL;
   } else {
     Dart_ThrowException(DartUtils::NewDartArgumentError(
         "Directory argument to SecurityContext.usePrivateKey is not "
         "a String or null"));
   }
 
   int status = SSL_CTX_load_verify_locations(context, filename, directory);
-  CheckStatus(status, "SSL_CTX_load_verify_locations", __LINE__);
+  CheckStatus(
+      status, "TlsException", "SSL_CTX_load_verify_locations");
 }
 
 
 void FUNCTION_NAME(SecurityContext_TrustBuiltinRoots)(
     Dart_NativeArguments args) {
   SSL_CTX* context = GetSecurityContext(args);
   X509_STORE* store = SSL_CTX_get_cert_store(context);
   BIO* roots_bio =
       BIO_new_mem_buf(const_cast<unsigned char*>(root_certificates_pem),
                       root_certificates_pem_length);
@@ skipping 14 matching lines @@
   Dart_Handle filename_object = ThrowIfError(Dart_GetNativeArgument(args, 1));
   const char* filename = NULL;
   if (Dart_IsString(filename_object)) {
     ThrowIfError(Dart_StringToCString(filename_object, &filename));
   } else {
     Dart_ThrowException(DartUtils::NewDartArgumentError(
         "file argument in SecurityContext.useCertificateChain"
         " is not a String"));
   }
   int status = SSL_CTX_use_certificate_chain_file(context, filename);
-  CheckStatus(status, "SSL_CTX_use_certificate_chain_file", __LINE__);
+  CheckStatus(status,
+              "TlsException",
+              "Failure in useCertificateChain");
 }
 
 
 void FUNCTION_NAME(SecurityContext_SetClientAuthorities)(
     Dart_NativeArguments args) {
   SSL_CTX* context = GetSecurityContext(args);
   Dart_Handle filename_object = ThrowIfError(Dart_GetNativeArgument(args, 1));
   const char* filename = NULL;
   if (Dart_IsString(filename_object)) {
     ThrowIfError(Dart_StringToCString(filename_object, &filename));
@@ skipping 404 matching lines @@
                         Dart_Handle protocols_handle) {
   is_server_ = is_server;
   if (in_handshake_) {
     FATAL("Connect called twice on the same _SecureFilter.");
   }
 
   int status;
   int error;
   BIO* ssl_side;
   status = BIO_new_bio_pair(&ssl_side, 10000, &socket_side_, 10000);
-  CheckStatus(status, "BIO_new_bio_pair", __LINE__);
+  CheckStatus(status, "TlsException", "BIO_new_bio_pair");
 
   assert(context != NULL);
   ssl_ = SSL_new(context);
   SSL_set_bio(ssl_, ssl_side, ssl_side);
   SSL_set_mode(ssl_, SSL_MODE_AUTO_RETRY);  // TODO(whesse): Is this right?
   SSL_set_ex_data(ssl_, filter_ssl_index, this);
 
   if (is_server_) {
     // Do not request a client certificate.
     // TODO(24069): Allow server to request a client certificate, when desired.
     SSL_set_verify(ssl_, SSL_VERIFY_NONE, NULL);
   } else {
     SetAlpnProtocolList(protocols_handle, ssl_, NULL, false);
     status = SSL_set_tlsext_host_name(ssl_, hostname);
-    CheckStatus(status, "Set SNI host name", __LINE__);
+    CheckStatus(status, "TlsException", "Set SNI host name");
     // Sets the hostname in the certificate-checking object, so it is checked
     // against the certificate presented by the server.
     X509_VERIFY_PARAM* certificate_checking_parameters = SSL_get0_param(ssl_);
     hostname_ = strdup(hostname);
     X509_VERIFY_PARAM_set_flags(certificate_checking_parameters,
                                 X509_V_FLAG_PARTIAL_CHAIN |
                                 X509_V_FLAG_TRUSTED_FIRST);
     X509_VERIFY_PARAM_set_hostflags(certificate_checking_parameters, 0);
     status = X509_VERIFY_PARAM_set1_host(certificate_checking_parameters,
                                          hostname_, strlen(hostname_));
-    CheckStatus(status, "Set hostname for certificate checking", __LINE__);
+    CheckStatus(status, "TlsException",
+                "Set hostname for certificate checking");
   }
   // Make the connection:
   if (is_server_) {
     status = SSL_accept(ssl_);
     if (SSL_LOG_STATUS) Log::Print("SSL_accept status: %d\n", status);
     if (status != 1) {
       // TODO(whesse): expect a needs-data error here.  Handle other errors.
       error = SSL_get_error(ssl_, status);
       if (SSL_LOG_STATUS) Log::Print("SSL_accept error: %d\n", error);
     }
@@ skipping 24 matching lines @@
 
 
 int printErrorCallback(const char *str, size_t len, void *ctx) {
   Log::PrintErr("%.*s\n", static_cast<int>(len), str);
   return 1;
 }
 
 void SSLFilter::Handshake() {
   // Try and push handshake along.
   int status;
-  int error;
   status = SSL_do_handshake(ssl_);
   if (callback_error != NULL) {
     // The SSL_do_handshake will try performing a handshake and might call
     // a CertificateCallback. If the certificate validation
     // failed the 'callback_error" will be set by the certificateCallback
     // logic and we propagate the error"
     Dart_PropagateError(callback_error);
   }

[Søren Gjesse, 2015/10/05 09:13:40]

Before there was a status check here as well.

[Bill Hesse, 2015/10/05 11:28:31]

There still is.  The CheckStatus call below absorbed it.

-  if (SSL_LOG_STATUS) Log::Print("SSL_handshake status: %d\n", status);
-  if (status != 1) {
-    error = SSL_get_error(ssl_, status);
+  if (SSL_want_write(ssl_) || SSL_want_read(ssl_)) {
+    in_handshake_ = true;
+    return;
+  }
+  CheckStatus(status,
+      "HandshakeException",
+      is_server_ ? "Handshake error in server" : "Handshake error in client");
+  // Handshake succeeded.
+  if (in_handshake_) {
+    // TODO(24071): Check return value of SSL_get_verify_result, this
+    //    should give us the hostname check.
+    int result = SSL_get_verify_result(ssl_);
     if (SSL_LOG_STATUS) {
-      Log::Print("ERROR: %d\n", error);
-      ERR_print_errors_cb(printErrorCallback, NULL);
-    }
-  }
-  if (status == 1) {
-    if (in_handshake_) {
-      // TODO(24071): Check return value of SSL_get_verify_result, this
-      //    should give us the hostname check.
-      int result = SSL_get_verify_result(ssl_);
-      if (SSL_LOG_STATUS) {
-        Log::Print("Handshake verification status: %d\n", result);
-        X509* peer_certificate = SSL_get_peer_certificate(ssl_);
-        if (peer_certificate == NULL) {
-          Log::Print("No peer certificate received\n");
-        } else {
-          X509_NAME* s_name = X509_get_subject_name(peer_certificate);
-          printf("Peer certificate SN: ");
-          X509_NAME_print_ex_fp(stdout, s_name, 4, 0);
-          printf("\n");
-        }
-      }
-      ThrowIfError(Dart_InvokeClosure(
-          Dart_HandleFromPersistent(handshake_complete_), 0, NULL));
-      in_handshake_ = false;
-    }
-  } else if (status == 0) {
-    if (is_server_) {
-      ThrowIOException("HandshakeException",
-                       "Handshake error in server");
-    } else {
-      ThrowIOException("HandshakeException",
-                       "Handshake error in client");
-    }
-  } else if (status < 0) {
-    if (SSL_want_write(ssl_) || SSL_want_read(ssl_)) {
-      if (!in_handshake_) {
-        in_handshake_ = true;
-      }
-    } else {
-      if (is_server_) {
-        ThrowIOException("HandshakeException",
-                         "Handshake error in server");
+      Log::Print("Handshake verification status: %d\n", result);
+      X509* peer_certificate = SSL_get_peer_certificate(ssl_);
+      if (peer_certificate == NULL) {
+        Log::Print("No peer certificate received\n");
       } else {
-        ThrowIOException("HandshakeException",
-                         "Handshake error in client");
+        X509_NAME* s_name = X509_get_subject_name(peer_certificate);
+        printf("Peer certificate SN: ");

[Søren Gjesse, 2015/10/05 09:13:40]

Debug print.

[Bill Hesse, 2015/10/05 11:28:31]

This is a logging print, guarded by SSL_LOG_STATUS.  It cannot use Log::Print
because the way to print a subject name in BoringSSL prints it to a file
descriptor, not to a string buffer or other place we can grab it from.

+        X509_NAME_print_ex_fp(stdout, s_name, 4, 0);
+        printf("\n");

[Søren Gjesse, 2015/10/05 09:13:40]

ditto.

       }
     }
+    ThrowIfError(Dart_InvokeClosure(
+        Dart_HandleFromPersistent(handshake_complete_), 0, NULL));
+    in_handshake_ = false;
   }
 }
 
 void SSLFilter::GetSelectedProtocol(Dart_NativeArguments args) {
   const uint8_t* protocol;
   unsigned length;
   SSL_get0_alpn_selected(ssl_, &protocol, &length);
   if (length == 0) {
     Dart_SetReturnValue(args, Dart_Null());
   } else {
@@ skipping 109 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