Closed
Bug 970542
Opened 11 years ago
Closed 10 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)
Core
Security: PSM
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)
(deleted),
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
briansmith
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
briansmith
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
briansmith
:
review+
|
Details | Diff | Splinter Review |
Probably blocks bug 926265.
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)
Assignee | ||
Comment 1•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8505832 -
Flags: review?(mmc) → review+
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8508088 -
Flags: review?(mmc)
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8508092 -
Flags: review?(dkeeler)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8508093 -
Flags: review?(dkeeler)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8508094 -
Flags: review?(dkeeler)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8505832 -
Attachment is obsolete: true
Attachment #8508095 -
Flags: review+
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8508096 -
Flags: review?(dkeeler)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8508103 -
Flags: review?(dkeeler)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8508104 -
Flags: review?(dkeeler)
Assignee | ||
Comment 11•10 years ago
|
||
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)
Assignee | ||
Comment 12•10 years ago
|
||
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)
Attachment #8508088 -
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]:
-----------------------------------------------------------------
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+
Assignee | ||
Comment 18•10 years ago
|
||
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.
Assignee | ||
Comment 19•10 years ago
|
||
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.
Assignee | ||
Comment 20•10 years ago
|
||
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.
Assignee | ||
Comment 21•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → mozilla36
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8511715 -
Flags: review?(dkeeler)
Assignee | ||
Comment 23•10 years ago
|
||
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)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8511721 -
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+
Assignee | ||
Comment 26•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Attachment #8513936 -
Attachment description: name-constraints-p5.patch → Part 5: Replace NSS-based name constraints implementation with new one
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8508469 -
Attachment is obsolete: true
Attachment #8513939 -
Flags: review+
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8511715 -
Attachment is obsolete: true
Attachment #8513940 -
Flags: review+
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+
Blocks: mozilla::pkix-nss
Assignee | ||
Comment 31•10 years ago
|
||
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.
Target Milestone: mozilla36 → mozilla37
Comment 32•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 1144902
You need to log in
before you can comment on or make changes to this bug.
Description
•