486404 - XPCOM allocator mismatches in PSM

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[[e-mail]]

Some PSM files mix and match the XPCOM and both the NSPR allocators.

nsNSSCallbacks.cpp's HandshakeCallback uses
status->mCipherName.Adopt(cipherName);
cipherName is allocated by PORT_strdup however Adopt should use NS_Alloc
(in fact cipherName is not freed on failure of do_GetService)

nsNSSCertificate::GetSerialNumber, GetSha1Fingerprint and GetMd5Fingerprint use
nsXPIDLCString fpStr; fpStr.Adopt(CERT_Hexify(&fpItem, 1));
CERT_Hexify's return value is allocated by PORT_Alloc so should be PORT_Free()d

nsNSSIOLayer.cpp's nsNSSSocketInfo's hostName propety uses
mHostName.Adopt(host ? nsCRT::strdup(host) : 0);
*host = (mHostName) ? nsCRT::strdup(mHostName) : nsnull;
Both the Adopt and the outparam should use an XPCOM allocator. (You could alternatively change mHostName to a char* and retain that allocator.)

nsNSSIOLayer.cpp also has two calls to SSL_RevealURL which return values allocated by PL_strdup so they should be freed with PL_strfree but just to be confusing charCleaner is also (correctly) used with the return value from CERT_GetCommonName. I can't see an easy fix for this one though.

nsSSLStatus::GetCipherName uses PL_strdup when it should be an XPCOM allocator.

Comment 1

[[e-mail]]

Attachment: Proposed patch

The only other alternative I could think of was to revert to manual string deallocation thus eliminating charCleaner entirely.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 370595 [details] [diff] [review]
Proposed patch


>     /* Get CN and O of the subject and O of the issuer */
>     char *ccn = CERT_GetCommonName(&serverCert->subject);
>-    charCleaner ccnCleaner(ccn);
>+    void *v = ccn;
>+    voidCleaner ccnCleaner(v);
>     NS_ConvertUTF8toUTF16 cn(ccn);

voidCleaner?

http://mxr.mozilla.org/mozilla/search?string=voidCleaner says

> No results found

Comment 3

[[e-mail]]

Comment on attachment 370595 [details] [diff] [review]
Proposed patch

>-NSSCleanupAutoPtrClass(char, PR_FREEIF)
>+NSSCleanupAutoPtrClass(char, PL_strfree)
>+NSSCleanupAutoPtrClass(void, PR_FREEIF)
The name of the cleaner class is created using C++ preprocessor magic.

The cleaner class passes a memory pointer to the deallocator. Thus PL_strfree needs to be called from the charCleaner because it's expecting a char* but with PR_FREEIF I can use any suitable type because it doesn't care, it's just most convenient for me to use a void* pointer as I can easily cast to it.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 370595 [details] [diff] [review]
Proposed patch

Looks good to me. I did not check any other places of PSM for mismatch.

Comment 5

[[e-mail]]

As far as I know this patch doesn't need any more reviews but I don't have checkin access to the security module so requesting checkin-needed.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

>  I don't have checkin access to the security module

The parts of the security module that are in NSS and NSPR are indeed
constrained, so that only peers may checkin.  

but I was not aware that PSM was under any such checkin restrictions.

Comment 7

[[e-mail]]

Of course. Silly me. I even have eight previous checkins to security/manager.

Comment 8

[[e-mail]]

Attachment: Missed two cases

I hadn't set up my debugging correctly and a rerun discovered two more files with mismatches, nsCertOverrideService.cpp and nsSDR.cpp; the other changes are the same as the previous patch.

Comment 9

[[e-mail]]

Bah, stupid keyword "autocomplete" :-(

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 372209 [details] [diff] [review]
Missed two cases

You should use PR_DELETE in nsSDR::DecryptString for freeing 'decoded'. It's allocated with PR_MALLOC in PL_Base64Decode. Same for call to PL_Base64Encode.

However, quick look to mxr shows that combination of all nsMemory:Free, PR_Free, NS_Free is used to free result of these functions, so you are probably OK with the patch as is :)

Comment 11

[[e-mail]]

Attachment: With PR_DELETE

You're right, although at least the PR_DELETE/PR_Free difference is not so much of a concern as the NS_Free/PL_strfree difference.

Comment 12

[[e-mail]]

Fix checked in to CVS.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This is burning trees on 1.9.0.

Comment 14

[Samuel Sidler (old account; do not CC)]

This needed approval before checkin. Please back out ASAP.

Comment 15

[[e-mail]]

PSM != NSS
PSM != NSS
PSM != NSS
PSM != NSS
PSM != NSS
PSM != NSS
PSM != NSS
PSM != NSS
PSM != NSS
PSM != NSS
PSM != NSS

Comment 16

[[e-mail]]

Pushed changeset c7ee4c717dd3 to mozilla-central.