Bug 1705119 Deadlock when using gcm and non-thread safe tokens.

1) Add code to treat softokn as a non-threadsafe module.
2) Add a cycle to test ssl against non-threadafe modules.
3) Fix deadlock by restricting the ContextMonitor to only be active around PKCS #11 function calls.

Differential Revision: https://phabricator.services.mozilla.com/D112092

4 files changed, 78 insertions, 9 deletions

diff --git a/lib/pk11wrap/pk11cxt.c b/lib/pk11wrap/pk11cxt.c
index e189716a9..b1569ebf9 100644
--- a/lib/pk11wrap/pk11cxt.c
+++ b/lib/pk11wrap/pk11cxt.c
@@ -173,7 +173,9 @@ pk11_contextInitMessage(PK11Context *context, CK_MECHANISM_PTR mech,
      * if those cases */
     if ((version.major >= 3) &&
         PK11_DoesMechanismFlag(slot, (mech)->mechanism, flags)) {
+        PK11_EnterContextMonitor(context);
         crv = (*initFunc)((context)->session, (mech), (key)->objectID);
+        PK11_ExitContextMonitor(context);
         if ((crv == CKR_FUNCTION_NOT_SUPPORTED) ||
             (crv == CKR_MECHANISM_INVALID)) {
             /* we have a 3.0 interface, and the flag was set (or ignored)
@@ -201,29 +203,41 @@ pk11_context_init(PK11Context *context, CK_MECHANISM *mech_info)
     context->simulate_message = PR_FALSE;
     switch (context->operation) {
         case CKA_ENCRYPT:
+            PK11_EnterContextMonitor(context);
             crv = PK11_GETTAB(context->slot)->C_EncryptInit(context->session, mech_info, symKey->objectID);
+            PK11_ExitContextMonitor(context);
             break;
         case CKA_DECRYPT:
+            PK11_EnterContextMonitor(context);
             if (context->fortezzaHack) {
                 CK_ULONG count = 0;
                 /* generate the IV for fortezza */
                 crv = PK11_GETTAB(context->slot)->C_EncryptInit(context->session, mech_info, symKey->objectID);
-                if (crv != CKR_OK)
+                if (crv != CKR_OK) {
+                    PK11_ExitContextMonitor(context);
                     break;
+                }
                 PK11_GETTAB(context->slot)
                     ->C_EncryptFinal(context->session,
                                      NULL, &count);
             }
             crv = PK11_GETTAB(context->slot)->C_DecryptInit(context->session, mech_info, symKey->objectID);
+            PK11_ExitContextMonitor(context);
             break;
         case CKA_SIGN:
+            PK11_EnterContextMonitor(context);
             crv = PK11_GETTAB(context->slot)->C_SignInit(context->session, mech_info, symKey->objectID);
+            PK11_ExitContextMonitor(context);
             break;
         case CKA_VERIFY:
+            PK11_EnterContextMonitor(context);
             crv = PK11_GETTAB(context->slot)->C_SignInit(context->session, mech_info, symKey->objectID);
+            PK11_ExitContextMonitor(context);
             break;
         case CKA_DIGEST:
+            PK11_EnterContextMonitor(context);
             crv = PK11_GETTAB(context->slot)->C_DigestInit(context->session, mech_info);
+            PK11_ExitContextMonitor(context);
             break;
 
         case CKA_NSS_MESSAGE | CKA_ENCRYPT:
@@ -272,12 +286,14 @@ pk11_context_init(PK11Context *context, CK_MECHANISM *mech_info)
      * handle session starvation case.. use our last session to multiplex
      */
     if (!context->ownSession) {
+        PK11_EnterContextMonitor(context);
         context->savedData = pk11_saveContext(context, context->savedData,
                                               &context->savedLength);
         if (context->savedData == NULL)
             rv = SECFailure;
         /* clear out out session for others to use */
         pk11_Finalize(context);
+        PK11_ExitContextMonitor(context);
     }
     return rv;
 }
@@ -396,9 +412,7 @@ pk11_CreateNewContextInSlot(CK_MECHANISM_TYPE type,
     mech_info.mechanism = type;
     mech_info.pParameter = param->data;
     mech_info.ulParameterLen = param->len;
-    PK11_EnterContextMonitor(context);
     rv = pk11_context_init(context, &mech_info);
-    PK11_ExitContextMonitor(context);
 
     if (rv != SECSuccess) {
         PK11_DestroyContext(context, PR_TRUE);
@@ -703,12 +717,12 @@ PK11_DigestBegin(PK11Context *cx)
      */
     PK11_EnterContextMonitor(cx);
     pk11_Finalize(cx);
+    PK11_ExitContextMonitor(cx);
 
     mech_info.mechanism = cx->type;
     mech_info.pParameter = cx->param->data;
     mech_info.ulParameterLen = cx->param->len;
     rv = pk11_context_init(cx, &mech_info);
-    PK11_ExitContextMonitor(cx);
 
     if (rv != SECSuccess) {
         return SECFailure;
diff --git a/lib/pk11wrap/pk11load.c b/lib/pk11wrap/pk11load.c
index 9e7a0a546..2bc41dbc4 100644
--- a/lib/pk11wrap/pk11load.c
+++ b/lib/pk11wrap/pk11load.c
@@ -528,7 +528,10 @@ secmod_LoadPKCS11Module(SECMODModule *mod, SECMODModule **oldModule)
     }
 #endif
 
-    mod->isThreadSafe = PR_TRUE;
+    /* This test operation makes sure our locking system is
+     * consistent even if we are using non-thread safe tokens by
+     * simulating unsafe tokens with safe ones. */
+    mod->isThreadSafe = !PR_GetEnvSecure("NSS_FORCE_TOKEN_LOCK");
 
     /* Now we initialize the module */
     rv = secmod_ModuleInit(mod, oldModule, &alreadyLoaded);
diff --git a/lib/pk11wrap/pk11skey.c b/lib/pk11wrap/pk11skey.c
index f724fe9b7..66b4ed6a1 100644
--- a/lib/pk11wrap/pk11skey.c
+++ b/lib/pk11wrap/pk11skey.c
@@ -368,6 +368,7 @@ PK11_GetWrapKey(PK11SlotInfo *slot, int wrap, CK_MECHANISM_TYPE type,
                 int series, void *wincx)
 {
     PK11SymKey *symKey = NULL;
+    CK_OBJECT_HANDLE keyHandle;
 
     PK11_EnterSlotMonitor(slot);
     if (slot->series != series ||
@@ -380,9 +381,10 @@ PK11_GetWrapKey(PK11SlotInfo *slot, int wrap, CK_MECHANISM_TYPE type,
         type = slot->wrapMechanism;
     }
 
-    symKey = PK11_SymKeyFromHandle(slot, NULL, PK11_OriginDerive,
-                                   slot->wrapMechanism, slot->refKeys[wrap], PR_FALSE, wincx);
+    keyHandle = slot->refKeys[wrap];
     PK11_ExitSlotMonitor(slot);
+    symKey = PK11_SymKeyFromHandle(slot, NULL, PK11_OriginDerive,
+                                   slot->wrapMechanism, keyHandle, PR_FALSE, wincx);
     return symKey;
 }
 
diff --git a/tests/all.sh b/tests/all.sh
index 99a7fd639..60cd99fdd 100755
--- a/tests/all.sh
+++ b/tests/all.sh
@@ -55,6 +55,9 @@
 #   sharedb      - run test suites with shareable database format
 #                  enabled (databases are created directly to this
 #                  format). This is the default and doesn't need to be run separately.
+#   threadunsafe - run test suites with thread unsafe environment variable
+#                  so simulate running NSS locking for PKCS #11 modules which
+#                  are not thread safe.
 #
 # Mandatory environment variables (to be set before testing):
 # -----------------------------------------------------------
@@ -75,6 +78,7 @@
 #   NSS_TESTS      - list of all test suites to run (separated by space
 #                    character, without trailing .sh)
 #                  - this list can be reduced for individual test cycles
+#   NSS_THREAD_TESTS - list of test suites run in the threadunsafe cycle
 #
 #   NSS_SSL_TESTS  - list of ssl tests to run (see ssl.sh)
 #   NSS_SSL_RUN    - list of ssl sub-tests to run (see ssl.sh)
@@ -83,7 +87,7 @@
 # ---------------
 #                           all.sh                       ~  (main)
 #                              |                               |
-#          +------------+------------+-----------+       ~  run_cycles
+#          +------------+------------+-----------+---    ~  run_cycles
 #          |            |            |           |             |
 #      standard       pkix       upgradedb     sharedb   ~  run_cycle_*
 #         ...           |           ...         ...            |
@@ -249,6 +253,37 @@ run_cycle_shared_db()
     run_tests
 }
 
+########################## run_thread_unsafe #########################
+# run test suites with an non-thread safe softoken
+# This simulates loading a non-threadsafe PKCS #11 module and makes
+# Sure we don't have any deadlocks in our locking code
+########################################################################
+run_cycle_thread_unsafe()
+{
+    TEST_MODE=THREAD_UNSAFE
+
+    TABLE_ARGS="bgcolor=lightgray"
+    html_head "Testing with non-threadsafe softoken"
+    html "</TABLE><BR>"
+
+    HOSTDIR="${HOSTDIR}/threadunsafe"
+    mkdir -p "${HOSTDIR}"
+    init_directories
+
+    NSS_FORCE_TOKEN_LOCK=1
+    export NSS_FORCE_TOKEN_LOCK
+
+    # run the tests for appropriate for thread unsafe
+    # basically it's the ssl tests right now. 
+    TESTS="${THREAD_TESTS}"
+    TESTS_SKIP="dbupgrade"
+
+    export -n NSS_SSL_TESTS
+    export -n NSS_SSL_RUN
+
+    run_tests
+}
+
 ############################# run_cycles ###############################
 # run test cycles defined in CYCLES variable
 ########################################################################
@@ -271,6 +306,9 @@ run_cycles()
         "sharedb")
             run_cycle_shared_db
             ;;
+        "threadunsafe")
+            run_cycle_thread_unsafe
+            ;;
         esac
         . ${ENV_BACKUP}
     done
@@ -288,7 +326,7 @@ if [ -z "${INIT_SOURCED}" -o "${INIT_SOURCED}" != "TRUE" ]; then
     . ./init.sh
 fi
 
-cycles="standard pkix"
+cycles="standard pkix threadunsafe"
 CYCLES=${NSS_CYCLES:-$cycles}
 
 NO_INIT_SUPPORT=`certutil --build-flags |grep -cw NSS_NO_INIT_SUPPORT`
@@ -297,6 +335,7 @@ if [ $NO_INIT_SUPPORT -eq 0 ]; then
 fi
 
 tests="cipher lowhash libpkix cert dbtests tools $RUN_FIPS sdr crmf smime ssl ocsp merge pkits ec gtests ssl_gtests policy"
+thread_tests="ssl ssl_gtests"
 # Don't run chains tests when we have a gyp build.
 if [ "$OBJDIR" != "Debug" -a "$OBJDIR" != "Release" ]; then
   tests="$tests chains"
@@ -304,6 +343,17 @@ fi
 TESTS=${NSS_TESTS:-$tests}
 
 ALL_TESTS=${TESTS}
+default_thread=""
+for i in ${ALL_TESTS}
+do
+    for j in ${thread_tests}
+    do
+        if [ $i = $j ]; then 
+            default_thread="$default_thread $i"
+        fi
+    done
+done
+THREAD_TESTS=${NSS_THREAD_TESTS-$default_thread}
 
 nss_ssl_tests="crl iopr policy normal_normal"
 if [ $NO_INIT_SUPPORT -eq 0 ]; then