Don't bother verifying CA or email certs during import

RESOLVED FIXED in Firefox 58

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
2 years ago
4 months 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)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 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

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

Updated

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

Updated

2 years ago
Whiteboard: [psm-backlog] → [psm-cleanup]
(Assignee)

Updated

2 years ago
Blocks: 337785
(Assignee)

Updated

2 years ago
Duplicate of this bug: 380605
(Assignee)

Updated

2 years ago
Blocks: 1186059
(Assignee)

Updated

a year ago
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
(Assignee)

Updated

8 months ago
Priority: -- → P3
(Assignee)

Updated

7 months ago
Assignee: nobody → dkeeler
Priority: P3 → P1
Whiteboard: [psm-cleanup] → [psm-assigned]
Comment hidden (mozreview-request)
(Reporter)

Comment 5

7 months 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

7 months 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

7 months 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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a189001e9886
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Updated

7 months ago
Duplicate of this bug: 1186059

Comment 12

5 months ago
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

5 months ago
Created attachment 8934177 [details] [diff] [review]
Ignore otherName constraints
(Assignee)

Comment 14

5 months ago
Go ahead and open a new bug referencing these two. Thanks!
Flags: needinfo?(Franz.Sirl-kernel)
(Assignee)

Updated

5 months ago
Attachment #8934177 - Attachment is obsolete: true

Comment 15

5 months ago
bug 1423112
Flags: needinfo?(Franz.Sirl-kernel)

Updated

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