550258 - slot->mechanismList not protected from races in [@ PK11_ReadMechanismList]

Status: RESOLVED WORKSFORME

(NSS :: Libraries, defect, P1)

Description

[Stefan Gustafsson]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; sv-SE; rv:1.9.1.8) Gecko/20100202 Firefox/3.5.8 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; sv-SE; rv:1.9.1.8) Gecko/20100202 Firefox/3.5.8 (.NET CLR 3.5.30729)

I'm testing my smart card PKCS#11 module (osca.dll) and gor a crash in NSS.
Directly after removing the smart card I connect to a https site.

The crash occured in pk11slot.c line 1092.
The dereference of mechanismList in the statement slot->mechanismList[i] was
accessing a NULL pointer since a second thread was freeing and reallocating 
the mechanismList at the same time.

Thread 1: Doing SSL authentication

nss3.dll!PK11_ReadMechanismList - CRASH
nss3.dll!PK11_InitToken
nss3.dll!nssSlot_Refresh
nss3.dll!nssSlot_IsTokenPresent
nss3.dll!nssSlot_GetToken
nss3.dll!nssTrustDomain_FindCertificatesBySubject
nss3.dll!find_cert_issuer
nss3.dll!nssCertificate_BuildChain
nss3.dll!CERT_FindCertIssuer
nss3.dll!cert_VerifyCertChainOld
nss3.dll!cert_VerifyCertChain
nss3.dll!CERT_VerifyCertChain
nss3.dll!CERT_VerifyCert
nss3.dll!CERT_VerifyCertNow
ssl3.dll!SSL_AuthCertificate

Thread 2: Normally doing C_WaitForSlotEvent

osca.dll!C_GetMechanismList
nss3.dll!PK11_ReadMechanismList
nss3.dll!PK11_InitToken
nss3.dll!nssSlot_Refresh
nss3.dll!nssSlot_IsTokenPresent
nss3.dll!nssToken_IsPresent
nss3.dll!pk11_IsPresentCertLoad
nss3.dll!PK11_IsPresent
nspr4.dll!_PR_NativeRunThread

I have browsed the NSS source code but have not found any lock protecting 
the PK11SlotInfo struct from simultanius access by two threads at the same
time. 

Best Regards,
Stefan Gustafsson

Reproducible: Didn't try

Steps to Reproduce:
1. Remove smart card triggering C_WaitForSlotEvent
2. At the same time connect to https site
3.

Comment 1

[timeless]

Attachment: extend monitor

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 481463 [details] [diff] [review]
extend monitor

This is not the right fix, IMO.  More comments to follow.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I agree that some mutual exclusion is needed here, but is now absent.
I don't believe the "slot monitor" is the right one to use, especially 
not if conditionally called with "if (!slot->isThreadSafe)".

AIUI, the slot monitor exists as a workaround for PKCS11 modules that lack 
their own internal mutual exclusion.  It protects the data that is private
to that module by effectively single-threading all calls to the module's 
PKCS#11 API functions.   It does not exist to protect the PK11wrap layer's 
own slot structure, which is presumed to be thread safe.  

Now, we see an apparent bug in the PK11wrap layer's own exclusion of access 
to its own slot structure.  This flaw exists for both PKCS#11 modules that '
are thought to be threadsafe, and those that are not, alike, so it's not 
appropriate to devise a solution that only works for modules with their own 
thread safety issues.

Bob, please advise about the right solution for this.  Is there another lock 
or monitor that exists to provide this mutual exclusion?  Clearly the slot 
struct's reference counter is insufficient for this purpose.

Comment 5

[Alvaro]

Hi,

We're still experiencing the same problem with SSL client authentication by smartcard when using Firefox 18.0.2, the browser crashes, even in safe mode.

Is there anybody at Mozilla who can deal with this issue which has been open since 2010?

For us, this issue is highly critical because it occurs systematically.

Regards.

Comment 6

[Robert Relyea]

The race looks like it's more of a race between nss_slotRefresh calls. I suspect there is more than mechanism lists that could cause an issue...

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.