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

cert: retain CRL distribution points extension. #127

Merged
merged 7 commits into from
Jul 26, 2023
Merged
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
6 changes: 5 additions & 1 deletion .github/workflows/testgen.yml
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,15 @@ jobs:
- name: Install Python requirements.
run: pip install -r requirements.txt

- name: Generate test files
- name: Generate integration test files
working-directory: ./tests
# Generate but don't run the test suite - we already do that in the
# other CI tasks that run `cargo test`.
run: python3 generate.py --no-test

- name: Generate CRL distribution point test files
working-directory: ./tests/crl_distrib_point/
run: python3 make_testcerts.py

- name: Enforce no diff
run: git diff --exit-code
456 changes: 455 additions & 1 deletion src/cert.rs

Large diffs are not rendered by default.

1 change: 0 additions & 1 deletion src/crl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ pub trait CertRevocationList: Sealed {
/// [^1]: <https://www.rfc-editor.org/rfc/rfc5280#section-5>
#[cfg(feature = "alloc")]
#[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
#[allow(dead_code)] // we parse some fields we don't expose now, but may choose to expose in the future.
#[derive(Debug, Clone)]
pub struct OwnedCertRevocationList {
/// A map of the revoked certificates contained in then CRL, keyed by the DER encoding
Expand Down
63 changes: 20 additions & 43 deletions src/der.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,16 +298,14 @@ impl<'a> BitStringFlags<'a> {
}

// ASN.1 BIT STRING fields for sets of flags are encoded in DER with some peculiar details related
// to padding. Notably this means we expect a Tag::BitString, a length, an indicator of the number
// of bits of padding, and then the actual bit values. See this Stack Overflow discussion[0], and
// ITU X690-0207[1] Section 8.6 and Section 11.2 for more information.
// to padding. Notably this means we expect an indicator of the number of bits of padding, and then
// the actual bit values. See this Stack Overflow discussion[0], and ITU X690-0207[1] Section 8.6
// and Section 11.2 for more information.
//
// [0]: https://security.stackexchange.com/a/10396
// [1]: https://www.itu.int/ITU-T/studygroups/com17/languages/X.690-0207.pdf
pub(crate) fn bit_string_flags<'a>(
input: &mut untrusted::Reader<'a>,
) -> Result<BitStringFlags<'a>, Error> {
expect_tag_and_get_value(input, Tag::BitString)?.read_all(Error::BadDer, |bit_string| {
pub(crate) fn bit_string_flags(input: untrusted::Input) -> Result<BitStringFlags<'_>, Error> {
input.read_all(Error::BadDer, |bit_string| {
// ITU X690-0207 11.2:
// "The initial octet shall encode, as an unsigned binary integer with bit 1 as the least
// significant bit, the number of unused bits in the final subsequent octet.
Expand Down Expand Up @@ -627,59 +625,38 @@ mod tests {
}
}

#[allow(clippy::as_conversions)] // infallible.
const BITSTRING_TAG: u8 = super::Tag::BitString as u8;

#[test]
fn misencoded_bit_string_flags() {
use super::{bit_string_flags, Error};

let mut bad_padding_example = untrusted::Reader::new(untrusted::Input::from(&[
BITSTRING_TAG, // BitString
0x2, // 2 bytes of content.
0x08, // 8 bit of padding (illegal!).
0x06, // 1 byte of bit flags asserting bits 5 and 6.
]));
let bad_padding_example = untrusted::Input::from(&[
0x08, // 8 bit of padding (illegal!).
0x06, // 1 byte of bit flags asserting bits 5 and 6.
]);
assert!(matches!(
bit_string_flags(&mut bad_padding_example),
bit_string_flags(bad_padding_example),
Err(Error::BadDer)
));

let mut bad_padding_example = untrusted::Reader::new(untrusted::Input::from(&[
BITSTRING_TAG, // BitString
0x2, // 2 bytes of content.
0x01, // 1 bit of padding.
// No flags value (illegal with padding!).
]));
let bad_padding_example = untrusted::Input::from(&[
0x01, // 1 bit of padding.
// No flags value (illegal with padding!).
]);
assert!(matches!(
bit_string_flags(&mut bad_padding_example),
bit_string_flags(bad_padding_example),
Err(Error::BadDer)
));

let mut trailing_zeroes = untrusted::Reader::new(untrusted::Input::from(&[
BITSTRING_TAG, // BitString
0x2, // 2 bytes of content.
0x01, // 1 bit of padding.
0xFF, // Flag data with
0x00, // trailing zeros.
]));
assert!(matches!(
bit_string_flags(&mut trailing_zeroes),
Err(Error::BadDer)
))
}

#[test]
fn valid_bit_string_flags() {
use super::bit_string_flags;

let mut example_key_usage = untrusted::Reader::new(untrusted::Input::from(&[
BITSTRING_TAG, // BitString
0x2, // 2 bytes of content.
0x01, // 1 bit of padding.
0x06, // 1 byte of bit flags asserting bits 5 and 6.
]));
let res = bit_string_flags(&mut example_key_usage).unwrap();
let example_key_usage = untrusted::Input::from(&[
0x01, // 1 bit of padding.
0x06, // 1 byte of bit flags asserting bits 5 and 6.
]);
let res = bit_string_flags(example_key_usage).unwrap();

assert!(!res.bit_set(0));
assert!(!res.bit_set(1));
Expand Down
2 changes: 2 additions & 0 deletions src/subject_name/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ pub use ip_address::IpAddr;
mod verify;
#[cfg(feature = "alloc")]
pub(super) use verify::list_cert_dns_names;
#[allow(unused_imports)] // TODO(@cpu): remove once used by cert module.
pub(crate) use verify::GeneralName;
pub(super) use verify::{
check_name_constraints, verify_cert_subject_name, SubjectCommonNameContents,
};
26 changes: 12 additions & 14 deletions src/subject_name/verify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -392,10 +392,11 @@ pub(crate) fn list_cert_dns_names<'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>),
UniformResourceIdentifier(untrusted::Input<'a>),

// The value is the `tag & ~(der::CONTEXT_SPECIFIC | der::CONSTRUCTED)` so
// that the name constraint checking matches tags regardless of whether
Expand All @@ -404,8 +405,10 @@ enum GeneralName<'a> {
}

impl<'a> GeneralName<'a> {
fn from_der(input: &mut untrusted::Reader<'a>) -> Result<Self, Error> {
pub(crate) fn from_der(input: &mut untrusted::Reader<'a>) -> Result<Self, Error> {
use ring::io::der::{CONSTRUCTED, CONTEXT_SPECIFIC};
use GeneralName::*;

#[allow(clippy::identity_op)]
const OTHER_NAME_TAG: u8 = CONTEXT_SPECIFIC | CONSTRUCTED | 0;
const RFC822_NAME_TAG: u8 = CONTEXT_SPECIFIC | 1;
Expand All @@ -419,18 +422,13 @@ impl<'a> GeneralName<'a> {

let (tag, value) = der::read_tag_and_get_value(input)?;
Ok(match tag {
DNS_NAME_TAG => GeneralName::DnsName(value),
DIRECTORY_NAME_TAG => GeneralName::DirectoryName(value),
IP_ADDRESS_TAG => GeneralName::IpAddress(value),

OTHER_NAME_TAG
| RFC822_NAME_TAG
| X400_ADDRESS_TAG
| EDI_PARTY_NAME_TAG
| UNIFORM_RESOURCE_IDENTIFIER_TAG
| REGISTERED_ID_TAG => {
GeneralName::Unsupported(tag & !(CONTEXT_SPECIFIC | CONSTRUCTED))
}
DNS_NAME_TAG => DnsName(value),
DIRECTORY_NAME_TAG => DirectoryName(value),
IP_ADDRESS_TAG => IpAddress(value),
UNIFORM_RESOURCE_IDENTIFIER_TAG => UniformResourceIdentifier(value),

OTHER_NAME_TAG | RFC822_NAME_TAG | X400_ADDRESS_TAG | EDI_PARTY_NAME_TAG
| REGISTERED_ID_TAG => Unsupported(tag & !(CONTEXT_SPECIFIC | CONSTRUCTED)),

_ => return Err(Error::BadDer),
})
Expand Down
7 changes: 5 additions & 2 deletions src/verify_cert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -468,14 +468,17 @@ impl KeyUsageMode {
// https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.3
fn check(self, input: Option<untrusted::Input>) -> Result<(), Error> {
let bit_string = match input {
Some(input) => input,
Some(input) => der::expect_tag_and_get_value(
&mut untrusted::Reader::new(input),
der::Tag::BitString,
)?,
// While RFC 5280 requires KeyUsage be present, historically the absence of a KeyUsage
// has been treated as "Any Usage". We follow that convention here and assume the absence
// of KeyUsage implies the required_ku_bit_if_present we're checking for.
None => return Ok(()),
};

let flags = der::bit_string_flags(&mut untrusted::Reader::new(bit_string))?;
let flags = der::bit_string_flags(bit_string)?;
#[allow(clippy::as_conversions)] // u8 always fits in usize.
match flags.bit_set(self as usize) {
true => Ok(()),
Expand Down
Binary file not shown.
Loading