1424085 - certificate reference leaks in IsCertificateDistrustImminent

Status: VERIFIED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

nsIX509Cert::GetCert returns a CERTCertificate whose reference count has already been increased. When IsCertificateDistrustImminent calls CertDNIsInList(rootCert->GetCert(), RootSymantecDNs) and CertDNIsInList(aCert->GetCert(), RootAppleAndGoogleDNs), the reference count on those certificates never gets a corresponding decrement, so we keep those certificates alive until shut down. This can be demonstrated by setting MOZ_LOG="pipnss:4", running Firefox, connecting to any host, and then shutting down. "[Main Thread]: E/pipnss NSS SHUTDOWN FAILURE" will be in the console output.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1424085 - add owning handles so cert references don't leak in IsCertificateDistrustImminent

nsIX509Cert::GetCert() returns a CERTCertificate whose reference count has
already been increased. Before this patch, when IsCertificateDistrustImminent
called CertDNIsInList(rootCert->GetCert(), RootSymantecDNs) and
CertDNIsInList(aCert->GetCert(), RootAppleAndGoogleDNs), the reference count on
those certificates would never get a corresponding decrement, so we would keep
those certificates alive until shut down. A reasonable and consistent solution
is to introduce a UniqueCERTCertificate handle in each case to own the
reference.

The status of this fix can be verified by setting MOZ_LOG="pipnss:4", running
Firefox, connecting to any host, and then shutting down. If an NSS resource
reference has been leaked, "[Main Thread]: E/pipnss NSS SHUTDOWN FAILURE" will
be in the console output. Otherwise,
"[Main Thread]: D/pipnss NSS shutdown =====>> OK <<=====" will be in the console
output.

This patch also removes nsIX509CertList::DeleteCert because it would also leak a
reference. Luckily, nothing was using it.

This patch also clarifies the implementation of nsIX509CertList::AddCert by
making the ownership transfers explicit.

Review commit: https://reviewboard.mozilla.org/r/206434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206434/

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

[Tracking Requested - why for this release]: this could potentially keep more certificates alive in memory than before (note that all certificates are ref-counted/de-duplicated, so the excess memory would probably only be on the order of how many distinct domains are visited per session) (and it may be that things like history, the network cache, etc., keep these certificate alive anyway, in which case this wouldn't have a noticeable impact).

Comment 3

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8935559 [details]
bug 1424085 - add owning handles so cert references don't leak in IsCertificateDistrustImminent

https://reviewboard.mozilla.org/r/206434/#review212108

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0004d8cbb7f2

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89457644e922
add owning handles so cert references don't leak in IsCertificateDistrustImminent r=jcj

Comment 6

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/89457644e922

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8935559 [details]
bug 1424085 - add owning handles so cert references don't leak in IsCertificateDistrustImminent

Approval Request Comment
[Feature/Bug causing the regression]: bug 1409259
[User impact if declined]: potentially unnecessary extra memory usage
[Is this code covered by automated tests?]: the feature added in bug 1409259 has tests, but not the resource leak
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: yes - steps to verify in comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: very low risk
[Why is the change risky/not risky?]: the fix involves adding a smart pointer to keep track of a resource - it's unlikely this will cause any problems
[String changes made/needed]: none

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8935559 [details]
bug 1424085 - add owning handles so cert references don't leak in IsCertificateDistrustImminent

Memory leak that regressed in 58. Let's uplift for 58.0b12.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrei, another issue good to verify in beta once this lands.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/53df9fbc528e

Comment 12

[Ciprian Georgiu, Desktop QA]

I have managed to reproduce this issue using an affected 58 Beta 10 build. The following failure "[Main Thread]: E/pipnss NSS SHUTDOWN FAILURE" was displayed in the logs/console output after closing Firefox.

This is verified fixed on 58.0b12 (20171218174357), 59.0a1 (2017-12-19) across platforms: Win 10 x64, Mac OS  X 10.11 and Ubuntu 16.04 x64. I can no longer see failures related to pipnss, "[Main Thread]: D/pipnss NSS shutdown =====>> OK <<=====".