762353 - PKIX_PL_OID_CreateBySECItem may return success when it fails to allocate a new OID, leading to possible stale pointer/use-after-free

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Ryan Sleevi]

See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_oid.c&rev=1.7&mark=280,297-301#280

If SECITEM_CopyItem (line 297) fails to copy data into oid->derOid, then it will fail to update *pOID (line 299), leaving the caller's original pointer. However, because no PKIX_ERROR is generated, the function will be seen as successful.

Because of this, in the event of a failure that incorrectly returns success, *pOID contains a stale pointer (such as it might while the loop in cert_PKIXMakeOIDList executes), and this may lead to use-after-frees.

There are several places where loops of the following pattern are used:
while (some condition) {
  PKIX_CHECK(PKIX_PL_OID_CreateBySecItem(...))
  PKIX_CHECK(PKIX_List_AppendItem(...))
  // Release the reference
}

If the same pointer is used for multiple iterations of this loop, and a call fails, the stale pointer may be left.

If PKIX_DECREF is used for "// Release the reference", then this will result in a NULL pointer deref (more aptly, NULL - [sizeof(PKIX_PL_Object header)]), which "should be" safe.

If PKIX_PL_Object_DecRef is used instead, this may result in a U-A-F on the next iteration of the loop, if the next call fails.

Note that PKIX_PL_OID_Create calls PKIX_PL_OID_CreateBySecItem, so both signatures must be checked.

Comment 1

[Ryan Sleevi]

Attachment: Proposed patch that 'throws' an error if SECITEM_CopyItem fails

Comment 2

[Wan-Teh Chang]

Comment on attachment 630835 [details] [diff] [review]
Proposed patch that 'throws' an error if SECITEM_CopyItem fails

r=wtc.

Patch checked in on the NSS trunk (NSS 3.14) and
the NSS_3_13_4_BRANCH (NSS 3.13.6).

Checking in pkix_pl_oid.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_oid.c,v  <--  pkix_pl_oid.c
new revision: 1.8; previous revision: 1.7
done

Checking in pkix_pl_oid.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_oid.c,v  <--  pkix_pl_oid.c
new revision: 1.6.8.1; previous revision: 1.6
done