From 655c2be095325492f88ea0778b155e7bcedee3b3 Mon Sep 17 00:00:00 2001
From: Jack Lloyd <jack@randombit.net>
Date: Wed, 3 Jul 2024 08:47:59 -0400
Subject: [PATCH] Address denial of service in name constraints processing

Checking a name constraint is quadratic to the number of names and
constraints. Place a hard limit on the total number of comparisons.

This issue was discovered and reported by Bing Shi.

CVE-2024-34702
---
 src/lib/x509/name_constraint.cpp | 30 ++++++++++++++++++++++++++++++
 src/lib/x509/pkix_types.h        |  2 ++
 src/scripts/run_limbo_tests.py   |  4 ----
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/src/lib/x509/name_constraint.cpp b/src/lib/x509/name_constraint.cpp
index 62c0a29cba1..44873b01ca4 100644
--- a/src/lib/x509/name_constraint.cpp
+++ b/src/lib/x509/name_constraint.cpp
@@ -11,6 +11,7 @@
 #include <botan/ber_dec.h>
 #include <botan/x509cert.h>
 #include <botan/internal/fmt.h>
+#include <botan/internal/int_utils.h>
 #include <botan/internal/loadstor.h>
 #include <botan/internal/parsing.h>
 #include <functional>
@@ -296,6 +297,27 @@ NameConstraints::NameConstraints(std::vector<GeneralSubtree>&& permitted_subtree
    }
 }
 
+namespace {
+
+bool exceeds_limit(size_t dn_count, size_t alt_count, size_t constraint_count) {
+   /**
+   * OpenSSL uses a similar limit, but applies it to the total number of
+   * constraints, while we apply it to permitted and excluded independently.
+   */
+   constexpr size_t MAX_NC_CHECKS = (1 << 20);
+
+   if(auto names = checked_add(dn_count, alt_count)) {
+      if(auto product = checked_mul(*names, constraint_count)) {
+         if(*product < MAX_NC_CHECKS) {
+            return false;
+         }
+      }
+   }
+   return true;
+}
+
+}  // namespace
+
 bool NameConstraints::is_permitted(const X509_Certificate& cert, bool reject_unknown) const {
    if(permitted().empty()) {
       return true;
@@ -303,6 +325,10 @@ bool NameConstraints::is_permitted(const X509_Certificate& cert, bool reject_unk
 
    const auto& alt_name = cert.subject_alt_name();
 
+   if(exceeds_limit(cert.subject_dn().count(), alt_name.count(), permitted().size())) {
+      return false;
+   }
+
    if(reject_unknown) {
       if(m_permitted_name_types.contains(GeneralName::NameType::Other) && !alt_name.other_names().empty()) {
          return false;
@@ -416,6 +442,10 @@ bool NameConstraints::is_excluded(const X509_Certificate& cert, bool reject_unkn
 
    const auto& alt_name = cert.subject_alt_name();
 
+   if(exceeds_limit(cert.subject_dn().count(), alt_name.count(), excluded().size())) {
+      return true;
+   }
+
    if(reject_unknown) {
       // This is one is overly broad: we should just reject if there is a name constraint
       // with the same OID as one of the other names
diff --git a/src/lib/x509/pkix_types.h b/src/lib/x509/pkix_types.h
index 0c1e4d3f0a5..5dd300e6687 100644
--- a/src/lib/x509/pkix_types.h
+++ b/src/lib/x509/pkix_types.h
@@ -66,6 +66,8 @@ class BOTAN_PUBLIC_API(2, 0) X509_DN final : public ASN1_Object {
 
       bool empty() const { return m_rdn.empty(); }
 
+      size_t count() const { return m_rdn.size(); }
+
       std::string to_string() const;
 
       const std::vector<std::pair<OID, ASN1_String>>& dn_info() const { return m_rdn; }
diff --git a/src/scripts/run_limbo_tests.py b/src/scripts/run_limbo_tests.py
index bce5834ddd6..69bf844ea54 100755
--- a/src/scripts/run_limbo_tests.py
+++ b/src/scripts/run_limbo_tests.py
@@ -50,10 +50,6 @@
     'webpki::san::wildcard-embedded-leftmost-san': 'CABF rule not RFC 5280',
     'webpki::ca-as-leaf': 'Not applicable outside of webpki',
 
-    'pathological::nc-dos-1': 'Todo',
-    'pathological::nc-dos-2': 'Todo',
-    'pathological::nc-dos-3': 'Todo',
-
     'webpki::explicit-curve': 'Deprecated but not gone yet',
     'rfc5280::nc::invalid-dnsname-leading-period': 'Common extension',