Don't bother verifying CA or email certs during import

RESOLVED FIXED in Firefox 58

Status

()

P1
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: Cykesiopka, Assigned: keeler)

Tracking

(Blocks: 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [psm-assigned], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
(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.
(Reporter)

Updated

3 years ago
See Also: → bug 1250258
(Reporter)

Updated

3 years ago
Blocks: 1257362
Whiteboard: [psm-backlog]
(Reporter)

Updated

3 years ago
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 hidden (mozreview-request)
(Reporter)

Comment 5

a year ago
mozreview-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

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+
(Assignee)

Comment 6

a year ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 9

a year ago
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

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a189001e9886
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
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?
Created attachment 8934177 [details] [diff] [review]
Ignore otherName constraints
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)

Updated

a year ago
Duplicate of this bug: 1426107
You need to log in before you can comment on or make changes to this bug.