Closed Bug 952572 Opened 6 years ago Closed 6 years ago

Hard code ANSSI(DCISS) to french gov dns space

Categories

(NSS :: Libraries, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
3.16.1

People

(Reporter: cviecco, Assigned: cviecco)

References

Details

Attachments

(3 files, 7 obsolete files)

The IGC/A / ANSSI root needs to be constrained to gouv.fr to limit the risk of missuasage of the cert.
Depends on: 743700
Attached patch name-contrain-ANSSI (obsolete) — Splinter Review
Assignee: nobody → cviecco
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.
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
Please prefer adding tests to the NSS test suite, rather than to a specific Mozilla application.
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.
Please either remove the printf statements, or add conditional compilation like using #ifdef DEBUG_cviecco. Typo "manuak".
Attached patch name-contrain-ANSSI (wip) (obsolete) — Splinter Review
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
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?
(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
Attached patch name-contrain-ANSSI (obsolete) — Splinter Review
Attachment #8374381 - Attachment is obsolete: true
Attached patch name-contrain-ANSSI (v2) (obsolete) — Splinter Review
Attachment #8390129 - Attachment is obsolete: true
Attachment #8390152 - Flags: feedback?(brian)
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)
Attached patch name-contrain-ANSSI (v3) (obsolete) — Splinter Review
Attachment #8390152 - Attachment is obsolete: true
Attached patch name-constrain-DCISS (obsolete) — Splinter Review
Attachment #8400763 - Attachment is obsolete: true
Attachment #8400776 - Flags: review?(kaie)
Attached patch dciss-testsSplinter Review
Attachment #8400777 - Flags: review?(kaie)
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 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-
Also, next to the rawANSSISubject variable, could you please add a comment with a human-readable version of the subject string?
Attached patch name-constrain-DCISS (v2) (obsolete) — Splinter Review
Attachment #8400776 - Attachment is obsolete: true
Attachment #8402848 - Flags: review?(kaie)
Comment on attachment 8402848 [details] [diff] [review]
name-constrain-DCISS (v2)

r=kaie

typo: "fo bug 952572"
Attachment #8402848 - Flags: review?(kaie) → review+
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+
Flags: needinfo?(cviecco)
Using HOST=localhost NSS_CYCLES=standard NSS_TESTS=chains time ./all.sh the tests pass through my code.
Flags: needinfo?(cviecco)
Fixing typo, keeping r+ from kaie
Attachment #8400777 - Attachment is obsolete: true
Attachment #8402942 - Flags: review+
Attachment #8400777 - Attachment is obsolete: false
Attachment #8402848 - Attachment is obsolete: true
Both patches checked in:

https://hg.mozilla.org/projects/nss/rev/742307da0792
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
OS: Linux → All
Priority: -- → P1
Hardware: x86_64 → All
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+
Depends on: 1028647
You need to log in before you can comment on or make changes to this bug.