Skip to content

Commit

Permalink
Address denial of service in name constraints processing
Browse files Browse the repository at this point in the history
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
  • Loading branch information
randombit committed Jul 8, 2024
1 parent 76ba92a commit 655c2be
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 4 deletions.
30 changes: 30 additions & 0 deletions src/lib/x509/name_constraint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -296,13 +297,38 @@ 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;
}

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;
Expand Down Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions src/lib/x509/pkix_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand Down
4 changes: 0 additions & 4 deletions src/scripts/run_limbo_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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',

Expand Down

0 comments on commit 655c2be

Please sign in to comment.