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

RESOLVED FIXED in 3.11.4

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Julien Pierre, Assigned: Julien Pierre)

Tracking

3.11.3
3.11.4

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

1.12 KB, patch
Wan-Teh Chang
: review+
Nelson Bolyard (seldom reads bugmail)
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
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 ...
(Assignee)

Comment 1

11 years ago
Created attachment 239473 [details] [diff] [review]
Fix : only increment numFuncs by 1
Attachment #239473 - Flags: superreview?(nelson)
Attachment #239473 - Flags: review?(wtchang)
(Assignee)

Comment 2

11 years ago
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
(Assignee)

Updated

11 years ago
Blocks: 115951

Updated

11 years ago
Attachment #239473 - Flags: review?(wtchang) → review+
Attachment #239473 - Flags: superreview?(nelson) → superreview+
(Assignee)

Comment 3

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.