Last Comment Bug 353608 - NSS_RegisterShutdown may fail, and appData argument to callbacks is always NULL
: NSS_RegisterShutdown may fail, and appData argument to callbacks is always NULL
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11.3
: All All
: P2 normal (vote)
: 3.11.4
Assigned To: Julien Pierre
:
Mentors:
Depends on:
Blocks: 115951
  Show dependency treegraph
 
Reported: 2006-09-20 22:35 PDT by Julien Pierre
Modified: 2006-09-21 13:10 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix : only increment numFuncs by 1 (1.12 KB, patch)
2006-09-20 22:37 PDT, Julien Pierre
wtc: review+
nelson: superreview+
Details | Diff | Splinter Review

Description Julien Pierre 2006-09-20 22:35:03 PDT
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 2006-09-20 22:37:27 PDT
Created attachment 239473 [details] [diff] [review]
Fix : only increment numFuncs by 1
Comment 2 Julien Pierre 2006-09-20 22:39:47 PDT
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 2006-09-21 13:10:38 PDT
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

Note You need to log in before you can comment on or make changes to this bug.