1464618 - Double free in sec_pkcs12_encoder_start_context()

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[mozillabugs]

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.

Comment 1

[Phabricator Automation]

Attachment: Bug 1464618 - null salt

Comment 2

[Phabricator Automation]

Comment on attachment 8983334 [details]
Bug 1464618 - null salt

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

https://phabricator.services.mozilla.com/D1553

Comment 3

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/e36e5500f6534ea7e5b48b29215ab4d844ce7e70