Closed
Bug 1311995
Opened 8 years ago
Closed 8 years ago
NSS should implement the equivalent date-based distrust for WoSign/StartCom roots
Categories
(NSS :: Libraries, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
3.28
People
(Reporter: KaiE, Assigned: KaiE)
References
Details
Attachments
(1 file)
18.40 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
NSS should implement the equivalent date-based distrust for WoSign/StartCom roots, as implemented by Mozilla application code for Firefox.
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee: nobody → kaie
Assignee | ||
Comment 2•8 years ago
|
||
(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.
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8803339 -
Flags: review?(rrelyea)
Assignee | ||
Comment 4•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
(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
Flags: needinfo?(grin)
Comment 7•8 years ago
|
||
(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•8 years ago
|
||
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.
Flags: needinfo?(grin)
Comment 9•8 years ago
|
||
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.
Attachment #8803339 -
Flags: review?(rrelyea) → review+
Assignee | ||
Comment 10•8 years ago
|
||
(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.
Assignee | ||
Comment 11•8 years ago
|
||
Checked in for NSS 3.29:
https://hg.mozilla.org/projects/nss/rev/07c29291dee3
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(rrelyea)
Assignee | ||
Comment 12•8 years ago
|
||
clang-format bustage fix:
https://hg.mozilla.org/projects/nss/rev/e1562d80ef2d
Assignee | ||
Comment 13•8 years ago
|
||
bustage fix, uint32_t type is unknown in NSS builds, use PRUint32:
https://hg.mozilla.org/projects/nss/rev/3563294c5080
Assignee | ||
Comment 14•8 years ago
|
||
additional clang-format bustage fixes:
https://hg.mozilla.org/projects/nss/rev/fbbf7d64d8ef
Assignee | ||
Comment 15•8 years ago
|
||
I've combined the previous 4 commits, and landed into the NSS 3.28 branch:
https://hg.mozilla.org/projects/nss/rev/86236577102e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.28
You need to log in
before you can comment on or make changes to this bug.
Description
•