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

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

BUG=410574
Committed: https://crrev.com/683a9a97368659d5792d71f3490f94c304971198
Cr-Commit-Position: refs/heads/master@{#371603}

[mattm, 2015-12-23 00:15:33]

mattm@chromium.org changed reviewers:
+ davidben@chromium.org, rsleevi@chromium.org

[mattm, 2015-12-23 00:15:33]

Saw that NIST PKITS tests show a different interpretation of name constraints
than I had.(Ex, see "4.13.6 Valid DN nameConstraints Test6" in PKITS.) The RFC
is kinda ambiguous.  I assume we should match PKITS.

[mattm, 2015-12-23 00:16:56]

On 2015/12/23 00:15:33, mattm wrote:
> Saw that NIST PKITS tests show a different interpretation of name constraints
> than I had.(Ex, see "4.13.6 Valid DN nameConstraints Test6" in PKITS.) The RFC
> is kinda ambiguous.  I assume we should match PKITS.

For reference, the test description is "In this test, the intermediate
certificate includes a nameConstraints extension that specifies a single
excluded subtree. The end entity certificate includes a subject name that falls
outside that subtree.
...
Expected Result: The path should validate successfully."

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

This interpretation seems correct, yeah. Section 6.1.3 checks permitted_subtrees
and excluded_subtrees separately.

https://codereview.chromium.org/1546653004/diff/1/net/cert/internal/name_cons...
File net/cert/internal/name_constraints.cc (right):

https://codereview.chromium.org/1546653004/diff/1/net/cert/internal/name_cons...
net/cert/internal/name_constraints.cc:402: if
(!extnvalue_parser.ReadTag(der::kOctetString, &subject_alt_name_tlv))
[Existing, but this should also check extnvalue_parser.HasMore(), right?]

https://codereview.chromium.org/1546653004/diff/1/net/cert/internal/name_cons...
net/cert/internal/name_constraints.cc:499: return true;
I think this check no longer does anything, does it?

https://codereview.chromium.org/1546653004/diff/1/net/cert/internal/name_cons...
net/cert/internal/name_constraints.cc:530: return true;
Ditto.

https://codereview.chromium.org/1546653004/diff/1/net/cert/internal/name_cons...
net/cert/internal/name_constraints.cc:560: return true;
Ditto.

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

https://codereview.chromium.org/1546653004/diff/1/net/cert/internal/name_cons...
File net/cert/internal/name_constraints.cc (right):

https://codereview.chromium.org/1546653004/diff/1/net/cert/internal/name_cons...
net/cert/internal/name_constraints.cc:402: if
(!extnvalue_parser.ReadTag(der::kOctetString, &subject_alt_name_tlv))
On 2016/01/05 19:34:44, davidben wrote:
> [Existing, but this should also check extnvalue_parser.HasMore(), right?]

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.

https://codereview.chromium.org/1546653004/diff/1/net/cert/internal/name_cons...
net/cert/internal/name_constraints.cc:499: return true;
On 2016/01/05 19:34:44, davidben wrote:
> I think this check no longer does anything, does it?

Yeah. Removed.

https://codereview.chromium.org/1546653004/diff/1/net/cert/internal/name_cons...
net/cert/internal/name_constraints.cc:530: return true;
On 2016/01/05 19:34:44, davidben wrote:
> Ditto.

Done.

https://codereview.chromium.org/1546653004/diff/1/net/cert/internal/name_cons...
net/cert/internal/name_constraints.cc:560: return true;
On 2016/01/05 19:34:44, davidben wrote:
> Ditto.

Done.

[Ryan Sleevi, 2016-01-06 01:53:29]

lgtm

[mattm, 2016-01-26 19:57:56]

The CQ bit was checked by mattm@chromium.org

[mattm, 2016-01-26 19:57:56]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1546653004/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-26 19:59:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1546653004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1546653004/60001

[commit-bot: I haz the power, 2016-01-26 21:36:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-26 21:38:09]

Description was changed from

==========
Name constraints with excluded names but no permitted names should allow names
not matching the excluded names.

BUG=410574
==========

to

==========
Name constraints with excluded names but no permitted names should allow names
not matching the excluded names.

BUG=410574

Committed: https://crrev.com/683a9a97368659d5792d71f3490f94c304971198
Cr-Commit-Position: refs/heads/master@{#371603}
==========

[commit-bot: I haz the power, 2016-01-26 21:38:10]

Patchset 4 (id:??) landed as
https://crrev.com/683a9a97368659d5792d71f3490f94c304971198
Cr-Commit-Position: refs/heads/master@{#371603}