2072682 - Use PORT_ReleaseAssert to guard changes to refcounts which may lead to UAF

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P3)

Description

[Dennis Jackson]

D325339 introduces PORT_ReleaseAssert`. We should guard any ref counts with suitable invariants. Examples:

• SEC_DestroyCrl
• CERT_DestroyGeneralNameList
• tls13_ReleaseAntiReplayContext
• pk11_getKeyFromList
• sftk_FreeSession & sftk_DestroySession
• PK11_FreeSlot & PK11_ReferenceSlot
• SECMOD_DestroyModule & SECMOD_ReferenceModule
• nssPKIObject_Destroy & nssPKIObject_AddRef
• PK11_FreeSlotListElement
• ssl_FreeKeyPair

Comment 1

[Dennis Jackson]

Attachment: Bug 2072682 - free never-live symkeys directly in pk11_getKeyFromList r=#nss-reviewers

Comment 2

[Dennis Jackson]

Attachment: Bug 2072682 - release-assert CRL and GeneralNameList reference counts r=#nss-reviewers

Comment 3

[Dennis Jackson]

Attachment: Bug 2072682 - release-assert SSL reference counts r=#nss-reviewers

Comment 4

[Dennis Jackson]

Attachment: Bug 2072682 - release-assert pk11wrap slot, module and symkey reference counts r=#nss-reviewers

Comment 5

[Dennis Jackson]

Attachment: Bug 2072682 - release-assert stan object reference counts r=#nss-reviewers

Comment 6

[Dennis Jackson]

Attachment: Bug 2072682 - release-assert softoken session and db reference counts r=#nss-reviewers

Comment 7

[Dennis Jackson]

Attachment: Bug 2072682 - take a reference in cmsutil's CMS decrypt-key callback r=#nss-reviewers

The CMS layer consumes the reference returned by
NSSCMSGetDecryptKeyCallback, so returning the caller's borrowed pointer
over-released the bulk key: cmsutil -C destroyed it while the content
info still held it, and the release assert in PK11_FreeSymKey caught the
subsequent double free. Release cmsutil's own reference once, in the
common cleanup, so the -D and batch paths are balanced too.

Comment 8

[Dennis Jackson]

Attachment: Bug 2072682 - initialize referenceCount in crlutil's CRL allocations r=#nss-reviewers

Both CRLs allocated with PORT_ArenaZNew were released at a reference
count of zero, which the old SEC_DestroyCrl condition silently accepted.

Comment 9

[Dennis Jackson]

Attachment: Bug 2072682 - abort on detected key object double frees r=#nss-reviewers

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/nss/rev/94285024c0eb
abort on detected key object double frees r=nss-reviewers,jschanck
https://hg.mozilla.org/projects/nss/rev/8a643df1bd5c
free never-live symkeys directly in pk11_getKeyFromList r=nss-reviewers,keeler
https://hg.mozilla.org/projects/nss/rev/97401b74aaec
release-assert CRL and GeneralNameList reference counts r=nss-reviewers,keeler
https://hg.mozilla.org/projects/nss/rev/889ed0cdd65f
release-assert SSL reference counts r=nss-reviewers,keeler
https://hg.mozilla.org/projects/nss/rev/8c204d8b9571
release-assert pk11wrap slot, module and symkey reference counts r=nss-reviewers,keeler
https://hg.mozilla.org/projects/nss/rev/f1a3e216d616
release-assert stan object reference counts r=nss-reviewers,keeler
https://hg.mozilla.org/projects/nss/rev/2704de033b19
release-assert softoken session and db reference counts r=nss-reviewers,keeler
https://hg.mozilla.org/projects/nss/rev/c282985dcde6
take a reference in cmsutil's CMS decrypt-key callback r=nss-reviewers,keeler
https://hg.mozilla.org/projects/nss/rev/5d33f08ca5fb
initialize referenceCount in crlutil's CRL allocations r=nss-reviewers,keeler