507422 - crash [@ PORT_FreeArena - lg_mkSecretKeyRep] when PORT_NewArena fails

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[timeless]

786 static NSSLOWKEYPrivateKey *lg_mkSecretKeyRep(const CK_ATTRIBUTE *templ,
799     arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
800     if (arena == NULL) { crv = CKR_HOST_MEMORY; goto loser; }

867 loser:
868     if (crv != CKR_OK) {
869         PORT_FreeArena(arena,PR_FALSE);

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

The right fix for this bug is to modify PORT_FreeArena so that it 
checks its first argument for NULL and simply returns if so.
All the other PORT_Free* functions do that.   I suspect the code
shown above was written by someone who assumes that PORT_FreeArena
had the same behavior as the rest of the PORT_Free functions, and
IMO, that is a reasonable assumption.  We should make it so,
rather than changing all the callers.

Also, changing PORT_FreeArena avoids any complications with FIPS validation.
changing lg_mkSecretKeyRep would cause those very complications.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v1 for NSS Trunk

Bob, 
Do you agree in principle with my assessment and proposed fix?
Please let me know with your review grade.

Comment 3

[Robert Relyea]

Comment on attachment 391660 [details] [diff] [review]
Patch v1 for NSS Trunk

r+ rrelyea

This is safe, and will handle the several cases in NSS where we may not have checked arena is NULL in the error case.

bob

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Checking in secport.c; new revision: 1.24; previous revision: 1.23