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

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

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

Probably blocks bug 926265.

Comment 1

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

Attachment: Remove old name constraints implementation

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.

Comment 2

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

Attachment: Part 1: Refactor name matching within CN AVAs to reduce duplicate logic

Comment 3

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

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 <[e-mail]>
># 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)

Comment 4

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

Attachment: Part 2: Add DNSName constraint matching logic

Comment 5

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

Attachment: Part 3: Add IPAddress name constraint matching

Comment 6

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

Attachment: Part 4: Add DirectoryName name constraint matching

Comment 7

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

Attachment: Part 6(a): Remove old name constraint implementation

Comment 8

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

Attachment: Part 6(b): New implementation of CheckNameConstraints

Comment 9

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

Attachment: Part 7: DNSName name constraint tests

Comment 10

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

Attachment: Part 8: Revert to stricter matching of absolute DNSName constraints

Comment 11

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

Attachment: Part 2: Add DNSName constraint matching logic

Rebased on top of changes for bug 1063281 and merge the non-test parts of part 8.

Comment 12

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

Attachment: Part 7: DNSName name constraint tests

Merged the test parts of part 8 into this.

Comment 13

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

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)

Comment 14

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

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?)

Comment 15

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

Comment on attachment 8508094 [details] [diff] [review]
Part 4: Add DirectoryName name constraint matching

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

Seems reasonable.

Comment 16

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

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

Comment 17

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

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?

Comment 18

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

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 19

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

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 20

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

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 21

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

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.

Comment 22

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

Attachment: Part 8: More CN-ID name constraint tests

Comment 23

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

Attachment: Part 9: IPAddress name constraint tests

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.

Comment 24

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

Attachment: Part 10: Clarify name constraints as reference IDs

Comment 25

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

Comment on attachment 8511715 [details] [diff] [review]
Part 8: More CN-ID name constraint tests

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

Looks good.

Comment 26

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

Attachment: Part 5: Replace NSS-based name constraints implementation with new one

The new part 5 replaces the old 6(a) and 6(b), which were already r+d by mmc and keeler, respectively.

Comment 27

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

Attachment: Part 6: DNSName constraint tests

Comment 28

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

Attachment: Part 7: More CN-ID name constraint tests

Comment 29

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

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?)

Comment 30

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

Comment on attachment 8511721 [details] [diff] [review]
Part 10: Clarify name constraints as reference IDs

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

LGTM.

Comment 31

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a07f4e3cd40c
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1793454fc0
https://hg.mozilla.org/integration/mozilla-inbound/rev/28003051667d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d0e1a6dcde
https://hg.mozilla.org/integration/mozilla-inbound/rev/c02e603a8db9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a40476316fdc
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6c8d35284f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/95e601e82231
https://hg.mozilla.org/integration/mozilla-inbound/rev/d42a1fc52756

The people interested in the handling of trailing dots in names should also look at bug  1107790, which is a follow-up to this bug.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a07f4e3cd40c
https://hg.mozilla.org/mozilla-central/rev/6c1793454fc0
https://hg.mozilla.org/mozilla-central/rev/28003051667d
https://hg.mozilla.org/mozilla-central/rev/e0d0e1a6dcde
https://hg.mozilla.org/mozilla-central/rev/c02e603a8db9
https://hg.mozilla.org/mozilla-central/rev/a40476316fdc
https://hg.mozilla.org/mozilla-central/rev/f6c8d35284f5
https://hg.mozilla.org/mozilla-central/rev/95e601e82231
https://hg.mozilla.org/mozilla-central/rev/d42a1fc52756