Closed Bug 970542 Opened 7 years ago Closed 6 years ago

Decode and verify name constraints in mozilla::pkix internally (without relying on NSS to do all the work)

Categories

(Core :: Security: PSM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: briansmith, Assigned: briansmith)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(9 files, 8 obsolete files)

2.65 KB, patch
keeler
: review+
Details | Diff | Splinter Review
3.66 KB, patch
keeler
: review+
Details | Diff | Splinter Review
5.59 KB, patch
keeler
: review+
Details | Diff | Splinter Review
16.39 KB, patch
keeler
: review+
Details | Diff | Splinter Review
14.18 KB, patch
keeler
: review+
Details | Diff | Splinter Review
4.47 KB, patch
keeler
: review+
Details | Diff | Splinter Review
31.53 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
12.01 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
6.35 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
Summary: Decode and verify name constraints in insanity::pkix internally (without relying on NSS to do all the work) → Decode and verify name constraints in mozilla::pkix internally (without relying on NSS to do all the work)
This patch just removes the old implementation. I'm planning for the new implement to live in pkixnames.cpp, because it shares most of the code with the CheckCertHostname function. This patch will get merged with the patch that adds the name constraint implementation to pkixnames.cpp after both are reviewed.
Assignee: nobody → brian
Status: NEW → ASSIGNED
Attachment #8505832 - Flags: review?(mmc)
Attachment #8505832 - Flags: review?(mmc) → review+
Comment on attachment 8508088 [details] [diff] [review]
Part 1: Refactor name matching within CN AVAs to reduce duplicate logic

># HG changeset patch
># User Brian Smith <brian@briansmith.org>
># Date 1413503067 25200
>#      Thu Oct 16 16:44:27 2014 -0700
># Node ID 8db8bab67334516f552547234ca2f416652e9911
># Parent  36c7c31f86841b67160b8c00633a9a11c264dcc0
>Bug 970542, Part 1: Refactor name matching within CN AVAs to reduce duplicate logic
>
>diff --git a/security/pkix/lib/pkixnames.cpp b/security/pkix/lib/pkixnames.cpp
>--- a/security/pkix/lib/pkixnames.cpp
>+++ b/security/pkix/lib/pkixnames.cpp
>@@ -391,37 +391,36 @@ SearchWithinAVA(Reader& rdn,
>   // deprecated it. universalString and bmpString are also deprecated, and they
>   // are a little harder to support because they are not single-byte ASCII
>   // superset encodings.
>   if (valueEncodingTag != der::PrintableString &&
>       valueEncodingTag != der::UTF8String) {
>     return Success;
>   }
> 
>-  switch (referenceIDType)
>-  {
>-    case GeneralNameType::dNSName:
>-      foundMatch = PresentedDNSIDMatchesReferenceDNSID(presentedID,
>-                                                       referenceID);
>-      break;
>-    case GeneralNameType::iPAddress:
>-    {
>-      // We don't fall back to matching CN-IDs for IPv6 addresses, so we'll
>-      // never get here for an IPv6 address.
>-      assert(referenceID.GetLength() == 4);
>-      uint8_t ipv4[4];
>-      foundMatch = ParseIPv4Address(presentedID, ipv4) &&
>-                   InputsAreEqual(Input(ipv4), referenceID);
>-      break;
>+  if (referenceIDType == GeneralNameType::dNSName) {
>+    return MatchPresentedIDWithReferenceID(GeneralNameType::dNSName,
>+                                           presentedID, referenceID,
>+                                           foundMatch);
>+  }
>+
>+  // We don't match CN-IDs for IPv6 addresses. MatchPresentedIDWithReferenceID
>+  // ensures that it won't match an IPv4 address with an IPv6 address, so we
>+  // don't need to check that referenceID is an IPv4 address here.
>+  if (referenceIDType == GeneralNameType::iPAddress) {
>+    uint8_t ipv4[4];
>+    if (ParseIPv4Address(presentedID, ipv4)) {
>+      return MatchPresentedIDWithReferenceID(GeneralNameType::iPAddress,
>+                                             Input(ipv4), referenceID,
>+                                             foundMatch);
>     }
>-    default:
>-      return NotReached("unexpected referenceIDType in SearchWithinAVA",
>-                        Result::FATAL_ERROR_INVALID_ARGS);
>   }
> 
>+  // We don't match CN-IDs for any other types of names.
>+
>   return Success;
> }
> 
> Result
> MatchPresentedIDWithReferenceID(GeneralNameType nameType,
>                                 Input presentedID,
>                                 Input referenceID,
>                                 /*out*/ bool& foundMatch)
Attachment #8508088 - Flags: review?(mmc) → review?(dkeeler)
Rebased on top of changes for bug 1063281 and merge the non-test parts of part 8.
Attachment #8508092 - Attachment is obsolete: true
Attachment #8508104 - Attachment is obsolete: true
Attachment #8508092 - Flags: review?(dkeeler)
Attachment #8508104 - Flags: review?(dkeeler)
Attachment #8508468 - Flags: review?(dkeeler)
Merged the test parts of part 8 into this.
Attachment #8508103 - Attachment is obsolete: true
Attachment #8508103 - Flags: review?(dkeeler)
Attachment #8508469 - Flags: review?(dkeeler)
Comment on attachment 8508468 [details] [diff] [review]
Part 2: Add DNSName constraint matching logic

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

Ok - this looks good. Mostly just some trailing whitespace issues.

::: security/pkix/lib/pkixnames.cpp
@@ +90,5 @@
>  };
>  
>  bool IsValidDNSID(Input hostname, ValidDNSIDMatchType matchType);
>  
> +bool PresentedDNSIDMatchesReferenceDNSID(

I found "ReferenceDNSID" a little confusing in this context, since it could also be a NameConstraint now, right? I'm not sure how it could be improved, though. My initial thought was to call this "PresentedDNSIDMatchesDNSID", but then we still have the parameter called 'referenceDNSID', and it wouldn't make much sense to change it. It's probably fine as is.

@@ +470,5 @@
> +//      example.com    example.com.
> +//      example.com.   exmaple.com.
> +//
> +// There are more subtleties documented inline in the code.
> +// 

nit: this comment block has a few lines with trailing whitespace

@@ +606,5 @@
> +        // dot.
> +        //
> +        // If the reference ID does not start with a dot then we skip the
> +        // prefix of the presented ID but also verify that the prefix ends with
> +        // a dot.

Maybe some examples would help illustrate the intention of this code. My understanding is we're trying to do this:

presented: "www.example.com"
reference: ".example.com"

reference starts with '.', so skip the leading extra bytes of presented -> ".example.com"

presented: "www.example.com"
reference: "example.com"

reference doesn't start with '.', so skip leading extra bytes but one -> ".example.com", and verify that the next byte is '.' -> "example.com"

(counter-examples would be good, too)
Attachment #8508468 - Flags: review?(dkeeler) → review+
Comment on attachment 8508093 [details] [diff] [review]
Part 3: Add IPAddress name constraint matching

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

Looks good.

::: security/pkix/lib/pkixnames.cpp
@@ +722,5 @@
>  }
>  
> +Result
> +MatchPresentedIPAddressWithConstraint(Input presentedID,
> +                                      Input iPAddressConstraint,

A comment that iPAddressConstraint is an IP address/CIDR mask would be appreciated.

@@ +726,5 @@
> +                                      Input iPAddressConstraint,
> +                                      /*out*/ bool& foundMatch)
> +{
> +  if (presentedID.GetLength() != 4 && presentedID.GetLength() != 16) {
> +  return Result::ERROR_BAD_DER;

nit: indentation

@@ +758,5 @@
> +    rv = constraintAddress.Read(constraintAddressByte);
> +    if (rv != Success) {
> +      return rv;
> +    }
> +    uint8_t constraintMaskByte;

Do we want to do any verification that the constraint mask is valid? (i.e. the set bits have to be contiguous and start from the most significant bit, right?)
Attachment #8508093 - Flags: review?(dkeeler) → review+
Comment on attachment 8508094 [details] [diff] [review]
Part 4: Add DirectoryName name constraint matching

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

Seems reasonable.
Attachment #8508094 - Flags: review?(dkeeler) → review+
Comment on attachment 8508096 [details] [diff] [review]
Part 6(b): New implementation of CheckNameConstraints

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

Ok - I think this looks good.

::: security/pkix/lib/pkixnames.cpp
@@ +270,5 @@
> +// SearchNames is used by CheckCertHostname and CheckNameConstraints. The
> +// main benefit of using the exact same code paths for both is that we ensure
> +// the consistency between name validation and name constraint enforcement
> +// regarding thing like "Which CN attributes should be considered as potential
> +// potential CN-IDs" and "Which character sets are acceptable for CN-IDs?"

nit: "potential" is repeated here

@@ +703,5 @@
> +  bool hasPermittedSubtreesMatch = false;
> +  bool hasPermittedSubtreesMismatch = false;
> +
> +  // GeneralSubtrees ::= SEQUENCE SIZE (1..MAX) OF GeneralSubtree
> +  // 

nit: trailing whitespace
Attachment #8508096 - Flags: review?(dkeeler) → review+
Comment on attachment 8508469 [details] [diff] [review]
Part 7: DNSName name constraint tests

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

Looks good - I just have a suggestion for a few more test cases.

::: security/pkix/test/gtest/pkixnames_tests.cpp
@@ +22,5 @@
>   * limitations under the License.
>   */
>  #include "pkix/pkix.h"
> +#include "pkixcheck.h"
> +#include "pkixutil.h"

nit: pkixutil would sort last here, I think

@@ +1571,5 @@
> +  { ByteString(), DNSName("bigfoo.bar.com"),
> +    GeneralSubtree(DNSName("foo.bar.com")),
> +    Result::ERROR_CERT_NOT_IN_NAME_SPACE, Success
> +  },
> +  

nit: trailing whitespace

@@ +1619,5 @@
> +    ByteString(), DNSName("*.example.com"),
> +    GeneralSubtree(DNSName(".example.com.")),
> +    Result::ERROR_CERT_NOT_IN_NAME_SPACE, Success
> +  },
> +  

nit: whitespace

@@ +1705,5 @@
> +    RDN(CN("a.example.com")) + RDN(CN("b.example.com")), NO_SAN,
> +    GeneralSubtree(DNSName("b.example.com")),
> +    Success, Result::ERROR_CERT_NOT_IN_NAME_SPACE
> +  },
> +};

Do we have sufficient coverage for cases that have both a CN that's a valid DNSName/IPAddress and a SAN?
Attachment #8508469 - Flags: review?(dkeeler) → review+
Comment on attachment 8508468 [details] [diff] [review]
Part 2: Add DNSName constraint matching logic

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

::: security/pkix/lib/pkixnames.cpp
@@ +90,5 @@
>  };
>  
>  bool IsValidDNSID(Input hostname, ValidDNSIDMatchType matchType);
>  
> +bool PresentedDNSIDMatchesReferenceDNSID(

I agree. I will try to address this with a add-on patch. Maybe that patch will just document at the top of the file the general way in which the name constraint code reuses the structure of the name matching code, to ensure they stay in sync, and how the naming works (e.g. that the constraint is the reference ID when we're matching constraints).

@@ +470,5 @@
> +//      example.com    example.com.
> +//      example.com.   exmaple.com.
> +//
> +// There are more subtleties documented inline in the code.
> +// 

Nixed.

@@ +606,5 @@
> +        // dot.
> +        //
> +        // If the reference ID does not start with a dot then we skip the
> +        // prefix of the presented ID but also verify that the prefix ends with
> +        // a dot.

Done.
Comment on attachment 8508093 [details] [diff] [review]
Part 3: Add IPAddress name constraint matching

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

::: security/pkix/lib/pkixnames.cpp
@@ +722,5 @@
>  }
>  
> +Result
> +MatchPresentedIPAddressWithConstraint(Input presentedID,
> +                                      Input iPAddressConstraint,

// https://tools.ietf.org/html/rfc5280#section-4.2.1.10 says:
//
//     For IPv4 addresses, the iPAddress field of GeneralName MUST contain
//     eight (8) octets, encoded in the style of RFC 4632 (CIDR) to represent
//     an address range [RFC4632].  For IPv6 addresses, the iPAddress field
//     MUST contain 32 octets similarly encoded.  For example, a name
//     constraint for "class C" subnet 192.0.2.0 is represented as the
//     octets C0 00 02 00 FF FF FF 00, representing the CIDR notation
//     192.0.2.0/24 (mask 255.255.255.0).

@@ +726,5 @@
> +                                      Input iPAddressConstraint,
> +                                      /*out*/ bool& foundMatch)
> +{
> +  if (presentedID.GetLength() != 4 && presentedID.GetLength() != 16) {
> +  return Result::ERROR_BAD_DER;

fixed.

@@ +758,5 @@
> +    rv = constraintAddress.Read(constraintAddressByte);
> +    if (rv != Success) {
> +      return rv;
> +    }
> +    uint8_t constraintMaskByte;

OK, I will try this as an another patch to this bug.
Comment on attachment 8508096 [details] [diff] [review]
Part 6(b): New implementation of CheckNameConstraints

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

::: security/pkix/lib/pkixnames.cpp
@@ +270,5 @@
> +// SearchNames is used by CheckCertHostname and CheckNameConstraints. The
> +// main benefit of using the exact same code paths for both is that we ensure
> +// the consistency between name validation and name constraint enforcement
> +// regarding thing like "Which CN attributes should be considered as potential
> +// potential CN-IDs" and "Which character sets are acceptable for CN-IDs?"

removed one.

@@ +703,5 @@
> +  bool hasPermittedSubtreesMatch = false;
> +  bool hasPermittedSubtreesMismatch = false;
> +
> +  // GeneralSubtrees ::= SEQUENCE SIZE (1..MAX) OF GeneralSubtree
> +  // 

nixed.
Comment on attachment 8508469 [details] [diff] [review]
Part 7: DNSName name constraint tests

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

::: security/pkix/test/gtest/pkixnames_tests.cpp
@@ +22,5 @@
>   * limitations under the License.
>   */
>  #include "pkix/pkix.h"
> +#include "pkixcheck.h"
> +#include "pkixutil.h"

Thanks. ABCs ain't never been my strong soot.

@@ +1705,5 @@
> +    RDN(CN("a.example.com")) + RDN(CN("b.example.com")), NO_SAN,
> +    GeneralSubtree(DNSName("b.example.com")),
> +    Success, Result::ERROR_CERT_NOT_IN_NAME_SPACE
> +  },
> +};

I will add some more tests in a new patch in this bug. Note that these are covered in test_name_constraints.js already too, though.
No longer depends on: 1088845
Target Milestone: --- → mozilla36
The changes to pkixnames.cpp are not strictly necessary, except they help us return different error codes for malformed vs. mismatched constraints.

I filed bug 1089430 about checking the subnet mask syntax. I think it is something that can be deferred until later, especially since NSS isn't doing it either.
Attachment #8511720 - Flags: review?(dkeeler)
Comment on attachment 8511715 [details] [diff] [review]
Part 8: More CN-ID name constraint tests

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

Looks good.
Attachment #8511715 - Flags: review?(dkeeler) → review+
The new part 5 replaces the old 6(a) and 6(b), which were already r+d by mmc and keeler, respectively.
Attachment #8508095 - Attachment is obsolete: true
Attachment #8508096 - Attachment is obsolete: true
Attachment #8513936 - Flags: review+
Attachment #8513936 - Attachment description: name-constraints-p5.patch → Part 5: Replace NSS-based name constraints implementation with new one
Comment on attachment 8511720 [details] [diff] [review]
Part 9: IPAddress name constraint tests

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

Looks great.

::: security/pkix/test/gtest/pkixnames_tests.cpp
@@ +1764,5 @@
>  
>    /////////////////////////////////////////////////////////////////////////////
> +  // Basic IP Address constraints (non-CN-ID)
> +
> +  // The Mozilla CA Policy says this means "no IPv4 addresses allowed."

This comment confuses me - what test is it referring to? (The excluded subtrees case?)
Attachment #8511720 - Flags: review?(dkeeler) → review+
Comment on attachment 8511721 [details] [diff] [review]
Part 10: Clarify name constraints as reference IDs

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

LGTM.
Attachment #8511721 - Flags: review?(dkeeler) → review+
Depends on: 1107946
Duplicate of this bug: 921898
You need to log in before you can comment on or make changes to this bug.