From bc040f2eb066f88bbbf17f31920f5cb679bb832c Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 31 Aug 2023 11:34:12 -0400 Subject: [PATCH 1/2] subject_name: clarify var name in check_name_constraints. --- src/subject_name/verify.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index f303e9fd..058da6f7 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -97,11 +97,11 @@ pub(crate) fn verify_cert_subject_name( // https://tools.ietf.org/html/rfc5280#section-4.2.1.10 pub(crate) fn check_name_constraints( - input: Option<&mut untrusted::Reader>, + constraints: Option<&mut untrusted::Reader>, subordinate_certs: &Cert, subject_common_name_contents: SubjectCommonNameContents, ) -> Result<(), Error> { - let input = match input { + let constraints = match constraints { Some(input) => input, None => return Ok(()), }; @@ -116,8 +116,8 @@ pub(crate) fn check_name_constraints( der::expect_tag(inner, subtrees_tag).map(Some) } - let permitted_subtrees = parse_subtrees(input, der::Tag::ContextSpecificConstructed0)?; - let excluded_subtrees = parse_subtrees(input, der::Tag::ContextSpecificConstructed1)?; + let permitted_subtrees = parse_subtrees(constraints, der::Tag::ContextSpecificConstructed0)?; + let excluded_subtrees = parse_subtrees(constraints, der::Tag::ContextSpecificConstructed1)?; let mut child = subordinate_certs; loop { From 03f7ca9f5768020582db3e8b81f90b8cd314d790 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 31 Aug 2023 11:47:56 -0400 Subject: [PATCH 2/2] verify_cert: bound name constraint comparisons. Enforcing name constraints during path building is quadratic with the number of names, and the number of name constraints. To avoid the possibility of excessive CPU consumption enforcing name constraints TLS libraries typically enforce a maximum number of comparisons while evaluating a constrained certificate against subsequent certificate names. In this commit we adopt the same limit as the default limit used by Go's `x509` package[0], bounding comparisons for a single constrained certificate to 250,000 comparisons. This is done per-constrained certificate and not overall path-building operation, but the number of constrained certificates that can be included in a path is constrained elsewhere (e.g. by Rustls imposing a 64KB limit to the entire chain). Other users of this library should consider similar countermeasures. [0]: https://github.com/golang/go/blob/9aaf5234bf652fc788782fc04a06044879b5957a/src/crypto/x509/verify.go#L588-L591 --- src/error.rs | 8 ++++++-- src/subject_name/verify.rs | 15 ++++++++++++++- src/verify_cert.rs | 3 ++- tests/generate.py | 9 +++++++++ tests/tls_server_certs.rs | 10 ++++++++++ .../name_constraints_abuse.ca.der | Bin 0 -> 30829 bytes .../name_constraints_abuse.ee.der | Bin 0 -> 39790 bytes 7 files changed, 41 insertions(+), 4 deletions(-) create mode 100644 tests/tls_server_certs/name_constraints_abuse.ca.der create mode 100644 tests/tls_server_certs/name_constraints_abuse.ee.der diff --git a/src/error.rs b/src/error.rs index 987d0a4d..74e2e3fc 100644 --- a/src/error.rs +++ b/src/error.rs @@ -93,6 +93,9 @@ pub enum Error { /// invalid labels. MalformedNameConstraint, + /// The maximum number of name constraint comparisons has been reached. + MaximumNameConstraintComparisonsExceeded, + /// The maximum number of signature checks has been reached. Path complexity is too great. MaximumSignatureChecksExceeded, @@ -238,8 +241,9 @@ impl Error { Error::BadDerTime => 2, Error::BadDer => 1, - // Special case error - not subject to ranking. - Error::MaximumSignatureChecksExceeded => 0, + // Special case errors - not subject to ranking. + Error::MaximumSignatureChecksExceeded + | Error::MaximumNameConstraintComparisonsExceeded => 0, // Default catch all error - should be renamed in the future. Error::UnknownIssuer => 0, diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index 058da6f7..ee3d80de 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -106,6 +106,8 @@ pub(crate) fn check_name_constraints( None => return Ok(()), }; + let mut comparisons = 0_usize; + fn parse_subtrees<'b>( inner: &mut untrusted::Reader<'b>, subtrees_tag: der::Tag, @@ -132,7 +134,12 @@ pub(crate) fn check_name_constraints( Err(err) => return Some(Err(err)), }; - check_presented_id_conforms_to_constraints(name, permitted_subtrees, excluded_subtrees) + check_presented_id_conforms_to_constraints( + name, + permitted_subtrees, + excluded_subtrees, + &mut comparisons, + ) }); if let Some(Err(err)) = result { @@ -154,6 +161,7 @@ fn check_presented_id_conforms_to_constraints( name: GeneralName, permitted_subtrees: Option, excluded_subtrees: Option, + comparisons: &mut usize, ) -> Option> { let subtrees = [ (Subtrees::PermittedSubtrees, permitted_subtrees), @@ -173,6 +181,11 @@ fn check_presented_id_conforms_to_constraints( let mut has_permitted_subtrees_match = false; let mut has_permitted_subtrees_mismatch = false; while !constraints.at_end() { + *comparisons += 1; + if *comparisons > 250_000 { + return Some(Err(Error::MaximumNameConstraintComparisonsExceeded)); + } + // http://tools.ietf.org/html/rfc5280#section-4.2.1.10: "Within this // profile, the minimum and maximum fields are not used with any name // forms, thus, the minimum MUST be zero, and maximum MUST be absent." diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 8a7403bf..aa534603 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -664,7 +664,8 @@ where for v in values { match f(v) { Ok(()) => return Ok(()), - err @ Err(Error::MaximumSignatureChecksExceeded) => return err, + err @ Err(Error::MaximumSignatureChecksExceeded) + | err @ Err(Error::MaximumNameConstraintComparisonsExceeded) => return err, Err(new_error) => error = error.most_specific(new_error), } } diff --git a/tests/generate.py b/tests/generate.py index 0c0746aa..a5a15342 100755 --- a/tests/generate.py +++ b/tests/generate.py @@ -558,6 +558,15 @@ def tls_server_certs(force: bool) -> None: ], ) + generate_tls_server_cert_test( + output, + "name_constraints_abuse", + expected_error="MaximumNameConstraintComparisonsExceeded", + valid_names=["a"], + sans=[x509.DNSName("a")] * 13000, + permitted_subtrees=[x509.DNSName("a")] * 6000, + ) + def signatures(force: bool) -> None: rsa_pub_exponent: int = 0x10001 diff --git a/tests/tls_server_certs.rs b/tests/tls_server_certs.rs index 06c84604..6004b2e1 100644 --- a/tests/tls_server_certs.rs +++ b/tests/tls_server_certs.rs @@ -364,3 +364,13 @@ fn invalid_dns_name_matching() { let ca = include_bytes!("tls_server_certs/invalid_dns_name_matching.ca.der"); assert_eq!(check_cert(ee, ca, &["dns.example.com"], &[]), Ok(())); } + +#[test] +fn name_constraints_abuse() { + let ee = include_bytes!("tls_server_certs/name_constraints_abuse.ee.der"); + let ca = include_bytes!("tls_server_certs/name_constraints_abuse.ca.der"); + assert_eq!( + check_cert(ee, ca, &["a"], &[]), + Err(webpki::Error::MaximumNameConstraintComparisonsExceeded) + ); +} diff --git a/tests/tls_server_certs/name_constraints_abuse.ca.der b/tests/tls_server_certs/name_constraints_abuse.ca.der new file mode 100644 index 0000000000000000000000000000000000000000..167324ad95da13a7cd5967a15f9047c230b537f5 GIT binary patch literal 30829 zcmeI*T}+c#7zgn8yf0uVRH}l|@->QV!i1a_N~9vnBw!g?WJJsuRSI^{t*wq))@+U? zuM@Tzl>q}?7p2arn@Gj+A#7Ho1|#f4s+)0AC&I+)Mk=~6-Bwm^mhF1G{NJ3D=Q+vA z$@84^yFHY*=qUeMVH?L1mJMBZ@+C6tPK{XI;m9b?l8D~B9;v3nfWV}>)pgTCLW2Am znk9>*;s9=~9~V4dXEK?!)hSww#;{qhO{v&qkj2vI*+oI{LZil@-B7W~XxdV((HXax zHfYMtCM}H#7cNbg$>ph3rpS=VO8ySzl={N|ygyH*|2#7BXE>gOFq9_(mM4ViiaIfL zX~*@ZsSmzka|Lf-?y@!a>YTgnpY%Dm@pSC9@Jn#2&(F?fvl6wl}(G$+t5#g`; zj_Iq1$W&2sx7ztfW?4v6`|JDG%Li*$Jeu+5Or%F{%{x0>m=NE;_e0(lc1m@4-!C-0 z$8slI+ylAd+PB!`iW=W|8k^xbK%qhE4Il_>S zd9#}G<`-y)uUXLx2$}Kor<`x{C39v|F|#S&ywn$`wee<3eMQt@Kn-vY@C-l!B!q;J z5E4Q{NC*ibAtZ!^kPs3=LP!V+At5A$gpd#tLPAIg2_YdQgoKa~5<)^q2nit}B!q;J z5E4Q{NC*ibAtZ!^kPs3=LP!V+At5A$gpd#tLPAIg2_YdQgoKa~5<)^q2nit}B!q;J z5E4Q{NC*ibAtZ!^kPs3=LP!V+At5A$gpd#tLPAIg2_YdQgoKa~5<)^q2nit}B!q;J z5E4Q{NC*ibAtZ!^kPs3=LP!V+{|^cO{cEmI7Q%$*E7z1r8%mEHyj84R+y792bwz%y zw>?Ojcz1i0>3~p?xXzlq zN0oHbD|qL*BwbE=?e2?;_yq5LnYeyQp1<~F!KH(Ny>&T+J<~k)qiCR|+jCo8<~EKA gN~Mb@ZC4#t69Iu&+KVLbCb%xBHBU#KiCek literal 0 HcmV?d00001 diff --git a/tests/tls_server_certs/name_constraints_abuse.ee.der b/tests/tls_server_certs/name_constraints_abuse.ee.der new file mode 100644 index 0000000000000000000000000000000000000000..d701e26d1e43e2ec29768c6e95e270b9fc931d10 GIT binary patch literal 39790 zcmeIv?@v>A6bA77?WIL&>$XY+fr79XvP$u`sH@JcY{N*(G(uO$j|miL6$V9nL69<$ z+fX-QOPz)wBH09F8G>VG*;s@?Iuv9;2xJNqT?HMX2m#|NU8ZAimc85lfG0WU#gmhq zoO}+$ozG#o+Jw((9^vtP$tCOk*XK;_-jAPtbktOLquP1;E+ZDKmf2!$ZZRQ3KE=GD z2xQg^Xq^`=l4P@Np}`z!DAwm4&NW14=I1Fw80p_bAJICKKF^SrnQvkX%=&Co0h^}J zC}a&xu%B49O`%jqF^a9*6^i74qLN`&A`|-GD>xGN@2d!(qB-&c#c;%j#}Ps`d#`Hv zAS`s^Ge?)wp9^iSEmm0!4W29E0e<(YZamdA%DXZmbv+wf?4|yms~CY6s~H$ zeO0nSwiunR4%URlOht@otkJ&9DV?KdM{b7nGn$eU%f{zdTG{U5q6@bQrK2qw0aXWk z4;N2-y`|!Ga&^+RA64y-_84pF>gnE@eP8Q5l1-|tJ89}8qMn1dCeN*jn=xIE zkLiC%TS~)!-VwsyTNsQPikoRtpOufjckHXj+T$k%>$OdlJz7t9fbr@fcecONX%oMh z)b`;;(T?<;)6`-`*L;MFaIY=RSNG}n^zOW?jvkCs2EFTT*;w|c?SVAqTOLg)(!kYN z7_O#hr41_|k-p@`)x5tF5;>wrK>z{}fB*y_009U<00Izz00bZa0SG_<0uX=z1Rwwb z2tWV=5P$##AOHafKmY;|fB*y_009U<00Izz00bZa0SG_<0uX=z1Rwwb2tWV=5P$## zAOHafKmY;|fB*y_009U<00Izz00bZa0SG_<0uX=z1Rwwb2tWV=5P$##AOHafKmY;| zfB*y_009U<00Izz00bZa0SG_<0uX=z1Rwwb2tWV=5P$##AOHafKmY;|fB*y_009U< z00Izz00bZa0SG_<0uX=z1R(G~1sIWl)=AeA^4yEZP>fixT4sy2xy6JC`4r6&Le;X? z*(Arc*ygr@Kh|%MMGriCr`x;CZqFQc%X*z`$x>YEolSDvHv`wR9Je*QpVg-?FY&vk zo7|sP4L^D7Rr!J>!IAJuZ!o)aZgmdD}sLbaeLGJce}RgQe}JN zjOVni)(HL@W2`dg5KmmtQJ!Ea|G_#X%=%@UEDbHWGrvs@{RNT7 Biv$1w literal 0 HcmV?d00001