From f4204bb2430ccc3730cd4d04b0726ffa5298bd06 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sun, 27 Nov 2022 13:55:08 +0100 Subject: [PATCH] Fix name constraints check 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 https://github.com/briansmith/ring/pull/1265. Closes #3. --- src/der.rs | 84 ++++++++++++++++++++++++++++++++++++------- src/name/verify.rs | 24 ++++++++++--- src/verify_cert.rs | 2 +- tests/integration.rs | 19 ++++++++++ tests/wpt/ca.der | Bin 0 -> 16523 bytes tests/wpt/ee.der | Bin 0 -> 8384 bytes 6 files changed, 112 insertions(+), 17 deletions(-) create mode 100644 tests/wpt/ca.der create mode 100644 tests/wpt/ee.der diff --git a/src/der.rs b/src/der.rs index c4cad81b..bb8213c7 100644 --- a/src/der.rs +++ b/src/der.rs @@ -13,17 +13,72 @@ // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. use crate::{calendar, time, Error}; -pub use ring::io::{ - der::{nested, Tag, CONSTRUCTED}, +pub(crate) use ring::io::{ + der::{CONSTRUCTED, CONTEXT_SPECIFIC}, Positive, }; +// Copied (and extended) from ring's src/der.rs +#[allow(clippy::upper_case_acronyms)] +#[derive(Clone, Copy, Eq, PartialEq)] +#[repr(u8)] +pub(crate) enum Tag { + Boolean = 0x01, + Integer = 0x02, + BitString = 0x03, + OctetString = 0x04, + 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, +} + +impl From for usize { + #[allow(clippy::as_conversions)] + fn from(tag: Tag) -> Self { + tag as Self + } +} + +impl From for u8 { + #[allow(clippy::as_conversions)] + fn from(tag: Tag) -> Self { + tag as Self + } // XXX: narrowing conversion. +} + #[inline(always)] -pub fn expect_tag_and_get_value<'a>( +pub(crate) fn expect_tag_and_get_value<'a>( input: &mut untrusted::Reader<'a>, tag: Tag, ) -> Result, Error> { - ring::io::der::expect_tag_and_get_value(input, tag).map_err(|_| Error::BadDER) + let (actual_tag, inner) = read_tag_and_get_value(input)?; + if usize::from(tag) != usize::from(actual_tag) { + return Err(Error::BadDER); + } + Ok(inner) +} + +// TODO: investigate taking decoder as a reference to reduce generated code +// size. +pub(crate) fn nested<'a, F, R, E: Copy>( + input: &mut untrusted::Reader<'a>, + tag: Tag, + error: E, + decoder: F, +) -> Result +where + F: FnOnce(&mut untrusted::Reader<'a>) -> Result, +{ + let inner = expect_tag_and_get_value(input, tag).map_err(|_| error)?; + inner.read_all(error, decoder) } pub struct Value<'a> { @@ -36,7 +91,10 @@ impl<'a> Value<'a> { } } -pub fn expect_tag<'a>(input: &mut untrusted::Reader<'a>, tag: Tag) -> Result, Error> { +pub(crate) fn expect_tag<'a>( + input: &mut untrusted::Reader<'a>, + tag: Tag, +) -> Result, Error> { let (actual_tag, value) = read_tag_and_get_value(input)?; if usize::from(tag) != usize::from(actual_tag) { return Err(Error::BadDER); @@ -46,7 +104,7 @@ pub fn expect_tag<'a>(input: &mut untrusted::Reader<'a>, tag: Tag) -> Result( +pub(crate) fn read_tag_and_get_value<'a>( input: &mut untrusted::Reader<'a>, ) -> Result<(u8, untrusted::Input<'a>), Error> { ring::io::der::read_tag_and_get_value(input).map_err(|_| Error::BadDER) @@ -54,7 +112,7 @@ pub fn read_tag_and_get_value<'a>( // TODO: investigate taking decoder as a reference to reduce generated code // size. -pub fn nested_of_mut<'a, E>( +pub(crate) fn nested_of_mut<'a, E>( input: &mut untrusted::Reader<'a>, outer_tag: Tag, inner_tag: Tag, @@ -75,7 +133,7 @@ where }) } -pub fn bit_string_with_no_unused_bits<'a>( +pub(crate) fn bit_string_with_no_unused_bits<'a>( input: &mut untrusted::Reader<'a>, ) -> Result, Error> { nested(input, Tag::BitString, Error::BadDER, |value| { @@ -89,7 +147,7 @@ pub fn bit_string_with_no_unused_bits<'a>( // Like mozilla::pkix, we accept the nonconformant explicit encoding of // the default value (false) for compatibility with real-world certificates. -pub fn optional_boolean(input: &mut untrusted::Reader) -> Result { +pub(crate) fn optional_boolean(input: &mut untrusted::Reader) -> Result { if !input.peek(Tag::Boolean.into()) { return Ok(false); } @@ -102,15 +160,17 @@ pub fn optional_boolean(input: &mut untrusted::Reader) -> Result { }) } -pub fn positive_integer<'a>(input: &'a mut untrusted::Reader) -> Result, Error> { +pub(crate) fn positive_integer<'a>( + input: &'a mut untrusted::Reader, +) -> Result, Error> { ring::io::der::positive_integer(input).map_err(|_| Error::BadDER) } -pub fn small_nonnegative_integer(input: &mut untrusted::Reader) -> Result { +pub(crate) fn small_nonnegative_integer(input: &mut untrusted::Reader) -> Result { ring::io::der::small_nonnegative_integer(input).map_err(|_| Error::BadDER) } -pub fn time_choice(input: &mut untrusted::Reader) -> Result { +pub(crate) fn time_choice(input: &mut untrusted::Reader) -> Result { let is_utc_time = input.peek(Tag::UTCTime.into()); let expected_tag = if is_utc_time { Tag::UTCTime diff --git a/src/name/verify.rs b/src/name/verify.rs index 30e428ac..05659d75 100644 --- a/src/name/verify.rs +++ b/src/name/verify.rs @@ -63,10 +63,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)?; @@ -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) + } + (GeneralName::DirectoryName(name), GeneralName::DirectoryName(base)) => Ok( presented_directory_name_matches_constraint(name, base, subtrees), ), @@ -319,3 +320,18 @@ fn general_name<'a>(input: &mut untrusted::Reader<'a>) -> Result }; Ok(name) } + +static COMMON_NAME: untrusted::Input = untrusted::Input::from(&[85, 4, 3]); + +fn common_name(input: untrusted::Input) -> Result { + 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) + }) + }) +} diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 722ba932..31734c5f 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -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))?; if potential_issuer.subject != cert.issuer { return Err(Error::UnknownIssuer); diff --git a/tests/integration.rs b/tests/integration.rs index 34270272..36788d57 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -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"); diff --git a/tests/wpt/ca.der b/tests/wpt/ca.der new file mode 100644 index 0000000000000000000000000000000000000000..f7d00160116c90b9ed17c2d9838a311570dad4d6 GIT binary patch literal 16523 zcmb_jdvKK170+fj;T1p#5FrUMgzzY?eEaR2%EN-dC>BL(TO3Pc32kX$LxPF6JoMVZ z@)V(T7)2y%6(~xJLv4$-rMA2V84BY}?HC(iB-0Gq>Oip-wCCInG|Qgf*$wv3y}$Fi z=bn4NbMG&4%!J1rGd{7^o9oHV^>%F?=u3uE|3@+;F2Aoj9b=+Vu6q`jq-3~u9sTh-T(srU`Sq?2I=a`R7YpR&+bOtY77{H?P~@{ODtEyz}cH zz1(`~;)8D|j;!0>^ow5SAHBcdvnL){F`#Mvlpl7lYH7=@T)C(2!o1sFFDm`ptF66v zPb={jPs@3re(m&QMRT`*uyn!5Ltj49R96t#Ryy;Qo|F2nY{@GxzBIk&q-O`Zt}}(<3f}~0+{2&<{fZAkKNk*G99iv+Na zBm1F{rQ9bY06LUoX*$UW;Cy1xS{xz~Dbd!C3w7c!f(S&Scs^2{C9?yq1)+`$kuaQT z48|6MD;I}I5Y~mm5X9)aD75Bqj3;HqBaI~ zVsI5AI6E-5Fw9#J&eVlbM`7L~aF+z(Ov6wo408~K5d`3T!Vqy`jRtXSFxN4d>j;b> z3av$9-l8}=a6Vy}Hy2t9!FmfrKT(K8;7o%sf&k2D9L5%a)`HNQD`ltH4CMY$?kX)9 z$DnQQ#0Jl5hOj1`4H_8HKvx698W_bOPen;W0DvWC5cS>2u0?6f%7!gU4VZ#b%b=jt z4l8ItwM-vXMVH!X1*Kz9L8&cJ&^Rh5n+UhDNij&X+C|if%}anJ(puF154@N{3RZn>0wV@LApLDx^&T2P`aEdXjrvO zPb`|Qx(f8vqUh3tf`U>Dub}iyp`i2#sGuP=W>=jrJqSX}c6Zwn=ssSO!+{x_Jx4=(WgYZ|Kkk(#PG!fP=}w3|R1 zCn;fP08;Qj8)$Ko5|kepX}KvQ(0V~ek|Nef79wam9ULK%p=JmNnjv!}l_DXR$JJ6c z*=Z$KX)01|k(5+Sf|yhmBxVhynv|T9n44*yBh0W=LvRgAlBbschdB#>Vf}E&NV{8>(HDypsi4WCp(icwMwfI8kZ_Gqcgemz?FYI>BD_<&PCSCJpGX@E#_>||_ zuAR}ZtUb}(qWR9QQm2?(J8MLarjW_`N?(D~J-_Ryd{16p7q7=LrH}J)AD<~*PCtym z)&s8I^cUT*wD3mE!vZ0}!@_$m4-1d&JS@CS^RV#DZu;}iORET$7M`7XSU6e`E6?K+ zC(q*&BX8veHo^>`Acc>)^()HDOuY3A;+I%>mX|nq9+wz+9+&ucYX|t1xObM9n0Fqh zi){L8(Kt2r;<&`Q+c<$8iEn3liEZa`iEHO^iD~C?iD&0=iDkF;rOGFHdQ=nVEAi_# z9%)aI9f@1Fex?4UOw{e1XkyvtT zADnvW07~-lKLrgVeh^Mv~c#mcbany_XxnL9?M!gw;3^0v*y z`C3xg8bBV{f)zl!_=*c8kOkM94byv-j9fn1u)OJ1u(a^mFuaxq&cyF#%kE~&?PksF zTET31T{Ap|$FbW%E~hj>wC33}x}D;4JIUr6KUWFje$ro1AXhAB16!5}iB7V(Do-qx zEZfI4WcqYF%imrDdusyP3dm`_2IkhdaFIDB*bhz@&{6!L6Z9Gx+a%MQkvC0W*3ZG# zCYj!hKIPneex~;S#nYz#lRf>d|J!dK!)~x!^wrzjk{?QYs`W@f9Z}|%I@%+a(ZdtPbU~BR08Pg}9ZS^hx)9!P} z-VNoy*nQsm-=1GOY)zBrwsk)ncX0OcZ~wAz=~thqzVz_igOh$R#(elt`IeVIIk50+ zr|)an{L=%AyRBWayRz+tf0rEH_fq|wU;X%-H}*d}byc{mux-dIQzwuAv`^$7U#^*O zsdZ-I^Jg}e|M}||Yw8cRZC*Mv@7|r~Dhghx&w2Z~FV!s>^NkTJhrPJu`e`%&u({X9 zsymB*clXJCEs?Po_jvEU?w;F^>^U@Q^v-4FpUsF?z1vX#{b9!%-M%07d27|gzrHi& G$^QarS!9*~ literal 0 HcmV?d00001 diff --git a/tests/wpt/ee.der b/tests/wpt/ee.der new file mode 100644 index 0000000000000000000000000000000000000000..7160db5aa8663bb6faf5bd1326faa302844d8188 GIT binary patch literal 8384 zcmb7Jdr(wW7~f@gQ9v{lkcSHjK5-WBy}NsN*GGwy)}#jFqfAi|L{Vf{AQ!6gbH8)o z96c*IN96XcDles%m#X8{A)LRrPfWqv1rPm|N5E17c+WmNn1lp2M~ zq3og{xv!T)sZ><=|GX?BZo|_p*N$(>$)Eb(hdaIu+&JaY&U^Kjv`f8LuemfjP-rT& zS2sU#mW4FcyqO+ZT=h&iv1Dm|`F@`n`R5LdN*J^3#@yoHr^R2i9Nky687u|aQ`ic_?KFh!OZRU{M1qaGYE%Et(9q}!1SXIB4 z3l}u|qlP}y|KL}5?5O6+u7-vg^So3_h4O1hgqm|iC^#R=qqdvci=(PYe|A;}s{_T| zm8Y+ri`&zvTo*fG?6-r9?rUdokrEuO4&j174y^4If2G%r4qKuF3cKAooD}G@r?7Y__4()159H4e+3+RW#;aNUMkk!V=dQ{aB zlxfY;F~nAzg{5t^>DgifTWn^FO>D7|CAQn`EV0!lvc&>J9Bj22SZk8p#0rSyJksY_ z{NyAnyVAN$eaa)teQ90bzZvjYfLvrHllTRoZvpFyKyE_*v-(EL1K`mEeF4agATBfT z#{lYL0dhU)2LY^WV0FG!N5I1a9t-F<5%3#WTZ`7U0J(_#0sUYG^)jM71AYUlBhWX2 zc+DU#3&^Jk>dOQF%_#psE&x9Tz;8kNATBdAKXlH3x|k3T$iD&P&B9#QYBK|UGsv@u z@&NoXg8b+~TmtaZ1o9(-KG%b|j6mNA;?;w7dEmbh$OX`EdgKqtry1l^1nZgrj|t?* zgz^CVH-h{KfX4v(#|ZdMKrVu~^k7{cOoQEOv01BgQAV$Jyv#}`&qd^8@^ihTw4UQ0_VGUyRA;TD| zw&W*=?danRIW4G7h?9m#Xh#Do#Hd*zMh70mM46n?haB3`M-O7O(I7^f8DccZLyR^x z#OQ+xG5Tmgj1ESK(cuCy8uTDW!!W>}9|5dGi0V!s4Pa-r(I5>mI!qx(gABl0h7;y_ zM841<2<%+LGQ?<5ffx-X5TgMfVlG>c zpa<;eyq1ZNE*Q{`mx+%qbkL55U5L?u1~Aut2N*r9$Z#5EdV&Uc%qg>vXt0HLG_*sE zE|36wezvj(HagE}fCqMT*2&a}25o3Zmr97y#SCIb8BTgAz;-gZrUwIPN0()Y(clX) zx_m>79;P5hk4F#_Wa6VoDq!c@o2~-4dnb22bUUcFbZKDQg}2(VKDZJ{&BA^PY_nvw zQ=26V)Vkb+({e;^rcRf!aF$NYNKR$EO-4L7M_WL`U0#EbTkTN^B%HDV{flhnM+B0B zi{OO{D1^2pkV2H?-V5>+<%YA%@2(?}dOn>;yYaNd=FwvwO02?1yNuoGJef#Icu{P9 z-AQ1r`br_W>9X_C&vQ|%$iEb$+o~>l!1#YfRPzCnVy1bnn3`?WXV~rYvKGu)D43HO zB^&6*>Gs33zBw8hLb=96zgNTTVZiArJmR zZpn80!48;*+_0&6xeBmX5oFT@Y-Bt`Ju0o!vOQSkc=dJA2`VDr+%oRj>n^*tZwn&v zv>S|6-PU8r=dyj_nrz!Rxpi47_H=#Zt5PsMY5$*c`p-Q5r(CHB$}