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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 1

[:Cykesiopka]

Attachment: bug897690_rm-AppendErrorTextUntrusted-GetChain-bad-err-msg_v1.patch

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c8b9ebbf330

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 3

[:Cykesiopka]

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

Comment 4

[:Cykesiopka]

Thanks for the review.

(Try link is in Comment 1)

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45f1e9acde3e

Comment 6

[(PTO May 7-12) Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/45f1e9acde3e