Closed Bug 1334976 Opened 4 years ago Closed 4 years ago

Use a new attribute in the builtins root CA list, to distinguish between Mozilla policy CAs and other CAs


(NSS :: Libraries, defect)

Not set


(Not tracked)



(Reporter: KaiE, Assigned: KaiE)




(3 files, 2 obsolete files)

No description provided.
We should set a new attribute for all CAs that are in the NSS builtins root module (nssckbi), to allow an application to distinguish between:
- a CA that has been approved by Mozilla for inclusion, according to the Mozilla CA policy,
  which Mozilla assumes are part of the public web PKI,
  and therefore should get the special treatment that Mozilla wants it to get,
  e.g. such as enforcing constraints for issued certificates, like in bug 1245280
- other CAs that have been locally installed and that cannot necessarily be treated
  as part of the public web PKI
Attached patch 1334976-v1.patch (obsolete) — Splinter Review
This patch is incomplete, it only adds the attribute to a small number of CAs for testing.
Attachment #8831614 - Attachment is obsolete: true
Code changes to support the new attribute.

- defines the new attribute CKA_NSS_MOZILLA_CA_POLICY as a constant
  in NSS' vendor space

- extends the addbuiltin command to add the new attribute, set to true,
  at the end of a certificate block, if any positive trust is present

- extends certutil. If a named certificate is printed with -L -n
  a check will be made, if it is inside a module having roots.
  If it is, the status of the CKA_NSS_MOZILLA_CA_POLICY will be printed.
  This is the same code that an application, such as Firefox, 
  can use to distinguish between locally installed CAs in foreign tokens
  containing roots, too, and roots that have been explicitly marked as
  "is a public web PKI CA approved by the Mozilla CA policy".

- that check requires the use of NSS API PK11_HasAttributeSet.
  The patch suggests to export that API.
Attachment #8832868 - Flags: review?(rrelyea)
Add the new attribute to each CA certificate that has at least one trust flag set to "explicitly trusted" (CKT_NSS_TRUSTED_DELEGATOR).
Attachment #8832870 - Flags: review?(rrelyea)
FYI, the new function "hasPositiveTrust" was created by copying the decision logic from the existing function getTrustString.

The new code in certutil's PrintCertificateAndTrust is based on the test that Firefox is using today, plus the new check.
Comment on attachment 8832868 [details] [diff] [review]

Review of attachment 8832868 [details] [diff] [review]:

r+ but I would like to see the change suggested for secutil.c


::: cmd/lib/secutil.c
@@ +3265,5 @@
> +                    }
> +                }
> +            }
> +        }
> +    }

You could use PK11_GetAllSlotsForCert() and then loop on them, rather than trying to loop over all the slots by hand.
Attachment #8832868 - Flags: review?(rrelyea) → review+
Attachment #8832870 - Flags: review?(rrelyea) → review+
Attached patch 1334976-certutil-v3.patch (obsolete) — Splinter Review
I rewrote the part that Bob requested. This is the subset.

Bob, I think I did it as you intended. I think we don't need to check for the "has roots" or "is present" attributes (like PSM did), because we check for the attribute, and it should be sufficient for our classification.

I've tested the code with variations of certdata.txt, to check that all three states are reported as expected.
Attachment #8835634 - Flags: review?(rrelyea)
Checked in.

Code changes, and fix for clang-format:

Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.30
I'm reopening, only to get better code for certutil.

I realize that the current loop isn't ideal, and if a cert is found on multiple slots, the latest result currently wins.

Instead, the "best" result should win, that is, if we can find any slot with the attribute to true.
Resolution: FIXED → ---
Attachment #8835634 - Attachment is obsolete: true
Attachment #8835634 - Flags: review?(rrelyea)
Attachment #8835664 - Flags: review?(rrelyea)
Comment on attachment 8835664 [details] [diff] [review]

Review of attachment 8835664 [details] [diff] [review]:

::: cmd/lib/secutil.c
@@ +3252,5 @@
>                  PORT_SetError(0);
>                  if (PK11_HasAttributeSet(se->slot, handle,
>                                           CKA_NSS_MOZILLA_CA_POLICY, PR_FALSE)) {
> +                    trueAttributeFound = PR_TRUE;
> +                } else if (!PORT_GetError()) {

I prefer PORG_GetError() != 0 for readability, but I realize that's just my preference (bikeshedding).

@@ +3270,1 @@
>      }

Just to be clear the semantic here is:

If any token has the true attribute, it overrides all other tokens.

I think that's the right semantic.
Attachment #8835664 - Flags: review?(rrelyea) → review+
patch v4 checked in

all patches checked in, marking fixed
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Because of bug 1340788, I wonder if we should change certutil again, to use the module iteration approach, until bug 1340788 gets fixed.
See Also: → 1340788
Blocks: 1345083
You need to log in before you can comment on or make changes to this bug.