Remove EV treatment for expired Buypass Class 3 CA 1 root certificate

RESOLVED FIXED in Firefox 42

Status

()

Core
Security: PSM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Kathleen Wilson, Assigned: Cykesiopka)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
The following root certificate has expired, so please remove EV treatment for it.

CN = Buypass Class 3 CA 1
O = Buypass AS-983163327
C = NO

SHA-1 Fingerprint: 61:57:3A:11:DF:0E:D8:7E:D5:92:65:22:EA:D0:56:D7:44:B3:23:71
(Assignee)

Comment 1

3 years ago
Created attachment 8643549 [details]
MozReview Request: Bug 1164609 - Remove EV treatment for expired Buypass Class 3 CA 1 root certificate.

Bug 1164609 - Remove EV treatment for expired Buypass Class 3 CA 1 root certificate.

Also restores assert that appears to have been mistakenly commented out in https://hg.mozilla.org/mozilla-central/rev/1dc5cbdf0dd8#l4.12 .
Attachment #8643549 - Flags: review?(dkeeler)
(Assignee)

Comment 2

3 years ago
Due to the commented out assert, this is just a soft-blocker to Bug 1190794, but it doesn't hurt to add the relation I guess.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c9fd8b0916c
Assignee: nobody → cykesiopka.bmo
Blocks: 1190794
Status: NEW → ASSIGNED
(Reporter)

Comment 3

3 years ago
(In reply to Cykesiopka from comment #1)
> Also restores assert that appears to have been mistakenly commented out in
> https://hg.mozilla.org/mozilla-central/rev/1dc5cbdf0dd8#l4.12 .

I'm not sure it was mistakenly commented out. Please check with Keeler.
(In reply to Kathleen Wilson from comment #3)
> (In reply to Cykesiopka from comment #1)
> > Also restores assert that appears to have been mistakenly commented out in
> > https://hg.mozilla.org/mozilla-central/rev/1dc5cbdf0dd8#l4.12 .
> 
> I'm not sure it was mistakenly commented out. Please check with Keeler.

It turns out it was. The behavior we want is for it to fail on debug builds so we know about it if we make a mistake changing the EV list. It will continue to work without failing on release builds.
Comment on attachment 8643549 [details]
MozReview Request: Bug 1164609 - Remove EV treatment for expired Buypass Class 3 CA 1 root certificate.

https://reviewboard.mozilla.org/r/15111/#review13609

Great - r=me.

::: security/certverifier/ExtendedValidation.cpp:1241
(Diff revision 1)
>      // version of system NSS is installed). We will just silently avoid

This comment should be updated to reflect that we have a debug assertion but that we silently continue on release builds.

::: security/certverifier/ExtendedValidation.cpp:1251
(Diff revision 1)
> -      //PR_NOT_REACHED("Could not find EV root in NSS storage");
> +      PR_NOT_REACHED("Could not find EV root in NSS storage");

It probably is a good idea to have this so we catch any mistakes we might make when modifying the EV root list. However, I think MOZ_ASSERT(true, ...) might be more appropriate than PR_NOT_REACHED.

Also it's distressing that this got commented-out at all, but that's a completely different issue.
Attachment #8643549 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 6

3 years ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #5)
> However, I think MOZ_ASSERT(true, ...)
> might be more appropriate than PR_NOT_REACHED.

I assume you mean MOZ_ASSERT(false, ...)?

I'm also unsure what the practical difference between MOZ_ASSERT and PR_NOT_REACHED is - they both seem to do the same thing. Is it just a "don't use PR_NOT_REACHED in new code" thing?
(My searches for this information came up empty, sorry!)
Flags: needinfo?(dkeeler)
(In reply to Cykesiopka from comment #6)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #5)
> > However, I think MOZ_ASSERT(true, ...)
> > might be more appropriate than PR_NOT_REACHED.
> 
> I assume you mean MOZ_ASSERT(false, ...)?

Whoops :) Yes, that's what I meant.

> I'm also unsure what the practical difference between MOZ_ASSERT and
> PR_NOT_REACHED is - they both seem to do the same thing. Is it just a "don't
> use PR_NOT_REACHED in new code" thing?
> (My searches for this information came up empty, sorry!)

Yeah, I guess they're roughly the same. My thought was that PR_NOT_REACHED indicates that that code is logically unreachable in a way that the compiler can't necessarily deduce, but I think it commonly is used interchangeably with PR_ASSERT or MOZ_ASSERT.

In any case, I contacted the developer who accidentally commented out the line and we're reverting that change in bug 1166323, so changing the PR_NOT_REACHED to a MOZ_ASSERT seems less important now (although you're welcome to if you want).
Flags: needinfo?(dkeeler)
(Assignee)

Comment 8

3 years ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #7)
> Yeah, I guess they're roughly the same. My thought was that PR_NOT_REACHED
> indicates that that code is logically unreachable in a way that the compiler
> can't necessarily deduce, but I think it commonly is used interchangeably
> with PR_ASSERT or MOZ_ASSERT.

I see, thanks for the explanation.

> In any case, I contacted the developer who accidentally commented out the
> line and we're reverting that change in bug 1166323, so changing the
> PR_NOT_REACHED to a MOZ_ASSERT seems less important now (although you're
> welcome to if you want).

OK, I'll leave the assert alone then.
(Assignee)

Comment 9

3 years ago
Created attachment 8644820 [details] [diff] [review]
bug1164609_rm-ev-buypass-class3-ca1_v2.patch

+ Update comment to note that debug builds assert, release builds silently continue
- Revert assert uncommenting change
Attachment #8643549 - Attachment is obsolete: true
Attachment #8644820 - Flags: review+
(Assignee)

Comment 10

3 years ago
Thanks for the review.

(Try push is in comment 2)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7e9801d71ae1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.