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

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Comment 1

[Kai Engert [:KaiE:]]

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

Comment 2

[Kai Engert [:KaiE:]]

Attachment: 1334976-v1.patch

This patch is incomplete, it only adds the attribute to a small number of CAs for testing.

Comment 3

[Kai Engert [:KaiE:]]

Attachment: 1334976-code-v2.patch

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.

Comment 4

[Kai Engert [:KaiE:]]

Attachment: 1334976-certdata-v2.patch

Add the new attribute to each CA certificate that has at least one trust flag set to "explicitly trusted" (CKT_NSS_TRUSTED_DELEGATOR).

Comment 5

[Kai Engert [:KaiE:]]

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 6

[Robert Relyea]

Comment on attachment 8832868 [details] [diff] [review]
1334976-code-v2.patch

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

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

bob

::: 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.

Comment 7

[Kai Engert [:KaiE:]]

Attachment: 1334976-certutil-v3.patch

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.

Comment 8

[Kai Engert [:KaiE:]]

Checked in.

Code changes, and fix for clang-format:
  https://hg.mozilla.org/projects/nss/rev/29858a467f45
  https://hg.mozilla.org/projects/nss/rev/2acb84c21d9d

certdata.txt:
  https://hg.mozilla.org/projects/nss/rev/867f6176020d

Comment 9

[Kai Engert [:KaiE:]]

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.

Comment 10

[Kai Engert [:KaiE:]]

Attachment: 1334976-certutil-v4.patch

Comment 11

[Robert Relyea]

Comment on attachment 8835664 [details] [diff] [review]
1334976-certutil-v4.patch

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.

Comment 12

[Kai Engert [:KaiE:]]

patch v4 checked in
https://hg.mozilla.org/projects/nss/rev/3094a08b9b21

all patches checked in, marking fixed

Comment 13

[Kai Engert [:KaiE:]]

Because of bug 1340788, I wonder if we should change certutil again, to use the module iteration approach, until bug 1340788 gets fixed.