Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address denial of service in name constraints processing #4186

Merged
merged 1 commit into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading