Closed Bug 1349705 Opened 3 years ago Closed 3 years ago

Add "TUBITAK Kamu SM SSL Kok Sertifikasi - Surum 1" to NSS

Categories

(NSS :: CA Certificates Code, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kwilson, Assigned: kaie)

References

Details

(Whiteboard: Added in NSS 3.30.2, Firefox 54 and NSS 3.28.5, ESR 52.2)

Attachments

(6 files, 4 obsolete files)

This bug requests inclusion in the NSS root store of the following root certificate owned by the Government of Turkey, Kamu Sertifikasyon Merkezi (Kamu SM).
	 
Friendly Name: TUBITAK Kamu SM SSL Kok Sertifikasi - Surum 1
Cert Location: 
https://bugzilla.mozilla.org/attachment.cgi?id=8738995
http://depo.kamusm.gov.tr/ssl/SSLKOKSM.S1.cer
SHA-1 Fingerprint: 31:43:64:9B:EC:CE:27:EC:ED:3A:3F:0B:8F:0D:E4:E8:91:DD:EE:CA
SHA-256 Fingerprint: 	46:ED:C3:68:90:46:D5:3A:45:3F:B3:10:4A:B8:0D:CA:EC:65:8B:26:60:EA:16:29:DD:7E:86:79:90:64:87:16
Trust Flags: Websites
Test URL: https://testssl.kamusm.gov.tr/

Constraints: 
Please constrain this root to the following TLDs:
gov.tr, k12.tr, pol.tr, mil.tr, tsk.tr, kep.tr, bel.tr, edu.tr and org.tr

This CA has been assessed in accordance with the Mozilla project guidelines, and the certificates approved for inclusion in bug #1262809. 

The next steps are as follows:
1) A representative of the CA must confirm that all the data in this bug is correct, and that the correct certificate has been attached.
2) A Mozilla representative creates a patch with the new certificate, and provides a special test version of Firefox.
3) A representative of the CA uses the test version of Firefox to confirm (by adding a comment in this bug) that the certificate has been correctly imported and that websites work correctly.
4) The Mozilla representative requests that another Mozilla representative review the patch.
5) The Mozilla representative adds (commits) the patch to NSS, then closes this bug as RESOLVED FIXED.
6) At some time after that, various Mozilla products will move to using a version of NSS which contains the certificates. This process is mostly under the control of the release drivers for those products.
Tuğba, Please see step #1 above.
Hi Kathleen,

Thanks for approving our request.

We have reviewed all the data in this bug. We confirm all the data is correct.Also, the correct certificate has been attached.

Best regards.
Depends on: 1350859
(In reply to Kathleen Wilson from comment #0)
> 	 
> Please constrain this root to the following TLDs:
> gov.tr, k12.tr, pol.tr, mil.tr, tsk.tr, kep.tr, bel.tr, edu.tr and org.tr

I think this is the first time, where we have a request for inclusion, where the CA is to be constrained to certain domain, right from the beginning.

In the past, there were requests to apply a constraint after inclusion
  (see https://wiki.mozilla.org/CA:Root_Store_Trust_Mods#ANSSI )
and to include a CA that already contains a domain constraint embedded into the CA (HARICA bug 711594).

To implement these constraints for NSS, it will be required to implement changes at the NSS code level, potentially it can be done in a way similar to ANSSI.
(In reply to Kai Engert (:kaie) from comment #3)
> (In reply to Kathleen Wilson from comment #0)
> > 	 
> > Please constrain this root to the following TLDs:
> > gov.tr, k12.tr, pol.tr, mil.tr, tsk.tr, kep.tr, bel.tr, edu.tr and org.tr
> ...
> To implement these constraints for NSS, it will be required to implement
> changes at the NSS code level, potentially it can be done in a way similar
> to ANSSI.

Yes, please.
Since this root addition isn't as straightforward as I had thought, it will take me a couple of days until I can help with it. If there's anyone else who would like to contribute the code changes, get that done quicker, it's appreciated.
Attached file tubitak.txt
I've prepared the addition for certdata.txt (attached), not yet the constraints.
(In reply to Kai Engert (:kaie) from comment #6)
> Created attachment 8851947 [details]
> tubitak.txt
> 
> I've prepared the addition for certdata.txt (attached), not yet the
> constraints.

Kai, would it help if I separate this request into two bugs? One bug for the root inclusion, so it can be done  with the March batch of root changes. And a separate bug for the constraints that will take extra work?
But if we did the inclusion first, there would be a phase, where software is released that trusts the CA, without having the constraints applied. I had assumed that isn't acceptable.
(In reply to Kai Engert (:kaie) from comment #8)
> But if we did the inclusion first, there would be a phase, where software is
> released that trusts the CA, without having the constraints applied. I had
> assumed that isn't acceptable.

The preference is to constrain this root cert in the same NSS release that it is included in.

So, if you are able to do both the inclusion and the constraints at the same time, that would be great.

But, if you would like me to ask Keeler to add the constraints, would it be easier to handle as a separate bug?
I hope to get this done today.
I would like to document how I produced the name constraints extension code that we need to embed into NSS C code.

For reference, I compared with the existing one we have for ANSSI, which looks like this:
#define ANSSI_NAME_CONSTRAINTS                                                 \
    "\x30\x5D\xA0\x5B"                                                         \
    "\x30\x05\x82\x03" ".fr"                                                   \
    "\x30\x05\x82\x03" ".gp"                                                   \
    "\x30\x05\x82\x03" ".gf"                                                   \
    "\x30\x05\x82\x03" ".mq"                                                   \
    "\x30\x05\x82\x03" ".re"                                                   \
    "\x30\x05\x82\x03" ".yt"                                                   \
    "\x30\x05\x82\x03" ".pm"                                                   \
    "\x30\x05\x82\x03" ".bl"                                                   \
    "\x30\x05\x82\x03" ".mf"                                                   \
    "\x30\x05\x82\x03" ".wf"                                                   \
    "\x30\x05\x82\x03" ".pf"                                                   \
    "\x30\x05\x82\x03" ".nc"                                                   \
    "\x30\x05\x82\x03" ".tf"

I created a binary file with these ANSSI contents, and when dumper with the dumpasn1 tool, I get:
  0  93: SEQUENCE {
  2  91:   [0] {
  4   5:     SEQUENCE {
  6   3:       [2] 2E 66 72
       :       }
 11   5:     SEQUENCE {
 13   3:       [2] 2E 67 70
       :       }
 18   5:     SEQUENCE {
 20   3:       [2] 2E 67 66
       :       }
 25   5:     SEQUENCE {
 27   3:       [2] 2E 6D 71
       :       }
 32   5:     SEQUENCE {
 34   3:       [2] 2E 72 65
       :       }
 39   5:     SEQUENCE {
 41   3:       [2] 2E 79 74
       :       }
 46   5:     SEQUENCE {
 48   3:       [2] 2E 70 6D
       :       }
 53   5:     SEQUENCE {
 55   3:       [2] 2E 62 6C
       :       }
 60   5:     SEQUENCE {
 62   3:       [2] 2E 6D 66
       :       }
 67   5:     SEQUENCE {
 69   3:       [2] 2E 77 66
       :       }
 74   5:     SEQUENCE {
 76   3:       [2] 2E 70 66
       :       }
 81   5:     SEQUENCE {
 83   3:       [2] 2E 6E 63
       :       }
 88   5:     SEQUENCE {
 90   3:       [2] 2E 74 66
       :       }
       :     }
       :   }

Then I used NSS certutil --extNC, to create a certificate that contains a Name Constraints extension, with DNS names, and all of the names given in the initial comment of this bug.

Note that we should prefix each name with a dot ".". I assume that's intended, to restrict the created names to subdomains of the mentioned names, not to the mentioned subdomains by itself.

In other words, as an example, I assume the CA is permitted to create for hostnames such as example.gov.tr, but isn't allowed to create a certificate for hostname "gov.tr". Adding the "." prefix will achieve that, in my understanding. We did the same for ANSSI.

After I created the certificate with the extension, I extracted the binary extension and got:
00000000  30 81 80 a0 7e 30 0c 82  07 2e 67 6f 76 2e 74 72  |0...~0....gov.tr|
00000010  80 01 00 30 0c 82 07 2e  6b 31 32 2e 74 72 80 01  |...0....k12.tr..|
00000020  00 30 0c 82 07 2e 70 6f  6c 2e 74 72 80 01 00 30  |.0....pol.tr...0|
00000030  0c 82 07 2e 6d 69 6c 2e  74 72 80 01 00 30 0c 82  |....mil.tr...0..|
00000040  07 2e 74 73 6b 2e 74 72  80 01 00 30 0c 82 07 2e  |..tsk.tr...0....|
00000050  6b 65 70 2e 74 72 80 01  00 30 0c 82 07 2e 62 65  |kep.tr...0....be|
00000060  6c 2e 74 72 80 01 00 30  0c 82 07 2e 65 64 75 2e  |l.tr...0....edu.|
00000070  74 72 80 01 00 30 0c 82  07 2e 6f 72 67 2e 74 72  |tr...0....org.tr|
00000080  80 01 00                                          |...|

This binary file dumped with dumpasn1:
  0 128: SEQUENCE {
  3 126:   [0] {
  5  12:     SEQUENCE {
  7   7:       [2] '.gov.tr'
 16   1:       [0] 00
       :       }
 19  12:     SEQUENCE {
 21   7:       [2] '.k12.tr'
 30   1:       [0] 00
       :       }
 33  12:     SEQUENCE {
 35   7:       [2] '.pol.tr'
 44   1:       [0] 00
       :       }
 47  12:     SEQUENCE {
 49   7:       [2] '.mil.tr'
 58   1:       [0] 00
       :       }
 61  12:     SEQUENCE {
 63   7:       [2] '.tsk.tr'
 72   1:       [0] 00
       :       }
 75  12:     SEQUENCE {
 77   7:       [2] '.kep.tr'
 86   1:       [0] 00
       :       }
 89  12:     SEQUENCE {
 91   7:       [2] '.bel.tr'
100   1:       [0] 00
       :       }
103  12:     SEQUENCE {
105   7:       [2] '.edu.tr'
114   1:       [0] 00
       :       }
117  12:     SEQUENCE {
119   7:       [2] '.org.tr'
128   1:       [0] 00
       :       }
       :     }
       :   }

You may note a difference in the embedded content. The extension produced by NSS includes an additional zero attribute after each DNS name. This is reasonable when referring to RFC 5280, section 4.2.1.10:
  https://www.ietf.org/rfc/rfc5280.txt
  "Within this profile, the minimum and maximum fields are not used with
   any name forms, thus, the minimum MUST be zero, and maximum MUST be
   absent."

(I don't know which tool had been used to create the extension that was used for ANSSI, which didn't create add minimum value fields set to zero.)

I wrapped the above binary hexdump to

30 81 80 a0 7e
30 0c 82 07
2e 67 6f 76 2e 74 72
80 01 00
30 0c 82 07
2e 6b 31 32 2e 74 72
80 01 00
30 0c 82 07
2e 70 6f 6c 2e 74 72
80 01 00
30 0c 82 07
2e 6d 69 6c 2e 74 72
80 01 00
30 0c 82 07
2e 74 73 6b 2e 74 72
80 01 00
30 0c 82 07
2e 6b 65 70 2e 74 72
80 01 00
30 0c 82 07
2e 62 65 6c 2e 74 72
80 01 00
30 0c 82 07
2e 65 64 75 2e 74 72
80 01 00
30 0c 82 07
2e 6f 72 67 2e 74 72
80 01 00

then I replaced the relevant portions with more readable strings

30 81 80 a0 7e
30 0c 82 07
".gov.tr"
80 01 00
30 0c 82 07
".k12.tr"
80 01 00
30 0c 82 07
".pol.tr"
80 01 00
30 0c 82 07
".mil.tr"
80 01 00
30 0c 82 07
".tsk.tr"
80 01 00
30 0c 82 07
".kep.tr"
80 01 00
30 0c 82 07
".bel.tr"
80 01 00
30 0c 82 07
".edu.tr"
80 01 00
30 0c 82 07
".org.tr"
80 01 00

and then I transformed that to C code:

"\x30\x81\x80\xa0\x7e"
"\x30\x0c\x82\x07" ".gov.tr" "\x80\x01\x00"
"\x30\x0c\x82\x07" ".k12.tr" "\x80\x01\x00"
"\x30\x0c\x82\x07" ".pol.tr" "\x80\x01\x00"
"\x30\x0c\x82\x07" ".mil.tr" "\x80\x01\x00"
"\x30\x0c\x82\x07" ".tsk.tr" "\x80\x01\x00"
"\x30\x0c\x82\x07" ".kep.tr" "\x80\x01\x00"
"\x30\x0c\x82\x07" ".bel.tr" "\x80\x01\x00"
"\x30\x0c\x82\x07" ".edu.tr" "\x80\x01\x00"
"\x30\x0c\x82\x07" ".org.tr" "\x80\x01\x00"
Attached file tubitak-name-constraints-extension.der (obsolete) —
Attached file tubitak-subject-dn.der
Attached patch 1349705-constraints.patch (obsolete) — Splinter Review
This is a patch to add the new name constraints.

I'll ask both Bob and David for review, to see who can do it first.
One review should be enough.
Attachment #8852872 - Flags: review?(rrelyea)
Attachment #8852872 - Flags: review?(dkeeler)
Attached patch 1349705-constraints-v2.patch (obsolete) — Splinter Review
Assignee: nobody → kaie
Attachment #8852872 - Attachment is obsolete: true
Attachment #8852872 - Flags: review?(rrelyea)
Attachment #8852872 - Flags: review?(dkeeler)
Attachment #8852873 - Flags: review?(rrelyea)
Attachment #8852873 - Flags: review?(dkeeler)
Comment on attachment 8852873 [details] [diff] [review]
1349705-constraints-v2.patch

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

Kai: If a SEQUENCE value is the default value, it is not encoded. This is invalid DER because it explicitly encodes a DEFAULT value.
Attachment #8852873 - Flags: review-
Attached file test.c (obsolete) —
To verify that the string encodings match the expected binary data (which I extracted from certificates as attached), I used this test.c test program, which writes the encoded strings to files. The file comparison was successful.
(In reply to Ryan Sleevi from comment #16)
> Kai: If a SEQUENCE value is the default value, it is not encoded. This is
> invalid DER because it explicitly encodes a DEFAULT value.

Are you referring to the additional zeros for the minimum values?

If that's invalid, then existing NSS code produces invalid NC extensions.
Yes, that is invalid DER to encode the minimum field of it is the DEFAULT value. That is why the ANSSI constraints did not.
Thanks Ryan for your suggestion.
I've filed bug 1352064 to handle the question if the NSS tools are wrong and should be fixed.
I have the impression that it's allowed if the field is present, but let's discuss that in the other bug.

For this bug, I agree it seems better to exclude it from the encoding, as that's the way it was done in the past, and as it also matches what I've usually seen in other certs (such as the Hellenic 2011 root that we have added to NSS).

I'm trying to adjust certutil to exclude this value, so that I don't require to use a different tool to create the encoding.
Updated name constraints extension, which dumps as:

  0 101: SEQUENCE {
  2  99:   [0] {
  4   9:     SEQUENCE {
  6   7:       [2] '.gov.tr'
       :       }
 15   9:     SEQUENCE {
 17   7:       [2] '.k12.tr'
       :       }
 26   9:     SEQUENCE {
 28   7:       [2] '.pol.tr'
       :       }
 37   9:     SEQUENCE {
 39   7:       [2] '.mil.tr'
       :       }
 48   9:     SEQUENCE {
 50   7:       [2] '.tsk.tr'
       :       }
 59   9:     SEQUENCE {
 61   7:       [2] '.kep.tr'
       :       }
 70   9:     SEQUENCE {
 72   7:       [2] '.bel.tr'
       :       }
 81   9:     SEQUENCE {
 83   7:       [2] '.edu.tr'
       :       }
 92   9:     SEQUENCE {
 94   7:       [2] '.org.tr'
       :       }
       :     }
       :   }
Attachment #8852868 - Attachment is obsolete: true
Attached file test.c
Attachment #8852875 - Attachment is obsolete: true
Attachment #8852873 - Attachment is obsolete: true
Attachment #8852873 - Flags: review?(rrelyea)
Attachment #8852873 - Flags: review?(dkeeler)
Attachment #8852927 - Flags: review?(rrelyea)
Attachment #8852927 - Flags: review?(dkeeler)
Attachment #8852927 - Flags: feedback?(ryan.sleevi)
Comment on attachment 8852927 [details] [diff] [review]
1349705-constraints-v3.patch

Thanks Kai.

I examined the nameConstraints extension (didn't vet the subject encoding, but I presume you've verified it's byte-for-byte-equivalent), and the does properly restrict the issuance to labels at-or-below those specified domains.

That is, it prevents a "gov.tr" certificate from being issued. I agree it's unclear whether that's desired or not (and if not, it's just a matter of deleting the leading "."), so f+
Attachment #8852927 - Flags: feedback?(ryan.sleevi) → feedback+
Comment on attachment 8852927 [details] [diff] [review]
1349705-constraints-v3.patch

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

LGTM - just the one comment.

::: lib/certdb/genname.c
@@ +1647,5 @@
> +    "\x30\x09\x82\x07" ".org.tr"
> +
> +static const SECItem builtInNameConstraints[][2] = {
> +    NAME_CONSTRAINTS_ENTRY(ANSSI),
> +    NAME_CONSTRAINTS_ENTRY(TUBITAK1) };

Might be nice to have clang-format handle this declaration/definition if it can.
Attachment #8852927 - Flags: review?(dkeeler) → review+
(In reply to Kathleen Wilson from comment #0)
> 3) A representative of the CA uses the test version of Firefox to confirm
> (by adding a comment in this bug) that the certificate has been correctly
> imported and that websites work correctly.

Tuğba,

The test build with this root cert included is here:
https://archive.mozilla.org/pub/firefox/try-builds/kaie@kuix.de-331b1a3ecff20c883f4f5b135332cb1869243439/

Please test as described here:
https://wiki.mozilla.org/CA:How_to_apply#Testing_Inclusion
Kathleen,

We tested packages in Mac OS, windows 64 bit and 32 bit platforms. There seems no problem. 

It is OK.   

Thanks.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #25)
> > +static const SECItem builtInNameConstraints[][2] = {
> > +    NAME_CONSTRAINTS_ENTRY(ANSSI),
> > +    NAME_CONSTRAINTS_ENTRY(TUBITAK1) };
> 
> Might be nice to have clang-format handle this declaration/definition if it
> can.

ok
Attachment #8852927 - Flags: review?(rrelyea)
Comment on attachment 8852927 [details] [diff] [review]
1349705-constraints-v3.patch

The name constraints patch has been checked in:
https://hg.mozilla.org/projects/nss/rev/f52feaa506006947b546dde5b954b59aae1bad98
Attachment #8852927 - Flags: checked-in+
I've documented these new constraints on our wiki page:
https://wiki.mozilla.org/CA:Root_Store_Trust_Mods
Thanks, Kai!
Constraints checked in to NSS 3.30 branch for NSS 3.30.2, because Kathleen has requested to include these fixes with Firefox 54.
https://hg.mozilla.org/projects/nss/rev/1fefea6530e1
Constraints checked in to NSS 3.28 branch for NSS 3.28.5, because Kathleen has requested to include these fixes with Firefox 52.2.
https://hg.mozilla.org/projects/nss/rev/68fbb1b5cc75
Hi Kai,

As we know our new root is included in NSS 3.30.2 as we see in https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.30.2_release_notes. So, we could not understand why constraints are applied in a lower version? Is this situation affects our SSL certificates issued from old root if it constrained in lower version?

Thanks.
The constraints are specifically limited to your new root CA, because they are tied to the specific subject name that is contained in your new root.

The reason why this has been checked in to an older branch, is because Mozilla maintains two different branches of Firefox.

(a) Firefox feature updates like 54, 55, 56, 57, etc.

(b) The more stable Firefox ESR release, which is kept feature stable for about one. The current branch is Firefox ESR 52, and next the upcoming versions are 52.2, 52.3, 52.3, 52.4, etc.

NSS 3.30.2 is used for (a)

NSS 3.28.5 is used for (b)
(In reply to Kai Engert (:kaie) from comment #35)
> The constraints are specifically limited to your new root CA, because they
> are tied to the specific subject name that is contained in your new root.
> 
> The reason why this has been checked in to an older branch, is because
> Mozilla maintains two different branches of Firefox.
> 
> (a) Firefox feature updates like 54, 55, 56, 57, etc.
> 
> (b) The more stable Firefox ESR release, which is kept feature stable for
> about one

year

>. The current branch is Firefox ESR 52, and next the upcoming
> versions are 52.2, 52.3, 52.3, 52.4, etc.
> 
> NSS 3.30.2 is used for (a)
> 
> NSS 3.28.5 is used for (b)
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Whiteboard: Added in NSS 3.30.2, Firefox 54 and NSS 3.28.5, ESR 52.2
You need to log in before you can comment on or make changes to this bug.