Skip to content

Commit

Permalink
Remove common name parsing from NameIterator
Browse files Browse the repository at this point in the history
This commit removes parsing of the subject common name field from
`NameIterator`, since `rustls-webpki` does not actually verify subject
common names except when enforcing name constraints. This fixes issues
with common names in formats that `rustls-webpki` doesn't currently
support, by removing this code entirely.

Fixes rustls-webpki/webpki#167
  • Loading branch information
hawkw authored and djc committed Sep 12, 2023
1 parent bb9fe71 commit c9914e4
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 73 deletions.
2 changes: 0 additions & 2 deletions src/der.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,7 @@ pub(crate) enum Tag {
OctetString = 0x04,
OID = 0x06,
Enum = 0x0A,
UTF8String = 0x0C,
Sequence = CONSTRUCTED | 0x10, // 0x30
Set = CONSTRUCTED | 0x11, // 0x31
UTCTime = 0x17,
GeneralizedTime = 0x18,

Expand Down
4 changes: 1 addition & 3 deletions src/subject_name/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,4 @@ pub use ip_address::IpAddr;
mod verify;
#[cfg(feature = "alloc")]
pub(super) use verify::list_cert_dns_names;
pub(super) use verify::{
check_name_constraints, verify_cert_subject_name, SubjectCommonNameContents,
};
pub(super) use verify::{check_name_constraints, verify_cert_subject_name};
47 changes: 1 addition & 46 deletions src/subject_name/verify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ pub(crate) fn verify_cert_dns_name(
iterate_names(
Some(cert.subject),
cert.subject_alt_name,
SubjectCommonNameContents::Ignore,
Err(Error::CertNotValidForName),
&mut |name| {
if let GeneralName::DnsName(presented_id) = name {
Expand Down Expand Up @@ -72,7 +71,6 @@ pub(crate) fn verify_cert_subject_name(
// only against Subject Alternative Names.
None,
cert.inner().subject_alt_name,
SubjectCommonNameContents::Ignore,
Err(Error::CertNotValidForName),
&mut |name| {
if let GeneralName::IpAddress(presented_id) = name {
Expand All @@ -90,7 +88,6 @@ pub(crate) fn verify_cert_subject_name(
pub(crate) fn check_name_constraints(
input: Option<&mut untrusted::Reader>,
subordinate_certs: &Cert,
subject_common_name_contents: SubjectCommonNameContents,
budget: &mut Budget,
) -> Result<(), Error> {
let input = match input {
Expand Down Expand Up @@ -118,7 +115,6 @@ pub(crate) fn check_name_constraints(
iterate_names(
Some(child.subject),
child.subject_alt_name,
subject_common_name_contents,
Ok(()),
&mut |name| {
check_presented_id_conforms_to_constraints(
Expand Down Expand Up @@ -310,16 +306,9 @@ enum NameIteration {
Stop(Result<(), Error>),
}

#[derive(Clone, Copy)]
pub(crate) enum SubjectCommonNameContents {
DnsName,
Ignore,
}

fn iterate_names<'names>(
subject: Option<untrusted::Input<'names>>,
subject_alt_name: Option<untrusted::Input<'names>>,
subject_common_name_contents: SubjectCommonNameContents,
result_if_never_stopped_early: Result<(), Error>,
f: &mut impl FnMut(GeneralName<'names>) -> NameIteration,
) -> Result<(), Error> {
Expand Down Expand Up @@ -349,20 +338,7 @@ fn iterate_names<'names>(
};
}

if let (SubjectCommonNameContents::DnsName, Some(subject)) =
(subject_common_name_contents, subject)
{
match common_name(subject) {
Ok(Some(cn)) => match f(GeneralName::DnsName(cn)) {
NameIteration::Stop(result) => result,
NameIteration::KeepGoing => result_if_never_stopped_early,
},
Ok(None) => result_if_never_stopped_early,
Err(err) => Err(err),
}
} else {
result_if_never_stopped_early
}
result_if_never_stopped_early
}

#[cfg(feature = "alloc")]
Expand All @@ -375,7 +351,6 @@ pub(crate) fn list_cert_dns_names<'names>(
iterate_names(
Some(cert.subject),
cert.subject_alt_name,
SubjectCommonNameContents::DnsName,
Ok(()),
&mut |name| {
if let GeneralName::DnsName(presented_id) = name {
Expand Down Expand Up @@ -448,23 +423,3 @@ impl<'a> GeneralName<'a> {
})
}
}

static COMMON_NAME: untrusted::Input = untrusted::Input::from(&[85, 4, 3]);

fn common_name(input: untrusted::Input) -> Result<Option<untrusted::Input>, Error> {
let inner = &mut untrusted::Reader::new(input);
der::nested(inner, der::Tag::Set, Error::BadDer, |tagged| {
der::nested(tagged, der::Tag::Sequence, Error::BadDer, |tagged| {
while !tagged.at_end() {
let name_oid = der::expect_tag_and_get_value(tagged, der::Tag::OID)?;
if name_oid == COMMON_NAME {
return der::expect_tag_and_get_value(tagged, der::Tag::UTF8String).map(Some);
} else {
// discard unused name value
der::read_tag_and_get_value(tagged)?;
}
}
Ok(None)
})
})
}
24 changes: 2 additions & 22 deletions src/verify_cert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,20 +58,6 @@ fn build_chain_inner(
}
}

// for the purpose of name constraints checking, only end-entity server certificates
// could plausibly have a DNS name as a subject commonName that could contribute to
// path validity
let subject_common_name_contents = if opts
.eku
.inner
.key_purpose_id_equals(EKU_SERVER_AUTH.oid_value)
&& used_as_ca == UsedAsCa::No
{
subject_name::SubjectCommonNameContents::DnsName
} else {
subject_name::SubjectCommonNameContents::Ignore
};

let result = loop_while_non_fatal_error(
Error::UnknownIssuer,
opts.trust_anchors,
Expand All @@ -91,12 +77,7 @@ fn build_chain_inner(
budget,
)?;

check_signed_chain_name_constraints(
cert,
trust_anchor,
subject_common_name_contents,
budget,
)?;
check_signed_chain_name_constraints(cert, trust_anchor, budget)?;

Ok(())
},
Expand Down Expand Up @@ -188,7 +169,6 @@ fn check_signed_chain(
fn check_signed_chain_name_constraints(
cert_chain: &Cert,
trust_anchor: &TrustAnchor,
subject_common_name_contents: subject_name::SubjectCommonNameContents,
budget: &mut Budget,
) -> Result<(), Error> {
let mut cert = cert_chain;
Expand All @@ -199,7 +179,7 @@ fn check_signed_chain_name_constraints(

loop {
untrusted::read_all_optional(name_constraints, Error::BadDer, |value| {
subject_name::check_name_constraints(value, cert, subject_common_name_contents, budget)
subject_name::check_name_constraints(value, cert, budget)
})?;

match &cert.ee_or_ca {
Expand Down

0 comments on commit c9914e4

Please sign in to comment.