Closed Bug 1001513 Opened 10 years ago Closed 8 years ago

nsIX509Cert.getChain generates assertion with certificate that does not verify to a trusted anchor and whose verification path contains a loop

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 867473

People

(Reporter: cviecco, Assigned: cviecco)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

      No description provided.
Assignee: nobody → cviecco
Attached patch test-cert-loop (obsolete) — Splinter Review
Attachment #8412892 - Attachment is obsolete: true
Attached patch fix-cert-loop-getchain (obsolete) — Splinter Review
Attachment #8412902 - Attachment is obsolete: true
Attachment #8412908 - Flags: review?(dkeeler)
Attachment #8412895 - Flags: review?(dkeeler)
Attachment #8412895 - Flags: review?(dkeeler)
Comment on attachment 8412908 [details] [diff] [review]
fix-cert-loop-getchain (v1.1)

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

Actually, I think it's problematic for GetChain to attempt to return an unverified chain if verification fails. Let's just return a chain consisting of the certificate itself and nothing else (note this will have an impact on GetIssuer). I'm also interested in Brian's opinion on this.
Attachment #8412908 - Flags: review?(dkeeler) → review-
Comment on attachment 8412908 [details] [diff] [review]
fix-cert-loop-getchain (v1.1)

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

First, this is not a good bug report. It is good that we attached the test certificate but, (a) what is the actual assertion that failed, and (b) in what circumstances, if any, does Firefox actually trigger that assertion, and (c) is this impacting the compatibility tests that mwobensmith created?

Searching through the code shows that getChain is used in some places--from both native code and from JS code--that make changing it tricky.

I didn't review the patch but I did notice a couple of issues when glancing at it.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +816,5 @@
> +getFirstPotentialIssuer(CERTCertificate* cert, PRTime time){
> +  ScopedCERTCertList certList(CERT_CreateSubjectCertList(nullptr,
> +                                                         CERT_GetDefaultCertDB(),
> +                                                         &cert->derIssuer,
> +                                                         time, true));

Could you construct an instanced of NSSCertDBTrustDomain and call FindPotentialIssuers instead? That would show that we're at least being consistent.

@@ +838,5 @@
> +  ::mozilla::pkix::ScopedCERTCertList certChain (CERT_NewCertList());
> +  if (!certChain) {
> +    return nullptr;
> +  }
> +  CERTCertificate* dup = CERT_DupCertificate(cert);

Please always used ScopedCERTCertificate and friends.
cviecco, please share more details about the bug so we can better understand the problem we're trying to solve. Thanks!
Flags: needinfo?(cviecco)
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #7)
> Comment on attachment 8412908 [details] [diff] [review]
> fix-cert-loop-getchain (v1.1)
> 
> Review of attachment 8412908 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> First, this is not a good bug report. It is good that we attached the test
> certificate but, (a) what is the actual assertion that failed, 

 Assertion within NSS when run with NSS_STRICT_SHUTDOWN:
 Assertion failure: secmod_PrivateModuleCount == 0, at pk11util.c:88

and (b) in
> what circumstances, if any, does Firefox actually trigger that assertion,

A site that:
1. has a cert that cannot be validated (dot not chain to a trust anchor)
2. The chain served by the server contains a loop.
3. And a user wanting to view the cert in the certificate viewer.

OR
1. insert looping special certs in the db and ee cert (with valid trust anchors)
2. remove the trust anchors for this chains
3. attempt to view the ca (with a DB that only contains the looping certs)

So it is a really edge case that generates memory leaks.

> and (c) is this impacting the compatibility tests that mwobensmith created?
It should not
> 
> Searching through the code shows that getChain is used in some places--from
> both native code and from JS code--that make changing it tricky.


> 
> I didn't review the patch but I did notice a couple of issues when glancing
> at it.
> 
> ::: security/manager/ssl/src/nsNSSCertificate.cpp
> @@ +816,5 @@
> > +getFirstPotentialIssuer(CERTCertificate* cert, PRTime time){
> > +  ScopedCERTCertList certList(CERT_CreateSubjectCertList(nullptr,
> > +                                                         CERT_GetDefaultCertDB(),
> > +                                                         &cert->derIssuer,
> > +                                                         time, true));
> 
> Could you construct an instanced of NSSCertDBTrustDomain and call
> FindPotentialIssuers instead? That would show that we're at least being
> consistent.
Agreed, with make that change.
> 
> @@ +838,5 @@
> > +  ::mozilla::pkix::ScopedCERTCertList certChain (CERT_NewCertList());
> > +  if (!certChain) {
> > +    return nullptr;
> > +  }
> > +  CERTCertificate* dup = CERT_DupCertificate(cert);
> 
> Please always used ScopedCERTCertificate and friends.
Will do.
Flags: needinfo?(cviecco)
(In reply to Camilo Viecco (:cviecco) from comment #9)
>  Assertion within NSS when run with NSS_STRICT_SHUTDOWN:
>  Assertion failure: secmod_PrivateModuleCount == 0, at pk11util.c:88
> 
> and (b) in
> > what circumstances, if any, does Firefox actually trigger that assertion,
> 
> A site that:
> 1. has a cert that cannot be validated (dot not chain to a trust anchor)
> 2. The chain served by the server contains a loop.
> 3. And a user wanting to view the cert in the certificate viewer.
> 
> OR
> 1. insert looping special certs in the db and ee cert (with valid trust
> anchors)
> 2. remove the trust anchors for this chains
> 3. attempt to view the ca (with a DB that only contains the looping certs)
> 
> So it is a really edge case that generates memory leaks.

Thanks. That's interesting. Please explain why the patch you wrote avoids the leak? Is it because it doesn't try to determine certificate trust? Is this papering over another bug; if so, what is the bug number for the underlying bug for the leak?
Flags: needinfo?(cviecco)
The reason I am avoiding the leak is that I am papering over (NEW) Bug 1004498. I will investigate further in the NSS bug for the reason of the leak.
Flags: needinfo?(cviecco)
(In reply to Camilo Viecco (:cviecco) from comment #11)
> The reason I am avoiding the leak is that I am papering over (NEW) Bug
> 1004498. I will investigate further in the NSS bug for the reason of the
> leak.

OK. I suggest that we just fix this bug by fixing bug 1004498. That seems the safest course of action given the variety of uses of the getChain function.

It seems likely that bug 1004498 is a long-standing issue. If so, then this is a long-standing issue too. Since this doesn't seem to be killing anybody, maybe this bug doesn't need to be a super high priority?
Flags: needinfo?(brian)
Depends on: 1004498
I filed bug 1004580 with a proposed resolution to this issue.
Summary: sIX509Cert.getChain generates assertion with certificate that does not verify to a trusted anchor and whose verification path contains a loop → nsIX509Cert.getChain generates assertion with certificate that does not verify to a trusted anchor and whose verification path contains a loop
This will be fixed by removing GetChain.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: