Closed Bug 353608 Opened 14 years ago Closed 14 years ago

NSS_RegisterShutdown may fail, and appData argument to callbacks is always NULL


(NSS :: Libraries, defect, P2)



(Not tracked)



(Reporter: julien.pierre, Assigned: julien.pierre)




(1 file)

There is a bug in NSS_RegisterShutdown :

    nssShutdownList.funcs[nssShutdownList.numFuncs++].func = sFunc;
    nssShutdownList.funcs[nssShutdownList.numFuncs++].appData = appData;

nssShutdownList.numFuncs is getting incremented twice.

There are many consequences to this :

1) The function pointer is not recorded together with its appData argument . One element of the array has the correct function pointer with a NULL argument value, and the other has a NULL function pointer with the correct argument value.

During shutdown, NSS checks that the function pointer is non-NULL before invoking the callback, so it doesn't crash when encountering the NULL function pointer . But it always invokes the function with a NULL argument, regardless of what the user registered .

2) The second invocation of NSS_RegisterShutdown will fail if the appData argument to the first registered callback was NULL .

That's because of the folllowing code :

    /* find an empty slot */
    i = nss_GetShutdownEntry(NULL, NULL);
    if (i > 0) {
	nssShutdownList.funcs[i].func = sFunc;
	nssShutdownList.funcs[i].appData = appData;
	return SECFailure;

After the first call to NSS_RegisterShutdown(function, NULL), there is an entry at offset 1 that contains a NULL function pointer with a NULL argument . nss_GetShutdownEntry(NULL, NULL) returns that entry, and then fails .

The conclusion is that once a function is registered with a NULL argument, no further callbacks can be registered !

3) There would be a potential buffer overflow since we check for 1 available entry, but write to 2 entries. However, the initial array size is 10, so we won't actually see this problem.

I discovered this while trying to add a shutdown callback for freebl in libssl for the bypass case as part of the fix for bug 115951 . I didn't get any error from NSS_RegisterShutdown . But very strangely, all the ECC bypass test cases saw selfserv fail in the handshake with PR_Read returning -1 and PR_GetError() returning 0 .

What happened was that my new NSS_RegisterShutdown was the first one, and I was passing a NULL argument to it. Then, libssl tried to register ssl3_ShutdownECDHECurves . The registration failed due to this bug, and the SSL handshake aborted, with no error . Then, selfserv crashed in printf because SECU_Strerror(0) returned NULL ...
Attachment #239473 - Flags: superreview?(nelson)
Attachment #239473 - Flags: review?(wtchang)
I forgot to mention also that ssl3_ShutdownECDHECurves doesn't use its appData argument, even though libssl registers it with a pointer argument. But the code just gets the pointer from a global variable, instead of looking at the argument ... This is why we didn't detect that the argument value being passed to shutdown callbacks was always NULL .
Assignee: nobody → julien.pierre.bugs
Priority: -- → P2
Blocks: 115951
Attachment #239473 - Flags: review?(wtchang) → review+
Attachment #239473 - Flags: superreview?(nelson) → superreview+
Wan-Teh, Nelson, thanks for the reviews.

I checked this in to the tip.

Checking in nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.75; previous revision: 1.74

And to NSS_3_11_BRANCH :

Checking in nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision:; previous revision:
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.