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.

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

Support for IPv4 and IPv6 #122

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 31 additions & 13 deletions src/name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ pub fn verify_cert_dns_name(
let cert = &cert.inner;
let dns_name = untrusted::Input::from(dns_name);
iterate_names(
cert.subject,
// For backward compatibility we always pass the subject.
Some(cert.subject),
cert.subject_alt_name,
Err(Error::CertNotValidForName),
&|name| {
Expand Down Expand Up @@ -182,9 +183,18 @@ pub fn check_name_constraints(

let mut child = subordinate_certs;
loop {
iterate_names(child.subject, child.subject_alt_name, Ok(()), &|name| {
check_presented_id_conforms_to_constraints(name, permitted_subtrees, excluded_subtrees)
})?;
iterate_names(
Some(child.subject),
child.subject_alt_name,
Ok(()),
&|name| {
check_presented_id_conforms_to_constraints(
name,
permitted_subtrees,
excluded_subtrees,
)
},
)?;

child = match child.ee_or_ca {
EndEntityOrCA::CA(child_cert) => child_cert,
Expand Down Expand Up @@ -379,13 +389,19 @@ fn presented_ip_address_matches_constraint(
}

#[derive(Clone, Copy)]
enum NameIteration {
pub(crate) enum NameIteration {
KeepGoing,
Stop(Result<(), Error>),
}

fn iterate_names(
subject: untrusted::Input, subject_alt_name: Option<untrusted::Input>,
/// Nowadays, the subject is ignored and only SANs are considered when
/// validating a certificate's "subject".
///
/// - https://groups.google.com/a/chromium.org/d/msg/security-dev/IGT2fLJrAeo/csf_1Rh1AwAJ
/// - https://bugs.chromium.org/p/chromium/issues/detail?id=308330
/// - https://bugzilla.mozilla.org/show_bug.cgi?id=1245280
pub(crate) fn iterate_names(
subject: Option<untrusted::Input>, subject_alt_name: Option<untrusted::Input>,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be misunderstanding how webpki, before this PR, is using subject. webpki uses subject ONLY to verify that the subject conforms to any directoryName constraints that may be present in the chain. It doesn't do any DNS name or IP address matching of the common name. And, as was mentioned in the initial review of this PR, we don't want to do any such processing of the common name. Instead we only want to look at iPAddress subjectAltNames.

Thus, unless I'm massively understanding what you're trying to accomplish in this PR, everything related to the subject and especially the subject's common name should be removed.

result_if_never_stopped_early: Result<(), Error>, f: &dyn Fn(GeneralName) -> NameIteration,
) -> Result<(), Error> {
match subject_alt_name {
Expand All @@ -409,10 +425,12 @@ fn iterate_names(
},
None => (),
}

match f(GeneralName::DirectoryName(subject)) {
NameIteration::Stop(result) => result,
NameIteration::KeepGoing => result_if_never_stopped_early,
match subject {
Some(subject) => match f(GeneralName::DirectoryName(subject)) {
NameIteration::Stop(result) => result,
NameIteration::KeepGoing => result_if_never_stopped_early,
},
_ => result_if_never_stopped_early,
}
}

Expand All @@ -422,7 +440,7 @@ fn iterate_names(
// constraint is different than the meaning of the identically-represented
// `GeneralName` in other contexts.
#[derive(Clone, Copy)]
enum GeneralName<'a> {
pub(crate) enum GeneralName<'a> {
DNSName(untrusted::Input<'a>),
DirectoryName(untrusted::Input<'a>),
IPAddress(untrusted::Input<'a>),
Expand Down Expand Up @@ -463,7 +481,7 @@ fn general_name<'a>(input: &mut untrusted::Reader<'a>) -> Result<GeneralName<'a>
Ok(name)
}

fn presented_dns_id_matches_reference_dns_id(
pub(crate) fn presented_dns_id_matches_reference_dns_id(
presented_dns_id: untrusted::Input, reference_dns_id: untrusted::Input,
) -> Option<bool> {
presented_dns_id_matches_reference_dns_id_internal(
Expand Down
180 changes: 180 additions & 0 deletions src/san.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
use crate::{
der::read_tag_and_get_value,
name::{iterate_names, presented_dns_id_matches_reference_dns_id, GeneralName, NameIteration},
Error,
};
use std::{
net::{IpAddr, Ipv4Addr, Ipv6Addr},
str::FromStr,
};

/// Subject Alternative Name (SAN)
#[non_exhaustive]
pub enum SubjectAlternativeName {
/// DNS name
Dns(String),
/// IPv4 or IPv6 address
Ip(IpAddr),
}

impl SubjectAlternativeName {
/// Binary OID of CommonName (CN) (id-at-commonName).
const OID_CN: [u8; 3] = [85, 4, 3];

fn traverse<'a>(
input: &'a untrusted::Input, agg: &mut Vec<(u8, Vec<u8>)>,
) -> Result<(), Error> {
let mut reader = untrusted::Reader::new(input.clone());
while let Ok((tag, value)) = read_tag_and_get_value(&mut reader) {
agg.push((tag, value.as_slice_less_safe().to_vec()));
Self::traverse(&value.clone(), agg)?;
}
Ok(())
}

/// Strings in Rust are unicode (UTF-8), and unicode codepoints are a
/// superset of iso-8859-1 characters. This specific conversion is
/// actually trivial.
fn latin1_to_string(s: &[u8]) -> String { s.iter().map(|&c| c as char).collect() }

fn ucs4_to_string(s: &[u8]) -> Result<String, Error> {
if s.len() % 4 == 0 {
let mut tmp = String::with_capacity(s.len() / 4);
for i in (0..s.len()).step_by(4) {
match std::char::from_u32(
(u32::from(s[i]) << 24)
| (u32::from(s[i]) << 16)
| (u32::from(s[i]) << 8)
| u32::from(s[i + 1]),
) {
Some(c) => tmp.push(c),
_ => return Err(Error::BadDER),
}
}
Ok(tmp)
} else {
Err(Error::BadDER)
}
}

fn bmp_to_string(s: &[u8]) -> Result<String, Error> {
if s.len() % 2 == 0 {
let mut tmp = String::with_capacity(s.len() / 2);
for i in (0..s.len()).step_by(2) {
match std::char::from_u32((u32::from(s[i]) << 8) | u32::from(s[i + 1])) {
Some(c) => tmp.push(c),
_ => return Err(Error::BadDER),
}
}
Ok(tmp)
} else {
Err(Error::BadDER)
}
}

fn extract_common_name(der: &untrusted::Input) -> Option<String> {
let mut input = vec![];
Self::traverse(der, &mut input).unwrap();
if let Some(oid_position) = input
.iter()
.position(|(tag, value)| *tag == 6u8 && value.as_slice() == Self::OID_CN)
{
match input.get(oid_position + 1) {
// PrintableString (Subset of ASCII, therefore valid UTF8)
Some((19u8, value)) => String::from_utf8(value.clone()).ok(),
// UTF8String
Some((12u8, value)) => String::from_utf8(value.clone()).ok(),
// UniversalString (UCS-4 32-bit encoded)
Some((28u8, value)) => Self::ucs4_to_string(value).ok(),
// BMPString (UCS-2 16-bit encoded)
Some((30u8, value)) => Self::bmp_to_string(value).ok(),
// VideotexString resp. TeletexString ISO-8859-1 encoded
Some((21u8, value)) => Some(Self::latin1_to_string(value.as_slice())),
_ => None,
}
} else {
None
}
}

fn matches_dns(dns: &str, name: &GeneralName) -> bool {
let dns_input = untrusted::Input::from(dns.as_bytes());
match name {
GeneralName::DNSName(d) =>
presented_dns_id_matches_reference_dns_id(d.clone(), dns_input).unwrap_or(false),
GeneralName::DirectoryName(d) => {
if let Some(x) = Self::extract_common_name(d) {
//x == dns
presented_dns_id_matches_reference_dns_id(
untrusted::Input::from(x.as_bytes()),
dns_input,
)
.unwrap_or(false)
} else {
false
}
},
_ => false,
}
}

fn matches_ip(ip: &IpAddr, name: &GeneralName) -> Result<bool, untrusted::EndOfInput> {
match name {
GeneralName::IPAddress(d) => match ip {
IpAddr::V4(v4) if d.len() == 4 => {
let mut reader = untrusted::Reader::new(d.clone());
let mut raw_ip_address: [u8; 4] = Default::default();
raw_ip_address.clone_from_slice(reader.read_bytes(4)?.as_slice_less_safe());
Ok(Ipv4Addr::from(raw_ip_address) == *v4)
},
IpAddr::V6(v6) if d.len() == 16 => {
let mut reader = untrusted::Reader::new(d.clone());
let mut raw_ip_address: [u8; 16] = Default::default();
raw_ip_address.clone_from_slice(reader.read_bytes(16)?.as_slice_less_safe());
Ok(Ipv6Addr::from(raw_ip_address) == *v6)
},
_ => Ok(false),
},
GeneralName::DirectoryName(d) =>
if let Some(x) = Self::extract_common_name(d) {
match IpAddr::from_str(x.as_str()) {
Ok(a) => Ok(a == *ip),
Err(_) => Ok(false),
}
} else {
Ok(false)
},
_ => Ok(false),
}
}

fn matches(&self, _cert: &super::EndEntityCert, name: &GeneralName) -> Result<bool, Error> {
match self {
SubjectAlternativeName::Dns(d) => Ok(Self::matches_dns(d, name)),
SubjectAlternativeName::Ip(ip) => Self::matches_ip(ip, name).map_err(|_| Error::BadDER),
//_ => Ok(false),
}
}

/// Check if this name is the subject of the provided certificate.
pub fn is_subject_of_legacy(
&self, cert: &super::EndEntityCert, check_cn: bool,
) -> Result<(), Error> {
let crt = &cert.inner;
iterate_names(
if check_cn { Some(crt.subject) } else { None },
crt.subject_alt_name,
Err(Error::CertNotValidForName),
&|name| match self.matches(cert, &name) {
Ok(true) => NameIteration::Stop(Ok(())),
Ok(false) => NameIteration::KeepGoing,
Err(e) => NameIteration::Stop(Err(e)),
},
)
}

/// Check if this name is the subject of the provided certificate.
pub fn is_subject_of(&self, cert: &super::EndEntityCert) -> Result<(), Error> {
self.is_subject_of_legacy(cert, false)
}
}
4 changes: 4 additions & 0 deletions src/webpki.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ mod calendar;
mod cert;
mod error;
mod name;
#[cfg(feature = "std")]
mod san;
mod signed_data;
mod time;

Expand All @@ -61,6 +63,8 @@ mod verify_cert;

pub use error::Error;
pub use name::{DNSNameRef, InvalidDNSNameError};
#[cfg(feature = "std")]
pub use san::SubjectAlternativeName;

#[cfg(feature = "std")]
pub use name::DNSName;
Expand Down
102 changes: 102 additions & 0 deletions tests/san.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
#[cfg(feature = "std")]
mod tests {

use std::net::{IpAddr, Ipv4Addr, Ipv6Addr};
use webpki::{Error, SubjectAlternativeName};

#[test]
fn dns_in_cn_legacy() {
let der = include_bytes!("san/dns_in_cn.der");
let cert = webpki::EndEntityCert::from(der).unwrap();
let name = SubjectAlternativeName::Dns("example.com".to_string());
assert_eq!(name.is_subject_of_legacy(&cert, true), Ok(()));
}

#[test]
fn dns_in_cn() {
let der = include_bytes!("san/dns_in_cn.der");
let cert = webpki::EndEntityCert::from(der).unwrap();
let name = SubjectAlternativeName::Dns("example.com".to_string());
assert_eq!(name.is_subject_of(&cert), Err(Error::CertNotValidForName));
}

#[test]
fn dns_wildcard_in_cn_legacy() {
let der = include_bytes!("san/dns_wildcard_in_cn.der");
let cert = webpki::EndEntityCert::from(der).unwrap();
let name = SubjectAlternativeName::Dns("sub.example.com".to_string());
assert_eq!(name.is_subject_of_legacy(&cert, true), Ok(()));
}

#[test]
fn dns_wildcard_in_cn() {
let der = include_bytes!("san/dns_wildcard_in_cn.der");
let cert = webpki::EndEntityCert::from(der).unwrap();
let name = SubjectAlternativeName::Dns("sub.example.com".to_string());
assert_eq!(name.is_subject_of(&cert), Err(Error::CertNotValidForName));
}

#[test]
fn dns_in_san() {
let der = include_bytes!("san/dns_in_san.der");
let cert = webpki::EndEntityCert::from(der).unwrap();
let name = SubjectAlternativeName::Dns("example.org".to_string());
assert_eq!(name.is_subject_of(&cert), Ok(()));
}

#[test]
fn dns_wildcard_in_san() {
let der = include_bytes!("san/dns_wildcard_in_san.der");
let cert = webpki::EndEntityCert::from(der).unwrap();
let name = SubjectAlternativeName::Dns("sub.example.org".to_string());
assert_eq!(name.is_subject_of(&cert), Ok(()));
}

#[test]
fn ip_in_cn_legacy() {
let der = include_bytes!("san/ip_in_cn.der");
let cert = webpki::EndEntityCert::from(der).unwrap();
let name = SubjectAlternativeName::Ip(IpAddr::V4(Ipv4Addr::LOCALHOST));
assert_eq!(name.is_subject_of_legacy(&cert, true), Ok(()));
}

#[test]
fn ip_in_cn() {
let der = include_bytes!("san/ip_in_cn.der");
let cert = webpki::EndEntityCert::from(der).unwrap();
let name = SubjectAlternativeName::Ip(IpAddr::V4(Ipv4Addr::LOCALHOST));
assert_eq!(name.is_subject_of(&cert), Err(Error::CertNotValidForName));
}

#[test]
fn ipv6_in_cn_legacy() {
let der = include_bytes!("san/ipv6_in_cn.der");
let cert = webpki::EndEntityCert::from(der).unwrap();
let name = SubjectAlternativeName::Ip(IpAddr::V6(Ipv6Addr::LOCALHOST));
assert_eq!(name.is_subject_of_legacy(&cert, true), Ok(()));
}

#[test]
fn ipv6_in_cn() {
let der = include_bytes!("san/ipv6_in_cn.der");
let cert = webpki::EndEntityCert::from(der).unwrap();
let name = SubjectAlternativeName::Ip(IpAddr::V6(Ipv6Addr::LOCALHOST));
assert_eq!(name.is_subject_of(&cert), Err(Error::CertNotValidForName));
}

#[test]
fn ip_in_san() {
let der = include_bytes!("san/ip_in_san.der");
let cert = webpki::EndEntityCert::from(der).unwrap();
let name = SubjectAlternativeName::Ip(IpAddr::V4(Ipv4Addr::LOCALHOST));
assert_eq!(name.is_subject_of(&cert), Ok(()));
}

#[test]
fn ipv6_in_san() {
let der = include_bytes!("san/ipv6_in_san.der");
let cert = webpki::EndEntityCert::from(der).unwrap();
let name = SubjectAlternativeName::Ip(IpAddr::V6(Ipv6Addr::LOCALHOST));
assert_eq!(name.is_subject_of(&cert), Ok(()));
}
}
Binary file added tests/san/dns_in_cn.der
Binary file not shown.
7 changes: 7 additions & 0 deletions tests/san/dns_in_cn.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"CN": "example.com",
"key": {
"algo": "rsa",
"size": 2048
}
}
Binary file added tests/san/dns_in_san.der
Binary file not shown.
Loading