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

RESOLVED FIXED in Firefox 31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cviecco, Assigned: cviecco)

Tracking

({regression})

unspecified
mozilla33
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox31 fixed, firefox32 fixed, firefox33 fixed)

Details

Attachments

(10 attachments)

Assignee

Description

5 years ago
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).
Assignee

Updated

5 years ago
Assignee: nobody → cviecco
Assignee

Comment 1

5 years ago
Assignee

Comment 3

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

Updated

5 years ago
Keywords: regression
Assignee

Updated

5 years ago
Attachment #8446093 - Flags: review?(dkeeler)
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.
Attachment #8446093 - Flags: review?(dkeeler)
Assignee

Comment 6

5 years ago
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)
Flags: needinfo?(brian)
(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.
Flags: needinfo?(brian)
Assignee

Comment 8

5 years ago
Agreed. Thanks for taking care of this.
Assignee

Updated

5 years ago
Assignee: cviecco → brian
Assignee

Updated

5 years ago
Attachment #8451917 - Flags: review?(dkeeler)
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=igca@sgdn.pm.gouv.fr,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.
Attachment #8446093 - Flags: review+
Blocks: 1035942
No longer depends on: 1035942
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 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 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
Attachment #8446093 - Flags: review?(dkeeler) → review+
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*
Attachment #8451917 - Flags: review?(dkeeler) → review+
Reassigning back to cviecco. I will file a separate bug for my patches.
Assignee: brian → cviecco
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla33
Assignee

Comment 16

5 years ago
Thank you brian and keeler.
Assignee

Comment 19

5 years ago
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
Attachment #8451917 - Flags: approval-mozilla-beta?
Attachment #8451917 - Flags: approval-mozilla-aurora?
Assignee

Comment 20

5 years ago
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
Attachment #8446093 - Flags: approval-mozilla-beta?
Attachment #8446093 - Flags: approval-mozilla-aurora?
Comment on attachment 8446093 [details] [diff] [review]
name-constrain-anssi-mozpkix

31 is going to be an ESR. Taking it.
Attachment #8446093 - Flags: approval-mozilla-beta?
Attachment #8446093 - Flags: approval-mozilla-beta+
Attachment #8446093 - Flags: approval-mozilla-aurora?
Attachment #8446093 - Flags: approval-mozilla-aurora+
Comment on attachment 8451917 [details] [diff] [review]
tests-anssi-name-constraints (v1)

Idem for tests
Attachment #8451917 - Flags: approval-mozilla-beta?
Attachment #8451917 - Flags: approval-mozilla-beta+
Attachment #8451917 - Flags: approval-mozilla-aurora?
Attachment #8451917 - Flags: approval-mozilla-aurora+
This needs rebasing for Aurora/Beta uplift.
Flags: needinfo?(cviecco)
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.
Assignee

Comment 26

5 years ago
Flags: needinfo?(cviecco)
Camilo, is it a big deal if we don't have this in 31?
Flags: needinfo?(cviecco)
Assignee

Comment 32

5 years ago
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
Flags: needinfo?(cviecco)
Assignee

Comment 34

5 years ago
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.
Attachment #8454089 - Flags: checkin?(kwierso)
Flags: needinfo?(kwierso)
Sylvestre, can we still take this on beta at this point?
Flags: needinfo?(kwierso) → needinfo?(sledru)
Attachment #8454089 - Flags: checkin?(kwierso)
Assignee

Comment 36

5 years ago
RyanVM: can I land it? beta try was green.
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?
Flags: needinfo?(sledru)
Summary: ANSSI(DCISS) Root cert is not name constrained under mozilla::pkix → ANSSI(DCSSI) Root cert is not name constrained under mozilla::pkix
Assignee

Comment 38

5 years ago
The performance impact is minimal, at most we are adding a string comparison gated by 3 integer comparisons.
Comment on attachment 8454089 [details] [diff] [review]
name-constraint-ANSSI-mozilla-pkix-beta

Accepting the uplift based on the comment #20
Attachment #8454089 - Flags: approval-mozilla-beta+

Updated

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