Closed Bug 1150114 Opened 9 years ago Closed 9 years ago

sec_error_cert_not_in_name_space for https://www.pki.bayern.de/ in Firefox 37 but not earlier versions

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox37 - wontfix
firefox38 + fixed
firefox39 + fixed
firefox40 + fixed

People

(Reporter: briansmith, Assigned: keeler)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

> We are getting squeals from one of our external subCAs (ie rootsigned with 
> nameConstraints).  Apparently some of their certs that worked through FF36
> are now throwing errors in FF37.  Other browsers are not affected.  I have
> confirmed this.
> 
> The SSL in question is issued to https://www.pki.bayern.de and the
> nameconstraint in the ICA is for bayern.de.  Is FF now requiring third and
> higher levels to be explicitly called out in the nameConstraints?
>
> Are you aware of changes made to nameConstraints in FF37, or advise on
> an (urgent) course of action? 

This is most likely caused by bug 970542 or bugs related to it.
Clarifying my description:

If you have the nameConstraint "domain.com", FF 36 and prior allowed certs with "example.domain.com" and now FF37 does not.  FF37 gives the Error code: sec_error_cert_not_in_name_space.  Other browsers continue to allow the use.

Reading this https://hg.mozilla.org/mozilla-central/rev/cfe200a463ab seems to say that now nameConstraints would need to include domain.com AND/OR .domain.com (note the leading dot).

If so, this is a significant change in policy regarding the use of nameConstraints by CAs.  A quick review of nameConstrained subCAs from other CAs show a mixed use of the domain.com AND .domain.com formats.

Feedback would be much appreciated.
I don't think that's the issue. The intermediate (subject "C=DE, ST=Bayern, O=Freistaat Bayern, CN=Bayerische SSL-CA-2014-02", serial number 65:7e:eb:fc:ab:b4:b0:8f:fc:43:94:65:15:e0:2f:32:40:57:23:cf) has a directory name entry in the permitted subtrees of the name constraint extension specified as "C = DE, ST = Bayern, O = Freistaat Bayern". The strings are encoded as PrintableString (ASN.1 tag 0x13). In the subject of the end-entity certificate, some strings are encoded as UTF8 (ASN.1 tag 0x0C). These do not match, so mozilla::pkix reports that the certificate is not in the name space permitted by the intermediate. Note that rfc 5280 states: "name restrictions MUST be stated identically to the encoding used in the subject field or subjectAltName extension" ( http://tools.ietf.org/html/rfc5280#section-4.2.1.10 )
(In reply to David Keeler [:keeler] (use needinfo?) from comment #2)
> I don't think that's the issue. The intermediate (subject "C=DE, ST=Bayern,
> O=Freistaat Bayern, CN=Bayerische SSL-CA-2014-02", serial number
> 65:7e:eb:fc:ab:b4:b0:8f:fc:43:94:65:15:e0:2f:32:40:57:23:cf) has a directory
> name entry in the permitted subtrees of the name constraint extension
> specified as "C = DE, ST = Bayern, O = Freistaat Bayern". The strings are
> encoded as PrintableString (ASN.1 tag 0x13). In the subject of the
> end-entity certificate, some strings are encoded as UTF8 (ASN.1 tag 0x0C).
> These do not match, so mozilla::pkix reports that the certificate is not in
> the name space permitted by the intermediate. Note that rfc 5280 states:
> "name restrictions MUST be stated identically to the encoding used in the
> subject field or subjectAltName extension" (
> http://tools.ietf.org/html/rfc5280#section-4.2.1.10 )

Yes, exactly. If you can, one way to work around this issue would be to re-issue the intermediate certificate with the name constraint encoded using UTF8String. Another way to work around this is to have the customer re-issue the end-entity certificates with the subject name encoded as PrintableString.
Intermediate cetificate's directoryName constraint:

    SEQUENCE(3 elem)
      SET(1 elem)
        SEQUENCE(2 elem)
          OBJECT IDENTIFIER: 2.5.4.6
          PrintableString: DE
      SET(1 elem)
        SEQUENCE(2 elem)
          OBJECT IDENTIFIER: 2.5.4.8
          PrintableString: Bayern             <--- MISMATCH #1
      SET(1 elem)
        SEQUENCE(2 elem)
          OBJECT IDENTIFIER: 2.5.4.10
          PrintableString: Freistaat Bayern   <--- MISMATCH #2

End-entity certificate's subject field:

    SEQUENCE(6 elem)
      SET(1 elem)
        SEQUENCE(2 elem)
          OBJECT IDENTIFIER: 2.5.4.6
          PrintableString: DE
      SET(1 elem)
        SEQUENCE(2 elem)
          OBJECT IDENTIFIER: 2.5.4.8
          UTF8String: Bayern                  <-- MISMATCH #1
      SET(1 elem)
        SEQUENCE(2 elem)
          OBJECT IDENTIFIER: 2.5.4.10
          UTF8String: Freistaat Bayern        <-- MISMATCH #2
      SET(1 elem)
        SEQUENCE(2 elem)
          OBJECT IDENTIFIER: 2.5.4.11
          UTF8String: ldbv
      SET(1 elem)
        SEQUENCE(2 elem)
          OBJECT IDENTIFIER: 2.5.4.3
          UTF8String: www.pki.bayern.de
      SET(1 elem)
        SEQUENCE(2 elem)
          OBJECT IDENTIFIER: 2.5.4.5
          PrintableString: 1000002
[Tracking Requested - why for this release]:
Compatibility regression. Plus, we want CAs to use name constraints in the way that this CA is doing, and our overly-strict checking is counterproductive to that goal.

Currently, the code does a binary comparison of the entire RDN using InputsAreEqual. Instead, we need to parse each RDN and compare the values using a function that allows PrintableString and UTF8String to be used interchangeably.
Assignee: brian → nobody
I won't have time to write the patch and tests for this this week, but I will have time to review a patch and tests if somebody else can write them.
Aaah, so I was looking in the right quarry but under the wrong rock! Thank you for your help in diagnosing this.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #2)
> These do not match, so mozilla::pkix reports that the certificate is not in
> the name space permitted by the intermediate. Note that rfc 5280 states:
> "name restrictions MUST be stated identically to the encoding used in the
> subject field or subjectAltName extension" (
> http://tools.ietf.org/html/rfc5280#section-4.2.1.10 )

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #5)
> [Tracking Requested - why for this release]:
> Compatibility regression. Plus, we want CAs to use name constraints in the
> way that this CA is doing, and our overly-strict checking is
> counterproductive to that goal.

I don't think that these conclusions are true. As I observed on other occasions (bug 1008133 comment 5 e.g.), RFC 5280 requires more than a simple binary comparison when testing DNs for equality. What section 4.2.1.10 also says is

   When applying restrictions of the form directoryName, an
   implementation MUST compare DN attributes.  At a minimum,
   implementations MUST perform the DN comparison rules specified in
   Section 7.1.

And section 7.1 states, inter alia:

                                     Conforming implementations MUST
   support UTF8String and PrintableString.  RFC 3280 required only
   binary comparison of attribute values encoded in UTF8String, however,
   this specification requires a more comprehensive handling of
   comparison.
 
and

   Conforming implementations MUST use the LDAP StringPrep profile
   (including insignificant space handling), as specified in [RFC4518],
   as the basis for comparison of distinguished name attributes encoded
   in either PrintableString or UTF8String.

The current implementation of mozilla::pkix apparently does not comply with three of these four MUSTs from RFC 5280, so it's hardly the CA which is to blame.
(In reply to Kaspar Brand from comment #8)
>    Conforming implementations MUST use the LDAP StringPrep profile
>    (including insignificant space handling), as specified in [RFC4518],
>    as the basis for comparison of distinguished name attributes encoded
>    in either PrintableString or UTF8String.
> 
> The current implementation of mozilla::pkix apparently does not comply with
> three of these four MUSTs from RFC 5280, so it's hardly the CA which is to
> blame.

The fact that we forgo implementing StringPrep is intentional and I hope we continue to not do StringPrep because it is a ridiculous relic of X.509's LDAP heritage.

However, I agree that we should match equal PrintableString and UTF8String values, and that is now what this bug is about.
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #5)
> [Tracking Requested - why for this release]:
> Compatibility regression. Plus, we want CAs to use name constraints in the
> way that this CA is doing, and our overly-strict checking is
> counterproductive to that goal.

Do you have an idea of the magnitude of the impact of this bug? Would you lobby to ship a point release to fix just this issue?
Flags: needinfo?(dkeeler)
Flags: needinfo?(brian)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #10)
> Do you have an idea of the magnitude of the impact of this bug? Would you
> lobby to ship a point release to fix just this issue?

IDK. I was going to ask you the same questions.
Flags: needinfo?(brian)
If this issue were more widespread, I would expect many more bug reports. I can only find this one, so I don't think we need to ship a point release to fix this (although maybe it's just that affected users haven't yet upgraded to 37). We should certainly fix it in 38, since that's an ESR.
Flags: needinfo?(dkeeler)
I also receive the same error on https://www1.hi-tier.de/HitCom/ with Firefox 37.0.1
Is the error caused by the certificate from hi-tier.de or by the "Bayerische SSL-CA-2014-01" CA ?
I'm just a user from this site.
https://www.statistik.bayern.de seems also be affected by this bug. No way to enter the site, need so switch browser.
(Nobody seems assigned to fix this yet, so resetting status to NEW so that this bug stops misleading me).
Status: ASSIGNED → NEW
David, can you provide a patch for 38? Thanks

Not tracking for 37
Flags: needinfo?(dkeeler)
I have a patch in progress. It should be ready for review soon.
Assignee: nobody → dkeeler
Flags: needinfo?(dkeeler)
Status: NEW → ASSIGNED
(In reply to David Keeler [:keeler] (use needinfo?) from comment #18)
> I have a patch in progress. It should be ready for review soon.

Hi there,

that sounds good to me. 
At witch time or date we can expect for the patch. We have big issues and need it quick.


PS: I´m not sure to use this channel in the right way - I´m sorry for an fault
Flags: needinfo?(dkeeler)
Hopefully we'll fix this in Firefox 38, which is scheduled to ship the week of May 12, 2015. It will land in pre-release channels before then, so you could use one of those before then (after we land the fix, that is).
Flags: needinfo?(dkeeler)
The problem also exists for https://www.bayern.de (which can be accessed without SSL) and extranet sites like https://….bayern.testa-de.net (SSL only).
Attached patch patch (obsolete) — Splinter Review
I think this is what we want to do, but I may be misunderstanding how RDNs/AVAs work - let me know what you think.
Attachment #8590986 - Flags: review?(brian)
Comment on attachment 8590986 [details] [diff] [review]
patch

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

Looks pretty good.

::: security/pkix/lib/pkixnames.cpp
@@ +1297,5 @@
> +  Result rv = der::ExpectTagAndGetValue(constraintRDN, der::SEQUENCE,
> +                                        constraintAVA);
> +  if (rv != Success) {
> +    return rv;
> +  }

I think we should have a function called ParseAVA like this:

Result ReadAVA(Reader& rdn,
               /*out*/ Input& attributeType,
               /*out*/ uint8_t& attributeValueTag,
               /*out*/ Input& attributeValue);

Then we could use it here twice, and also use it within SearchWithinAVA. That would remove all the repeated code for parsing AVAs and make it easier to see that this code is correct.

Also, if we do this, I think this function will be simple enough that we can inline it into the caller like before. That seems better because your comment that explains this code is on top of that function.

@@ +1352,5 @@
> +  if (constraintAttributeValueTag == presentedAttributeValueTag ||
> +      (constraintAttributeValueTag == der::Tag::UTF8String &&
> +       presentedAttributeValueTag == der::Tag::PrintableString) ||
> +      (constraintAttributeValueTag == der::Tag::PrintableString &&
> +       presentedAttributeValueTag == der::Tag::UTF8String)) {

You can write this as:

matches = (constraintAttributeValueTag == ...)

@@ +1353,5 @@
> +      (constraintAttributeValueTag == der::Tag::UTF8String &&
> +       presentedAttributeValueTag == der::Tag::PrintableString) ||
> +      (constraintAttributeValueTag == der::Tag::PrintableString &&
> +       presentedAttributeValueTag == der::Tag::UTF8String)) {
> +    matches = true;

Should we also check the bytes of one of the values to ensure that they are all only PrintableString (ASCII) characters? Otherwise we'd be allowing a UTF-8-encoded PrintableString to match a UTF8String, which seems unnecessarily liberal.

Of course, when we do the binary comparison above and elsewhere, we allow invalid UTF8Strings and invalid PrintableStrings. But, this seems a little different and more prone for something to go wrong since we're matching two different encodings.

@@ +1354,5 @@
> +       presentedAttributeValueTag == der::Tag::PrintableString) ||
> +      (constraintAttributeValueTag == der::Tag::PrintableString &&
> +       presentedAttributeValueTag == der::Tag::UTF8String)) {
> +    matches = true;
> +    return Success;

In particular, no need for this return statement.

@@ +1391,5 @@
>  //     permittedSubtrees wherever possible.
>  //
>  // Consequently, we implement the comparison in the simplest possible way. For
>  // permittedSubtrees, we rely on implementations to follow that MUST-level
>  // requirement for compatibility. For excludedSubtrees, we simply prohibit any

Replace "Consequently, we implement the comparison in the simplest possible way. For permittedSubtrees, we rely on implementations to follow that MUST-level requirement for compatibility." with your text. (Move it up here.)

@@ +1396,5 @@
>  // non-empty directoryName constraint to ensure we are not being too lenient.
>  // We support empty DirectoryName constraints in excludedSubtrees so that a CA
>  // can say "Do not allow any DirectoryNames in issued certificates."
> +//
> +// The MUST-level requirement for compatibility is relaxed in the case of

Prefix this with "For permittedSubtrees, "

@@ +1400,5 @@
> +// The MUST-level requirement for compatibility is relaxed in the case of
> +// PrintableString and UTF8String. That is, if a name constraint has been
> +// encoded using UTF8String and the presented ID has been encoded with a
> +// PrintableString (or vice-versa), they are considered to match if they are
> +// equal everywhere except for the tag identifying the encoding.

Seems like "See" should be on the above line.

::: security/pkix/test/gtest/pkixnames_tests.cpp
@@ +2421,5 @@
>    },
> +
> +  /////////////////////////////////////////////////////////////////////////////
> +  // DirectoryName name constraint tests
> +  { RDN(OU("Example Organization")) + RDN(CN("example.com")), NO_SAN,

This should have a comment like "// One AVA per RDN"

@@ +2431,5 @@
> +    RDN(OU("Example Organization") + CN("example.com")), NO_SAN,
> +    GeneralSubtree(DirectoryName(Name(RDN(OU("Example Organization") +
> +                                          CN("example.com"))))),
> +    Success, Result::ERROR_CERT_NOT_IN_NAME_SPACE
> +  },

If we are going to have the above, why not also have the cases where the subject DN is one AVA per RDN and the constraint is one RDN with multiple AVAs, and vice versa? In particular, this would demonstrate that we match constraints by comparing *whole* RDNs and that a constraint whose first RDN's attributes is a prefix of the subject DN's first RDN's attributes is not a match.

@@ +2432,5 @@
> +    GeneralSubtree(DirectoryName(Name(RDN(OU("Example Organization") +
> +                                          CN("example.com"))))),
> +    Success, Result::ERROR_CERT_NOT_IN_NAME_SPACE
> +  },
> +  { RDN(OU("Example Organization")) + RDN(CN("example.com")), NO_SAN,

Needs a comment "// The constraint is a prefix of the subject DN."

@@ +2436,5 @@
> +  { RDN(OU("Example Organization")) + RDN(CN("example.com")), NO_SAN,
> +    GeneralSubtree(DirectoryName(Name(RDN(OU("Example Organization"))))),
> +    Success, Result::ERROR_CERT_NOT_IN_NAME_SPACE
> +  },
> +  { // Note that for excludedSubtrees, we simply prohibit any non-empty

Add a comment "// The name constraint is not a prefix of the subject DN."

@@ +2443,5 @@
> +    GeneralSubtree(DirectoryName(Name(RDN(OU("Example Organization")) +
> +                                      RDN(CN("example.com"))))),
> +    Result::ERROR_CERT_NOT_IN_NAME_SPACE, Result::ERROR_CERT_NOT_IN_NAME_SPACE
> +  },
> +  { RDN(OU("Other Example Organization") + CN("example.com")), NO_SAN,

Add a comment "// Same as the previous one, but one RDN with multiple AVAs.

@@ +2448,5 @@
> +    GeneralSubtree(DirectoryName(Name(RDN(OU("Example Organization") +
> +                                          CN("example.com"))))),
> +    Result::ERROR_CERT_NOT_IN_NAME_SPACE, Result::ERROR_CERT_NOT_IN_NAME_SPACE
> +  },
> +  { // In this case, the constraint uses a different encoding from the subject.

Add "We consider them to match because we allow UTF8Strings and PrintableStrings to compare equal when their contents are equal."

You need a test case to test that we don't allow UTF8Strings and PrintableStrings to compare equal when their contents are NOT equal.

@@ +2455,5 @@
> +                                                     der::PrintableString)) +
> +                                              RDN(CN("example.com"))))),
> +    Success, Result::ERROR_CERT_NOT_IN_NAME_SPACE
> +  },
> +  { // Same again.

Comment "// Same as above, but with UTF8String/PrintableString switched."

::: security/pkix/test/lib/pkixtestutil.h
@@ +252,5 @@
> +}
> +
> +template <size_t L>
> +inline ByteString
> +DirectoryName(const char (&bytes)[L])

You don't need this one, right? It's never used.
Attachment #8590986 - Flags: review?(brian) → review-
Wish I could have caught this, as I see this now on a compatibility pass. 

Only one site in the Pulse top 220k list turned up with this error:

https://techsupport.dodea.edu/

So, likely this is pretty rare.
Attached patch patch v2 (obsolete) — Splinter Review
Thank you for the review. I think this is a lot better now. I believe I've addressed your comments, unless indicated below.

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #23)
> Comment on attachment 8590986 [details] [diff] [review]
> 
> ::: security/pkix/lib/pkixnames.cpp
> @@ +1353,5 @@
> > +      (constraintAttributeValueTag == der::Tag::UTF8String &&
> > +       presentedAttributeValueTag == der::Tag::PrintableString) ||
> > +      (constraintAttributeValueTag == der::Tag::PrintableString &&
> > +       presentedAttributeValueTag == der::Tag::UTF8String)) {
> > +    matches = true;
> 
> Should we also check the bytes of one of the values to ensure that they are
> all only PrintableString (ASCII) characters? Otherwise we'd be allowing a
> UTF-8-encoded PrintableString to match a UTF8String, which seems
> unnecessarily liberal.
> 
> Of course, when we do the binary comparison above and elsewhere, we allow
> invalid UTF8Strings and invalid PrintableStrings. But, this seems a little
> different and more prone for something to go wrong since we're matching two
> different encodings.

I agree, but maybe that could be done in a follow-up.

> @@ +1400,5 @@
> > +// The MUST-level requirement for compatibility is relaxed in the case of
> > +// PrintableString and UTF8String. That is, if a name constraint has been
> > +// encoded using UTF8String and the presented ID has been encoded with a
> > +// PrintableString (or vice-versa), they are considered to match if they are
> > +// equal everywhere except for the tag identifying the encoding.
> 
> Seems like "See" should be on the above line.

Things got moved around a bit, but it turned out the "See" stayed where it was (moving it to the end of the previous line would have made that line too long).
Attachment #8590986 - Attachment is obsolete: true
Attachment #8592544 - Flags: review?(brian)
The following site works again. The certificate has been replaced:

https://www.statistik.bayern.de/

And here is another one that exhibits the problem:

https://www.klassifikationsserver.de/
Comment on attachment 8592544 [details] [diff] [review]
patch v2

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

Please file a follow-up bug about checking the allowed charset in PrintableString. It seems likely to me that if a CA certificate has a valid UTF-8 constraint, but the issued certificate is an invalid PrintableString with UTF-8 chars, then the CA didn't actually check that the name is reasonable; i.e. it is evidence of a breach. But, the same could be said of any invalid name encoding, so it's reasonable to defer the work on validating name contents to a separate bug.

::: security/pkix/lib/pkixnames.cpp
@@ +535,5 @@
>  //       universalString         UniversalString (SIZE (1..MAX)),
>  //       utf8String              UTF8String (SIZE (1..MAX)),
>  //       bmpString               BMPString (SIZE (1..MAX)) }
>  Result
> +SearchWithinAVA(Input typeInput, uint8_t valueEncodingTag, Input presentedID,

Should this be called MatchAVA instead? It's not searching. (This was a pre-existing issue.)

@@ +566,2 @@
>    if (fallBackToCommonName == FallBackToSearchWithinSubject::Yes &&
>        type.MatchRest(id_at_commonName)) {

Nit: InputsAreEqual(type, Input(id_at_commonName))? Then you can avoid constructing the reader and you can rename typeInput to type. Seems cleaner to avoid Reader when we're just matching on equality.

@@ -576,5 @@
> -    Input presentedID;
> -    rv = der::ReadTagAndGetValue(rdn, valueEncodingTag, presentedID);
> -    if (rv != Success) {
> -      return rv;
> -    }

NOTE: I guess when I wrote this code, I was trying to parse the value lazily as a micro-optimization. But it should be rare that we even execute this function at all, because this function is only executed when there are no matching subjectAltNames, which should be rare and bad. So, the improved clarity seems definitely better.

@@ +1332,5 @@
>  //     and valid paths will be rejected.  To avoid acceptance of invalid
>  //     paths, CAs SHOULD state name constraints for distinguished names as
>  //     permittedSubtrees wherever possible.
>  //
> +// For permittedSubtrees, the MUST-level requirement for compatibility is

Should be "requirement is relaxed for compatibility" unless I'm misunderstanding.
Attachment #8592544 - Flags: review?(brian) → review+
Also, very good work.
Attached patch patch v2.1Splinter Review
Thanks for the review. Carrying over r+.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9db1e4540663
Attachment #8592544 - Attachment is obsolete: true
Attachment #8594196 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6e816960e784
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
David, can we have an uplift request for aurora & beta (38 is an esr)? Thanks
Flags: needinfo?(dkeeler)
Comment on attachment 8594196 [details] [diff] [review]
patch v2.1

Approval Request Comment
[Feature/regressing bug #]: bug 970542 (decoding name constraints in mozilla::pkix)
[User impact if declined]: some sites won't be available
[Describe test coverage new/current, TreeHerder]: has tests
[Risks and why]: low - the changes are fairly self-contained
[String/UUID change made/needed]: none
Flags: needinfo?(dkeeler)
Attachment #8594196 - Flags: approval-mozilla-beta?
Attachment #8594196 - Flags: approval-mozilla-aurora?
Comment on attachment 8594196 [details] [diff] [review]
patch v2.1

It has test, it is a boring regression, the code is localized, taking it for 38. Should be in beta 6 or 7.
Attachment #8594196 - Flags: approval-mozilla-beta?
Attachment #8594196 - Flags: approval-mozilla-beta+
Attachment #8594196 - Flags: approval-mozilla-aurora?
Attachment #8594196 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: