Closed Bug 1464224 Opened 2 years ago Closed 2 years ago

"Release" of uninitialized pointers in nss_ckmk_CreateCertificate()

Categories

(NSS :: Libraries, enhancement)

3.34.1
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mozillabugs, Assigned: franziskus)

Details

Attachments

(1 file)

nss_ckmk_CreateCertificate() (security\nss\lib\ckfw\nssmkey\mobject.c) attempts to "release" uninitialized pointers on some error paths.

The bug is that line 1328 does not initialize |itemRef| and line 1329 does not initialize |keychainRef|. If the calls on lines 1332-33, lines 1340-41, or line 1348 fail, control goes to lines 1388 et seq, which attempts to "release" both uninitialized pointers.

1317: static ckmkInternalObject *
1318: nss_ckmk_CreateCertificate(
1319:     NSSCKFWSession *fwSession,
1320:     CK_ATTRIBUTE_PTR pTemplate,
1321:     CK_ULONG ulAttributeCount,
1322:     CK_RV *pError)
1323: {
1324:     NSSItem value;
1325:     ckmkInternalObject *io = NULL;
1326:     OSStatus macErr;
1327:     SecCertificateRef certRef;
1328:     SecKeychainItemRef itemRef;
1329:     SecKeychainRef keychainRef;
1330:     CSSM_DATA certData;
1331: 
1332:     *pError = nss_ckmk_GetAttribute(CKA_VALUE, pTemplate,
1333:                                     ulAttributeCount, &value);
1334:     if (CKR_OK != *pError) {
1335:         goto loser;
1336:     }
1337: 
1338:     certData.Data = value.data;
1339:     certData.Length = value.size;
1340:     macErr = SecCertificateCreateFromData(&certData, CSSM_CERT_X_509v3,
1341:                                           CSSM_CERT_ENCODING_BER, &certRef);
1342:     if (noErr != macErr) {
1343:         CKMK_MACERR("Create cert from data Failed", macErr);
1344:         *pError = CKR_GENERAL_ERROR; /* need to map macErr */
1345:         goto loser;
1346:     }
1347: 
1348:     *pError = ckmk_GetSafeDefaultKeychain(&keychainRef);
1349:     if (CKR_OK != *pError) {
1350:         goto loser;
1351:     }
1352: 
1353:     macErr = SecCertificateAddToKeychain(certRef, keychainRef);
1354:     itemRef = (SecKeychainItemRef)certRef;
...
1388: loser:
1389:     if (0 != itemRef) {
1390:         CFRelease(itemRef);
1391:     }
1392:     if (0 != keychainRef) {
1393:         CFRelease(keychainRef);
1394:     }
1395: 
1396:     return io;
1397: }


https://opensource.apple.com/source/Security/Security-57740.51.3/base/SecBase.h.auto.html defines |SecKeychainItemRef| and |SecKeychainRef| as pointers thusly:

   typedef struct CF_BRIDGED_TYPE(id) SECTYPE(SecKeychain) *SecKeychainRef;

   typedef struct CF_BRIDGED_TYPE(id) SECTYPE(SecKeychainItem) *SecKeychainItemRef;

I don't have POC because I don't have a Mac.
Flags: sec-bounty?
franziskus: does Firefox use this code at all? I didn't think we had any Mac keychain integration.
Flags: needinfo?(franziskuskiefer)
This code hasn't been compiled since 2007.
> franziskus: does Firefox use this code at all? I didn't think we had any Mac keychain integration.

I didn't know this code existed... it hasn't been compiled since 2007.
Assignee: nobody → franziskuskiefer
Flags: needinfo?(franziskuskiefer)
Target Milestone: --- → 3.38
Group: crypto-core-security
Flags: sec-bounty? → sec-bounty-
Comment on attachment 8982461 [details]
Bug 1464224 - delete nssmkey

Martin Thomson [:mt:] has approved the revision.

https://phabricator.services.mozilla.com/D1486
Attachment #8982461 - Flags: review+
https://hg.mozilla.org/projects/nss/rev/9391fc982e713f66db0007f86f24abb74d9b51d3
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.