Closed Bug 1257403 Opened 5 years ago Closed 3 years ago

Don't bother verifying CA or email certs during import

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: Cykesiopka, Assigned: keeler)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file, 1 obsolete file)

(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.
See Also: → 1250258
Blocks: 1257362
Whiteboard: [psm-backlog]
Whiteboard: [psm-backlog] → [psm-cleanup]
Duplicate of this bug: 380605
Duplicate of this bug: 1337412
The same reasoning applies to email certificates.
Summary: Don't bother verifying CA certs during import → Don't bother verifying CA or email certs during import
Priority: -- → P3
Assignee: nobody → dkeeler
Priority: P3 → P1
Whiteboard: [psm-cleanup] → [psm-assigned]
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.
Attachment #8911405 - Flags: review?(cykesiopka.bmo) → review+
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.
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a189001e9886
don't bother verifying CA or email certificates when importing r=Cykesiopka
https://hg.mozilla.org/mozilla-central/rev/a189001e9886
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Duplicate of this bug: 1186059
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?
Attached patch Ignore otherName constraints (obsolete) — Splinter Review
Go ahead and open a new bug referencing these two. Thanks!
Flags: needinfo?(Franz.Sirl-kernel)
Attachment #8934177 - Attachment is obsolete: true
bug 1423112
Flags: needinfo?(Franz.Sirl-kernel)
Duplicate of this bug: 1426107
You need to log in before you can comment on or make changes to this bug.