Closed Bug 1453505 Opened 2 years ago Closed Last year

Latent heap corruption on allocator mismatch in pk11_copyAttributes()

Categories

(NSS :: Libraries, enhancement)

enhancement
Not set

Tracking

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox60 wontfix, firefox61 fixed)

RESOLVED FIXED
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: mozillabugs, Assigned: ttaubert)

Details

(Keywords: sec-moderate, Whiteboard: [adv-main61+][post-critsmash-triage])

Attachments

(1 file)

pk11_copyAttributes() (security\nss\lib\pk11wrap\pk11merge.c) allocates memory on the PR heap, then frees the same memory on the global heap. If these are different heaps, this mismatch will corrupt both heaps.

The bug is that pk11_copyAttributes() allocates |newTemplate| by calling PORT_NewArray() on line 72, which calls PORT_Alloc_Util(), which calls PORT_Alloc() (security\nss\lib\util\secport.c), which calls PR_Malloc(). But line 100 uses global free() to release |newTemplate|.

 57: static SECStatus
 58: pk11_copyAttributes(PLArenaPool *arena,
 59:                     PK11SlotInfo *targetSlot, CK_OBJECT_HANDLE targetID,
 60:                     PK11SlotInfo *sourceSlot, CK_OBJECT_HANDLE sourceID,
 61:                     CK_ATTRIBUTE *copyTemplate, CK_ULONG copyTemplateCount)
 62: {
 63:     SECStatus rv;
 64:     CK_ATTRIBUTE *newTemplate = NULL;
 65:     CK_RV crv;
 66: 
 67:     crv = PK11_GetAttributes(arena, sourceSlot, sourceID,
 68:                              copyTemplate, copyTemplateCount);
 69:     /* if we have missing attributes, just skip them and create the object */
 70:     if (crv == CKR_ATTRIBUTE_TYPE_INVALID) {
 71:         int i, j;
 72:         newTemplate = PORT_NewArray(CK_ATTRIBUTE, copyTemplateCount);
 73:         /* remove the unknown attributes. If we don't have enough attributes
 74: 	 * PK11_CreateNewObject() will fail */
 75:         for (i = 0, j = 0; i < copyTemplateCount; i++) {
 76:             if (copyTemplate[i].ulValueLen != -1) {
 77:                 newTemplate[j] = copyTemplate[i];
 78:                 j++;
 79:             }
 80:         }
 81:         copyTemplate = newTemplate;
 82:         copyTemplateCount = j;
 83:         crv = PK11_GetAttributes(arena, sourceSlot, sourceID,
 84:                                  copyTemplate, copyTemplateCount);
 85:     }
 86:     if (crv != CKR_OK) {
 87:         PORT_SetError(PK11_MapError(crv));
 88:         return SECFailure;
 89:     }
...
 99:     if (newTemplate) {
100:         free(newTemplate);
101:     }
102:     return rv;
103: }


On Windows builds, these functions use the same heap, so this is only a theoretical bug there. If, however, |_PR_ZONE_ALLOCATOR| is defined in the build, and either the symbol |nspr_use_zone_allocator| or the environment variable |NSPR_USE_ZONE_ALLOCATOR| is defined to |1|, PR_MALLOC() uses a different heap from free().

See https://bugzilla.mozilla.org/show_bug.cgi?id=1449355 and https://bugzilla.mozilla.org/show_bug.cgi?id=1449358#c1 for further details on when these heaps could be nonidentical. 

BTW, lines 86-89 also appear to leak |newTemplate| .
Flags: sec-bounty?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: sec-moderate
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Version: 3.34.1 → trunk
Comment on attachment 8967665 [details]
Bug 1453505 - Don't leak newTemplate in pk11_copyAttributes() r=franziskus

Franziskus Kiefer [:fkiefer or :franziskus] has approved the revision.

https://phabricator.services.mozilla.com/D937
Attachment #8967665 - Flags: review+
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Assignee: nobody → ttaubert
Flags: sec-bounty? → sec-bounty+
Group: crypto-core-security → core-security-release
Whiteboard: [adv-main60+]
Whiteboard: [adv-main60+] → [adv-main61+]
Flags: qe-verify-
Whiteboard: [adv-main61+] → [adv-main61+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.