AppendErrorTextUntrusted uses nxI509Cert::GetChain to show a misleading error message

RESOLVED FIXED in Firefox 38

Status

()

Core
Security: PSM
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: briansmith, Assigned: Cykesiopka)

Tracking

(Blocks: 1 bug)

Trunk
mozilla38
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment)

case SEC_ERROR_UNKNOWN_ISSUER:
      {
        nsCOMPtr<nsIArray> chain;
        ix509->GetChain(getter_AddRefs(chain));
        uint32_t length = 0;
        if (chain && NS_FAILED(chain->GetLength(&length)))
          length = 0;
        if (length == 1)
          errorID = "certErrorTrust_MissingChain";
        else
          errorID = "certErrorTrust_UnknownIssuer";
        break;
      }

It seems like with libpkix and with insanity::pkix, we can't really distinguish between certErrorTrust_MissingChain and certErrorTrust_UnknownIssuer easily. Both of them either build a chain or don't.

Also, the distinction between "missing chain" and "unknown issuer" is a lot more subtle than this code makes things out to be. In particular, the chain could be missing only *some* of the intermediates. Or, the chain the server sent may have the *wrong* intermediates.

Accordingly, I think we should fix this bug by just always showing the "unknown issuer" message, removing the "missing chain" error message completely.
(Assignee)

Comment 1

3 years ago
Created attachment 8561350 [details] [diff] [review]
bug897690_rm-AppendErrorTextUntrusted-GetChain-bad-err-msg_v1.patch

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c8b9ebbf330
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Attachment #8561350 - Flags: review?(dkeeler)
Comment on attachment 8561350 [details] [diff] [review]
bug897690_rm-AppendErrorTextUntrusted-GetChain-bad-err-msg_v1.patch

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

Great - r=me.

::: security/manager/locales/en-US/chrome/pipnss/pipnss.properties
@@ -266,5 @@
>  
>  certErrorIntro=%S uses an invalid security certificate.
>  
>  certErrorTrust_SelfSigned=The certificate is not trusted because it is self-signed.
>  certErrorTrust_UnknownIssuer=The certificate is not trusted because the issuer certificate is unknown.

This should probably be another bug, but it would be nice to improve the unknown issuer string to suggest that the server may be misconfigured and isn't sending the appropriate intermediate certificates.
Attachment #8561350 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 3

3 years ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #2)
> ::: security/manager/locales/en-US/chrome/pipnss/pipnss.properties
> @@ -266,5 @@
> >  
> >  certErrorIntro=%S uses an invalid security certificate.
> >  
> >  certErrorTrust_SelfSigned=The certificate is not trusted because it is self-signed.
> >  certErrorTrust_UnknownIssuer=The certificate is not trusted because the issuer certificate is unknown.
> 
> This should probably be another bug, but it would be nice to improve the
> unknown issuer string to suggest that the server may be misconfigured and
> isn't sending the appropriate intermediate certificates.

I filed Bug 1131227.
(Assignee)

Comment 4

3 years ago
Thanks for the review.

(Try link is in Comment 1)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/45f1e9acde3e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/45f1e9acde3e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.