dart:io | Add SecureSocket.importPrivateCertificates, that reads a PKCS#12 file.

runtime/bin/secure_socket.cc

Index: runtime/bin/secure_socket.cc
diff --git a/runtime/bin/secure_socket.cc b/runtime/bin/secure_socket.cc
index ad6ae8209e48fbb292dd26ea8476b7a1ece759ec..d2d58182e641325e290503ea496c6a0c32fd02a3 100644
--- a/runtime/bin/secure_socket.cc
+++ b/runtime/bin/secure_socket.cc
@@ -10,9 +10,12 @@
 #include <stdio.h>
 #include <string.h>
 
+#include <certdb.h>
 #include <key.h>
 #include <keyt.h>
 #include <nss.h>
+#include <p12.h>
+#include <p12plcy.h>
 #include <pk11pub.h>
 #include <prerror.h>
 #include <prinit.h>
@@ -40,7 +43,7 @@ bool SSLFilter::library_initialized_ = false;
 dart::Mutex* SSLFilter::mutex_ = new dart::Mutex();
 // The password is needed when creating secure server sockets.  It can
 // be null if only secure client sockets are used.
-const char* SSLFilter::password_ = NULL;
+char* SSLFilter::password_ = NULL;
 
 // Forward declaration.
 static void ProcessFilter(Dart_Port dest_port_id,
@@ -105,6 +108,14 @@ static void SetFilter(Dart_NativeArguments args, SSLFilter* filter) {
 }
 
 

[Søren Gjesse, 2013/08/07 07:32:28]

Maybe move this function closer to where it is used.

[Bill Hesse, 2013/08/08 17:39:21]

I have had to keep moving it up and up, as I add uses of it.
And it is used in multiple places.  But I will move it to before its first use.

+char* PasswordCallback(PK11SlotInfo* slot, PRBool retry, void* arg) {
+  if (!retry) {
+    return PL_strdup(static_cast<char*>(arg));  // Freed by NSS internals.
+  }
+  return NULL;
+}
+
+
 void FUNCTION_NAME(SecureSocket_Init)(Dart_NativeArguments args) {
   Dart_EnterScope();
   Dart_Handle dart_this = ThrowIfError(Dart_GetNativeArgument(args, 0));
@@ -244,7 +255,7 @@ void FUNCTION_NAME(SecureSocket_InitializeLibrary)
                                       &certificate_database));
   } else if (!Dart_IsNull(certificate_database_object)) {
     Dart_ThrowException(DartUtils::NewDartArgumentError(
-        "Non-String certificate directory argument to SetCertificateDatabase"));
+        "SecureSocket.initialize: database argument is not a String or null"));
   }
   // Leave certificate_database as NULL if no value was provided.
 
@@ -259,7 +270,7 @@ void FUNCTION_NAME(SecureSocket_InitializeLibrary)
     password = "";
   } else {
     Dart_ThrowException(DartUtils::NewDartArgumentError(
-        "Password argument to SetCertificateDatabase is not a String or null"));
+        "SecureSocket.initialize: password argument is not a String or null"));
   }
 
   Dart_Handle builtin_roots_object =
@@ -270,10 +281,22 @@ void FUNCTION_NAME(SecureSocket_InitializeLibrary)
     ThrowIfError(Dart_BooleanValue(builtin_roots_object, &builtin_roots));
   } else {
     Dart_ThrowException(DartUtils::NewDartArgumentError(
-        "UseBuiltinRoots argument to SetCertificateDatabase is not a bool"));
+        "SecureSocket.initialize: useBuiltinRoots argument is not a bool"));
   }
 
-  SSLFilter::InitializeLibrary(certificate_database, password, builtin_roots);
+  Dart_Handle read_only_object =
+      ThrowIfError(Dart_GetNativeArgument(args, 3));
+  // Check that the type is boolean, and get the boolean value from it.
+  bool read_only = true;
+  if (Dart_IsBoolean(read_only_object)) {
+    ThrowIfError(Dart_BooleanValue(read_only_object, &read_only));
+  } else {
+    Dart_ThrowException(DartUtils::NewDartArgumentError(
+        "SecureSocket.initialize: readOnly argument is not a bool"));
+  }
+
+  SSLFilter::InitializeLibrary(
+      certificate_database, password, builtin_roots, read_only);
   Dart_ExitScope();
 }
 
@@ -363,6 +386,170 @@ void FUNCTION_NAME(SecureSocket_AddCertificate)
 }
 
 
+/*
+ * Called by the PKCS#12 decoder if a certificate's nickname collides with
+ * the nickname of a different existing certificate in the database.
+ */
+SECItem* nickname_callback(SECItem *old_nickname,
+                                     PRBool *cancel,
+                                     void *arg) {

[wtc, 2013/08/06 17:52:48]

Nit: fix the indentation of the second and third function arguments.

[Bill Hesse, 2013/08/08 17:39:21]

Done.

+  *cancel = PR_TRUE;
+  return NULL;
+}
+
+
+void FUNCTION_NAME(SecureSocket_ImportPrivateCertificates)

[wtc, 2013/08/06 17:52:48]

Certificates with corresponding private keys are called "user certificates" in
NSS. I don't think "user certificates" is standard terminology, but "private
certificates" isn't, either. Perhaps we can name this function
SecureSocket_ImportCertificatesAndPrivateKeys or
SecureSocket_ImportCertificatesWithPrivateKeys, if you think "user certificates"
is confusing?

[Bill Hesse, 2013/08/08 17:39:21]

Done.

+    (Dart_NativeArguments args) {
+  Dart_EnterScope();
+  Dart_Handle pk12_object = ThrowIfError(Dart_GetNativeArgument(args, 0));
+  if (!Dart_IsList(pk12_object)) {
+    Dart_ThrowException(DartUtils::NewDartArgumentError(
+        "SecureSocket.importPrivateCertificates: certificates is not a List"));
+  }
+
+  Dart_Handle password_object = ThrowIfError(Dart_GetNativeArgument(args, 1));
+  if (!Dart_IsString(password_object)) {
+    Dart_ThrowException(DartUtils::NewDartArgumentError(
+        "SecureSocket.importPrivateCertificates: password is not a String"));
+  }
+
+  intptr_t length;
+  ThrowIfError(Dart_ListLength(pk12_object, &length));
+  uint8_t* pk12 = Dart_ScopeAllocate(length + 1);  // Why +1 ?

[Søren Gjesse, 2013/08/07 07:32:28]

Yes, why + 1?

+  if (pk12 == NULL) {
+    FATAL("Out of memory in SecureSocket.importPrivateCertificates");
+  }
+  ThrowIfError(Dart_ListGetAsBytes(pk12_object, 0, pk12, length));
+
+  // A big-endian Unicode (UTF16) password.

[Søren Gjesse, 2013/08/07 07:32:28]

So the PKCS12 functions use a password in big-endian UTF16 stored in a SECItem
with type siBuffer? It is not possible to use type siUTF8String which seems more
logical? Probably not.

[Søren Gjesse, 2013/08/07 07:32:28]

Also you could consider building this in Dart code, but of cause now it is
already in C++.

[Bill Hesse, 2013/08/08 17:39:21]

The behavior depends on whether the host is big-endian or not, and we already
have that in Utils::HostToBigEndian16.

+  intptr_t password_length;
+  ThrowIfError(Dart_StringLength(password_object, &password_length));
+  password_length++;
+  uint16_t* password = reinterpret_cast<uint16_t*>(
+      Dart_ScopeAllocate(2 * password_length));
+  if (password == NULL) {
+    FATAL("Out of memory in SecureSocket.importPrivateCertificates");
+  }
+  intptr_t returned_length = password_length;
+  ThrowIfError(Dart_StringToUTF16(password_object, password, &returned_length));
+  ASSERT(password_length == returned_length + 1);
+  password[password_length - 1] = 0;
+  for (int i = 0; i < password_length; ++i) {
+    password[i] = Utils::HostToBigEndian16(password[i]);
+  }
+  SECItem p12_password;
+  p12_password.type = siBuffer;
+  p12_password.data = (unsigned char*)password;

[wtc, 2013/08/06 17:52:48]

Nit: reinterpret_cast<unsigned char*>(password)

[Bill Hesse, 2013/08/08 17:39:21]

Done.

+  p12_password.len = 2 * password_length;
+
+  Dart_SetReturnValue(args, Dart_Null());
+  // Set the password callback for the certificate database we are importing to.

[Søren Gjesse, 2013/08/07 07:32:28]

Please explain how this callback is used by nss. It seems to just return one of
its arguments.

[Bill Hesse, 2013/08/08 17:39:21]

 It returns a newly allocated copy of its argument.

This is built into NSS, that the password for a slot is gotten from a callback,
and it is freed by the callee.
It could always return the password, without using the arg, but this ensures
that the caller of the password callback has been given the password, and is
thus authorized.

+  PK11_SetPasswordFunc(PasswordCallback);
+  PK11SlotInfo* slot = PK11_GetInternalKeySlot();
+  SECStatus status = PK11_Authenticate(slot, PR_TRUE, SSLFilter::GetPassword());
+  if (status == SECFailure) {
+    ThrowPRException("CertificateException",
+                     "Could not authenticate to certificate database");
+  }
+
+  SEC_PKCS12DecoderContext* context = SEC_PKCS12DecoderStart(
+      &p12_password,
+      slot,
+      SSLFilter::GetPassword(),
+      NULL,
+      NULL,
+      NULL,
+      NULL,
+      NULL);
+  if (!context) {
+    FATAL("Unexpected error: SecureSocket.addPrivateCertificates DecoderStart");
+  }
+
+  if (SECSuccess == SEC_PKCS12DecoderUpdate(context, pk12, length) &&
+      SECSuccess == SEC_PKCS12DecoderVerify(context) &&
+      SECSuccess == SEC_PKCS12DecoderValidateBags(context, nickname_callback) &&
+      SECSuccess == SEC_PKCS12DecoderImportBags(context)) {
+    SEC_PKCS12DecoderFinish(context);

[Søren Gjesse, 2013/08/07 07:32:28]

"Hiding" the Dart_ExitScope here for the succesful return seems a bit dangerous.
How about:

if (SECSuccess != SEC_PKC... ||
    SECSuccess != SEC_PKC... ||
    ... ) {
  SEC_PKCS12DecoderFinish(context);
  // throw
}
SEC_PKCS12DecoderFinish(context);
Dart_ExitScope();

[Bill Hesse, 2013/08/08 17:39:21]

No more EnterScope and ExitScopes.

+    Dart_ExitScope();
+    return;
+  } else {

[wtc, 2013/08/06 17:52:48]

Nit: some projects have a convention of omitting "else" after a return
statement.
I am not sure if Dart follows this convention.

[Bill Hesse, 2013/08/08 17:39:21]

Code rewritten differently.

+    SEC_PKCS12DecoderFinish(context);
+    ThrowPRException("CertificateException",
+                     "Could not import PKCS#12 file");
+  }

[Søren Gjesse, 2013/08/07 07:32:28]

Nice use of Dart_ScopeAllocate - Only the context to "free" before returning.

The "slot" does not need "freeing/releasing"?

How about the PK11_SetPasswordFunc - should that be reset?

[Bill Hesse, 2013/08/08 17:39:21]

The password callback stays the same for the entire process.

+}
+
+
+void FUNCTION_NAME(SecureSocket_ChangeTrust)(Dart_NativeArguments args) {
+  Dart_EnterScope();
+  Dart_Handle nickname_object = ThrowIfError(Dart_GetNativeArgument(args, 0));
+  if (!Dart_IsString(nickname_object)) {
+    Dart_ThrowException(DartUtils::NewDartArgumentError(
+        "SecureSocket.changeTrust: nickname argument is not a String"));
+  }
+  const char* nickname;
+  ThrowIfError(Dart_StringToCString(nickname_object, &nickname));
+
+  Dart_Handle trust_object = ThrowIfError(Dart_GetNativeArgument(args, 1));
+  if (!Dart_IsString(trust_object)) {
+    Dart_ThrowException(DartUtils::NewDartArgumentError(
+        "SecureSocket.changeTrust: trust argument is not a String"));
+  }
+  const char* trust_string;
+  ThrowIfError(Dart_StringToCString(trust_object, &trust_string));
+
+  CERTCertificate* certificate =
+      PK11_FindCertFromNickname(nickname, SSLFilter::GetPassword());
+  if (certificate == NULL) {
+    ThrowCertificateException("Cannot find certificate with nickname %s",
+                              nickname);
+  }
+  CERTCertTrust trust;
+  SECStatus status = CERT_DecodeTrustString(&trust, trust_string);
+  if (status != SECSuccess) {
+    ThrowPRException("CertificateException", "Trust string cannot be decoded");

[wtc, 2013/08/06 17:52:48]

If these ThrowPRException calls also return from the function, you need to
make sure CERT_DestroyCertificate(certificate) is called.

[Bill Hesse, 2013/08/08 17:39:21]

Done.

+  }
+  status = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(), certificate, &trust);
+  if (status != SECSuccess) {
+    ThrowCertificateException("Cannot set trust on certificate %s", nickname);
+  }
+
+  CERT_DestroyCertificate(certificate);
+  Dart_ExitScope();
+}
+
+
+void FUNCTION_NAME(SecureSocket_RemoveCertificate)(Dart_NativeArguments args) {
+  Dart_EnterScope();
+  Dart_Handle nickname_object =
+      ThrowIfError(Dart_GetNativeArgument(args, 0));
+  if (!Dart_IsString(nickname_object)) {
+    Dart_ThrowException(DartUtils::NewDartArgumentError(
+        "SecureSocket.removeCertificate: nickname is not a String"));
+  }
+  const char* nickname;
+  ThrowIfError(Dart_StringToCString(nickname_object, &nickname));
+
+  CERTCertificate* certificate =
+      PK11_FindCertFromNickname(nickname, SSLFilter::GetPassword());
+  if (certificate == NULL) {
+    ThrowCertificateException("Cannot find certificate with nickname %s",
+                              nickname);
+  }
+  SECKEYPrivateKey* key =
+      PK11_FindKeyByAnyCert(certificate, SSLFilter::GetPassword());
+  // Free the copy returned from FindKeyByAnyCert.
+  SECKEY_DestroyPrivateKey(key);
+  SECStatus status = (key == NULL) ?
+      SEC_DeletePermCertificate(certificate) :
+      PK11_DeleteTokenCertAndKey(certificate, SSLFilter::GetPassword());
+  if (status == SECFailure) {
+    CERT_DestroyCertificate(certificate);
+    ThrowCertificateException("Cannot remove certificate %s", nickname);
+  }

[wtc, 2013/08/06 17:52:48]

CERT_DestroyCertificate(certificate) should also be called when
the certificate in the database is successfully deleted. So this
if statement should be rewritten as:

  CERT_DestroyCertificate(certificate);
  if (status == SECFailure) {
    ThrowCertificateException("Cannot remove certificate %s", nickname);
  }

Nit: in general, it is better to test != SECSuccess than == SECFailure
because SECSuccess is 0. This is old school C programmer's optimization :-)

[Bill Hesse, 2013/08/08 17:39:21]

Done.

+  Dart_ExitScope();
+}
+
 
 void FUNCTION_NAME(SecureSocket_PeerCertificate)
     (Dart_NativeArguments args) {
@@ -516,7 +703,7 @@ bool SSLFilter::ProcessAllBuffers(int starts[kNumBuffers],
 
 void SSLFilter::Init(Dart_Handle dart_this) {
   if (!library_initialized_) {
-    InitializeLibrary(NULL, "", true, false);
+    InitializeLibrary(NULL, "", true, true, false);
   }
   ASSERT(string_start_ == NULL);
   string_start_ = Dart_NewPersistentHandle(DartUtils::NewString("start"));
@@ -589,14 +776,6 @@ void SSLFilter::RegisterBadCertificateCallback(Dart_Handle callback) {
 }
 
 
-char* PasswordCallback(PK11SlotInfo* slot, PRBool retry, void* arg) {
-  if (!retry) {
-    return PL_strdup(static_cast<char*>(arg));  // Freed by NSS internals.
-  }
-  return NULL;
-}
-
-
 static const char* builtin_roots_module =
 #if defined(TARGET_OS_LINUX) || defined(TARGET_OS_ANDROID)
     "name=\"Root Certs\" library=\"libnssckbi.so\"";
@@ -613,6 +792,7 @@ static const char* builtin_roots_module =
 void SSLFilter::InitializeLibrary(const char* certificate_database,
                                   const char* password,
                                   bool use_builtin_root_certificates,
+                                  bool read_only,
                                   bool report_duplicate_initialization) {
   MutexLocker locker(mutex_);
   SECStatus status;
@@ -636,7 +816,7 @@ void SSLFilter::InitializeLibrary(const char* certificate_database,
         }
       }
     } else {
-      PRUint32 init_flags = NSS_INIT_READONLY;
+      PRUint32 init_flags = read_only ? NSS_INIT_READONLY : 0;
       if (!use_builtin_root_certificates) {
         init_flags |= NSS_INIT_NOMODDB;
       }
@@ -661,6 +841,16 @@ void SSLFilter::InitializeLibrary(const char* certificate_database,
       ThrowPRException("TlsException",
                        "Failed NSS_SetDomesticPolicy call.");
     }
+
+    // Allow encoding and decoding of private keys in PKCS#12 files (.pk files).

[wtc, 2013/08/06 17:52:48]

Common file suffixes for PKCS #11 files are .p12 and .pfx (used by Microsoft).
Some Linux apps use .pkcs12. I have not seen the .pk file suffix before.

+    SEC_PKCS12EnableCipher(PKCS12_RC4_40, 1);
+    SEC_PKCS12EnableCipher(PKCS12_RC4_128, 1);
+    SEC_PKCS12EnableCipher(PKCS12_RC2_CBC_40, 1);
+    SEC_PKCS12EnableCipher(PKCS12_RC2_CBC_128, 1);
+    SEC_PKCS12EnableCipher(PKCS12_DES_56, 1);
+    SEC_PKCS12EnableCipher(PKCS12_DES_EDE3_168, 1);
+    SEC_PKCS12SetPreferredCipher(PKCS12_DES_EDE3_168, 1);

[wtc, 2013/08/06 17:52:48]

I know these function calls are modeled after the NSS pk12util tool.
But many of these ciphers are weak or old. I suggest you start with just

    SEC_PKCS12EnableCipher(PKCS12_DES_EDE3_168, 1);
    SEC_PKCS12SetPreferredCipher(PKCS12_DES_EDE3_168, 1);

(PKCS12_DES_EDE3_168 is Triple DES.)

This will help us discover whether any of the weak or old cipher suites
is actually needed for Dart.

Also, move this block of code before the NSS_SetDomesticPolicy() call.
NSS_SetDomesticPolicy is actually a libSSL function, so it is better for
it to stay with the other SSL_xxx calls.

[Bill Hesse, 2013/08/08 17:39:21]

Done.

+
     // Enable TLS, as well as SSL3 and SSL2.
     status = SSL_OptionSetDefault(SSL_ENABLE_TLS, PR_TRUE);
     if (status != SECSuccess) {
@@ -751,23 +941,20 @@ void SSLFilter::Connect(const char* host_name,
           const_cast<char*>(certificate_name));
       if (certificate == NULL) {
         ThrowCertificateException(
-            "Cannot find server certificate by distinguished name: %s",
+            "Cannot find server certificate with distinguished name %s",
             certificate_name);
       }
     } else {
       // Look up certificate using the nickname certificate_name.
       certificate = PK11_FindCertFromNickname(
-          const_cast<char*>(certificate_name),
-          static_cast<void*>(const_cast<char*>(password_)));
+          const_cast<char*>(certificate_name), GetPassword());
       if (certificate == NULL) {
         ThrowCertificateException(
-            "Cannot find server certificate by nickname: %s",
+            "Cannot find server certificate with nickname %s",
             certificate_name);
       }
     }
-    SECKEYPrivateKey* key = PK11_FindKeyByAnyCert(
-        certificate,
-        static_cast<void*>(const_cast<char*>(password_)));
+    SECKEYPrivateKey* key = PK11_FindKeyByAnyCert(certificate, GetPassword());

[wtc, 2013/08/06 17:52:48]

You don't need to pass the password as the "void *wincx" argument to
PK11_FindCertFromNickname and PK11_FindKeyByAnyCert.  "wincx" stands for
"window context", and is supposed to be the window handle for the password
dialog. You can pass NULL as the "void *wincx" argument.

Your 'PasswordCallback' function can just duplicate the static data member
SSLFilter::password_ and return the copy.

Of course, the current method also works.

     if (key == NULL) {
       CERT_DestroyCertificate(certificate);
       if (PR_GetError() == -8177) {