1179495 - sec_error_cert_not_in_name_space error gets thrown on a certificate that does not match the exclusion constraints

Status: RESOLVED WONTFIX

(Core :: Security: PSM, defect)

Comment 1

[Ryan Sleevi]

Can you attach 
  - An intermediate with such an encoding
  - A leaf that raises the error

It's somewhat ambiguous what you mean by matches the excluded constraints (several interpretations), so having certificates (or a site) which demonstrates the issue will make it far easier to debug.

Comment 2

[Benjamin Smedberg]

Overzealous exceptions are not a security issue. Please leave this as a public bug.

Comment 3

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

I've been asked to remove the specific company name from the initial comment. It has been replaced with "Example".

(In reply to Nikki Alex from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101
> Firefox/38.0
> Build ID: 20150514102509
> 
> 
> 
> Actual results:
> 
> sec_error_cert_not_in_name_space error gets thrown on a certificate that
> does not match the exclusion constraints listed below… specifically the
> Directory Address component.  
> 
> Permitted=None
> Excluded
>      [1]Subtrees (0..Max):
>           RFC822 Name=example.com
>      [2]Subtrees (0..Max):
>           RFC822 Name=.example.com
>      [3]Subtrees (0..Max):
>           DNS Name=example.com
>      [4]Subtrees (0..Max):
>           Directory Address:
>                O=Example
>                C=US
> 
> 
> 
> Expected results:
> 
> This error should only occur when a certificate does match the exception. We
> have internal certificate authorities that need to support this requirement

Comment 4

[Nikki Alex]

Attachment: errorcert.cer

Sanitized CA that experiences the issue

Comment 5

[Nikki Alex]

Attachment: FF_38_Issuing_CA.CRT

Issuing certificate for 1st attachment

Comment 6

[rian shelley]

I believe this is caused by a rejection of the directory name constraint. Specifically, the MatchPresentedDirectoryNameWithConstraint() function seems to explicitly disable directory name matching in excluded subtrees. From the code: (https://hg.mozilla.org/mozilla-central/file/tip/security/pkix/lib/pkixnames.cpp#l1332)

  // For excludedSubtrees, we simply prohibit any 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."

Is there a reason for disabling this? 

The certificate appears to work correctly if I comment out the switch statement here: https://hg.mozilla.org/mozilla-central/file/tip/security/pkix/lib/pkixnames.cpp#l1355

Comment 7

[rian shelley]

After looking through the unit tests, I believe that the answer is that in terms of character encoding and set matching, the code tends to be very cautious about matches, which means that inverting that logic for the exclude criteria ends up with a check that is potentially too lenient. 

I would be willing to submit a patch for this, but is there any guidance for what is acceptable in terms of matching the encoding and the number of AVAs per RDN?

(In reply to rian shelley from comment #6)
> I believe this is caused by a rejection of the directory name constraint.
> Specifically, the MatchPresentedDirectoryNameWithConstraint() function seems
> to explicitly disable directory name matching in excluded subtrees. From the
> code:
> (https://hg.mozilla.org/mozilla-central/file/tip/security/pkix/lib/pkixnames.
> cpp#l1332)
> 
>   // For excludedSubtrees, we simply prohibit any 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."
> 
> Is there a reason for disabling this? 
> 
> The certificate appears to work correctly if I comment out the switch
> statement here:
> https://hg.mozilla.org/mozilla-central/file/tip/security/pkix/lib/pkixnames.
> cpp#l1355

Comment 8

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

(In reply to rian shelley from comment #7)
> After looking through the unit tests, I believe that the answer is that in
> terms of character encoding and set matching, the code tends to be very
> cautious about matches, which means that inverting that logic for the
> exclude criteria ends up with a check that is potentially too lenient. 

That's more or less the issue, yes.

> I would be willing to submit a patch for this, but is there any guidance for
> what is acceptable in terms of matching the encoding and the number of AVAs
> per RDN?

Try to follow the relevant RFCs as closely as possible. In particular, RFC5280 stipulates that the encodings must match. I'm not aware of a limit in the number of AVAs per RDN (although practically speaking there is one, since mozilla::pkix won't process certificates larger than 16K).

Comment 9

[rian shelley]

I have a working implementation using the ICU usprep_prepare function, but it requires that I pass a buffer for string conversion and normalization. I can't find any examples within the pkix code that uses either heap allocation or large stack buffers. Are there any security requirements imposed on the library that I should be adhering to? I can't find any documentation indicating either way. 

I'm sure its possible to write a compliant string comparison routine checking one character at a time, but it would need to contain state, and freshly minted unicode processing routines are notorious for having security holes. I would much rather use the ICU library if possible.

Comment 10

[rian shelley]

Attachment: Implementation of RFC4518 directory name comparison

Comment 11

[rian shelley]

I've attached a patch that fixes the problem. Is there anybody who can take a look at it?

Comment 12

[Ryan Sleevi]

Comment on attachment 8727081 [details] [diff] [review]
Implementation of RFC4518 directory name comparison

Going to slap an r? to ekr and dkeeler

ekr: I'm not sure who handles policy decisions at Mozilla - namely, do you want to support this, are you OK with the code size increase, etc. On the Chrome side, we considered an approach like this, but ultimately have rejected it, in particular for the 200K codesize hit. We're opting for something 'good enough' which doesn't bring into the whole StringPrep tables, but that's a decision that means there isn't strict spec compliance.

keeler: I'm guessing you're the best one to review changes to moz::pkix? And you'd know what component this should be moved to, since it's not NSS side?

Comment 13

[Eric Rescorla (:ekr)]

Comment on attachment 8727081 [details] [diff] [review]
Implementation of RFC4518 directory name comparison

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

Forwarding to Barnes

Comment 14

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

Comment on attachment 8727081 [details] [diff] [review]
Implementation of RFC4518 directory name comparison

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

Setting aside for the moment whether or not we want to introduce this dependency, there are a few issues with this patch as-is:
* This doesn't compile for me with clang on linux with 'ac_add_options --enable-warnings-as-errors' in .mozconfig. It looks like most of the issues are in the icu headers, although the c-style casts introduced in pkixnames.cpp are problematic as well. This will have to be dealt with one way or another (either by fixing icu or adding appropriate "ignore these errors" annotations in the relevant moz.build file).
* The style is mostly right, but there are a few deviations from our guidelines: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
* pkixnames_tests.cpp has been modified recently, and this doesn't apply cleanly anymore.
* In-tree documentation will have to be updated (for example, there are comments in pkixnames.cpp that indicate this implementation doesn't support what this patch adds).
* The commented-out tests in pkixnames_test.cpp are worrisome - if icu doesn't correctly implement the functionality we're looking for, we should either fix it or find/write an implementation that is correct.

Before you spend time addressing these issues, however, we should have a larger discussion about whether or not we should support this feature. As the certificate verification library for Firefox, mozilla::pkix is primarily focused on verifying TLS server auth certificates. Perhaps this is due to a lack of imagination, but I can't think of a compelling use-case for supporting non-empty DNs in excluded subtrees in name constraints in the web PKI. Indeed, most of the web has gotten along fine without it for a year (or longer, as I gather that Chrome doesn't fully support it either). I'm not aware of any CA in our root program that requires this. At this point, adding this functionality represents nothing but risk for the overwhelming majority of our users. So, to be blunt, I don't think we should implement this. I'm interested to hear what Richard says, though. I would also be interested in hearing why this feature is important to those who want it added.

Comment 15

[rian shelley]

I'll admit that submitting this patch was an attempt at soliciting this discussion. Which I agree is sorely needed. I also agree that pulling in the ICU library and the coding style it requires represents an unnecessary risk. Is there any plan for real internationalization support within the pkix library?

There may be an acceptable middle ground that will solve the original poster's issue and avoid the need to pull in the icu libraries. What about an implementation that only processes the directory exclusions if both  sides of the comparison are restricted to the set of characters valid in a PrintableString? This could be a simple implementation that also allows for future growth if we decide we do need to implement the full comparison algorithm.

Comment 16

[Richard Barnes [:rbarnes]]

Comment on attachment 8727081 [details] [diff] [review]
Implementation of RFC4518 directory name comparison

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

Sorry, I reviewed this a little bit ago and forgot to hit "submit".

I share Keeler's concerns here that adding support for DirName constraints is all down-side.  The Web PKI simply doesn't do anything with directory names besides (1) use them for subject/issuer chaining, and (2) display the O= and C= values for EV certificates.  So there's no use at all in this for DV certificates, which are >95% of all validations (http://mzl.la/1Mc3gDB).  I suppose you could argue that there's some use case for EV, but no public CA certificates have such a constraint.  Net of all that, I'm marking this WONTFIX.

Rian: There may be some case for doing some i18n in moz::pkix, e.g., to assure that DNs are properly compared when doing subject/issuer chaining.  But again, this doesn't really seem to be enough of a problem in practice to merit the risk of taking on that dependency.

Comment 17

[rian shelley]

Is there any room for a partial fix that doesn't require the icu dependency? 

I took this on because I'm annoyed at being told to "just use internet explorer".

Comment 18

[rian shelley]

(In reply to Richard Barnes [:rbarnes] from comment #16)
> Comment on attachment 8727081 [details] [diff] [review]
> Implementation of RFC4518 directory name comparison
> 
> Review of attachment 8727081 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I reviewed this a little bit ago and forgot to hit "submit".
> 
> I share Keeler's concerns here that adding support for DirName constraints
> is all down-side.  The Web PKI simply doesn't do anything with directory
> names besides (1) use them for subject/issuer chaining, and (2) display the
> O= and C= values for EV certificates.  So there's no use at all in this for
> DV certificates, which are >95% of all validations (http://mzl.la/1Mc3gDB). 
> I suppose you could argue that there's some use case for EV, but no public
> CA certificates have such a constraint.  Net of all that, I'm marking this
> WONTFIX.
> 
> Rian: There may be some case for doing some i18n in moz::pkix, e.g., to
> assure that DNs are properly compared when doing subject/issuer chaining. 
> But again, this doesn't really seem to be enough of a problem in practice to
> merit the risk of taking on that dependency.

Is there any room for a partial fix that doesn't require the icu dependency? something along the lines of allowing exclusion constraints if both sides of the string comparison are restricted to the characters valid in PrintableString or something.

Comment 19

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

Rian, it's been a long time since I worked on this but IIRC the problem isn't *just* about encodings. DirectoryNames are really complex and mozilla::pkix generally tries to avoid that complexity. Unfortunately, a *safe* implementation of DirectoryName exclusion constraints is complicated as it requires normalization of encodings and then also normalization of the structure of the AVAs and RDNs (IIRC).

This is compounded by the fact that mozilla::pkix is designed to be usable in an embedding context where there is no heap. So, not only would you need to avoid ICU, but you'd need to avoid `malloc`, `operator new`, `std::string`, `std::vector`, etc.

The simplicity of mozilla::pkix is its key virtue. Accordingly, there's a strong bias towards keeping it simple. In this case, it's more important to keep mozilla::pkix simple than it is to fix this bug.

Accordingly, I think a better solution is (1) replace the certificate with the DirectoryName exclusion constraints the one that doesn't have such constraints (I'm guessing EKU can be used to solve the problem that the DirectoryName constraints were trying to solve), (2) ask the CA that issued the certificate with the DirectoryName exclusion constraints to revoke the certificate, and (3) remove DirectoryName exclusion constraints from the Web PKI profile.

Comment 20

[Sash]

All,

We have the same issue. In our opinion Firefox should support the same behavior as IE and Chrome. The exclusions should be looked at only. All other domains are permitted. We have a similar setup with our named constraints.

Permitted=None
> Excluded
>      [1]Subtrees (0..Max):
>           RFC822 Name=example.com
>      [2]Subtrees (0..Max):
>           RFC822 Name=.example.com
>      [3]Subtrees (0..Max):
>           DNS Name=example.com
>      [4]Subtrees (0..Max):
>           Directory Address:
>                O=Example
>                C=US
Can this be fixed in a future release?

Thanks