Closed Bug 894031 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(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)

RESOLVED FIXED
1.1 QE5
blocking-b2g leo+
Tracking Status
firefox26 --- unaffected
firefox27 --- unaffected
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed
b2g-v1.2 --- unaffected

People

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

Details

(Whiteboard: memleak)

Attachments

(1 file)

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
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.
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?
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+
Needs r+ from bsmith first per cviecco.
Assignee: nobody → leo.bugzilla.gecko
Keywords: checkin-needed
Brian,

Please review code so that the patch can land.
Flags: needinfo?(brian)
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/f37ae8a910e8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.