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

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

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;
	PZ_Unlock(nssShutdownList.lock);
	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 ...

Comment 1

[Julien Pierre]

Attachment: Fix : only increment numFuncs by 1

Comment 2

[Julien Pierre]

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 .

Comment 3

[Julien Pierre]

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
done

And to NSS_3_11_BRANCH :

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