diff --git a/security/pkix/lib/pkixbuild.cpp b/security/pkix/lib/pkixbuild.cpp index 14bf100a6aed..a7c9264a863b 100644 --- a/security/pkix/lib/pkixbuild.cpp +++ b/security/pkix/lib/pkixbuild.cpp @@ -406,13 +406,4 @@ BuildCertChain(TrustDomain& trustDomain, return SECSuccess; } -PLArenaPool* -BackCert::GetArena() -{ - if (!arena) { - arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE); - } - return arena.get(); -} - } } // namespace mozilla::pkix diff --git a/security/pkix/lib/pkixcheck.cpp b/security/pkix/lib/pkixcheck.cpp index b8d6161977a7..b961a0d214d2 100644 --- a/security/pkix/lib/pkixcheck.cpp +++ b/security/pkix/lib/pkixcheck.cpp @@ -404,56 +404,49 @@ CheckBasicConstraints(EndEntityOrCA endEntityOrCA, return Success; } -Result -BackCert::GetConstrainedNames(/*out*/ const CERTGeneralName** result) -{ - if (!constrainedNames) { - if (!GetArena()) { - return FatalError; - } - - constrainedNames = - CERT_GetConstrainedCertificateNames(GetNSSCert(), arena.get(), - includeCN == IncludeCN::Yes); - if (!constrainedNames) { - return MapSECStatus(SECFailure); - } - } - - *result = constrainedNames; - return Success; -} - // 4.2.1.10. Name Constraints Result -CheckNameConstraints(BackCert& cert) +CheckNameConstraints(const BackCert& cert) { if (!cert.encodedNameConstraints) { return Success; } - PLArenaPool* arena = cert.GetArena(); + ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE)); if (!arena) { - return FatalError; + return MapSECStatus(SECFailure); } // Owned by arena const CERTNameConstraints* constraints = - CERT_DecodeNameConstraintsExtension(arena, cert.encodedNameConstraints); + CERT_DecodeNameConstraintsExtension(arena.get(), cert.encodedNameConstraints); if (!constraints) { return MapSECStatus(SECFailure); } - for (BackCert* prev = cert.childCert; prev; prev = prev->childCert) { - const CERTGeneralName* names = nullptr; - Result rv = prev->GetConstrainedNames(&names); - if (rv != Success) { - return rv; + for (const BackCert* child = cert.childCert; child; + child = child->childCert) { + ScopedCERTCertificate + nssCert(CERT_NewTempCertificate(CERT_GetDefaultCertDB(), + const_cast(&child->GetDER()), + nullptr, false, true)); + if (!nssCert) { + return MapSECStatus(SECFailure); } - PORT_Assert(names); + + bool includeCN = child->includeCN == BackCert::IncludeCN::Yes; + // owned by arena + const CERTGeneralName* + names(CERT_GetConstrainedCertificateNames(nssCert.get(), arena.get(), + includeCN)); + if (!names) { + return MapSECStatus(SECFailure); + } + CERTGeneralName* currentName = const_cast(names); do { - if (CERT_CheckNameSpace(arena, constraints, currentName) != SECSuccess) { + if (CERT_CheckNameSpace(arena.get(), constraints, currentName) + != SECSuccess) { // XXX: It seems like CERT_CheckNameSpace doesn't always call // PR_SetError when it fails. We set the error code here, though this // may be papering over some fatal errors. NSS's diff --git a/security/pkix/lib/pkixcheck.h b/security/pkix/lib/pkixcheck.h index 0366742960df..4e94af7ef7c2 100644 --- a/security/pkix/lib/pkixcheck.h +++ b/security/pkix/lib/pkixcheck.h @@ -42,7 +42,7 @@ Result CheckIssuerIndependentProperties( unsigned int subCACount, /*optional out*/ TrustLevel* trustLevel = nullptr); -Result CheckNameConstraints(BackCert& cert); +Result CheckNameConstraints(const BackCert& cert); } } // namespace mozilla::pkix diff --git a/security/pkix/lib/pkixutil.h b/security/pkix/lib/pkixutil.h index d98695ec20db..82baf544ffdd 100644 --- a/security/pkix/lib/pkixutil.h +++ b/security/pkix/lib/pkixutil.h @@ -90,9 +90,9 @@ MapSECStatus(SECStatus srv) class BackCert { public: - // IncludeCN::No means that GetConstrainedNames won't include the subject CN - // in its results. IncludeCN::Yes means that GetConstrainedNames will include - // the subject CN in its results. + // IncludeCN::No means that name constraint enforcement should not consider + // the subject CN as a possible dNSName. IncludeCN::Yes means that name + // constraint enforcement will consider the subject CN as a possible dNSName. MOZILLA_PKIX_ENUM_CLASS IncludeCN { No = 0, Yes = 1 }; // nssCert and childCert must be valid for the lifetime of BackCert @@ -105,7 +105,6 @@ public: , encodedNameConstraints(nullptr) , encodedInhibitAnyPolicy(nullptr) , childCert(childCert) - , constrainedNames(nullptr) , includeCN(includeCN) { } @@ -135,6 +134,7 @@ public: const SECItem* encodedInhibitAnyPolicy; BackCert* const childCert; + const IncludeCN includeCN; // Only non-const so that we can pass this to TrustDomain::IsRevoked, // which only takes a non-const pointer because VerifyEncodedOCSPResponse @@ -143,21 +143,9 @@ public: // of that CERT_DupCertificate so that we can make all these things const. /*const*/ CERTCertificate* GetNSSCert() const { return nssCert.get(); } - // Returns the names that should be considered when evaluating name - // constraints. The list is constructed lazily and cached. The result is a - // weak reference; do not try to free it, and do not hold long-lived - // references to it. - Result GetConstrainedNames(/*out*/ const CERTGeneralName** result); - - PLArenaPool* GetArena(); - private: ScopedCERTCertificate nssCert; - ScopedPLArenaPool arena; - CERTGeneralName* constrainedNames; - IncludeCN includeCN; - BackCert(const BackCert&) /* = delete */; void operator=(const BackCert&); /* = delete */; };