Skip to content

Commit

Permalink
Fix name constraints check
Browse files Browse the repository at this point in the history
There were two bugs. webpki didn't:

1. read the X.509 Name Constraints field in its entirety, nor

2. check the certificate subject against the constraints correctly

(1) is a simple fix, (2) requires reading the Common Name from the
certificate.

Closes #20, #134.
  • Loading branch information
bnoordhuis committed May 1, 2021
1 parent 5060c2b commit 3cef1bf
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 4 deletions.
24 changes: 20 additions & 4 deletions src/name/verify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,7 @@ pub fn check_name_constraints(
if !inner.peek(subtrees_tag.into()) {
return Ok(None);
}
let subtrees = der::nested(inner, subtrees_tag, Error::BadDer, |tagged| {
der::expect_tag_and_get_value(tagged, der::Tag::Sequence)
})?;
Ok(Some(subtrees))
der::expect_tag_and_get_value(inner, subtrees_tag).map(Some)
}

let permitted_subtrees = parse_subtrees(input, der::Tag::ContextSpecificConstructed0)?;
Expand Down Expand Up @@ -167,6 +164,10 @@ fn check_presented_id_conforms_to_constraints_in_subtree(
dns_name::presented_id_matches_constraint(name, base).ok_or(Error::BadDer)
}

(GeneralName::DirectoryName(name), GeneralName::DnsName(base)) => {
common_name(name).map(|cn| cn == base)
}

(GeneralName::DirectoryName(name), GeneralName::DirectoryName(base)) => Ok(
presented_directory_name_matches_constraint(name, base, subtrees),
),
Expand Down Expand Up @@ -326,3 +327,18 @@ fn general_name<'a>(input: &mut untrusted::Reader<'a>) -> Result<GeneralName<'a>
};
Ok(name)
}

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

fn common_name(input: untrusted::Input) -> Result<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| {
let value = der::expect_tag_and_get_value(tagged, der::Tag::OID)?;
if value != COMMON_NAME {
return Err(Error::BadDer);
}
der::expect_tag_and_get_value(tagged, der::Tag::UTF8String)
})
})
}
19 changes: 19 additions & 0 deletions tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,25 @@ pub fn netflix() {
);
}

#[cfg(feature = "alloc")]
#[test]
pub fn wpt() {
let ee: &[u8] = include_bytes!("wpt/ee.der");
let ca = include_bytes!("wpt/ca.der");

let anchors = vec![webpki::TrustAnchor::try_from_cert_der(ca).unwrap()];
let anchors = webpki::TlsServerTrustAnchors(&anchors);

#[allow(clippy::unreadable_literal)] // TODO: Make this clear.
let time = webpki::Time::from_seconds_since_unix_epoch(1619256684);

let cert = webpki::EndEntityCert::try_from(ee).unwrap();
assert_eq!(
Ok(()),
cert.verify_is_valid_tls_server_cert(ALL_SIGALGS, &anchors, &[], time)
);
}

#[test]
pub fn ed25519() {
let ee: &[u8] = include_bytes!("ed25519/ee.der");
Expand Down
Binary file added tests/wpt/ca.der
Binary file not shown.
Binary file added tests/wpt/ee.der
Binary file not shown.

0 comments on commit 3cef1bf

Please sign in to comment.