1030204 - ANSSI(DCSSI) Root cert is not name constrained under mozilla::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Camilo Viecco (:cviecco)]

Attachment: sacoche.ac-caen.fr.pem

In 952572 the ANSII root was name constrained in NSS. We assumed that fix was working in mozilla::pkix as we where using NSS for decoding but the fix actually modified the find extension function and not the decoding.

mozilla::pkix  CheckNameConstraints is similar to NSS's CERT_CompareNameSpace  but the way we decide that we do not have extensions is differenet on mozilla::pkix, this we are not appending the fake name constraints extension to the anssi certs.

The attached cert should not be valid due to name constraints (is the ANSSI root is name constrained).

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: AC-Infrastructures.pem

Comment 2

[Camilo Viecco (:cviecco)]

Attachment: AC-Enseignement-Scolaire.pem

Comment 3

[Camilo Viecco (:cviecco)]

Attachment: AC-Education-Nationale.pem

The full chain is:
 sacoche.ac-caen.fr.pem -> AC infra -> AC-enseignment -> AC Educat -> ANSSI

Comment 4

[Camilo Viecco (:cviecco)]

Attachment: name-constrain-anssi-mozpkix

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8446093 [details] [diff] [review]
name-constrain-anssi-mozpkix

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

We spoke over vidyo that there may be a better way to approach this. Clearing review for now.

Comment 6

[Camilo Viecco (:cviecco)]

Brian, since we are not using the same function used in nss we need to implement something like the hardcoding done inside nss. So the options I see are:
1. Do something like the proposed patch in this bug to hardcode it (maybe split to a new function so #ifdef more cleanly)
2. Create a new funcion for the trustdomains so that the backcert creation can be manipulated by the trustdomain.
3 ????

What do you think? We need this on 31 (now in beta)

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Camilo Viecco (:cviecco) from comment #6)
> 1. Do something like the proposed patch in this bug to hardcode it (maybe
> split to a new function so #ifdef more cleanly)
> 2. Create a new funcion for the trustdomains so that the backcert creation
> can be manipulated by the trustdomain.
> 3 ????

The determination of whether/what extra name constraints to add to a cert should be made by NSSCertDBTrustDomain. I don't think it is best to do that in BackCert::Init; instead I think it would be better if we changed the signature of CheckNameConstraints so that it takes an extra parameter "const SECItem& nameConstraintsa", and then have pkixbuild do something like this:

  if (potentialIssuer.encodedNameConstraints) {
    rv = CheckNameConstraints(*potentialIssuer.encodedNameConstraints,
                              potentialIssuer.childCert);
    if (rv != Success) {
       return rv;
    }
  }

  // The item pointed to by hardCodedNameConstraints must live forever,
  // according to the contract of
  // TrustDomain::GetHardCodedNameConstraintsByName, so we don't free it.
  SECItem const* hardCodedNameConstraints = nullptr;
  SECStatus srv trustDomain.GetHardCodedNameConstraintsByName(
                         potentialIssuer->GetSubject(),
                         hardCodednameConstraints);
  if (srv != SECSuccess) {
    return MapSECStatus(srv);
  }
  if (hardCodedNameConstraints) {
    rv = CheckNameConstraints(*hardCodedNameConstraints,
                              potentialIssuer.childCert);
    if (rv != Success) {
       return rv;
    }
  }

Then NSSCertDBTrustDomain::GetHardCodedNameConstraintsByName can contain basically the code that you wrote.

Also, cviecco, I know you have two big Firefox 31 bugs assigned to you. If you agree with what I suggest here, you can assign this one to me and I will finish writing the patch I've half-written in this comment tomorrow, in exchange for timely reviews of my pending patches.

Comment 8

[Camilo Viecco (:cviecco)]

Agreed. Thanks for taking care of this.

Comment 9

[Camilo Viecco (:cviecco)]

Attachment: tests-anssi-name-constraints (v1)

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8446093 [details] [diff] [review]
name-constrain-anssi-mozpkix

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

I think that for Firefox 31 we should just use this patch. I have written a patch that moves the logic to the "proper" place (into NSSCertDBTrustDomain), but I would need to almost completely rewrite it for Firefox 31 (and maybe again for Firefox 32) to account for the numerous changes that have occured since Firefox 31 and Firefox 33. Since the goal of this bug is to fix the regression in an about-to-ship product and I already have mostly finished the work on a cleaner implementation, it seems OK to me to land this patch mostly as-is.

David, what do you think?

::: security/pkix/lib/pkixcheck.cpp
@@ +436,5 @@
> +                                     ".pf"
> +                                     "\x30\x05\x82\x03"
> +                                     ".nc"
> +                                     "\x30\x05\x82\x03"
> +                                     ".tf";

Well, this is not the way I would do the formatting, but it is exactly the same as in the NSS implementation, so OK.

@@ +441,5 @@
> +
> +  /* The stringified value for the subject is:
> +     E=[e-mail],CN=IGC/A,OU=DCSSI,O=PM/SGDN,L=Paris,ST=France,C=FR
> +   */
> +  const char rawANSSISubject[] = "\x30\x81\x85\x31\x0B\x30\x09\x06\x03\x55\x04"

1. static

2. File a bug against the NSS implementation to have it use static.

@@ +455,5 @@
> +                                 "\x0D\x01\x09\x01\x16\x14\x69\x67\x63\x61\x40"
> +                                 "\x73\x67\x64\x6E\x2E\x70\x6D\x2E\x67\x6F\x75"
> +                                 "\x76\x2E\x66\x72";
> +
> +  const SECItem anssi_subject = { siBuffer,

1. Please make all three of the constants static.

2. Use the style that is used throughout mozilla::pkix (and PSM) for both SECItem constants:

   const SECItem NAME_IN_ALL_CAPS = {
     siBuffer,
     reinterpret_cast<uint8_t*>(const_cast<char*>(x)),
     sizeof(x) - 1
   };

@@ +470,2 @@
>    if (!cert.encodedNameConstraints) {
> +    if (SECITEM_ItemsAreEqual(&cert.GetSubject(), &anssi_subject)) {

These additional name constraints should be enforced even if the certificate has its own name constraints extension, in addition to the name constraints that are in the certificate. However, I see that this logic is the same as the logic in NSS (i.e. the logic in NSS is wrong). Please file a bug against NSS to correct the NSS logic.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: additional-name-constraints.patch

For reference, here is the patch for mozilla::pkix that I think we should replace cviecco's implementation with (in addition to changes to NSSCertDBTrustDomain in an additional patch). Because of the FindPotentialIssuers change, it would have to be substantially changed if we were to go this route for Firefox 31/32. I don't think it is worth it.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8446093 [details] [diff] [review]
name-constrain-anssi-mozpkix

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

::: security/pkix/lib/pkixcheck.cpp
@@ +470,2 @@
>    if (!cert.encodedNameConstraints) {
> +    if (SECITEM_ItemsAreEqual(&cert.GetSubject(), &anssi_subject)) {

To clarify this: I am not asking for a change here. I think the current implementation is acceptable, though unfortunate, given that Firefox 30 has the same bug.

The primary problem here is that somebody could issue a cross-signing certificate to ANSSI with a name constraint would override our built name constraint, effectively disabling this security mechanism. But, my new implementation will fix this at least for Firefox 33.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8446093 [details] [diff] [review]
name-constrain-anssi-mozpkix

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

Using this as a stop-gap for 31 until we have a better solution seems ok to me.

::: security/pkix/lib/pkixcheck.cpp
@@ +411,5 @@
> +  // name constraints. In this case for ANSSI.
> +  const char constraintFranceGov[] = "\x30\x5D" /* sequence len = 93*/
> +                                     "\xA0\x5B" /* element len =91 */
> +                                     "\x30\x05" /* sequence len 5 */
> +                                     "\x82\x03" /* entry len 3 */

at least be consistent with the comments (e.g. either consistently use or omit '=', be consistent about spaces, etc.)

@@ +413,5 @@
> +                                     "\xA0\x5B" /* element len =91 */
> +                                     "\x30\x05" /* sequence len 5 */
> +                                     "\x82\x03" /* entry len 3 */
> +                                     ".fr"
> +                                     "\x30\x05\x82\x03" /* sequence len5, entry len 3 */

same here

@@ +455,5 @@
> +                                 "\x0D\x01\x09\x01\x16\x14\x69\x67\x63\x61\x40"
> +                                 "\x73\x67\x64\x6E\x2E\x70\x6D\x2E\x67\x6F\x75"
> +                                 "\x76\x2E\x66\x72";
> +
> +  const SECItem anssi_subject = { siBuffer,

maybe "anssiSubject"

@@ +458,5 @@
> +
> +  const SECItem anssi_subject = { siBuffer,
> +                                 reinterpret_cast<unsigned char *>(
> +                                   const_cast<char *>(rawANSSISubject)),
> +                                 sizeof(rawANSSISubject)-1};

nit: spaces around operators

@@ +463,5 @@
> +
> +  const SECItem permitFranceGovNC = { siBuffer,
> +                                      reinterpret_cast<unsigned char *>(
> +                                        const_cast<char *>(constraintFranceGov)),
> +                                      sizeof(constraintFranceGov)-1};

nit: spaces

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8451917 [details] [diff] [review]
tests-anssi-name-constraints (v1)

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

::: security/manager/ssl/tests/unit/test_name_constraints.js
@@ +270,5 @@
>      check_cert_err_generic(cert, 0, certificateUsageSSLClient);
>    }
> +
> +  // DCISS tests
> +  // The certs used here where generated by the NSS test suite and are

nit: "were", not "where"

@@ +271,5 @@
>    }
> +
> +  // DCISS tests
> +  // The certs used here where generated by the NSS test suite and are
> +  // originally located as security/nss/tests/libpkix/cert/NameConstraints.dciss*

Just security/nss/tests/libpkix/cert/, I assume, since dcisscopy.der doesn't match NameConstraints.dciss*

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Reassigning back to cviecco. I will file a separate bug for my patches.

Comment 16

[Camilo Viecco (:cviecco)]

Thank you brian and keeler.

Comment 17

[Camilo Viecco (:cviecco)]

https://tbpl.mozilla.org/?tree=Try&rev=8d0ac41834d0

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/218997c808a1
https://hg.mozilla.org/mozilla-central/rev/23c24ca46931

Comment 19

[Camilo Viecco (:cviecco)]

Comment on attachment 8451917 [details] [diff] [review]
tests-anssi-name-constraints (v1)

Approval Request Comment
[Feature/regressing bug #]: Bug 915930 
[User impact if declined]: Users will not be protected agains mississuances from DCISS (French gov CA)
[Describe test coverage new/current, TBPL]: Tests added with this group of patches
[Risks and why]:  blocking sites not from ANSSI of bad list. Minimized via tests in this patch
[String/UUID change made/needed]: None

Comment 20

[Camilo Viecco (:cviecco)]

Comment on attachment 8446093 [details] [diff] [review]
name-constrain-anssi-mozpkix

Approval Request Comment
[Feature/regressing bug #]: Bug 915930 
[User impact if declined]: Users will not be protected agains mississuances from DCISS (French gov CA)
[Describe test coverage new/current, TBPL]: Tests added with this group of patches
[Risks and why]:  blocking sites not from ANSSI of bad list. Minimized via tests in this patch
[String/UUID change made/needed]: None

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8446093 [details] [diff] [review]
name-constrain-anssi-mozpkix

31 is going to be an ESR. Taking it.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8451917 [details] [diff] [review]
tests-anssi-name-constraints (v1)

Idem for tests

Comment 23

[Sylvestre Ledru [:Sylvestre]]

FYI, it is DCSSI and ANSSI (not DCISS & ANSII)
ANSSI being the new name of DCSSI
For more information (en français, désolé):
http://fr.wikipedia.org/wiki/Direction_centrale_de_la_s%C3%A9curit%C3%A9_des_syst%C3%A8mes_d%27information
http://fr.wikipedia.org/wiki/Agence_nationale_de_la_s%C3%A9curit%C3%A9_des_syst%C3%A8mes_d%27information

Comment 24

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing for Aurora/Beta uplift.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

FYI, the rebase would have to append today. The gtb for beta 9 is today and I won't take this one for the RC.

Comment 26

[Camilo Viecco (:cviecco)]

Attachment: name-constraint-anssi-mozillapkix-aurora-beta

Comment 27

[Camilo Viecco (:cviecco)]

Attachment: test-name-constraints-anssi-aurora-beta

Comment 28

[Camilo Viecco (:cviecco)]

pushed to aurora
   https://hg.mozilla.org/releases/mozilla-aurora/rev/93a1f34ce735
   https://hg.mozilla.org/releases/mozilla-aurora/rev/8a86bcf49ef5

Comment 29

[Camilo Viecco (:cviecco)]

pushed to beta:
   https://hg.mozilla.org/releases/mozilla-beta/rev/ff83e05223eb
   https://hg.mozilla.org/releases/mozilla-beta/rev/a56eae7ad1c2

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Camilo, is it a big deal if we don't have this in 31?

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Camilo Viecco (:cviecco) from comment #29)
> pushed to beta:
>    https://hg.mozilla.org/releases/mozilla-beta/rev/ff83e05223eb
>    https://hg.mozilla.org/releases/mozilla-beta/rev/a56eae7ad1c2

Backed out of beta for build bustage in https://hg.mozilla.org/releases/mozilla-beta/rev/470b9d3ffc0f

https://tbpl.mozilla.org/php/getParsedLog.php?id=43546381&tree=Mozilla-Beta

Comment 32

[Camilo Viecco (:cviecco)]

Sledu. Yes it is a big deal. 
KWierso: thanks for the backout (I am with a really bad internet connection).

Beta try push:
https://tbpl.mozilla.org/?tree=Try&rev=b59ce11969df

Comment 33

[Camilo Viecco (:cviecco)]

Attachment: name-constraint-ANSSI-mozilla-pkix-beta

Comment 34

[Camilo Viecco (:cviecco)]

Comment on attachment 8454089 [details] [diff] [review]
name-constraint-ANSSI-mozilla-pkix-beta

Wes, thanks again for the backout. This one builds OK locally. Try url attached in previous comment.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Sylvestre, can we still take this on beta at this point?

Comment 36

[Camilo Viecco (:cviecco)]

RyanVM: can I land it? beta try was green.

Comment 37

[Sylvestre Ledru [:Sylvestre]]

It is not too late, we can take it in the last beta.

By the way, I have a question. Will this patch have an impact on the performances?

Comment 38

[Camilo Viecco (:cviecco)]

The performance impact is minimal, at most we are adding a string comparison gated by 3 integer comparisons.

Comment 39

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8454089 [details] [diff] [review]
name-constraint-ANSSI-mozilla-pkix-beta

Accepting the uplift based on the comment #20

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cc0c595e628a
https://hg.mozilla.org/releases/mozilla-beta/rev/8c758dc3a712