From db96cc1317ed922c1f9cb089c15d1f1a2762c19a Mon Sep 17 00:00:00 2001 From: Robert Relyea Date: Wed, 14 Apr 2021 10:54:50 -0700 Subject: [PATCH] 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 --- lib/pk11wrap/pk11cxt.c | 22 ++++++++++++++--- lib/pk11wrap/pk11load.c | 5 +++- lib/pk11wrap/pk11skey.c | 6 +++-- tests/all.sh | 54 +++++++++++++++++++++++++++++++++++++++-- 4 files changed, 78 insertions(+), 9 deletions(-) diff --git a/lib/pk11wrap/pk11cxt.c b/lib/pk11wrap/pk11cxt.c index e189716a9e..b1569ebf94 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 9e7a0a546d..2bc41dbc47 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 f724fe9b71..66b4ed6a11 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 99a7fd6396..60cd99fdd2 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 "
" + + 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