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

RESOLVED FIXED in 3.30

Status

NSS
Libraries
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

3.28
3.30
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

9 months ago
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
(Assignee)

Comment 2

9 months ago
Created attachment 8831614 [details] [diff] [review]
1334976-v1.patch

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

Updated

9 months ago
Attachment #8831614 - Attachment is obsolete: true
(Assignee)

Comment 3

9 months ago
Created attachment 8832868 [details] [diff] [review]
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.
Attachment #8832868 - Flags: review?(rrelyea)
(Assignee)

Comment 4

9 months ago
Created attachment 8832870 [details] [diff] [review]
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).
Attachment #8832870 - Flags: review?(rrelyea)
(Assignee)

Comment 5

9 months ago
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

9 months ago
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.
Attachment #8832868 - Flags: review?(rrelyea) → review+

Updated

9 months ago
Attachment #8832870 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 7

8 months ago
Created attachment 8835634 [details] [diff] [review]
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.
Attachment #8835634 - Flags: review?(rrelyea)
(Assignee)

Comment 8

8 months ago
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
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 3.30
(Assignee)

Comment 9

8 months ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

8 months ago
Created attachment 8835664 [details] [diff] [review]
1334976-certutil-v4.patch
Attachment #8835634 - Attachment is obsolete: true
Attachment #8835634 - Flags: review?(rrelyea)
Attachment #8835664 - Flags: review?(rrelyea)
Blocks: 1334127

Comment 11

8 months ago
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.
Attachment #8835664 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 12

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

all patches checked in, marking fixed
Status: REOPENED → RESOLVED
Last Resolved: 8 months ago8 months ago
Resolution: --- → FIXED
(Assignee)

Comment 13

8 months ago
Because of bug 1340788, I wonder if we should change certutil again, to use the module iteration approach, until bug 1340788 gets fixed.
(Assignee)

Updated

8 months ago
See Also: → bug 1340788
(Assignee)

Updated

8 months ago
Blocks: 1345083
You need to log in before you can comment on or make changes to this bug.