Last Comment Bug 952572 - Hard code ANSSI(DCISS) to french gov dns space
: Hard code ANSSI(DCISS) to french gov dns space
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
P1 normal (vote)
: 3.16.1
Assigned To: Camilo Viecco (:cviecco)
:
:
Mentors:
Depends on: 1028647 743700
Blocks:
  Show dependency treegraph
 
Reported: 2013-12-20 10:38 PST by Camilo Viecco (:cviecco)
Modified: 2014-06-25 08:05 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
name-contrain-ANSSI (4.55 KB, patch)
2013-12-20 10:41 PST, Camilo Viecco (:cviecco)
no flags Details | Diff | Splinter Review
name-contrain-ANSSI (wip) (34.60 KB, patch)
2014-02-11 13:01 PST, Camilo Viecco (:cviecco)
no flags Details | Diff | Splinter Review
name-contrain-ANSSI (216.05 KB, patch)
2014-03-12 16:14 PDT, Camilo Viecco (:cviecco)
no flags Details | Diff | Splinter Review
name-contrain-ANSSI (v2) (213.91 KB, patch)
2014-03-12 16:47 PDT, Camilo Viecco (:cviecco)
no flags Details | Diff | Splinter Review
name-contrain-ANSSI (v3) (6.23 KB, patch)
2014-04-02 10:40 PDT, Camilo Viecco (:cviecco)
no flags Details | Diff | Splinter Review
name-constrain-DCISS (4.59 KB, patch)
2014-04-02 10:54 PDT, Camilo Viecco (:cviecco)
kaie: review-
Details | Diff | Splinter Review
dciss-tests (7.56 KB, patch)
2014-04-02 10:56 PDT, Camilo Viecco (:cviecco)
kaie: review+
Details | Diff | Splinter Review
name-constrain-DCISS (v2) (4.98 KB, patch)
2014-04-07 12:41 PDT, Camilo Viecco (:cviecco)
kaie: review+
Details | Diff | Splinter Review
name-constrain-DCISS (v2.1) (4.98 KB, patch)
2014-04-07 15:17 PDT, Camilo Viecco (:cviecco)
cviecco: review+
Details | Diff | Splinter Review
Fix compiler warnings in lib/certdb/genname.c, function getNameExtensionsBuiltIn (1.33 KB, patch)
2014-06-05 18:05 PDT, Wan-Teh Chang
ryan.sleevi: review+
wtc: checked‑in+
Details | Diff | Splinter Review

Description User image Camilo Viecco (:cviecco) 2013-12-20 10:38:34 PST
The IGC/A / ANSSI root needs to be constrained to gouv.fr to limit the risk of missuasage of the cert.
Comment 1 User image Camilo Viecco (:cviecco) 2013-12-20 10:41:14 PST
Created attachment 8350708 [details] [diff] [review]
name-contrain-ANSSI
Comment 2 User image Kathleen Wilson 2014-01-08 11:33:53 PST
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.
Comment 3 User image Kai Engert (:kaie) 2014-01-20 07:43:08 PST
Please prefer adding tests to the NSS test suite, rather than to a specific Mozilla application.
Comment 4 User image Kai Engert (:kaie) 2014-01-20 08:07:07 PST
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 User image Kai Engert (:kaie) 2014-01-20 08:08:52 PST
Please either remove the printf statements, or add conditional compilation like using #ifdef DEBUG_cviecco. Typo "manuak".
Comment 6 User image Camilo Viecco (:cviecco) 2014-02-11 13:01:56 PST
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.
Comment 7 User image Kathleen Wilson 2014-02-11 13:40:53 PST
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 User image Kathleen Wilson 2014-02-12 10:59:16 PST
(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!
Comment 9 User image Camilo Viecco (:cviecco) 2014-03-12 16:14:09 PDT
Created attachment 8390129 [details] [diff] [review]
name-contrain-ANSSI
Comment 10 User image Camilo Viecco (:cviecco) 2014-03-12 16:47:20 PDT
Created attachment 8390152 [details] [diff] [review]
name-contrain-ANSSI (v2)
Comment 11 User image Camilo Viecco (:cviecco) 2014-03-13 14:11:37 PDT
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
Comment 12 User image Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2014-03-14 18:35:00 PDT
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.
Comment 13 User image Camilo Viecco (:cviecco) 2014-04-02 10:40:53 PDT
Created attachment 8400763 [details] [diff] [review]
name-contrain-ANSSI (v3)
Comment 14 User image Camilo Viecco (:cviecco) 2014-04-02 10:54:10 PDT
Created attachment 8400776 [details] [diff] [review]
name-constrain-DCISS
Comment 15 User image Camilo Viecco (:cviecco) 2014-04-02 10:56:25 PDT
Created attachment 8400777 [details] [diff] [review]
dciss-tests
Comment 16 User image Kai Engert (:kaie) 2014-04-07 04:47:43 PDT
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
Comment 17 User image Kai Engert (:kaie) 2014-04-07 04:54:39 PDT
Also, next to the rawANSSISubject variable, could you please add a comment with a human-readable version of the subject string?
Comment 18 User image Camilo Viecco (:cviecco) 2014-04-07 12:41:12 PDT
Created attachment 8402848 [details] [diff] [review]
name-constrain-DCISS (v2)
Comment 19 User image Kai Engert (:kaie) 2014-04-07 13:11:15 PDT
Comment on attachment 8402848 [details] [diff] [review]
name-constrain-DCISS (v2)

r=kaie

typo: "fo bug 952572"
Comment 20 User image Kai Engert (:kaie) 2014-04-07 13:21:29 PDT
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
Comment 21 User image Camilo Viecco (:cviecco) 2014-04-07 15:16:39 PDT
Using HOST=localhost NSS_CYCLES=standard NSS_TESTS=chains time ./all.sh the tests pass through my code.
Comment 22 User image Camilo Viecco (:cviecco) 2014-04-07 15:17:39 PDT
Created attachment 8402942 [details] [diff] [review]
name-constrain-DCISS (v2.1)

Fixing typo, keeping r+ from kaie
Comment 23 User image Kai Engert (:kaie) 2014-04-08 11:11:09 PDT
Both patches checked in:

https://hg.mozilla.org/projects/nss/rev/742307da0792
Comment 24 User image Wan-Teh Chang 2014-06-05 18:05:59 PDT
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
Comment 25 User image Ryan Sleevi 2014-06-07 12:58:19 PDT
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

Note You need to log in before you can comment on or make changes to this bug.