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

RESOLVED FIXED
2 years ago
2 years 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

2 years 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

2 years 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

2 years ago
Attachment #8831614 - Attachment is obsolete: true
(Assignee)

Comment 3

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Attachment #8832870 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 7

2 years 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

2 years 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: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.30
(Assignee)

Comment 9

2 years 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

2 years 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)

Comment 11

2 years 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

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

all patches checked in, marking fixed
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

2 years 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

2 years ago
See Also: → bug 1340788
(Assignee)

Updated

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