Closed Bug 1045739 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox32 --- unaffected
firefox33 + fixed
firefox34 + fixed
firefox35 + fixed
b2g-v2.0 --- unaffected
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: cviecco, Assigned: keeler)

References

Details

(Keywords: regression)

Attachments

(5 files, 2 obsolete files)

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).
OS: Linux → All
Hardware: x86_64 → All
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
Keeler assigned to you as you r+ the change. Please let me know if you differ on my diagnosis.
Assignee: nobody → dkeeler
Flags: needinfo?(dkeeler)
Flags: needinfo?(brian)
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)
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]:
David, are you planning to propose a fix this bug in 33? Beta 6 goes to build today.
Thanks
Flags: needinfo?(dkeeler)
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)
Attached patch test (obsolete) — Splinter Review
This tests the patch you wrote. Without your patch, it fails in CheckRevocation. With the patch, it passes.
Attachment #8494677 - Flags: review?(brian)
David, Brian, Beta 8 go to build is today. Are you planning to land these changes today? Thanks
Flags: needinfo?(dkeeler)
Flags: needinfo?(brian)
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)
(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.
Nuts - that wasn't it anyway: https://tbpl.mozilla.org/?tree=Try&rev=cb74aedca078
I'll have to figure this out tomorrow.
Attached patch test fixedSplinter Review
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+
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?
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+
(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)
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)
Attached patch test patch for aurora (obsolete) — Splinter Review
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+
For posterity.
Attachment #8500713 - Attachment is obsolete: true
Attachment #8502060 - Flags: review+
You need to log in before you can comment on or make changes to this bug.