Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
rjrelyea committed Apr 14, 2021
1 parent 1bb2991 commit db96cc1
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 9 deletions.
22 changes: 18 additions & 4 deletions lib/pk11wrap/pk11cxt.c
Expand Up @@ -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)
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
5 changes: 4 additions & 1 deletion lib/pk11wrap/pk11load.c
Expand Up @@ -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);
Expand Down
6 changes: 4 additions & 2 deletions lib/pk11wrap/pk11skey.c
Expand Up @@ -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 ||
Expand All @@ -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;
}

Expand Down
54 changes: 52 additions & 2 deletions tests/all.sh
Expand Up @@ -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):
# -----------------------------------------------------------
Expand All @@ -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)
Expand All @@ -83,7 +87,7 @@
# ---------------
# all.sh ~ (main)
# | |
# +------------+------------+-----------+ ~ run_cycles
# +------------+------------+-----------+--- ~ run_cycles
# | | | | |
# standard pkix upgradedb sharedb ~ run_cycle_*
# ... | ... ... |
Expand Down Expand Up @@ -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
########################################################################
Expand All @@ -271,6 +306,9 @@ run_cycles()
"sharedb")
run_cycle_shared_db
;;
"threadunsafe")
run_cycle_thread_unsafe
;;
esac
. ${ENV_BACKUP}
done
Expand All @@ -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`
Expand All @@ -297,13 +335,25 @@ 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"
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
Expand Down

0 comments on commit db96cc1

Please sign in to comment.