1257403 - Don't bother verifying CA or email certs during import

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[:Cykesiopka]

(In reply to David Keeler [:keeler] (use needinfo?) from Bug 1250258 comment #6)
> > +  // Iterate through the filtered cert list and import verified certs into
> > +  // permanent storage.
> > +  // Note: We verify the certs in order to prevent DoS attacks. See Bug 249004.
> 
> Since mozilla::pkix doesn't have the same flaw classic verification does,
> and since there's a UI prompt before importing CA certificates when
> navigated to, I don't think we need to do this verification here.

(In reply to :Cykesiopka from Bug 1250258 comment #7)
> > Since mozilla::pkix doesn't have the same flaw classic verification does, and since there's a UI prompt before importing CA certificates when navigated to, I don't think we need to do this verification here.
> 
> Fair point. I'll defer this to another bug just so it's easier to track.

This is that bug.

Comment 3

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

The same reasoning applies to email certificates.

Comment 4

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

Attachment: bug 1257403 - don't bother verifying CA or email certificates when importing

Incidentally, this means we can remove certificateUsageVerifyCA and
certificateUsageStatusResponder from CertVerifier, since we no longer use them.

Review commit: https://reviewboard.mozilla.org/r/182872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182872/

Comment 5

[:Cykesiopka]

Comment on attachment 8911405 [details]
bug 1257403 - don't bother verifying CA or email certificates when importing

https://reviewboard.mozilla.org/r/182872/#review189256

Sorry for the delay. Looks good!

::: security/manager/ssl/nsNSSCertificateDB.cpp:502
(Diff revision 1)
>                             const nsNSSShutDownPreventionLock& /*proofOfLock*/,
> -                   /*out*/ const UniqueCERTCertList& filteredCerts)
> +                   /*out*/ const UniqueCERTCertList& temporaryCerts)
>  {
>    NS_ENSURE_ARG_MIN(numcerts, 1);
>    NS_ENSURE_ARG_POINTER(certs);
> -  NS_ENSURE_ARG_POINTER(filteredCerts.get());
> +  NS_ENSURE_ARG_POINTER(temporaryCerts.get());

Optional: We can probably get rid of the unnecessary `.get()` part while we're here.

::: security/manager/ssl/nsNSSCertificateDB.cpp
(Diff revision 1)
> -    mozilla::pkix::Result result =
> -      certVerifier->VerifyCert(node->cert, certificateUsageEmailRecipient,
> -                               mozilla::pkix::Now(), ctx, nullptr, certChain);
> -    if (result != mozilla::pkix::Success) {
> -      nsCOMPtr<nsIX509Cert> certToShow = nsNSSCertificate::Create(node->cert);
> -      DisplayCertificateAlert(ctx, "NotImportingUnverifiedCert", certToShow, locker);

"NotImportingUnverifiedCert" is no longer used after this removal and the one below, so we should remove the relevant line in pipnss.properties as well.

Comment 6

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

Comment on attachment 8911405 [details]
bug 1257403 - don't bother verifying CA or email certificates when importing

https://reviewboard.mozilla.org/r/182872/#review189256

Thanks!

> Optional: We can probably get rid of the unnecessary `.get()` part while we're here.

Sounds good.

> "NotImportingUnverifiedCert" is no longer used after this removal and the one below, so we should remove the relevant line in pipnss.properties as well.

Good catch - thanks.

Comment 7

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

Comment on attachment 8911405 [details]
bug 1257403 - don't bother verifying CA or email certificates when importing

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182872/diff/1-2/

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4d51829e11d

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a189001e9886
don't bother verifying CA or email certificates when importing r=Cykesiopka

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a189001e9886

Comment 12

[Franz.Sirl-kernel]

I just tried Thunderbird 58.0b1 and it seems the the merged bug 1337412 is not fixed completely.

1) it's still not possible to sign emails for receivers with otherName constraints
2) CA's with otherName constraints like https://crt.sh/?id=12729343 still show "cannot be verified for unknown reasons"

For 1) I'll attach a simple patch to ignore otherName constraints, I'll attach it.

Alternatively, should I open a new bug or re-open bug 1337412 for further discussion?

Comment 13

[Franz.Sirl-kernel]

Attachment: Ignore otherName constraints

Comment 14

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

Go ahead and open a new bug referencing these two. Thanks!

Comment 15

[Franz.Sirl-kernel]

bug 1423112