1453505 - Latent heap corruption on allocator mismatch in pk11_copyAttributes()

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[mozillabugs]

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| .

Comment 1

[Phabricator Automation]

Attachment: Bug 1453505 - Don't leak newTemplate in pk11_copyAttributes() r=franziskus

Comment 2

[Phabricator Automation]

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

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/7768f56506bb