248981 - SECMOD_LoadPKCS11Module is not thread-safe

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

This function reuses a single global variable, secmodLockFunctions, when loading
a PKCS#11 module . But the LibraryParameters field gets modified. The fix is to
make a copy of the structure and modify that copy.

Comment 1

[Julien Pierre]

Attachment: modify a copy of CK_C_INITIALIZE_ARGS rather than global

Comment 2

[Julien Pierre]

Marking P3 since most apps will not load PKCS#11 modules simultaneously.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Is C_Initialize allowed to modify its args?  Isn't that a P1 bug?
Shouldn't the fix be to have the module make the copy?  
(I don't know the answer, but that seems more reasonable to me, unless
the caller is supposed to notice the change to the args.)

Comment 4

[Julien Pierre]

Nelson,

I don't think C_Initialize is allowed to modify its args, but that isn't the
issue here .The code I patched is in the PKCS#11 wrapper code that loads PKCS#11
modules and executes before C_Initialize . It uses a global pre-set structure
with all the lock functions and NULL library parameters, sets the library
parameters field in that structure according to the caller's arguments, then
passes a pointer to that structure to the module being loaded.

Comment 5

[Robert Relyea]

Comment on attachment 151918 [details] [diff] [review]
modify a copy of CK_C_INITIALIZE_ARGS rather than global

Good solution. Another option, which is a slight optimization would be to
declare a CK_VOID_PTR, and only make the copy in the case where libraryParams
was not null, otherwise use the secmodLockFunctions. (requires casting way the
const unfortunately, so the existing code is probably safer, thus the r+).

bob

Comment 6

[Julien Pierre]

Thanks for the review. Checked in to the tip.


Checking in pk11load.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11load.c,v  <--  pk11load.c
new revision: 1.11; previous revision: 1.10
done

Comment 7

[Wan-Teh Chang]

Comment on attachment 151918 [details] [diff] [review]
modify a copy of CK_C_INITIALIZE_ARGS rather than global

This patch causes the following command to crash on
Windows:

	0012f789()	
>	nssckbi.dll!nssCKFWInstance_CreateMutex(NSSCKFWInstanceStr * fwInstance=0x00598750, NSSArenaStr * arena=0x005995f8, unsigned long * pError=0x0012f828)  Line 505 + 0x14	C
	nssckbi.dll!nssCKFWHash_Create(NSSCKFWInstanceStr *
fwInstance=0x00598750, NSSArenaStr * arena=0x005995f8, unsigned long *
pError=0x0012f828)  Line 123 + 0x11  C
	nssckbi.dll!nssCKFWToken_CloseAllSessions(NSSCKFWTokenStr *
fwToken=0x00599700)  Line 1611 + 0x17	    C
	nssckbi.dll!NSSCKFWC_CloseAllSessions(NSSCKFWInstanceStr *
fwInstance=0x00598750, unsigned long slotID=1)	Line 1414 + 0x9      C
	nssckbi.dll!builtinsC_CloseAllSessions(unsigned long slotID=1)	Line
420 + 0x10 C
	nss3.dll!PK11_DestroySlot(PK11SlotInfoStr * slot=0x00599298)  Line 485
+ 0xf	 C
	nss3.dll!PK11_FreeSlot(PK11SlotInfoStr * slot=0x00599298)  Line 520 +
0x9	  C
	nss3.dll!SECMOD_DestroyModule(SECMODModuleStr * module=0x005982a0) 
Line 699 + 0x12     C
	nss3.dll!SECMOD_DestroyModuleListElement(SECMODModuleListStr *
element=0x00555d98)  Line 743 + 0xc	 C
	nss3.dll!SECMOD_DestroyModuleList(SECMODModuleListStr *
list=0x00555da8)  Line 758 + 0x11	C
	nss3.dll!SECMOD_Shutdown()  Line 94 + 0xb	C
	nss3.dll!NSS_Shutdown()  Line 559 + 0x5 C
	certutil.exe!certutil_main(int argc=4, char * * argv=0x00552bb8, int
initialize=1)  Line 3068 + 0xb	   C
	certutil.exe!main(int argc=4, char * * argv=0x00552bb8)  Line 3082 +
0xf	   C
	certutil.exe!mainCRTStartup()  Line 398 + 0xe	C
	kernel32.dll!7c81774d() 	

I think the crash is that CKFW saves a pointer to
the C_Initialize argument, and now that pointer
points to a stack variable that has gone out of
scope.

Note that NSS_Shutdown calling nssCKFWHash_Create
is a known problem (bug 169313).

Comment 8

[Wan-Teh Chang]

Attachment: Patch for ckfw (proof of concept)

This patch fixes the crash in nssckbi.dll.  I just
put this together quickly.  There may be a more
elegant way to fix this.

We should still fix pk11load.c so that it won't
crash in the broken nssckbi.dll's out there.

Comment 9

[Robert Relyea]

On reading the PKCS #11 spec, I agree that the nssckbi is at fault here and 
should be fixed. The spec makes no guarrenttees that the 
CK_C_INITIALIZATION_ARGS won't go out of scope after C_Initialize returns (it's 
silent on that semantic, but one would be surprised if the memory *didn't* go 
out of scope unless one is supposed to 'own' the memory).

Unfortunately because nssckbi had this bug, it means old versions of nssckbi 
will crash under the new NSS:(. We should also implement my (at time 
halfhearted) suggestion.

bob

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Raising to P1, because this is now a crash.
IMO, If possible, we should get some fix checked in today, or back out the 
patch that introduced the crash.

Is Wan-Teh's patch an acceptable solution?  
or an acceptable interim solution?

Comment 11

[Wan-Teh Chang]

Attachment: Patch to prevent crashes in old nssckbi modules

This patch implement the solution Bob described in comment 5.

Comment 12

[Robert Relyea]

Comment on attachment 152626 [details] [diff] [review]
Patch to prevent crashes in old nssckbi modules

r=relyea.

I believe we need both patches.

Comment 13

[Wan-Teh Chang]

Comment on attachment 152626 [details] [diff] [review]
Patch to prevent crashes in old nssckbi modules

I checked in this patch on the tip.

Checking in pk11load.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11load.c,v  <--  pk11load.c
new revision: 1.12; previous revision: 1.11
done

Comment 14

[Wan-Teh Chang]

Attachment: Patch for ckfw (proof of concept) v2

I found that my previous patch for ckfw is not
complete.  ckfw saves the CK_C_INITIALIZE_ARGS_PTR
argument in another place (nsprstub.c).  This
new patch covers that, too.

With this patch applied, and without the patch
for pk11load.c, all.sh also passes.

Comment 15

[Wan-Teh Chang]

Comment on attachment 152642 [details] [diff] [review]
Patch for ckfw (proof of concept) v2

To review this patch, please do the following:

1. Verify that NSSCKFWC_Initialize (in lib/ckfw/wrap.c)
is the C_Initialize method for the nssckbi module.

2. Verify that NSSCKFWC_Initialize passes its pInitArgs
argument to two functions: nssSetLockArgs and 
nssCKFWInstance_Create.

3. Verify that this patch makes a copy of *pInitArgs
(if pInitArgs is not NULL) in those two functions.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 152642 [details] [diff] [review]
Patch for ckfw (proof of concept) v2

r=nelson  I verified the things that Wan-Teh requested.

I observe that the patch to nssSetLockArgs() explicitly assigns
a value to nssstub_pInitArgs whether or not pInitArgs is NULL,
however the patch to nssCKFWInstance_Create() in instance.c
changes that function so that it only explicitly assigns a
value to fwInstance->pInitArgs when pInitArgs is not NULL.

Given that fwInstance was zeroed out when allocated, and
that all supported platforms use an all-zeros value in memory
for NULL, the patch is not incorrect for our supported platforms.

But it would be easier to verify the correctness if nssCKFWInstance_Create()
explicitly assigned a value to fwInstance->pInitArgs whether or not pInitArgs
was NULL.  
This is only a suggestion, and not required for checkin.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

The bug is reported against 3.9.2 (on the 3.9 branch), but is targetted
against 3.10 (the trunk).  If this bug exists on the branch, then the fix
must also target the branch.  

According to comment 13 above, one patch has been checked in on 
the trunk only, IINM.  Does it need to be on the branch also?

Comment 18

[Wan-Teh Chang]

Attachment: Alternate patch for ckfw

Some may consider this patch a better fix for ckfw
than the "proof of concept" patch.

In this patch, I only have one copy of
CK_C_INITIALIZE_ARGS instead of two.

Note that the changes for ckfw.h are code
hygiene changes that we should check in anyway.

Comment 19

[Wan-Teh Chang]

Comment on attachment 152735 [details] [diff] [review]
Alternate patch for ckfw

Nelson, please indicate which ckfw patch you like
better.

As I explained, the ckfw.h changes in this patch
should be checked in regardless of the fix we
select.

Comment 20

[Wan-Teh Chang]

The cvs revision of pk11load.c on the NSS_3_9_BRANCH
is 1.9.  So Julien did not check in his patch on the
NSS_3_9_BRANCH.  Therefore we do not need to check in
my patch on the NSS_3_9_BRANCH.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Re: comment 20, 
If this bug was introduced only on the trunk after the 3.9 branch was 
created, then it is not a bug in NSS version3.9.2 as this bug states.  
So, I am changing the bug version to 3.10.  I will review the new patch 
next week.

Comment 22

[Wan-Teh Chang]

This bug was introduced in NSS 3.4, when the "LibraryParameters"
field was added to (Bob Relyea's proposed extension of)
CK_C_INITIALIZE_ARGS.

Julien's fix for this bug (only checked in on the trunk) introduced
a regression.  So I reopened the bug and submitted patches to fix
the regression.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 152735 [details] [diff] [review]
Alternate patch for ckfw

Both patches are good.	r=nelson
The second one cleans up ckfw.h too, and keeps only one copy of the 
init args.  One stylistic nit:

>+  nssSetLockArgs(nssCKFWInstance_GetInitArgs(*pFwInstance));

I'd prefer that was two separate lines of code:
    something = nssCKFWInstance_GetInitArgs(*pFwInstance);
    nssSetLockArgs(something);

The latter style makes it easier to set a breakpoint to step into 
nssSetLockArgs without also stepping into nssCKFWInstance_GetInitArgs.
It also makes it easier to see the value returned by 
nssCKFWInstance_GetInitArgs.  
But that change is not necessary for this checkin.

Comment 24

[Julien Pierre]

I think there has been a bit of confusion about this defect, particularly the
releases and targets.

There are really two bugs being discussed here, both pre-existing :

1) the thread-unsafety of the function SECMOD_LoadPKCS11Module . This bug has
been traced by Wan-Teh to have been introduced in NSS 3.4

2) the bug in nssckbi where the stack parameters to C_Initialize are assumed to
be global data, which is not guaranteed by the spec, but happens to be the case
if the module is loaded by an NSS implementation prior to 3.10

I would say that the first bug is a P3, which is why I gave it that priority and
targeted the fix for 3.10 (trunk) .

But the second bug is definitely a P1. In fact, Sun needs the fix for this
second problem to go into NSS 3.9.3, because the root cert module may not be
loaded by NSS, but rather by the JDK 1.5 PKCS#11 module, which will not
guarantee that the data passed to C_Initialize is global.

Comment 25

[Julien Pierre]

After trying to create a fix for 249488, I am having second thoughts about these
2 ckfw patches (even though I put an sr+ on one of them).

I think one big problem is that pInitArgs gets examined in nssCKFWMutex_Create,
every time a mutex is created, rather than in nssCKFWInstance_Create, at
initialization time . Once I change that, these CKFW patches will pretty much be
obsoleted.

Does everyone agree to continue the discussion of the root cert module issue in
bug 249488 , and close this bug, as the thread-safety issue of
SECMOD_LoadPKCS11Module has been resolved ?

Comment 26

[Julien Pierre]

Since there were no objections, I am marking this bug fixed. The root cert
module issue continues to be discussed in bug 249488 .

Comment 27

[Wan-Teh Chang]

When Bob Relyea carried the fix for bug 252702
back to the NSS_3_9_BRANCH (pk11load.c, rev. 1.9.16.1,
date 2004/07/29 22:53:33), he also carried back
the first two patches (for pk11load.c) in this bug.
So I'm changing the target milestone to NSS 3.9.3.