OCSP checks are being done for expired certificates, resulting in incorrect error to the user

RESOLVED FIXED in Firefox 33, Firefox OS v2.1

Status

()

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

People

(Reporter: cviecco, Assigned: keeler)

Tracking

({regression})

unspecified
mozilla35
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32 unaffected, firefox33+ fixed, firefox34+ fixed, firefox35+ fixed, b2g-v2.0 unaffected, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Garret Robinson notices that going to https://kuix.de:5146 fives sec_error_ocsp_unauthorized_request instead of sec_error_expired certificate. I checked again and on nightly on linux(debug) getting an intermetent error. On OSX the error is on nightly (34) and aurora(33) but not on beta (32).
(Reporter)

Updated

4 years ago
status-firefox32: --- → unaffected
status-firefox33: --- → affected
status-firefox34: --- → affected
Keywords: regression, regressionwindow-wanted
OS: Linux → All
Hardware: x86_64 → All
(Reporter)

Comment 1

4 years ago
Hg blame + Manual checks lead me to say that this was due to changeset c21ea604d839 ( bug 1033563) part 1:  https://bugzilla.mozilla.org/attachment.cgi?id=8450013
(Reporter)

Comment 2

4 years ago
Keeler assigned to you as you r+ the change. Please let me know if you differ on my diagnosis.
Assignee: nobody → dkeeler
Keywords: regressionwindow-wanted
Flags: needinfo?(dkeeler)
Flags: needinfo?(brian)
(Reporter)

Comment 3

4 years ago
Clarifiation By manual check I mean I did a couple of builds and went to the site above
OK on  revision 911d02f2c02a (error expired)
not OK on revision c21ea604d839 (error ocsp)
This is almost definitely caused by part 1 of bug 1033563. I can look at this next week.
Blocks: 1033563
Flags: needinfo?(brian)
Target Milestone: --- → mozilla34
Flags: needinfo?(dkeeler)
Created attachment 8473185 [details] [diff] [review]
do-not-check-revocation-for-expired-certs.patch

I spent a little time investigating this to see if it would result in a major change. It seems like the change is very simple, but I haven't written a test for it. See this attachment; with this patch, I can add a cert error override for https://kuix.de:5146/.
[Tracking Requested - why for this release]:
tracking-firefox33: --- → ?
tracking-firefox34: --- → ?
Duplicate of this bug: 794728
status-firefox35: --- → affected
tracking-firefox33: ? → +
tracking-firefox34: ? → +
tracking-firefox35: --- → +
David, are you planning to propose a fix this bug in 33? Beta 6 goes to build today.
Thanks
Flags: needinfo?(dkeeler)
(Assignee)

Comment 9

4 years ago
Well, Brian wrote this patch, so he should probably drive this. FWIW, I had a look and the patch is probably fine as-is. It just needs a test or two.
Flags: needinfo?(dkeeler) → needinfo?(brian)
I agree my patch is great, but I probably won't have time to write tests for this within the Firefox 33 timeframe.
Flags: needinfo?(brian)
(Assignee)

Comment 11

4 years ago
Created attachment 8494677 [details] [diff] [review]
test

This tests the patch you wrote. Without your patch, it fails in CheckRevocation. With the patch, it passes.
Attachment #8494677 - Flags: review?(brian)
(Assignee)

Updated

4 years ago
Attachment #8473185 - Flags: review+
David, Brian, Beta 8 go to build is today. Are you planning to land these changes today? Thanks
Flags: needinfo?(dkeeler)
Flags: needinfo?(brian)
(Assignee)

Comment 13

4 years ago
Comment on attachment 8494677 [details] [diff] [review]
test

Let's see if we can get this reviewed and landed today.
Attachment #8494677 - Flags: review?(brian) → review?(mmc)
Flags: needinfo?(dkeeler)
Comment on attachment 8494677 [details] [diff] [review]
test

Review of attachment 8494677 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/test/gtest/pkixbuild_tests.cpp
@@ +300,5 @@
>                               nullptr/*stapledOCSPResponse*/));
>    }
>  }
> +
> +class ExpiredCertTrustDomain : public TrustDomain

This class needs a comment, like "Mock class initialized with a root DER that is only used in GetCertTrust."

@@ +308,5 @@
> +  {
> +    EXPECT_EQ(Success, rootCert.Init(rootDER.data(), rootDER.length()));
> +  }
> +
> +  virtual Result GetCertTrust(EndEntityOrCA, const CertPolicyId&,

Are these unused params because of the superclass? At least name them EndEntityOrCA ignoredEE or something.

@@ +323,5 @@
> +
> +  virtual Result FindIssuer(Input encodedIssuerName,
> +                            IssuerChecker& checker, Time time)
> +  {
> +    bool keepGoing;

explicitly initialize to false?

@@ +368,5 @@
> +{
> +  const char* rootCN = "Root CA";
> +  ScopedTestKeyPair rootKey;
> +  ByteString rootDER(CreateCert(rootCN, rootCN, EndEntityOrCA::MustBeCA,
> +                                nullptr, rootKey, nullptr));

safer to check CreateCert == SECSuccess before converting to ByteString, or is that already handled in the conversion?

@@ +378,5 @@
> +  ByteString issuerDER(CNToDERName(rootCN));
> +  EXPECT_NE(ENCODING_FAILED, issuerDER);
> +  ByteString subjectDER(CNToDERName("Expired End-Entity Cert"));
> +  EXPECT_NE(ENCODING_FAILED, subjectDER);
> +  ByteString extensions[2];

It looks like this is ignored if it is null. Are these extensions being used implicitly in the test below? If not, maybe better to pass nullptr.
Attachment #8494677 - Flags: review?(mmc) → review+
Flags: needinfo?(brian)
(Assignee)

Comment 15

4 years ago
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #14)

Thanks for the review. I've addressed most of your comments with comments in the patch. Unfortunately, now I'm getting gtest failures on windows, so I'm looking into that.

https://tbpl.mozilla.org/?tree=Try&rev=787ce0cf396c

> @@ +368,5 @@
> > +{
> > +  const char* rootCN = "Root CA";
> > +  ScopedTestKeyPair rootKey;
> > +  ByteString rootDER(CreateCert(rootCN, rootCN, EndEntityOrCA::MustBeCA,
> > +                                nullptr, rootKey, nullptr));
> 
> safer to check CreateCert == SECSuccess before converting to ByteString, or
> is that already handled in the conversion?

There's no conversion here - CreateCert returns a ByteString. Unless I'm misunderstanding your comment?

> @@ +378,5 @@
> > +  ByteString issuerDER(CNToDERName(rootCN));
> > +  EXPECT_NE(ENCODING_FAILED, issuerDER);
> > +  ByteString subjectDER(CNToDERName("Expired End-Entity Cert"));
> > +  EXPECT_NE(ENCODING_FAILED, subjectDER);
> > +  ByteString extensions[2];
> 
> It looks like this is ignored if it is null. Are these extensions being used
> implicitly in the test below? If not, maybe better to pass nullptr.

So it seems. I changed this to nullptr, but I'm getting BAD_DER failures. I'm wondering if it's related to this...
In that case, please feel free to ignore the comment about extensios since we're trying to get this into beta.
(Assignee)

Comment 17

4 years ago
Nuts - that wasn't it anyway: https://tbpl.mozilla.org/?tree=Try&rev=cb74aedca078
I'll have to figure this out tomorrow.
(Assignee)

Comment 18

4 years ago
Created attachment 8498352 [details] [diff] [review]
test fixed

Ok - looks like I figured it out: https://tbpl.mozilla.org/?tree=Try&rev=31a4cee56f2b
(For future reference, having Inputs as member variables doesn't work very well, since they can't be re-used. I'm not sure why there would be a platform-specific difference, but oh well.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d98f2e35fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/165c3fd176ec
Attachment #8494677 - Attachment is obsolete: true
Attachment #8498352 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f7d98f2e35fd
https://hg.mozilla.org/mozilla-central/rev/165c3fd176ec
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox35: affected → fixed
Resolution: --- → FIXED
Target Milestone: mozilla34 → mozilla35
(Assignee)

Comment 20

4 years ago
Created attachment 8499077 [details] [diff] [review]
patch 1/2 for beta

Approval Request Comment
[Feature/regressing bug #]: mozilla::pkix refactoring
[User impact if declined]: some sites with expired certificates will no longer be browsable (it basically depends on if the CA that issued the certificate is still keeping track of it)
[Describe test coverage new/current, TBPL]: has a test (next patch)
[Risks and why]: this patch is fairly small, so there shouldn't be much risk
[String/UUID change made/needed]: none
Attachment #8499077 - Flags: review+
Attachment #8499077 - Flags: approval-mozilla-beta?
(Assignee)

Comment 21

4 years ago
Created attachment 8499078 [details] [diff] [review]
patch 2/2 for beta

Approval Request Comment
(see previous comment - this patch is the test for this fix)
Attachment #8499078 - Flags: review+
Attachment #8499078 - Flags: approval-mozilla-beta?
Comment on attachment 8499077 [details] [diff] [review]
patch 1/2 for beta

Approved to decrease the breakage in term of certificates support.

David, any reason you didn't ask for an aurora uplift?
Attachment #8499077 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(dkeeler)
Attachment #8499078 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/d8ebbb47a75e
https://hg.mozilla.org/releases/mozilla-beta/rev/9150826eaf1b
status-b2g-v2.0: --- → unaffected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
status-firefox33: affected → fixed
(Assignee)

Comment 24

4 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #22)
> Comment on attachment 8499077 [details] [diff] [review]
> patch 1/2 for beta
> 
> Approved to decrease the breakage in term of certificates support.
> 
> David, any reason you didn't ask for an aurora uplift?

Thanks, Sylvestre. I'm still working on an aurora-specific patch, but I wanted to get the beta patch out the door as soon as it was ready, since that's more urgent.
Flags: needinfo?(dkeeler)
David - Can you please get the Aurora patch completed and nommed this week while 34 is still on Aurora?
Flags: needinfo?(dkeeler)
(Assignee)

Comment 26

4 years ago
Comment on attachment 8473185 [details] [diff] [review]
do-not-check-revocation-for-expired-certs.patch

Approval Request Comment
(same as comment 20)

This applies on aurora, so no branch-specific patch needed in this case. The test does need an aurora-specific patch, which I'll be uploading shortly.
Attachment #8473185 - Flags: approval-mozilla-aurora?
Flags: needinfo?(dkeeler)
(Assignee)

Comment 27

4 years ago
Created attachment 8500713 [details] [diff] [review]
test patch for aurora

Approval Request Comment
(see previous comment - this is the accompanying test patch)
Attachment #8500713 - Flags: review+
Attachment #8500713 - Flags: approval-mozilla-aurora?
Comment on attachment 8473185 [details] [diff] [review]
do-not-check-revocation-for-expired-certs.patch

Thanks. Aurora+
Attachment #8473185 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8500713 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/5f8dea9b01ac
https://hg.mozilla.org/releases/mozilla-aurora/rev/013c6a8663ab
status-b2g-v2.1: affected → fixed
status-firefox34: affected → fixed
Flags: in-testsuite+
Backed out for gcc 4.4 bustage.
https://hg.mozilla.org/releases/mozilla-aurora/rev/49bd5bad84d2

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=303716&repo=mozilla-aurora
status-b2g-v2.1: fixed → affected
status-firefox34: fixed → affected
Flags: needinfo?(dkeeler)
Duplicate of this bug: 462966
(Assignee)

Comment 33

4 years ago
Created attachment 8502060 [details] [diff] [review]
test patch for aurora fixed

For posterity.
Attachment #8500713 - Attachment is obsolete: true
Attachment #8502060 - Flags: review+
(Assignee)

Updated

4 years ago
status-firefox34: affected → fixed
Thanks :)
status-b2g-v2.1: affected → fixed
You need to log in before you can comment on or make changes to this bug.