Fix name constraints check #7
Conversation
src/der.rs
Outdated
| #[allow(clippy::upper_case_acronyms)] | ||
| #[derive(Clone, Copy, Eq, PartialEq)] | ||
| #[repr(u8)] | ||
| pub enum Tag { | ||
| Boolean = 0x01, | ||
| Integer = 0x02, | ||
| BitString = 0x03, | ||
| OctetString = 0x04, | ||
| Null = 0x05, | ||
| OID = 0x06, | ||
| UTF8String = 0x0C, | ||
| Sequence = CONSTRUCTED | 0x10, // 0x30 | ||
| Set = CONSTRUCTED | 0x11, // 0x31 | ||
| UTCTime = 0x17, | ||
| GeneralizedTime = 0x18, | ||
|
|
||
| #[allow(clippy::identity_op)] | ||
| ContextSpecificConstructed0 = CONTEXT_SPECIFIC | CONSTRUCTED | 0, | ||
| ContextSpecificConstructed1 = CONTEXT_SPECIFIC | CONSTRUCTED | 1, | ||
| ContextSpecificConstructed3 = CONTEXT_SPECIFIC | CONSTRUCTED | 3, | ||
| } |
There was a problem hiding this comment.
nit, take it or leave it: it might be worth including a comment stating that this is based on code from ring, so that readers can refer to the upstream code for context? also, ring is relatively permissively licensed, but it does look like the license requires reproducing a copyright notice: https://github.com/briansmith/ring/blob/0f3bf0031a8dbba741b26f1f02ebde6b7db4a3d6/LICENSE#L11-L23
There was a problem hiding this comment.
I'll add a comment. W.r.t. the license, der.rs from ring is under the same license as webpki, ISC with copyright attributed to Brian. I don't think additional attribution is necessary but I don't mind being proven wrong.
src/der.rs
Outdated
| #[allow(clippy::upper_case_acronyms)] | ||
| #[derive(Clone, Copy, Eq, PartialEq)] | ||
| #[repr(u8)] | ||
| pub enum Tag { |
There was a problem hiding this comment.
nit: should this maybe be
| pub enum Tag { | |
| pub(crate) enum Tag { |
since it's not publicly re-exported from the der module, and the der module is private?
There was a problem hiding this comment.
I followed the existing style of using pub and controlling exports in src/lib.rs. No strong preference though. If more reviewers think it's a good idea, I'll make the change.
src/der.rs
Outdated
|
|
||
| // TODO: investigate taking decoder as a reference to reduce generated code | ||
| // size. | ||
| pub fn nested<'a, F, R, E: Copy>( |
There was a problem hiding this comment.
nit: should this be
| pub fn nested<'a, F, R, E: Copy>( | |
| pub(crate) fn nested<'a, F, R, E: Copy>( |
since it's never publicly re-exported?
Codecov Report
@@ Coverage Diff @@
## main #7 +/- ##
==========================================
+ Coverage 74.60% 83.30% +8.69%
==========================================
Files 19 19
Lines 1788 1839 +51
==========================================
+ Hits 1334 1532 +198
+ Misses 454 307 -147
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
It would be great if someone can do some thinking on what Brian meant in briansmith/ring#1265 (comment):
As for the visibility comments, I would like to |
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. Requires lifting some DER parsing logic from ring to parse UTF8String and Set fields. Ring doesn't support those and isn't likely to in the near future, see briansmith/ring#1265. Closes briansmith#3.
|
Updated, PTAL. Used |
| @@ -81,7 +81,7 @@ pub fn build_chain( | |||
|
|
|||
| loop_while_non_fatal_error(intermediate_certs, |cert_der| { | |||
| let potential_issuer = | |||
| cert::parse_cert(untrusted::Input::from(*cert_der), EndEntityOrCa::Ca(cert))?; | |||
| cert::parse_cert(untrusted::Input::from(cert_der), EndEntityOrCa::Ca(cert))?; | |||
There was a problem hiding this comment.
FYI: triggered clippy::explicit-auto-deref
|
Can I get a review, please? Thanks. |
| @@ -160,6 +157,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) | |||
There was a problem hiding this comment.
isn't base here a constraint rather than a name? ie it should match if it is equal (as currently) but also match (say) if the name is "www.foo.com" and the constraint allows ".foo.com"?
There was a problem hiding this comment.
i think my conclusion on this is: it's an excellent start, and fixes web-platform-tests's use of name constraints to reduce the strength of their testing CA. but for me it's underlining a big testing debt here. i will look at that in the coming days; especially and also in the effort to land briansmith/webpki#131
| 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 { |
There was a problem hiding this comment.
This goes wrong if the first AttributeTypeAndValue in the Name has a different OID, and also if there is no commonName in the Subject. I have some test cases coming for this.
|
Cards on the table: I've kind of moved on and don't expect to revisit this PR anytime soon. No hard feelings if you want to close it. Feel free to adopt it, of course (edit: which I think you've done.) |
|
Thanks, this was used as a basis for the fix now on main. |
There were two bugs. webpki didn't:
read the X.509 Name Constraints field in its entirety, nor
check the certificate subject against the constraints correctly
(1) is a simple fix, (2) requires reading the Common Name from the certificate.
Requires lifting some DER parsing logic from ring to parse UTF8String and Set fields. Ring doesn't support those and isn't likely to in the near future, see briansmith/ring#1265.
Closes #3.