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

RESOLVED FIXED in 3.13.6

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: Ryan Sleevi, Assigned: Ryan Sleevi)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [no-esr])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 630835 [details] [diff] [review]
Proposed patch that 'throws' an error if SECITEM_CopyItem fails
Attachment #630835 - Flags: review?(wtc)

Comment 2

6 years ago
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
Attachment #630835 - Flags: review?(wtc) → review+

Updated

6 years ago
Assignee: nobody → ryan.sleevi
Status: NEW → RESOLVED
Last Resolved: 6 years ago
OS: Windows 7 → All
Priority: -- → P2
Hardware: x86_64 → All
Resolution: --- → FIXED
Summary: PKIX_PL_OID_CreateBySecItem may return success when it fails to allocate a new OID, leading to possible stale pointer/use-after-free → PKIX_PL_OID_CreateBySECItem may return success when it fails to allocate a new OID, leading to possible stale pointer/use-after-free
Whiteboard: [3.13.6]
Target Milestone: --- → 3.14

Updated

6 years ago
Whiteboard: [3.13.6] → [3.13.6] [no-esr]

Updated

6 years ago
Whiteboard: [3.13.6] [no-esr] → [no-esr]
Target Milestone: 3.14 → 3.13.6

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.