894031 - If certification is invalid, there is a memory leakage in AuthCertificate

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[leo.bugzilla.gecko]

Problem
There is not a code to destroy an instance of CERTCertList in case of invalid certification.
The CERTCertList is gotten from CERT_GetCertChainFromCert in AuthCertificate() in SSLServerCertVerification.cpp.

Reproducing Steps
1. Device time sets old time like 01/Jan/1980
2. Launch the webapps using connection with https or
   open a web page using https in browser
3. PSM_SSL_PKIX_AuthCertificate in AuthCertificate() returns error.
4. In this case, there is not a code to destroy an instance of CERTCertList.

Gecko-branch : b2g-18

Comment 1

[leo.bugzilla.gecko]

Master branch is safe in memory leakage because of using ScopedCERTCertList. (below code)

http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#l872
==> CERTCertList *verifyCertChain = nullptr;
http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#l895
==> ScopedCERTCertList certList(verifyCertChain);

ScopedCERTCertList ensures that verifyCertChain will be deleted in the out of block.

But b2g18 branch uses a raw pointer to CERTCertList instance which should be deleted explicitly like below.
   if (certList) {
     CERT_DestroyCertList(certList);
   }

There is the explicit code to delete CERTCertList instance only in "true" case block of condition, "if (rv == SECSuccess)", in line 916, but not in "else" case of the same condition.
We need to add the code for destruction in "else" case.

Comment 2

[leo.bugzilla.gecko]

Attachment: ssl-server-cert-list.patch

This is the patch code for b2g18 to delete certList instance in both "true" and "else" case.

Comment 3

[Camilo Viecco (:cviecco)]

Comment on attachment 800044 [details] [diff] [review]
ssl-server-cert-list.patch

Review of attachment 800044 [details] [diff] [review]:
-----------------------------------------------------------------

Drive by review. Looks good (assuming you are using the b2g18 branch).

Comment 4

[Dietrich Ayala (:dietrich)]

Nominating to block, since this is a memory leak that can occur on 18 (already fixed on trunk).

Comment 5

[Preeti Raghunath(:Preeti)]

leo+ for mem leak fixing

Comment 6

[Camilo Viecco (:cviecco)]

Comment on attachment 800044 [details] [diff] [review]
ssl-server-cert-list.patch

Review of attachment 800044 [details] [diff] [review]:
-----------------------------------------------------------------

I am r+ it but you need an actual psm peer to chekin this. See bsmith.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Needs r+ from bsmith first per cviecco.

Comment 8

[Preeti Raghunath(:Preeti)]

Brian,

Please review code so that the patch can land.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/f37ae8a910e8

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/f37ae8a910e8