Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validation of IP address name constraints should be stricter #130

Open
briansmith opened this issue Jun 4, 2020 · 1 comment · May be fixed by #131
Open

Validation of IP address name constraints should be stricter #130

briansmith opened this issue Jun 4, 2020 · 1 comment · May be fixed by #131

Comments

@briansmith
Copy link
Owner

briansmith commented Jun 4, 2020 •
edited
Loading

This was reported by Gregor Kopf for Cure53. Thanks Gregor!

RFC 5280 has this to say about IP address name constraints:

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

Gregor noted that this code:

webpki/src/name.rs

Lines 357 to 375 in 049c5ad

let (constraint_address, constraint_mask) = constraint.read_all(Error::BadDER, |value| {
let address = value.read_bytes(constraint.len() / 2).unwrap();
let mask = value.read_bytes(constraint.len() / 2).unwrap();
Ok((address, mask))
})?;
let mut name = untrusted::Reader::new(name);
let mut constraint_address = untrusted::Reader::new(constraint_address);
let mut constraint_mask = untrusted::Reader::new(constraint_mask);
loop {
let name_byte = name.read_byte().unwrap();
let constraint_address_byte = constraint_address.read_byte().unwrap();
let constraint_mask_byte = constraint_mask.read_byte().unwrap();
if ((name_byte ^ constraint_address_byte) & constraint_mask_byte) != 0 {
return Ok(false);
}
if name.at_end() {
break;
}
"... would support non-contiguous subnet masks - i.e., masks that have one-bits after the first zero bit. I know that the respective RFCs are not too clear about whether such masks should be allowed or not."

The question is whether it is correct (or at least better) to support non-contiguous subnet masks or to reject non-contiguous masks. Even more than that, should we support masks that aren't prefixes?

Chromium's name constraint logic does have these requirements. See https://chromium.googlesource.com/chromium/src/+blame/51ba3e6902ecd7284d3be51db648127d1be2187f/net/cert/internal/name_constraints.cc#239

I don't remember making a conscious decision about this in the past. The RFC 5280 encoding of subnet mask is wasteful and error-prone if it really intends to be restricted to masks that can be represented in CIDR notation. OTOH we don't have any motivation to support masks that can't be represented with CIDR notation. webpki should align with Chromium here.
@briansmith briansmith mentioned this issue Jun 4, 2020
Open
@djc djc linked a pull request Jun 8, 2020 that will close this issue
Draft
@briansmith
Copy link
Owner Author

I want to emphasize that the severity of this issue is very low due to the points I already shared above:

  • webpki doesn't (yet) support validating a certificate for an IP address yet: webpki/cert: extend verify_is_valid_for to iPAddress SAN #54. So using webpki to validate a certificate, you'll never run into the situation where a malformed IP address name constraint caused you to accept a certificate for an IP address that should have been masked off in the constraint.

  • The name constraint is in the CA certificate, and so a wrong mask would be an error on the CA's part. The threat model for certificate validation requires us to trust the CA.

To help people feel better about the IP address support we're planning to add to resolve #54, we should add a check that the mask is contiguous, probably by helping @djc get his PR merged.

@daxpedda daxpedda mentioned this issue Apr 6, 2021
Open
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Only allow contiguous netmasks djc/webpki
1 participant
@briansmith