Hard code ANSSI(DCISS) to french gov dns space

RESOLVED FIXED in 3.16.1

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: cviecco, Assigned: cviecco)

Tracking

(Depends on: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

4 years ago
The IGC/A / ANSSI root needs to be constrained to gouv.fr to limit the risk of missuasage of the cert.
(Assignee)

Updated

4 years ago
Depends on: 743700
(Assignee)

Comment 1

4 years ago
Created attachment 8350708 [details] [diff] [review]
name-contrain-ANSSI
(Assignee)

Updated

4 years ago
Assignee: nobody → cviecco

Comment 2

4 years ago
The CA has requested that we use the same list that Google will be using.

To my knowledge the list is:
.fr, .gp, .gf, .mq, .re, .yt, .pm, .bl, .mf, .wf, .pf, .nc, .tf 

Limiting certificate issuance to *.gouv.fr would be too restrictive, because there are several institutions that do not belong to to the "gouvernement" and don't use the ".gouv.fr" extension, such as jurisdictions, universities, Parliament, etc. However, they belong to the public sector and are allowed to be certified by the IGC/A root.

Updated

4 years ago
Summary: Name contraint ANSSI to DNS:gouv.fr → Add ability to apply name constraints to root certs, and name contraint ANSSI to DNS:gouv.fr

Comment 3

4 years ago
Please prefer adding tests to the NSS test suite, rather than to a specific Mozilla application.

Comment 4

4 years ago
Comment on attachment 8350708 [details] [diff] [review]
name-contrain-ANSSI

What's the meaning of comment "delta =4" ?

In the past, when we hardcoded comparison to identify a particular certificate, we preferred to compare the binary encoding, to ensure that encoding of visible text won't cause mistakes. Take a look at PSM's nsIdentityChecking.cpp for the comparison strategy that we had agreed on in the past. You may use "pp -t certificate-identity" to print certificate encodings that's suitable for embedding in the C code.

Comment 5

4 years ago
Please either remove the printf statements, or add conditional compilation like using #ifdef DEBUG_cviecco. Typo "manuak".
(Assignee)

Comment 6

4 years ago
Created attachment 8374381 [details] [diff] [review]
name-contrain-ANSSI (wip)

This has the current name constraint value, a psm layer test but has an awful mechanism to compare certs.
Attachment #8350708 - Attachment is obsolete: true

Comment 7

4 years ago
Camilo, Thanks for working on this. I can't tell from the patch which domains the root is being constrained to. Can you add comments to make it more easily readable/clear?

Comment 8

4 years ago
(In reply to Kathleen Wilson from comment #7)
> Camilo, Thanks for working on this. I can't tell from the patch which
> domains the root is being constrained to. Can you add comments to make it
> more easily readable/clear?

I just checked again, and it looks good. Not sure why I couldn't see it before. So you can ignore Comment #7.

Thanks!
Target Milestone: --- → 3.16.1
(Assignee)

Comment 9

4 years ago
Created attachment 8390129 [details] [diff] [review]
name-contrain-ANSSI
Attachment #8374381 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Created attachment 8390152 [details] [diff] [review]
name-contrain-ANSSI (v2)
Attachment #8390129 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8390152 - Flags: feedback?(brian)
(Assignee)

Comment 11

4 years ago
Comment on attachment 8390152 [details] [diff] [review]
name-contrain-ANSSI (v2)

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

Acutally I think this is ready for review as we had discussed this before
Attachment #8390152 - Flags: feedback?(brian) → review?(brian)
Comment on attachment 8390152 [details] [diff] [review]
name-contrain-ANSSI (v2)

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

Camilo, as we discussed before, let's write the insanity::pkix version and then backport it to NSS. I will review this after I review the insanity::pkix version.
Attachment #8390152 - Flags: review?(brian)
(Assignee)

Comment 13

3 years ago
Created attachment 8400763 [details] [diff] [review]
name-contrain-ANSSI (v3)
Attachment #8390152 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
Created attachment 8400776 [details] [diff] [review]
name-constrain-DCISS
Attachment #8400763 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8400776 - Flags: review?(kaie)
(Assignee)

Comment 15

3 years ago
Created attachment 8400777 [details] [diff] [review]
dciss-tests
Attachment #8400777 - Flags: review?(kaie)
(Assignee)

Updated

3 years ago
Summary: Add ability to apply name constraints to root certs, and name contraint ANSSI to DNS:gouv.fr → Hard code ANSSI(DCISS) to french gov dns space

Comment 16

3 years ago
Comment on attachment 8400776 [details] [diff] [review]
name-constrain-DCISS

Code looks good, I'd just like to ask for a few minor changes. Please change at least (a) and (b).

(a)
Please don't use char*, rather use const char [], because it allows you to use sizeof()

Instead of the constants 136/95, please use sizeof(name-of-array)-1 (minus 1 to exclude the zero terminator), this saves me from counting manually to confirm your numbers.

(b)
Your getNameExtensionsBuiltIn function has an arena parameter, but you don't use it.

In CERT_FindNameConstraintsExten, the data returned from either CERT_FindCertExtension or your getNameExtensionsBuiltIn will be freed unconditionally with PORT_Free(constraintsExtension.data). This means, it's correct that your call to SECITEM_CopyItem uses a NULL parameter for the arena.

Conclusion: Please remove the arena parameter from getNameExtensionsBuiltIn.

(nits)
please add a comment that mentions bug # 952572

please consistently use const/non-const for both char arrays

maybe instead of permit_france_gov, call it restrict_france_gov or constraint_france_gov?

you could move SECStatus rv into the scope where you call SECITEM_CopyItem
Attachment #8400776 - Flags: review?(kaie) → review-

Comment 17

3 years ago
Also, next to the rawANSSISubject variable, could you please add a comment with a human-readable version of the subject string?
(Assignee)

Comment 18

3 years ago
Created attachment 8402848 [details] [diff] [review]
name-constrain-DCISS (v2)
Attachment #8400776 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8402848 - Flags: review?(kaie)

Comment 19

3 years ago
Comment on attachment 8402848 [details] [diff] [review]
name-constrain-DCISS (v2)

r=kaie

typo: "fo bug 952572"
Attachment #8402848 - Flags: review?(kaie) → review+

Comment 20

3 years ago
Comment on attachment 8400777 [details] [diff] [review]
dciss-tests

The NSS tests execute in 4 cycles, standard, pkix, upgradedb, sharedb (see all.sh)

You create the test certs in a file that seems to belong to libpkix, tests/libpkix/certs/make-nc, that's why I'm asking:

Did you double check that your test gets executed in the "standard" test cycle, which uses the classic NSS engine? You can restrict the cycles being executed with NSS_CYCLES=standard prior to running all.sh

If the test gets executed with the classic verification engine, too, then it's sufficient and r=kaie
Attachment #8400777 - Flags: review?(kaie) → review+

Updated

3 years ago
Flags: needinfo?(cviecco)
(Assignee)

Comment 21

3 years ago
Using HOST=localhost NSS_CYCLES=standard NSS_TESTS=chains time ./all.sh the tests pass through my code.
Flags: needinfo?(cviecco)
(Assignee)

Comment 22

3 years ago
Created attachment 8402942 [details] [diff] [review]
name-constrain-DCISS (v2.1)

Fixing typo, keeping r+ from kaie
Attachment #8400777 - Attachment is obsolete: true
Attachment #8402942 - Flags: review+

Updated

3 years ago
Attachment #8400777 - Attachment is obsolete: false

Updated

3 years ago
Attachment #8402848 - Attachment is obsolete: true

Comment 23

3 years ago
Both patches checked in:

https://hg.mozilla.org/projects/nss/rev/742307da0792
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
OS: Linux → All
Priority: -- → P1
Hardware: x86_64 → All

Comment 24

3 years ago
Created attachment 8435426 [details] [diff] [review]
Fix compiler warnings in lib/certdb/genname.c, function getNameExtensionsBuiltIn

Ryan already reviewed this patch at https://codereview.chromium.org/316353002

Patch checked in: https://hg.mozilla.org/projects/nss/rev/2a7348f013cb
Attachment #8435426 - Flags: review?(ryan.sleevi)
Attachment #8435426 - Flags: checked-in+

Comment 25

3 years ago
Comment on attachment 8435426 [details] [diff] [review]
Fix compiler warnings in lib/certdb/genname.c, function getNameExtensionsBuiltIn

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

r+ as reviewed
Attachment #8435426 - Flags: review?(ryan.sleevi) → review+

Updated

3 years ago
Depends on: 1028647
You need to log in before you can comment on or make changes to this bug.