1311995 - NSS should implement the equivalent date-based distrust for WoSign/StartCom roots

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

NSS should implement the equivalent date-based distrust for WoSign/StartCom roots, as implemented by Mozilla application code for Firefox.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: patch v1

This patch copies the distinguished names and date code from the patch from bug 1309707, plus adjustments for C.

I hope I have used reasonable places to check for the blacklisting.

In the NSS classic code, function nssCertificateArray_FindBestCertificate might potentially be a better place for the check, to ensure that the full set of potential issuer certs is still tested. On the other hand, if we can identify that one of the distrusted roots is found as an issuer cert, that might be sufficient reason to stop looking for other chains?

I have tested this only manually, using slight code modifications. I had compiled using an earlier cutoff date, to verify that a test site was indeed trusted with this code change. My testing was done using the tstclnt utility. To test the libpkix code path, too, I had temporarily added a call to CERT_SetUsePKIXForValidation(PR_TRUE) in my local build.

Comment 2

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #1)
> In the NSS classic code, function nssCertificateArray_FindBestCertificate
> might potentially be a better place for the check, to ensure that the full
> set of potential issuer certs is still tested. On the other hand, if we can
> identify that one of the distrusted roots is found as an issuer cert, that
> might be sufficient reason to stop looking for other chains?

The complication with using nssCertificateArray_FindBestCertificate is, that we'd have to pass around the issance time of the leaf cert into this code.

The attached implementation, which performs the check inside cert_VerifyCertChainOld, is simpler.

Comment 3

[Kai Engert (:KaiE:)]

The distinguished named used in this patch for NSS match the ones that have been checked in for PSM at https://hg.mozilla.org/mozilla-central/rev/77880cde0de1

The implementation strategy is different. While PSM can wait until it has the full chain, this seemed difficult to do with NSS.

I used the stragegy, that for each potential issuer cert, (whether direct issuer or further up the chain), that potential issuer cert is checked for being a blacklisted one. The time from the end entity cert is used for the ok/blacklisted decision.

I have tested this patch manually, using the NSS vfychain utility.

I've tested cert chains from an expected-good, and an expected-fail site.

For both chains, I've tested the three possibly combinations of the -p argument, which is:
* no -p using the classic verify code
* one -p using the classic call but forwarded to libpkix
* two -p using the libpkix verify call

I've verified with the debugger that all tests reach the new test function, with the expected call stack.

In other words, the attached patch should implement the blocking, regardless which of NSS' internal certificate verification code is used.

Comment 4

[Kai Engert (:KaiE:)]

Here is a test site that uses a certificate that should be blocked by this change:
  https://notbefore-after-21st-test.samspin.net/

Comment 5

[Peter Gervai]

(In reply to Kai Engert (:kaie) from comment #4)
> Here is a test site that uses a certificate that should be blocked by this
> change:

My problem is exactly the opposite: sites are blocked right now which were registered before the threshold date (and the block cannot be lifted by human will, no matter how strong it is). In case of EV certs this is a pretty uncomfortable problem right now.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Peter Gervai from comment #5)
> My problem is exactly the opposite: sites are blocked right now which were
> registered before the threshold date (and the block cannot be lifted by
> human will, no matter how strong it is). In case of EV certs this is a
> pretty uncomfortable problem right now.

Blocked using Firefox? If so, please file a new bug with the details of the sites that are failing here: https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=Security%3A%20PSM

Comment 7

[Peter Gervai]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #6)

> Blocked using Firefox? 

Yes.

> If so, please file a new bug with the details of the

It's there above, inserted by YF: bug#1316342

Would you like me to open another one (the bug was opened since I wasn't even able to see _why_ it's been blocked, but the URL is in there as the ssllabs test, I didn't want too much exposure), or would it fit the purpose?

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

From what I can tell, that certificate has a notBefore on October 24 2016, which is after the cutoff date of the 21st, so it's expected that it would be treated as revoked. As for not being able to inspect the certificate, that is definitely a deficiency we're working on.

Comment 9

[Robert Relyea]

Comment on attachment 8803339 [details] [diff] [review]
patch v1

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

r+ though I'd like it if my question in pkix_build.c is addressed to make sure we don't have a potential race issue.

bob

::: lib/libpkix/pkix/top/pkix_build.c
@@ +2076,5 @@
>          PKIX_INCREF(state->validityDate);
>          validityDate = state->validityDate;
>          canBeCached = state->canBeCached;
>          PKIX_DECREF(*pValResult);
> +        targetCert = state->buildConstants.targetCert;

Is there a reason we are grabbing the targetCert here rather than below? If it's just convenience, that's fine. It it's becauce it could change between here and the end, then I worry about the targetCert disappearing on us unless we get a reference (which we will then need to free.

Comment 10

[Kai Engert (:KaiE:)]

(In reply to Robert Relyea from comment #9)
> @@ +2076,5 @@
> >          PKIX_INCREF(state->validityDate);
> >          validityDate = state->validityDate;
> >          canBeCached = state->canBeCached;
> >          PKIX_DECREF(*pValResult);
> > +        targetCert = state->buildConstants.targetCert;
> 
> Is there a reason we are grabbing the targetCert here rather than below? If
> it's just convenience, that's fine. It it's becauce it could change between
> here and the end, then I worry about the targetCert disappearing on us
> unless we get a reference (which we will then need to free.

I don't think the contents of "state" can go away, it's a parameter to the function.

I don't think state->buildConstants will change, because it's named "constant", and I don't see any assignments.

But the variable "state" is reassigned to child and parent states while the function loops and searches for best result.

Because for the desired date comparison, we must use the date from the original cert, the code uses this assignment to remember the cert that was our starting point.

Comment 11

[Kai Engert (:KaiE:)]

Checked in for NSS 3.29:
https://hg.mozilla.org/projects/nss/rev/07c29291dee3

Comment 12

[Kai Engert (:KaiE:)]

clang-format bustage fix:
https://hg.mozilla.org/projects/nss/rev/e1562d80ef2d

Comment 13

[Kai Engert (:KaiE:)]

bustage fix, uint32_t type is unknown in NSS builds, use PRUint32:
https://hg.mozilla.org/projects/nss/rev/3563294c5080

Comment 14

[Kai Engert (:KaiE:)]

additional clang-format bustage fixes:
https://hg.mozilla.org/projects/nss/rev/fbbf7d64d8ef

Comment 15

[Kai Engert (:KaiE:)]

I've combined the previous 4 commits, and landed into the NSS 3.28 branch:
https://hg.mozilla.org/projects/nss/rev/86236577102e