b=79153 r=javi sr=blizzard

Crypto key generation feedback, prevent crash, cleanup code, add a call from second code path.

security/manager/ssl/src/nsCrypto.cpp

@@ -23,6 +23,8 @@
 #include "nsNSSComponent.h"
 #include "nsCrypto.h"
 #include "nsKeygenHandler.h"
+#include "nsKeygenThread.h"
+#include "nsINSSDialogs.h"
 #include "nsNSSCertificate.h"
 #include "nsPKCS12Blob.h"
 #include "nsPK11TokenDB.h"
@@ -595,6 +597,10 @@ cryptojs_generateOneKeyPair(JSContext *cx, nsKeyPairInfo *keyPairInfo,
                             PK11SlotInfo *slot, PRBool willEscrow)
                             
 {
+  nsIGeneratingKeypairInfoDialogs * dialogs;
+  nsKeygenThread *KeygenRunnable = 0;
+  nsCOMPtr<nsIKeygenThread> runnable;
+
   PRUint32 mechanism = cryptojs_convert_to_mechanism(keyPairInfo->keyGenType);
   void *keyGenParams = nsConvertToActualKeyGenParams(mechanism, params, 
                                                      (params) ? nsCRT::strlen(params):0, 
@@ -634,11 +640,46 @@ cryptojs_generateOneKeyPair(JSContext *cx, nsKeyPairInfo *keyPairInfo,
     isPerm = PR_TRUE;
   }
 
-  keyPairInfo->privKey = PK11_GenerateKeyPair(slot, mechanism, keyGenParams,
+  rv = getNSSDialogs((void**)&dialogs,
-                                              &keyPairInfo->pubKey, isPerm, 
+                     NS_GET_IID(nsIGeneratingKeypairInfoDialogs));
-                                              isPerm, uiCxt);
+
+  if (NS_SUCCEEDED(rv)) {
+    KeygenRunnable = new nsKeygenThread();
+    if (KeygenRunnable) {
+      NS_ADDREF(KeygenRunnable);
+    }
+  }
+
+  if (NS_FAILED(rv) || !KeygenRunnable) {
+    rv = NS_OK;
+    keyPairInfo->privKey = PK11_GenerateKeyPair(slot, mechanism, keyGenParams,
+                                                &keyPairInfo->pubKey, isPerm, 
+                                                isPerm, uiCxt);
+  } else {
+    KeygenRunnable->SetParams( slot, mechanism, keyGenParams, isPerm, isPerm, uiCxt );
+
+    runnable = do_QueryInterface(KeygenRunnable);
+
+    if (runnable) {
+      rv = dialogs->DisplayGeneratingKeypairInfo(uiCxt, runnable);
+
+      // We call join on the thread, 
+      // so we can be sure that no simultaneous access to the passed parameters will happen.
+      KeygenRunnable->Join();
+
+      NS_RELEASE(dialogs);
+      if (!NS_FAILED(rv)) {
+        rv = KeygenRunnable->GetParams(&keyPairInfo->privKey, &keyPairInfo->pubKey);
+      }
+    }
+  }
+
   nsFreeKeyGenParams(mechanism, keyGenParams);
 
+  if (KeygenRunnable) {
+    NS_RELEASE(KeygenRunnable);
+  }
+
   if (!keyPairInfo->privKey || !keyPairInfo->pubKey) {
     if (intSlot)
       PK11_FreeSlot(intSlot);

security/manager/ssl/src/nsKeygenHandler.cpp

@@ -358,7 +358,7 @@ nsKeygenFormProcessor::GetPublicKey(nsString& aValue, nsString& aChallenge,
     CERTPublicKeyAndChallenge pkac;
     SECKeySizeChoiceInfo *choice = SECKeySizeChoiceList;
     nsIGeneratingKeypairInfoDialogs * dialogs;
-    nsKeygenThread KeygenRunnable;
+    nsKeygenThread *KeygenRunnable = 0;
     nsCOMPtr<nsIKeygenThread> runnable;
 
     // Get the key size //
@@ -440,40 +440,37 @@ found_match:
     rv = getNSSDialogs((void**)&dialogs,
                        NS_GET_IID(nsIGeneratingKeypairInfoDialogs));
 
-    if (NS_FAILED(rv)) {
+    if (NS_SUCCEEDED(rv)) {
+        KeygenRunnable = new nsKeygenThread();
+        if (KeygenRunnable) {
+            NS_ADDREF(KeygenRunnable);
+        }
+    }
+
+    if (NS_FAILED(rv) || !KeygenRunnable) {
+        rv = NS_OK;
         privateKey = PK11_GenerateKeyPair(slot, keyGenMechanism, params,
                                           &publicKey, PR_TRUE, PR_TRUE, nsnull);
     } else {
-        GenerateKeypairParameters gkp;
+        KeygenRunnable->SetParams( slot, keyGenMechanism, params, PR_TRUE, PR_TRUE, nsnull );
-        gkp.privateKey = nsnull;
-        gkp.publicKey = nsnull;
-        gkp.slot = slot;
-        gkp.keyGenMechanism = keyGenMechanism;
-        gkp.params = params;
-        KeygenRunnable.SetParams(&gkp);
-        // Our parameters instance will be modified by the thread.
 
-        runnable = do_QueryInterface(&KeygenRunnable);
+        runnable = do_QueryInterface(KeygenRunnable);
         
         if (runnable) {
             rv = dialogs->DisplayGeneratingKeypairInfo(m_ctx, runnable);
 
-            // We call join on the thread,
+            // We call join on the thread, 
-            // so we can be sure that no simultaneous access will happen.
+            // so we can be sure that no simultaneous access to the passed parameters will happen.
-            KeygenRunnable.Join();
+            KeygenRunnable->Join();
 
             NS_RELEASE(dialogs);
             if (!NS_FAILED(rv)) {
-                privateKey = gkp.privateKey;
+                rv = KeygenRunnable->GetParams(&privateKey, &publicKey);
-                publicKey = gkp.publicKey;
-                slot = gkp.slot;
-                keyGenMechanism = gkp.keyGenMechanism;
-                params = gkp.params;
             }
         }
     }
     
-    if (!privateKey) {
+    if (NS_FAILED(rv) || !privateKey) {
         goto loser;
     }
     // just in case we'll need to authenticate to the db -jp //
@@ -547,6 +544,9 @@ loser:
     if (slot != nsnull) {
         PK11_FreeSlot(slot);
     }
+    if (KeygenRunnable) {
+      NS_RELEASE(KeygenRunnable);
+    }
     return rv;
 }
 

security/manager/ssl/src/nsKeygenThread.cpp

@@ -32,25 +32,94 @@ nsKeygenThread::nsKeygenThread()
  iAmRunning(PR_FALSE),
  keygenReady(PR_FALSE),
  statusDialogClosed(PR_FALSE),
- threadHandle(nsnull),
+ alreadyReceivedParams(PR_FALSE),
- params(nsnull)
+ privateKey(nsnull),
+ publicKey(nsnull),
+ slot(nsnull),
+ keyGenMechanism(0),
+ params(nsnull),
+ isPerm(PR_FALSE),
+ isSensitive(PR_FALSE),
+ wincx(nsnull),
+ threadHandle(nsnull)
 {
+  NS_INIT_ISUPPORTS();
   mutex = PR_NewLock();
 }
 
 nsKeygenThread::~nsKeygenThread()
 {
-  if (mutex)
+  if (mutex) {
     PR_DestroyLock(mutex);
+  }
+  
+  if (statusDialogPtr) {
+    NS_RELEASE(statusDialogPtr);
+  }
 }
 
-void nsKeygenThread::SetParams(GenerateKeypairParameters *p)
+void nsKeygenThread::SetParams(
+    PK11SlotInfo *a_slot,
+    PRUint32 a_keyGenMechanism,
+    void *a_params,
+    PRBool a_isPerm,
+    PRBool a_isSensitive,
+    void *a_wincx )
 {
   PR_Lock(mutex);
-  params = p;
+ 
+    if (!alreadyReceivedParams) {
+      alreadyReceivedParams = PR_TRUE;
+      if (a_slot) {
+        slot = PK11_ReferenceSlot(a_slot);
+      }
+      else {
+        slot = nsnull;
+      }
+      keyGenMechanism = a_keyGenMechanism;
+      params = a_params;
+      isPerm = a_isPerm;
+      isSensitive = a_isSensitive;
+      wincx = a_wincx;
+    }
+
   PR_Unlock(mutex);
 }
 
+nsresult nsKeygenThread::GetParams(
+    SECKEYPrivateKey **a_privateKey,
+    SECKEYPublicKey **a_publicKey)
+{
+  if (!a_privateKey || !a_publicKey) {
+    return NS_ERROR_FAILURE;
+  }
+
+  nsresult rv;
+  
+  PR_Lock(mutex);
+  
+    // GetParams must not be called until thread creator called
+    // Join on this thread.
+    NS_ASSERTION(keygenReady, "logic error in nsKeygenThread::GetParams");
+
+    if (keygenReady) {
+      *a_privateKey = privateKey;
+      *a_publicKey = publicKey;
+
+      privateKey = 0;
+      publicKey = 0;
+      
+      rv = NS_OK;
+    }
+    else {
+      rv = NS_ERROR_FAILURE;
+    }
+  
+  PR_Unlock(mutex);
+  
+  return rv;
+}
+
 static void PR_CALLBACK nsKeygenThreadRunner(void *arg)
 {
   nsKeygenThread *self = NS_STATIC_CAST(nsKeygenThread *, arg);
@@ -78,15 +147,15 @@ nsresult nsKeygenThread::StartKeyGeneration(nsIDOMWindowInternal *statusDialog)
 
   PR_Lock(mutex);
 
-    statusDialogPtr = wi;
-    NS_ADDREF(statusDialogPtr);
-    wi = 0;
-
     if (iAmRunning || keygenReady) {
       PR_Unlock(mutex);
       return NS_OK;
     }
 
+    statusDialogPtr = wi;
+    NS_ADDREF(statusDialogPtr);
+    wi = 0;
+
     iAmRunning = PR_TRUE;
 
     threadHandle = PR_CreateThread(PR_USER_THREAD, nsKeygenThreadRunner, NS_STATIC_CAST(void*, this), 
@@ -119,8 +188,6 @@ nsresult nsKeygenThread::UserCanceled(PRBool *threadAlreadyClosedDialog)
     // it again to avoid problems.
     statusDialogClosed = PR_TRUE;
 
-    NS_RELEASE(statusDialogPtr);
-
   PR_Unlock(mutex);
 
   return NS_OK;
@@ -128,22 +195,21 @@ nsresult nsKeygenThread::UserCanceled(PRBool *threadAlreadyClosedDialog)
 
 void nsKeygenThread::Run(void)
 {
-  GenerateKeypairParameters *p = 0;
+  PRBool canGenerate = PR_FALSE;
 
   PR_Lock(mutex);
 
-    if (params) {
+    if (alreadyReceivedParams) {
-      p = params;
+      canGenerate = PR_TRUE;
-      // Make sure it's impossible that will use the same parameters again.
+      keygenReady = PR_FALSE;
-      params = 0;
     }
 
   PR_Unlock(mutex);
 
-  if (p)
+  if (canGenerate)
-    p->privateKey = PK11_GenerateKeyPair(p->slot, p->keyGenMechanism,
+    privateKey = PK11_GenerateKeyPair(slot, keyGenMechanism,
-                                         p->params, &p->publicKey,
+                                         params, &publicKey,
-                                         PR_TRUE, PR_TRUE, nsnull);
+                                         isPerm, isSensitive, wincx);
   
   // This call gave us ownership over privateKey and publicKey.
   // But as the params structure is owner by our caller,
@@ -158,6 +224,15 @@ void nsKeygenThread::Run(void)
     keygenReady = PR_TRUE;
     iAmRunning = PR_FALSE;
 
+    // forget our parameters
+    if (slot) {
+      PK11_FreeSlot(slot);
+      slot = 0;
+    }
+    keyGenMechanism = 0;
+    params = 0;
+    wincx = 0;
+
     if (!statusDialogClosed)
       windowToClose = statusDialogPtr;
 

security/manager/ssl/src/nsKeygenThread.h

@@ -26,15 +26,6 @@
 
 #include "nsIKeygenThread.h"
 
-struct GenerateKeypairParameters
-{
-  SECKEYPrivateKey *privateKey;
-  SECKEYPublicKey *publicKey;
-  PK11SlotInfo *slot;
-  PRUint32 keyGenMechanism;
-  void *params;
-};
-
 class nsKeygenThread : public nsIKeygenThread
 {
 private:
@@ -45,11 +36,19 @@ private:
   PRBool iAmRunning;
   PRBool keygenReady;
   PRBool statusDialogClosed;
-  
+  PRBool alreadyReceivedParams;
+
+  SECKEYPrivateKey *privateKey;
+  SECKEYPublicKey *publicKey;
+  PK11SlotInfo *slot;
+  PRUint32 keyGenMechanism;
+  void *params;
+  PRBool isPerm;
+  PRBool isSensitive;
+  void *wincx;
+
   PRThread *threadHandle;
   
-  GenerateKeypairParameters *params;
-
 public:
   nsKeygenThread();
   virtual ~nsKeygenThread();
@@ -57,10 +56,17 @@ public:
   NS_DECL_NSIKEYGENTHREAD
   NS_DECL_ISUPPORTS
 
-  // This transfers a reference of parameters to the thread
+  void SetParams(
-  // The parameters will not be copied, the caller keeps ownership.
+    PK11SlotInfo *a_slot,
-  // Once the thread is finished, the original instance will be accessed 
+    PRUint32 a_keyGenMechanism,
-  void SetParams(GenerateKeypairParameters *p);
+    void *a_params,
+    PRBool a_isPerm,
+    PRBool a_isSensitive,
+    void *a_wincx );
+
+  nsresult GetParams(
+    SECKEYPrivateKey **a_privateKey,
+    SECKEYPublicKey **a_publicKey);
   
   void Join(void);