Add secure sockets to dart:io

runtime/bin/tls_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/tls_socket.h"
+
+#include <errno.h>
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <prinit.h>
+#include <prerror.h>
+#include <prnetdb.h>
+#include <nss.h>

[Mads Ager (google), 2012/11/12 11:39:08]

Can we list this in alphabetic order or are there dependencies on the order
here?

[Bill Hesse, 2012/11/13 20:11:08]

Done.

+#include <ssl.h>
+
+#include "bin/builtin.h"
+#include "bin/dartutils.h"
+#include "bin/thread.h"
+#include "bin/utils.h"
+#include "bin/net/nss_memio.h"

[Mads Ager (google), 2012/11/12 11:39:08]

Ditto?

[Bill Hesse, 2012/11/13 20:11:08]

Done.

Is it allowable to put dart_api separately, since it is a big deal that a file
uses the Dart API?

+#include "platform/utils.h"
+
+#include "include/dart_api.h"
+
+bool TlsFilter::library_initialized_ = false;
+static const int kTlsFilterNativeFieldIndex = 0;
+
+static TlsFilter* GetTlsFilter(Dart_NativeArguments args) {
+  TlsFilter* filter;
+

[Mads Ager (google), 2012/11/12 11:39:08]

I would delete this empty line.

+  Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
+  ASSERT(Dart_IsInstance(dart_this));
+  ThrowIfError(Dart_GetNativeInstanceField(dart_this,

[Mads Ager (google), 2012/11/12 11:39:08]

Please move the first argument to the next line as well when using the one
argument per line style.

[Bill Hesse, 2012/11/13 20:11:08]

Done.

+      kTlsFilterNativeFieldIndex,
+      reinterpret_cast<intptr_t*>(&filter)));
+  return filter;
+}
+
+
+static void SetTlsFilter(Dart_NativeArguments args, TlsFilter* filter) {
+  Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
+  ASSERT(Dart_IsInstance(dart_this));
+  ThrowIfError(Dart_SetNativeInstanceField(dart_this,

[Mads Ager (google), 2012/11/12 11:39:08]

Ditto

[Bill Hesse, 2012/11/13 20:11:08]

Done.

+      kTlsFilterNativeFieldIndex,
+      reinterpret_cast<intptr_t>(filter)));
+}
+
+
+void FUNCTION_NAME(TlsSocket_Init)(Dart_NativeArguments args) {
+  Dart_EnterScope();
+  TlsFilter* filter = new TlsFilter;
+  Dart_Handle dart_this = Dart_GetNativeArgument(args, 0);
+  if (Dart_IsError(dart_this)) {
+    delete filter;
+    Dart_PropagateError(dart_this);
+  }
+  SetTlsFilter(args, filter);
+  filter->Init(dart_this);
+  Dart_ExitScope();
+}
+
+
+void FUNCTION_NAME(TlsSocket_Connect)(Dart_NativeArguments args) {
+  Dart_EnterScope();
+  Dart_Handle host_name = ThrowIfError(Dart_GetNativeArgument(args, 1));
+
+  const char* host_name_string = NULL;
+  // TODO(whesse): Is truncating a Dart string containing \0 what we want?
+  ThrowIfError(Dart_StringToCString(host_name, &host_name_string));
+
+  GetTlsFilter(args)->Connect(host_name_string);
+  Dart_ExitScope();
+}
+
+
+void FUNCTION_NAME(TlsSocket_Destroy)(Dart_NativeArguments args) {
+  Dart_EnterScope();
+  TlsFilter* filter = GetTlsFilter(args);
+  SetTlsFilter(args, NULL);
+  filter->Destroy();
+  delete filter;
+  Dart_ExitScope();
+}
+
+
+void FUNCTION_NAME(TlsSocket_Handshake)(Dart_NativeArguments args) {
+  Dart_EnterScope();
+  GetTlsFilter(args)->Handshake();
+  Dart_ExitScope();
+}
+
+
+void FUNCTION_NAME(TlsSocket_RegisterHandshakeCallbacks)(
+    Dart_NativeArguments args) {
+  Dart_EnterScope();
+  Dart_Handle handshake_start = ThrowIfError(Dart_GetNativeArgument(args, 1));
+  Dart_Handle handshake_finish = ThrowIfError(Dart_GetNativeArgument(args, 2));
+  if (!Dart_IsClosure(handshake_start) ||
+      !Dart_IsClosure(handshake_finish)) {
+    Dart_ThrowException(DartUtils::NewDartArgumentError(
+        "Illegal argument to RegisterHandshakeCallbacks"));
+  }
+  GetTlsFilter(args)->RegisterHandshakeCallbacks(handshake_start,
+                                                 handshake_finish);
+  Dart_ExitScope();
+}
+
+
+void FUNCTION_NAME(TlsSocket_ProcessBuffer)(Dart_NativeArguments args) {
+  Dart_EnterScope();
+  Dart_Handle buffer_id_object = ThrowIfError(Dart_GetNativeArgument(args, 1));
+  int64_t buffer_id = DartUtils::GetIntegerValue(buffer_id_object);
+  if (buffer_id < 0 || buffer_id >= TlsFilter::kNumBuffers) {
+    Dart_ThrowException(DartUtils::NewDartArgumentError(
+        "Illegal argument to ProcessBuffer"));
+  }
+
+  intptr_t bytes_read =
+      GetTlsFilter(args)->ProcessBuffer(static_cast<int>(buffer_id));
+  Dart_SetReturnValue(args, Dart_NewInteger(bytes_read));
+  Dart_ExitScope();
+}
+
+
+void FUNCTION_NAME(TlsSocket_SetCertificateDatabase)
+    (Dart_NativeArguments args) {
+  Dart_EnterScope();
+  Dart_Handle dart_pkcert_dir = ThrowIfError(Dart_GetNativeArgument(args, 0));
+  // Check that the type is string, and get the UTF-8 C string value from it.
+  if (Dart_IsString(dart_pkcert_dir)) {
+    const char* pkcert_dir = NULL;
+    ThrowIfError(Dart_StringToCString(dart_pkcert_dir, &pkcert_dir));
+    TlsFilter::InitializeLibrary(pkcert_dir);
+  } else {
+    Dart_ThrowException(DartUtils::NewDartArgumentError(
+        "Non-String argument to SetCertificateDatabase"));
+  }
+  Dart_ExitScope();
+}
+
+
+void TlsFilter::Init(Dart_Handle dart_this) {
+  stringStart_ = ThrowIfError(
+      Dart_NewPersistentHandle(DartUtils::NewString("start")));
+  stringLength_ = ThrowIfError(
+      Dart_NewPersistentHandle(DartUtils::NewString("length")));
+
+  InitializeBuffers(dart_this);
+  memio_ = memio_CreateIOLayer(kMemioBufferSize);
+}
+
+
+void TlsFilter::InitializeBuffers(Dart_Handle dart_this) {
+  // Create TlsFilter buffers as ExternalUint8Array objects.
+  Dart_Handle dart_buffers_object = ThrowIfError(
+      Dart_GetField(dart_this, DartUtils::NewString("buffers")));
+  Dart_Handle dart_buffer_object = ThrowIfError(
+      Dart_ListGetAt(dart_buffers_object, kReadPlaintext));
+  Dart_Handle tls_external_buffer_class = ThrowIfError(

[Søren Gjesse, 2012/11/12 12:04:57]

Wouldn't it be simpler to just look up the class "_TlsExternalBuffer" directly?

[Bill Hesse, 2012/11/13 20:11:08]

I beleive we need a library for that, and so the number of lookups are the same.
 And this does get the correct dynamic type of the object.

+      Dart_InstanceGetClass(dart_buffer_object));
+  Dart_Handle dart_buffer_size = ThrowIfError(
+      Dart_GetField(tls_external_buffer_class, DartUtils::NewString("kSize")));
+  buffer_size_ = DartUtils::GetIntegerValue(dart_buffer_size);
+  if (buffer_size_ <= 0 || buffer_size_ > 1024 * 1024) {
+    Dart_ThrowException(
+        DartUtils::NewString("Invalid buffer size in _TlsExternalBuffer"));
+  }
+
+  for (int i = 0; i < kNumBuffers; ++i) {
+    dart_buffer_objects_[i] = ThrowIfError(
+        Dart_NewPersistentHandle(Dart_ListGetAt(dart_buffers_object, i)));
+    buffers_[i] = new uint8_t[buffer_size_];
+    Dart_Handle data = ThrowIfError(
+      Dart_NewExternalByteArray(buffers_[i],
+                                buffer_size_, NULL, NULL));

[Mads Ager (google), 2012/11/12 11:39:08]

Might fit on one line? If not, either move more to first line or use
one-argument per line?

+    ThrowIfError(Dart_SetField(dart_buffer_objects_[i],
+                               DartUtils::NewString("data"),

[Søren Gjesse, 2012/11/12 12:04:57]

Move DartUtils::NewString("data") out out the loop.

+                               data));
+  }
+}
+
+
+void TlsFilter::RegisterHandshakeCallbacks(Dart_Handle start,
+                                           Dart_Handle finish) {
+  handshake_start_ = ThrowIfError(Dart_NewPersistentHandle(start));

[Mads Ager (google), 2012/11/12 11:39:08]

Where are these persistent handles destroyed? Is this only called once? Can we
either assert that the handles are null or make sure to destroy them before we
allocated new ones?

[Bill Hesse, 2012/11/13 20:11:08]

This is only called once, in the constructor of the TlsFilter.  The handles are
now destroyed in TlsFilter::Destroy.

On 2012/11/12 11:39:08, Mads Ager wrote:

+  handshake_finish_ = ThrowIfError(Dart_NewPersistentHandle(finish));
+}
+
+
+void TlsFilter::InitializeLibrary(const char* pkcert_database) {
+  LockInitMutex();
+  if (!library_initialized_) {
+    PR_Init(PR_USER_THREAD, PR_PRIORITY_NORMAL, 0);
+    // TODO: Allow read-write opening of pkcert_database using

[Mads Ager (google), 2012/11/12 11:39:08]

Let's remove this TODO for now? We are not currently planning to allow writing
to the database.

[Søren Gjesse, 2012/11/12 12:04:57]

Please add whesse to TODO.

[Bill Hesse, 2012/11/13 20:11:08]

Done.

+    //     NSS_InitReadWrite, controlled by an optional argument.
+    SECStatus status =

[Søren Gjesse, 2012/11/12 12:04:57]

Fits on one line.

[Bill Hesse, 2012/11/13 20:11:08]

Done.

+        NSS_Init(pkcert_database);

[Mads Ager (google), 2012/11/12 11:39:08]

Fits on one line.

+    if (status != SECSuccess) {
+      ReportError("Unsuccessful NSS_Init call.\n", PR_GetError());

[Mads Ager (google), 2012/11/12 11:39:08]

Remove the \n from the strings here and let ReportError deal with that?

[Bill Hesse, 2012/11/13 20:11:08]

Done.

+    }
+
+    status = NSS_SetDomesticPolicy();
+    if (status != SECSuccess) {
+      ReportError("Unsuccessful NSS_SetDomesticPolicy call.\n", PR_GetError());
+    }
+  } else {
+      ReportError("Called TlsFilter::InitializeLibrary more than once", 0);

[Mads Ager (google), 2012/11/12 11:39:08]

Indentation is off.

[Bill Hesse, 2012/11/13 20:11:08]

Done.

+  }
+  UnlockInitMutex();
+}
+
+
+void TlsFilter::Connect(const char* host) {
+  if (in_handshake_) {
+    ReportError("Connect called while already in handshake state.", 0);
+  }
+  PRFileDesc* my_socket = memio_;
+
+  my_socket = SSL_ImportFD(NULL, my_socket);
+  if (my_socket == NULL) {
+    ReportError("Unsuccessful SSL_ImportFD call", 0);
+  }
+
+  if (SSL_SetURL(my_socket, host) == -1) {
+    ReportError("Unsuccessful SetURL call", 0);
+  }
+
+  SECStatus status = SSL_ResetHandshake(my_socket, PR_FALSE);
+  if (status != SECSuccess) {
+    ReportError("Unsuccessful SSL_ResetHandshake call", PR_GetError());
+  }
+
+  // SetPeerAddress
+  PRNetAddr host_address;
+  char host_entry_buffer[PR_NETDB_BUF_SIZE];
+  PRHostEnt host_entry;
+  PRStatus rv = PR_GetHostByName(host, host_entry_buffer,
+                                 PR_NETDB_BUF_SIZE, &host_entry);
+  if (rv != PR_SUCCESS) {
+    ReportError("Unsuccessful PR_GetHostByName call", PR_GetError());
+  }
+
+  int index = PR_EnumerateHostEnt(0, &host_entry, 443, &host_address);

[Søren Gjesse, 2012/11/12 12:04:57]

Should 443 be hardcoded here? How about TLS on other ports?

+  if (index == -1 || index == 0) {
+    ReportError("Unsuccessful PR_EnumerateHostEnt call", 0);
+  }
+
+  memio_SetPeerName(my_socket, &host_address);
+  memio_ = my_socket;
+}
+
+
+void TlsFilter::Handshake() {

[Mads Ager (google), 2012/11/12 11:39:08]

This looks a little convoluted to me.

Don't we always want the handshake_start_ and handshake_finish_ callbacks? How
about not relying on SSL_ForceHandshake returning PR_WOULD_BLOCK for that?

void TlsFilter::Handshake() {
  if (!in_handshake_) {
    // Notify dart code that handshake is starting.
    ThrowIfError(Dart_InvokeClosure(handleshake_start_, 0, NULL);
    in_handshake_ = true;
  }
  SECStatus status = SSL_Forcehandshake(memio_);
  if (status == SECSuccess) {
    // Notify dart code that handshake is complete.
    ThrowIfError(Dart_InvokeClosure(handshake_finish, ...));
  } else if (error != PR_WOULD_BLOCK_ERROR) {
    ReportError("...", ...)
  }
  // Forcehandshake failed with PR_WOULD_BLOCK_ERROR.
  // TlsFilter::Handshake will be called again when more
  // data has arrived on the socket.
}

Writing this, do we need the start callback at all? Can't the dart code setup
the handler, then call this function? Then we only need to call the
handshake_done callback?

[Mads Ager (google), 2012/11/14 10:18:54]

How about this comment, Bill? Can this be simplified?

[Bill Hesse, 2012/11/14 13:33:30]

Done.

+  SECStatus status = SSL_ForceHandshake(memio_);
+  if (status == SECSuccess) {
+    if (in_handshake_) {
+      ThrowIfError(Dart_InvokeClosure(handshake_finish_, 0, NULL));
+      in_handshake_ = false;
+    }
+  } else {
+    PRErrorCode error = PR_GetError();
+    if (error == PR_WOULD_BLOCK_ERROR) {
+      if (!in_handshake_) {
+        ThrowIfError(Dart_InvokeClosure(handshake_start_, 0, NULL));
+        in_handshake_ = true;
+      }
+    } else {
+      ReportError("Unexpected handshake error", error);
+    }
+  }
+}
+
+
+void TlsFilter::Destroy() {
+  for (int i = 0; i < kNumBuffers; ++i) {
+    Dart_DeletePersistentHandle(dart_buffer_objects_[i]);
+  }
+  Dart_DeletePersistentHandle(stringStart_);
+  Dart_DeletePersistentHandle(stringLength_);
+  // TODO(whesse): Destroy OpenSSL objects here.

[Mads Ager (google), 2012/11/12 11:39:08]

OpenSSL?!?

I made this comment in my last round of comments as well.

[Bill Hesse, 2012/11/13 20:11:08]

Done.

+}
+
+
+intptr_t TlsFilter::ProcessBuffer(int buffer_index) {

[Søren Gjesse, 2012/11/12 12:04:57]

Maybe add a short comment on how the four buffers work together with the filter.

Thinking about it would it work if this was just called ProcessBuffers and
always processed all four buffers?

[Bill Hesse, 2012/11/13 20:11:08]

Because it is easiest to get an int return value back into dart, and then adjust
all the fields using that value, it is easiest to make it 4 separate calls.  We
also want to know individually if they succeed or not.

+  Dart_Handle buffer_object = dart_buffer_objects_[buffer_index];
+  Dart_Handle start_object = ThrowIfError(
+      Dart_GetField(buffer_object, stringStart_));
+  Dart_Handle length_object = ThrowIfError(
+      Dart_GetField(buffer_object, stringLength_));
+  int64_t unsafe_start = DartUtils::GetIntegerValue(start_object);
+  int64_t unsafe_length = DartUtils::GetIntegerValue(length_object);
+  if (unsafe_start < 0 || unsafe_start >= buffer_size_ ||

[Mads Ager (google), 2012/11/12 11:39:08]

What happens here if we get a bigint? Can that happen? Is the length
user-supplied here? I think GetIntegerValue will return 0 in that case in
release mode (it just asserts that it could get the value). You could use
GetInt64Value here which returns a boolean and checks that the integer fits in
64 bits.

However, we are in control of the sizes here aren't we? The Dart code should not
allow something that is larger than buffer_size_. So maybe an ASSERT here is
actually enough. This is a "can't happen" situation, right?

[Bill Hesse, 2012/11/13 20:11:08]

Yes, this is a "can't happen" situation.  We control these values in our Dart
implementation code.

On 2012/11/12 11:39:08, Mads Ager wrote:

+      unsafe_length < 0 || unsafe_length > buffer_size_) {
+     Dart_ThrowException(DartUtils::NewDartArgumentError(
+         "Illegal .start or .length on a _TlsExternalBuffer"));
+  }
+  intptr_t start = static_cast<intptr_t>(unsafe_start);
+  intptr_t length = static_cast<intptr_t>(unsafe_length);
+
+  int bytes_processed = 0;
+  switch (buffer_index) {
+    case kReadPlaintext: {
+      int bytes_free = buffer_size_ - start - length;
+      bytes_processed = PR_Read(memio_,

[Søren Gjesse, 2012/11/12 12:04:57]

Indentation.

[Bill Hesse, 2012/11/13 20:11:08]

Done.

+                           buffers_[buffer_index] + start + length,

[Mads Ager (google), 2012/11/12 11:39:08]

Indentation is off.

+                           bytes_free);
+      if (bytes_processed < 0) {
+        ASSERT(bytes_processed == -1);
+        // TODO(whesse): Handle unexpected errors here.
+        PRErrorCode pr_error = PR_GetError();
+        ASSERT(PR_WOULD_BLOCK_ERROR == pr_error);
+        bytes_processed = 0;
+      }
+      break;
+    }
+
+    case kWriteEncrypted: {
+      const uint8_t* buf1;
+      const uint8_t* buf2;
+      unsigned int len1;
+      unsigned int len2;
+      int bytes_free = buffer_size_ - start - length;
+      memio_Private* secret = memio_GetSecret(memio_);
+      memio_GetWriteParams(secret, &buf1, &len1, &buf2, &len2);
+      int bytes_to_send =
+          dart::Utils::Minimum(len1, static_cast<unsigned>(bytes_free));
+      if (bytes_to_send > 0) {
+        memmove(buffers_[buffer_index] + start + length, buf1, bytes_to_send);
+        bytes_processed = bytes_to_send;
+      }
+      bytes_to_send = dart::Utils::Minimum(len2,
+          static_cast<unsigned>(bytes_free - bytes_processed));
+      if (bytes_to_send > 0) {
+        memmove(buffers_[buffer_index] + start + length + bytes_processed, buf2,
+                bytes_to_send);
+        bytes_processed += bytes_to_send;
+      }
+      if (bytes_processed > 0) {
+        memio_PutWriteResult(secret, bytes_processed);
+      }
+      break;
+    }
+
+    case kReadEncrypted: {
+      if (length > 0) {
+        bytes_processed = length;
+        memio_Private* secret = memio_GetSecret(memio_);
+        uint8_t* memio_buf;
+        int free_bytes = memio_GetReadParams(secret, &memio_buf);
+        if (free_bytes < bytes_processed) bytes_processed = free_bytes;
+        memmove(memio_buf,
+                buffers_[buffer_index] + start,
+                bytes_processed);
+        memio_PutReadResult(secret, bytes_processed);
+      }
+      break;
+    }
+
+    case kWritePlaintext: {
+      if (length > 0) {
+        bytes_processed = PR_Write(memio_,

[Søren Gjesse, 2012/11/12 12:04:57]

Indentation.

[Bill Hesse, 2012/11/13 20:11:08]

Done.

+                              buffers_[buffer_index] + start,
+                              length);
+      }
+
+      if (bytes_processed < 0) {
+        ASSERT(bytes_processed == -1);
+        // TODO(whesse): Handle unexpected errors here.
+        PRErrorCode pr_error = PR_GetError();
+        ASSERT(PR_WOULD_BLOCK_ERROR == pr_error);
+        bytes_processed = 0;
+      }
+      break;
+    }
+  }
+  return bytes_processed;
+}