297735 - C_Initialize in softoken should return CKR_CANT_LOCK with application-provided locks

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

Softoken always use NSPR locks, which technically are OS native locks (except
for the WINNT fiber case).

It does so even if the application initializes it with locks that it provides,
and does not set the CKF_OS_LOCKING_OK flag. This is in violation of the PKCS#11
specification. Since softoken doesn't actually use the locks provided, it must
return CKR_CANT_LOCK in this case .

Alternately, softoken could be changed to use the provided locks, but it would
be much more work to remove the NSPR dependency. This bug is meant to fix the
PKCS#11 spec issue, not

Comment 1

[Julien Pierre]

... not to remove the NSPR dependency in softoken .

Comment 2

[Julien Pierre]

Attachment: fail if CKF_OS_LOCKING_OK is not set

Comment 3

[Robert Relyea]

Comment on attachment 186288 [details] [diff] [review]
fail if CKF_OS_LOCKING_OK is not set

This code will fail to return CKR_CANT_LOCK if init_args && (init_args->flags &
CKF_OS_LOCKING_OK == 0)

That is it will fail if someone doesn't supply the LibraryParameters.

In that case it will return CKR_ARGUMENTS_BAD. This probably isn't a problem
until bug 222651 gets fixed. I would entertain another patch that fails the
locking independently of the state of init_args->LibraryParamenters).

r+, though I'd like to see an update.

Comment 4

[Julien Pierre]

Attachment: return CKR_CANT_LOCK even if LibraryParameters are not set

Comment 5

[Wan-Teh Chang]

Comment on attachment 187260 [details] [diff] [review]
return CKR_CANT_LOCK even if LibraryParameters are not set

>+    if (init_args && !(init_args->flags & CKF_OS_LOCKING_OK)) {
>+        /* softoken always uses NSPR (ie. OS locking), and doesn't know how
>+           to use lock functions provided by the application. */
>+        crv = CKR_CANT_LOCK;
>+        goto loser2;
>+    }

Rather than adding a new label "loser2", you can just
return crv here.

Please use the prevalent multi-line comment style in this
file.

I think the logic of the test should be:

    init_args &&
    (CKF_OS_LOCKING_OK isn't set) &&
    (the function pointer fields are supplied)

The reason is that if CKF_OS_LOCKING_OK isn't set and
the function pointer fields are NOT supplied, it means
the application won't be accessing the Cryptoki library
from multiple threads simultaneously.  In this case,
it is fine for the softoken to use OS (NSPR) locking.
This case is equivalent to the !init_args case, which
we allow.

Comment 6

[Julien Pierre]

Attachment: allow single-threading with non-NULL CK_C_INITIALIZE_ARGS

Wan-Teh,

You are right, I overlooked the possibility of doing single-threading even with
non-NULL CK_C_INITIALIZE_ARGS . This is explicitly allowed in the PKCS#11 spec
. 

Re: the goto loser2, I removed it since there were already 5 different return
paths . Someday I think they'll probably have to be fixed to allow for cleanup
on exit, but not now.

Comment 7

[Wan-Teh Chang]

Comment on attachment 187444 [details] [diff] [review]
allow single-threading with non-NULL CK_C_INITIALIZE_ARGS

r=wtc.

I suggest that we do this check first thing in the
nsc_CommonInitialize function because it is a form
of invalid argument checking.

If some, but not all, of the mutex function pointers
are non-NULL, we are supposed to return CKR_ARGUMENTS_BAD.
We probably should implement this check before your
check.

    if (init_args &&
	!(!init_args->CreateMutex && ...) /* not all NULL_PTR */ &&
	!(init_args->CreateMutex && ...)  /* not all non-NULL_PTR */ ) {
	return CKR_BAD_ARGUMENTS;
    }

If we do this, then your check only needs to test
init_args->CreateMutex.

Comment 8

[Julien Pierre]

Wan-Teh,

Currently, nscCommonInitialize is structured in such a way as to do other
things, like the FIPS check, before the arguments check. In my patch, I
preserved that structure. All the processing of init_args is done in code the
two if blocks that are next to each other, right after the call to
nsslowkey_SetDefaultKeyDBAlg . I don't think it's required to change that
structure to fix this bug.

Regarding the checking of the 4 lock pointers. Indeed, the PKCS#11 spec says
that we should return CKR_ARGUMENTS_BAD if only some of the mutex pointers are
NULL . I don't think such a strict requirement makes sense here. If
CKF_OS_LOCKING_OK is also specified, in addition to a few lock pointers being
set and others NULL; if the module supports OS locking, then it can just work
with OS locking, and ignore the lock pointers, as if they were all NULL. My
current patch allows this case. I think we would be overly restrictive to check
all 4 lock pointers upfront if we can still function due to CKF_OS_LOCKING_OK
being set. I think the case for this could be made to the PKCS#11 working group.

However, for the case where CKF_OS_LOCKING_OK is not set, I'm fine with making
the distinction between all lock pointers being set and returning CKR_CANT_LOCK,
or only some of them being set, and returning CKR_ARGUMENTS_BAD. I will produce
a new patch that includes that change.

Comment 9

[Julien Pierre]

Attachment: updated patch

Return CKR_ARGUMENTS_BAD if some locks are NULL but not others, and
CKF_OS_LOCKING_OK is not set

Comment 10

[Wan-Teh Chang]

Comment on attachment 188258 [details] [diff] [review]
updated patch

We shouldn't go half way in checking for bad arguments,
but I'll leave it up to you and Bob.

Comment 11

[Robert Relyea]

Comment on attachment 188258 [details] [diff] [review]
updated patch

r+ rrelyea

Comment 12

[Julien Pierre]

Bob, thanks for the review. I have checked in this patch to the tip for 3.11 .

Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.102; previous revision: 1.101
done

Comment 13

[Robert Relyea]

Comment on attachment 187444 [details] [diff] [review]
allow single-threading with non-NULL CK_C_INITIALIZE_ARGS

updated patch already reviewd.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

In light of this bug, and the change it made to softoken, it appears that 
the comment in the source, seen at 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pkcs11.c&rev=1.155&mark=2514-2518#2514
is wrong.  

Bob, do you agree?  Or is there a remaining desire to see softoken be made
to work with application-supplied locking primitives?