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

RESOLVED FIXED in 1.1 QE5

Status

P1
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: leo.bugzilla.gecko, Assigned: leo.bugzilla.gecko)

Tracking

unspecified
1.1 QE5
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:leo+, firefox26 unaffected, firefox27 unaffected, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed, b2g-v1.2 unaffected)

Details

(Whiteboard: memleak)

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

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

Comment 2

5 years ago
Created attachment 800044 [details] [diff] [review]
ssl-server-cert-list.patch

This is the patch code for b2g18 to delete certList instance in both "true" and "else" case.
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).
Attachment #800044 - Flags: feedback+
Nominating to block, since this is a memory leak that can occur on 18 (already fixed on trunk).
blocking-b2g: --- → leo?
(Assignee)

Updated

5 years ago
Attachment #800044 - Flags: feedback+ → review?(cviecco)
leo+ for mem leak fixing
blocking-b2g: leo? → leo+
Whiteboard: memleak
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.
Attachment #800044 - Flags: review?(cviecco) → review+
Keywords: checkin-needed
Needs r+ from bsmith first per cviecco.
Assignee: nobody → leo.bugzilla.gecko
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
status-b2g-v1.2: --- → unaffected
status-firefox26: --- → unaffected
status-firefox27: --- → unaffected
Keywords: checkin-needed
Brian,

Please review code so that the patch can land.
Flags: needinfo?(brian)
https://hg.mozilla.org/releases/mozilla-b2g18/rev/f37ae8a910e8
status-b2g18: affected → fixed
Flags: needinfo?(brian)
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/f37ae8a910e8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-b2g-v1.1hd: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.