Closed Bug 1464618 Opened 2 years ago Closed 2 years ago

Double free in sec_pkcs12_encoder_start_context()

Categories

(NSS :: Libraries, defect)

3.34.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mozillabugs, Assigned: franziskus, NeedInfo)

Details

(Keywords: csectype-uaf, sec-low)

Attachments

(1 file)

sec_pkcs12_encoder_start_context() (security\nss\lib\pkcs12\p12e.c) frees the |SECITEM *salt| twice in certain error conditions.

The bug is that line 1591 is not followed by |salt = NULL;|, which means that if one or more of the conditions are satisfied that cause the |goto loser| on lines 1606, 1613, 1627, 1631, or 1636 to be executed, lines 1647-49 will be executed, thus overwriting |sizeof (SECITEM)| bytes of unallocated memory at |salt| with zeroes, and freeing |salt| again, which presumably corrupts the heap from which it's allocated:

1482: static sec_PKCS12EncoderContext *
1483: sec_pkcs12_encoder_start_context(SEC_PKCS12ExportContext *p12exp)
1484: {
1485:     sec_PKCS12EncoderContext *p12enc = NULL;
1486:     unsigned int i, nonEmptyCnt;
1487:     SECStatus rv;
1488:     SECItem ignore = { 0 };
1489:     void *mark;
1490:     SECItem *salt = NULL;
1491:     SECItem *params = NULL;
...
1551:     } else {
...
1560:             salt = sec_pkcs12_generate_salt();
...
1589:             params = PK11_CreatePBEParams(salt, &pwd,
1590:                                           NSS_PBE_DEFAULT_ITERATION_COUNT);
1591:             SECITEM_ZfreeItem(salt, PR_TRUE);
1592:             SECITEM_ZfreeItem(&pwd, PR_FALSE);
1593: 
1594:             /* get the PBA Mechanism to generate the key */
1595:             switch (p12exp->integrityInfo.pwdInfo.algorithm) {
...
1605:                 default:
1606:                     goto loser;
1607:             }
1608: 
1609:             /* generate the key */
1610:             symKey = PK11_KeyGen(NULL, integrityMechType, params, 20, NULL);
1611:             PK11_DestroyPBEParams(params);
1612:             if (!symKey) {
1613:                 goto loser;
1614:             }
1615: 
1616:             /* initialize HMAC */
1617:             /* Get the HMAC mechanism from the hash OID */
1618:             hmacMechType = sec_pkcs12_algtag_to_mech(
1619:                 p12exp->integrityInfo.pwdInfo.algorithm);
1620: 
1621:             p12enc->hmacCx = PK11_CreateContextBySymKey(hmacMechType,
1622:                                                         CKA_SIGN, symKey, &ignore);
1623: 
1624:             PK11_FreeSymKey(symKey);
1625:             if (!p12enc->hmacCx) {
1626:                 PORT_SetError(SEC_ERROR_NO_MEMORY);
1627:                 goto loser;
1628:             }
1629:             rv = PK11_DigestBegin(p12enc->hmacCx);
1630:             if (rv != SECSuccess)
1631:                 goto loser;
1632:         }
1633:     }
1634: 
1635:     if (!p12enc->aSafeCinfo) {
1636:         goto loser;
1637:     }
1638: 
1639:     PORT_ArenaUnmark(p12exp->arena, mark);
1640: 
1641:     return p12enc;
1642: 
1643: loser:
1644:     sec_pkcs12_encoder_destroy_context(p12enc);
1645:     if (p12exp->arena != NULL)
1646:         PORT_ArenaRelease(p12exp->arena, mark);
1647:     if (salt) {
1648:         SECITEM_ZfreeItem(salt, PR_TRUE);
1649:     }
1650:     if (params) {
1651:         PK11_DestroyPBEParams(params);
1652:     }
1653: 
1654:     return NULL;
1655: }

I will attempt a POC.
Flags: needinfo?(mozillabugs)
Flags: sec-bounty?
Assignee: nobody → franziskuskiefer
Comment on attachment 8983334 [details]
Bug 1464618 - null salt

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

https://phabricator.services.mozilla.com/D1553
Attachment #8983334 - Flags: review+
https://hg.mozilla.org/projects/nss/rev/e36e5500f6534ea7e5b48b29215ab4d844ce7e70
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.38
Group: crypto-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.