Closed Bug 1324096 Opened 7 years ago Closed 7 years ago

PSM should check the roots module for a flag, that allows to distinguish between Mozilla-CA-Policy CAs and other CAs.

Categories

(Core :: Security: PSM, defect, P1)

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: KaiE, Assigned: KaiE)

References

Details

(Whiteboard: [psm-assigned][psm-blocked])

Attachments

(2 files, 5 obsolete files)

CertVerifier.cpp in Firefox 50 contains code that looks at the root CA of a chain, and attempts to make a decision, whether the CA is a "builtin" root CA, or not.

Based on that decision, CertVerifier enables additional constraints for certificates after a cutoff date, such as requiring that a SAN extension is present.

Apparently the intention was to limit this requirement to the root CAs of the Mozilla root program.

However, this logic doesn't work in all environments.

In the Fedora Linux, we replace the NSS builtin root module with a different one, which is dynamic. And that one may contain CAs that a system administrator has installed for the whole system.

We received reports that connectivity to some sites is now broken with Firefox 50 in Fedora. This is with certificates from private CAs, where the CA has been installed for the whole system, and where the end entitiy certificate doesn't contain a SAN.

For now, I'm simply making you aware of this situation.

(The Fedora use of the alternative root module has already caused other issues, such as HPKP not working, see also https://bugzilla.redhat.com/show_bug.cgi?id=1207335 )
Thanks for the heads-up. It doesn't seem like there's anything actionable for Mozilla to do right now. Maybe the Fedora replacement builtin roots module should have multiple slots - one with the actual built in roots and one without?
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
David, thanks for your feedback.

In the meantime, we have continued the discussion, on what fix might be best. We believe that using the token name isn't the best approach. We want to try using a new attribute, as described in the following bug:
  https://bugs.freedesktop.org/show_bug.cgi?id=99453

If implemented as in the initial description, we would request a small additional check for Firefox. It could work like this:


Check for the has-roots flags (just like Firefox does today)

If the flag is found, do an additional test, before making the final decision:

Check if the certificate object contains a bool attribute CKA_MOZILLA_CA_POLICY, by using the function PK11_GetAttributes().

If the attribute cannot be found, do exactly what you do today (assume it's a Mozilla CA).

If the attribute was found with the value TRUE, do what you do today (it's a Mozilla CA).

If the attribute was found, but with the value FALSE, then treat it as not-a-Mozilla-CA.

Once we have the details (e.g. exact attribute naming) settled in the p11-kit project, then we'd help with the patch to get this implemented.

Would this approach be acceptable for you?
Status: RESOLVED → REOPENED
Flags: needinfo?(dkeeler)
Resolution: WONTFIX → ---
Summary: A CA stored in a root CA module isn't necessarily a CA from the Mozilla root program → PSM should check the roots module for a flag, that allows to distinguish between Mozilla-CA-Policy CAs and other CAs.
Assignee: nobody → kaie
Sounds great!
Flags: needinfo?(dkeeler)
Priority: -- → P1
Whiteboard: [psm-assigned]
Attached patch 1324096-v1.patch (obsolete) — Splinter Review
Depends on PK11_HasAttributeSet, which isn't exported yet.
Depends on: 1334976
Attached patch 1324096-v2.patch (obsolete) — Splinter Review
minor whitespace and a typo fix
Attachment #8831611 - Attachment is obsolete: true
Whiteboard: [psm-assigned] → [psm-assigned][psm-blocked]
Attached patch 1324096-v3.patch (obsolete) — Splinter Review
patch v3, influenced by review comments from Bob Relyea.
As soon as the NSS revision from bug 1334976 is uplifted into mozilla-central, I'll run a try build to check it's working as expected, and then I'll request review from David.
Attachment #8832872 - Attachment is obsolete: true
Note, that the suggested logic from comment 2 has changed.

It shouldn't be necessary to check if it's a "roots" module that contains the cert, it should be sufficient if the attribute is present, and set to true, in any slot.

Also, I'd like to change the behaviour for "attribute not present", and want it to be treated as "not a builtin".

The reason is, in the p11-kit-trust module, there are many code pathes that can create a certificate, and it would be difficult to ensure that the attribute is always present.

Also, if anyone else ever creates a replacement module for libnssckbi.so, they might not realize that it's necessary to set the attribute. Handling "attribute missing" as "not a builtin" is the safer approach, to avoid that CAs are accidentally treated as being a Mozilla Policy CA.
Comment on attachment 8835650 [details] [diff] [review]
1324096-v3.patch

m-c has picked up the required NSS changes, this is now ready for review.

Try build here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dde8e367526d9bf8e366548c7cd6045907f0583
Attachment #8835650 - Flags: review?(dkeeler)
Comment on attachment 8835650 [details] [diff] [review]
1324096-v3.patch

oops (patch doesn't build)
Attachment #8835650 - Attachment is obsolete: true
Attachment #8835650 - Flags: review?(dkeeler)
While the regular scenario (root CA present once in root module) seems to work, the additional scenario (root CA also present in database) doesn't work yet.

TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/unit/test_cert_isBuiltInRoot_reload.js | run_test - [run_test : 113] GeoTrust root is a built-in - false == true

I'll need to debug why it isn't working.
The test failed, because I had change "find cert in modules" to a different approach suggested by Bob.

But the suggested PK11_GetAllSlotsForCert doesn't find all slots! If the input cert was obtained from a lookup using a dbkey, as that test case does, then PK11_GetAllSlotsForCert doesn't return the other builtin module slot!

I've changed my patch to use the module iteration that PSM currently uses, and with that change, we can find the slot that has the attribute.
Attached patch 1324096-v5.patchSplinter Review
this patch passes the test cases locally.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b6b3e8ce375c7f1428faa42083968192d6039d3
Attachment #8838626 - Attachment is obsolete: true
(In reply to Kai Engert (:kaie) from comment #13)
> If the
> input cert was obtained from a lookup using a dbkey, as that test case does,
> then PK11_GetAllSlotsForCert doesn't return the other builtin module slot!

FindCertByDBKey is an internal function of PSM, which resulst in a call to CERT_FindCertByIssuerAndSN.

I've filed bug 1340788 to track that issue separately.

Given that bug, I suggest that for this enhancement, we use the classic module list iteration approach (as implemented in patch v5), at least until that bug is fixed.
Target Milestone: --- → mozilla54
Comment on attachment 8838843 [details] [diff] [review]
1324096-v5.patch

David, could you please review?

Given that you already have test cases for both categories of builtin and non-builtin CAs (which was a great help for me), it seems we are covered.

I think the try build looks ok, only intermittent bugs, which seem unrelated to security.

Also, I've tested this patch in Fedora, using the local patches from NSS and p11-kit, and I could confirm that switching this new policy flag changes behavior of internal sites as expected.

It would be good to have this fixed for FF52.
Thanks in advance
Attachment #8838843 - Flags: review?(dkeeler)
Comment on attachment 8838843 [details] [diff] [review]
1324096-v5.patch

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

Great - thanks, Kai!
I noted a few nits, but otherwise looks good to me.
(I think you're right that we have sufficient test coverage already.)

::: security/certverifier/CertVerifier.cpp
@@ +149,5 @@
>        // PK11_HasRootCerts should return true if and only if the given slot has
>        // an object with a CKA_CLASS of CKO_NETSCAPE_BUILTIN_ROOT_LIST, which
>        // should be true only of the builtin root list.
>        // If we can find a copy of the given certificate on the slot with the
>        // builtin root list, that certificate must be a builtin.

Let's also update the documentation to say the certificate has to have the CKA_NSS_MOZILLA_CA_POLICY attribute set to true.

@@ +151,5 @@
>        // should be true only of the builtin root list.
>        // If we can find a copy of the given certificate on the slot with the
>        // builtin root list, that certificate must be a builtin.
> +      if (PK11_IsPresent(slot) && PK11_HasRootCerts(slot)) {
> +        CK_OBJECT_HANDLE handle = PK11_FindCertInSlot(slot, cert, NULL);

nit: s/NULL/nullptr/

@@ +153,5 @@
>        // builtin root list, that certificate must be a builtin.
> +      if (PK11_IsPresent(slot) && PK11_HasRootCerts(slot)) {
> +        CK_OBJECT_HANDLE handle = PK11_FindCertInSlot(slot, cert, NULL);
> +        if (handle != CK_INVALID_HANDLE) {
> +          if (PK11_HasAttributeSet(slot, handle, CKA_NSS_MOZILLA_CA_POLICY,

Seems like we could combine these two conditions:

if (handle != CK_INVALID_HANDLE &&
    PK11_HasAttributeSet(slot, handle, CKA_NSS_MOZILLA_CA_POLICY, false)) {
...

@@ +154,5 @@
> +      if (PK11_IsPresent(slot) && PK11_HasRootCerts(slot)) {
> +        CK_OBJECT_HANDLE handle = PK11_FindCertInSlot(slot, cert, NULL);
> +        if (handle != CK_INVALID_HANDLE) {
> +          if (PK11_HasAttributeSet(slot, handle, CKA_NSS_MOZILLA_CA_POLICY,
> +                                   PR_FALSE)) {

nit: s/PR_FALSE/false/

@@ +155,5 @@
> +        CK_OBJECT_HANDLE handle = PK11_FindCertInSlot(slot, cert, NULL);
> +        if (handle != CK_INVALID_HANDLE) {
> +          if (PK11_HasAttributeSet(slot, handle, CKA_NSS_MOZILLA_CA_POLICY,
> +                                   PR_FALSE)) {
> +            /* Attribute was found, and is set to TRUE */

nit: //-style comments are preferred over /* */-style
Attachment #8838843 - Flags: review?(dkeeler) → review+
Attached patch 1324096-v6.patch (obsolete) — Splinter Review
David, thanks a lot for the r+ of the new logic.

I've addressed all your requests. I realized it makes sense to rewrite the whole comment, to give more context about past and presence. I'll land this updated patch, but let me know if there's anything about the comment that you'd like to have worded differently.
oops. I will exclude that "build = false" change when landing (it's something I need for successful building in my local environment.)
Attached patch 1324096-v7.patchSplinter Review
Attachment #8839983 - Attachment is obsolete: true
Pushed by kaie@kuix.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ded5e348dad
PSM should check the roots module for a flag, that allows to distinguish between Mozilla-CA-Policy CAs and other CAs, r=dkeeler
Looks great - thanks for taking care of this.
https://hg.mozilla.org/mozilla-central/rev/7ded5e348dad
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Firefox 54 now seems to be breaking newly installed self-signed SSL certificates and I suspect the new requirement for CKA_NSS_MOZILLA_CA_POLICY is the cause.  For ISVs that depend on installing self-signed certificates, I cannot find a way to make it trusted since the switch to Firefox 54.  More information available here: https://stackoverflow.com/questions/44550970.  I would be happy to open and track in a separate bug report as well if needed.
This is probably due to bug 1294580. I'll also comment on stackoverflow, but what you'll probably want to do is generate another self-signed certificate with the same subject, issuer, and public key as the one you're trying to trust. However, instead of end-entity extensions, you'll want to specify that it's a CA certificate with "basicConstraints:cA" and that it can issue certificates with "keyUsage:cRLSign,keyCertSign". It might also be a good idea to add a nameConstraints extension to restrict it to only apply to a certain set of domains. If you add that certificate to Firefox's trust DB, everything should work as before.
Agreed, the self-signed certificate issues are mostly likely tied to bug 1294580.  Will reply there.
See Also: → 880269
You need to log in before you can comment on or make changes to this bug.