From 84928c61c33c7d2597b441c6c310636c3722d3ef Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 24 Sep 2022 13:07:16 +0200 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 | 57 +++++++++++++++++++++++++++++++++++++++++-- src/name/verify.rs | 24 +++++++++++++++--- tests/integration.rs | 19 +++++++++++++++ tests/wpt/ca.der | Bin 0 -> 16523 bytes tests/wpt/ee.der | Bin 0 -> 8384 bytes 5 files changed, 94 insertions(+), 6 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..88ca4e7a 100644 --- a/src/der.rs +++ b/src/der.rs @@ -14,16 +14,69 @@ use crate::{calendar, time, Error}; pub use ring::io::{ - der::{nested, Tag, CONSTRUCTED}, + der::{CONSTRUCTED, CONTEXT_SPECIFIC}, Positive, }; +#[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, +} + +impl From for usize { + fn from(tag: Tag) -> Self { + tag as Self + } +} + +impl From for u8 { + fn from(tag: Tag) -> Self { + tag as Self + } // XXX: narrowing conversion. +} + #[inline(always)] pub 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 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> { 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/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$}