Name constraints with excluded names but no permitted names should allow names not matching the exc…

net/cert/internal/name_constraints.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/cert/internal/name_constraints.h"
 
 #include "base/strings/string_util.h"
 #include "net/cert/internal/verify_name_match.h"
 #include "net/der/input.h"
 #include "net/der/parser.h"
@@ skipping 381 matching lines @@
   // id-ce-subjectAltName OBJECT IDENTIFIER ::=  { id-ce 17 }
   //
   // SubjectAltName ::= GeneralNames
   //
   // GeneralNames ::= SEQUENCE SIZE (1..MAX) OF GeneralName
 
   GeneralNames san_names;
   if (subject_alt_name_extnvalue_tlv.Length()) {
     der::Parser extnvalue_parser(subject_alt_name_extnvalue_tlv);
     der::Input subject_alt_name_tlv;
     if (!extnvalue_parser.ReadTag(der::kOctetString, &subject_alt_name_tlv))

[davidben, 2016/01/05 19:34:44]

[Existing, but this should also check extnvalue_parser.HasMore(), right?]

[mattm, 2016/01/05 20:40:25]

yeah. Although, looking at parse_certificate.h and verify_certificate_chain, I
probably just need to change this function to take the subject_alt_name_tlv as
the parameter, rather than the OctetString. I'll do that separately.

       return false;
 
     der::Parser subject_alt_name_parser(subject_alt_name_tlv);
     der::Parser san_sequence_parser;
     if (!subject_alt_name_parser.ReadSequence(&san_sequence_parser))
       return false;
     // Should not have trailing data after subjectAltName sequence.
     if (subject_alt_name_parser.HasMore())
       return false;
     // The subjectAltName sequence should have at least 1 element.
@@ skipping 76 matching lines @@
       subject_rdn_sequence.Length() == 0) {
     return true;
   }
 
   return IsPermittedDirectoryName(subject_rdn_sequence);
 }
 
 bool NameConstraints::IsPermittedDNSName(const std::string& name) const {
   // If there are no name constraints for DNS names, all names are accepted.
   if (!(ConstrainedNameTypes() & GENERAL_NAME_DNS_NAME))
     return true;

[davidben, 2016/01/05 19:34:44]

I think this check no longer does anything, does it?

[mattm, 2016/01/05 20:40:25]

Yeah. Removed.

 
   for (const std::string& excluded_name : excluded_subtrees_.dns_names) {
     // When matching wildcard hosts against excluded subtrees, consider it a
     // match if the constraint would match any expansion of the wildcard. Eg,
     // *.bar.com should match a constraint of foo.bar.com.
     if (DNSNameMatches(name, excluded_name, WILDCARD_PARTIAL_MATCH))
       return false;
   }
+
+  // If permitted subtrees are not constrained, any name that is not excluded is
+  // allowed.
+  if (!(permitted_subtrees_.present_name_types & GENERAL_NAME_DNS_NAME))
+    return true;
+
   for (const std::string& permitted_name : permitted_subtrees_.dns_names) {
     // When matching wildcard hosts against permitted subtrees, consider it a
     // match only if the constraint would match all expansions of the wildcard.
     // Eg, *.bar.com should match a constraint of bar.com, but not foo.bar.com.
     if (DNSNameMatches(name, permitted_name, WILDCARD_FULL_MATCH))
       return true;
   }
 
   return false;
 }
 
 bool NameConstraints::IsPermittedDirectoryName(
     const der::Input& name_rdn_sequence) const {
   // If there are no name constraints for directory names, all names are
   // accepted.
   if (!(ConstrainedNameTypes() & GENERAL_NAME_DIRECTORY_NAME))
     return true;

[davidben, 2016/01/05 19:34:44]

Ditto.

[mattm, 2016/01/05 20:40:25]

Done.

 
   for (const auto& excluded_name : excluded_subtrees_.directory_names) {
     if (VerifyNameInSubtree(
             name_rdn_sequence,
             der::Input(excluded_name.data(), excluded_name.size()))) {
       return false;
     }
   }
+
+  // If permitted subtrees are not constrained, any name that is not excluded is
+  // allowed.
+  if (!(permitted_subtrees_.present_name_types & GENERAL_NAME_DIRECTORY_NAME))
+    return true;
+
   for (const auto& permitted_name : permitted_subtrees_.directory_names) {
     if (VerifyNameInSubtree(
             name_rdn_sequence,
             der::Input(permitted_name.data(), permitted_name.size()))) {
       return true;
     }
   }
 
   return false;
 }
 
 bool NameConstraints::IsPermittedIP(const IPAddressNumber& ip) const {
   // If there are no name constraints for IP Address names, all names are
   // accepted.
   if (!(ConstrainedNameTypes() & GENERAL_NAME_IP_ADDRESS))
     return true;

[davidben, 2016/01/05 19:34:44]

Ditto.

[mattm, 2016/01/05 20:40:25]

Done.

 
   for (const auto& excluded_ip : excluded_subtrees_.ip_address_ranges) {
     if (IPNumberMatchesPrefix(ip, excluded_ip.first, excluded_ip.second))
       return false;
   }
+
+  // If permitted subtrees are not constrained, any name that is not excluded is
+  // allowed.
+  if (!(permitted_subtrees_.present_name_types & GENERAL_NAME_IP_ADDRESS))
+    return true;
+
   for (const auto& permitted_ip : permitted_subtrees_.ip_address_ranges) {
     if (IPNumberMatchesPrefix(ip, permitted_ip.first, permitted_ip.second))
       return true;
   }
 
   return false;
 }
 
 int NameConstraints::ConstrainedNameTypes() const {
   return (permitted_subtrees_.present_name_types |
           excluded_subtrees_.present_name_types);
 }
 
 }  // namespace net