From f2e92f7de537d982501d6bdf147e05643b7dce2a Mon Sep 17 00:00:00 2001 From: Mark Goodwin Date: Fri, 13 Nov 2015 16:49:08 +0000 Subject: [PATCH] Bug 901698 - Implement OCSP-must-staple; r=keeler --- security/certverifier/CertVerifier.cpp | 24 ++++++++ security/certverifier/CertVerifier.h | 2 + .../en-US/chrome/pipnss/nsserrors.properties | 1 + .../manager/ssl/SSLServerCertVerification.cpp | 34 ++++++----- security/manager/ssl/SharedSSLState.h | 6 ++ security/manager/ssl/nsNSSComponent.cpp | 6 ++ security/pkix/include/pkix/Result.h | 2 + security/pkix/include/pkix/pkix.h | 8 +++ security/pkix/include/pkix/pkixnss.h | 1 + security/pkix/lib/pkixbuild.cpp | 5 ++ security/pkix/lib/pkixcert.cpp | 6 ++ security/pkix/lib/pkixcheck.cpp | 59 +++++++++++++++++++ security/pkix/lib/pkixcheck.h | 4 ++ security/pkix/lib/pkixnss.cpp | 2 + security/pkix/lib/pkixutil.h | 5 ++ 15 files changed, 151 insertions(+), 14 deletions(-) diff --git a/security/certverifier/CertVerifier.cpp b/security/certverifier/CertVerifier.cpp index fda14b301b6..069af61145d 100644 --- a/security/certverifier/CertVerifier.cpp +++ b/security/certverifier/CertVerifier.cpp @@ -28,6 +28,7 @@ namespace mozilla { namespace psm { const CertVerifier::Flags CertVerifier::FLAG_LOCAL_ONLY = 1; const CertVerifier::Flags CertVerifier::FLAG_MUST_BE_EV = 2; +const CertVerifier::Flags CertVerifier::FLAG_TLS_IGNORE_STATUS_REQUEST = 4; CertVerifier::CertVerifier(OcspDownloadConfig odc, OcspStrictConfig osc, @@ -583,6 +584,29 @@ CertVerifier::VerifySSLServerCert(CERTCertificate* peerCert, PR_SetError(MapResultToPRErrorCode(result), 0); return SECFailure; } + + Input stapledOCSPResponseInput; + Input* responseInputPtr = nullptr; + if (stapledOCSPResponse) { + result = stapledOCSPResponseInput.Init(stapledOCSPResponse->data, + stapledOCSPResponse->len); + if (result != Success) { + // The stapled OCSP response was too big. + PR_SetError(SEC_ERROR_OCSP_MALFORMED_RESPONSE, 0); + return SECFailure; + } + responseInputPtr = &stapledOCSPResponseInput; + } + + if (!(flags & FLAG_TLS_IGNORE_STATUS_REQUEST)) { + result = CheckTLSFeaturesAreSatisfied(peerCertInput, responseInputPtr); + + if (result != Success) { + PR_SetError(MapResultToPRErrorCode(result), 0); + return SECFailure; + } + } + Input hostnameInput; result = hostnameInput.Init(uint8_t_ptr_cast(hostname), strlen(hostname)); if (result != Success) { diff --git a/security/certverifier/CertVerifier.h b/security/certverifier/CertVerifier.h index 4fd53b07ae5..98b2abc221f 100644 --- a/security/certverifier/CertVerifier.h +++ b/security/certverifier/CertVerifier.h @@ -54,6 +54,8 @@ public: static const Flags FLAG_LOCAL_ONLY; // Don't perform fallback DV validation on EV validation failure. static const Flags FLAG_MUST_BE_EV; + // TLS feature request_status should be ignored + static const Flags FLAG_TLS_IGNORE_STATUS_REQUEST; // These values correspond to the SSL_OCSP_STAPLING telemetry. enum OCSPStaplingStatus { diff --git a/security/manager/locales/en-US/chrome/pipnss/nsserrors.properties b/security/manager/locales/en-US/chrome/pipnss/nsserrors.properties index 25655715522..e771b1586be 100644 --- a/security/manager/locales/en-US/chrome/pipnss/nsserrors.properties +++ b/security/manager/locales/en-US/chrome/pipnss/nsserrors.properties @@ -319,3 +319,4 @@ MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE=A certificate that is not ye MOZILLA_PKIX_ERROR_SIGNATURE_ALGORITHM_MISMATCH=The signature algorithm in the signature field of the certificate does not match the algorithm in its signatureAlgorithm field. MOZILLA_PKIX_ERROR_OCSP_RESPONSE_FOR_CERT_MISSING=The OCSP response does not include a status for the certificate being verified. MOZILLA_PKIX_ERROR_VALIDITY_TOO_LONG=The server presented a certificate that is valid for too long. +MOZILLA_PKIX_ERROR_REQUIRED_TLS_FEATURE_MISSING=A required TLS feature is missing. diff --git a/security/manager/ssl/SSLServerCertVerification.cpp b/security/manager/ssl/SSLServerCertVerification.cpp index c70600fbcad..b54e019c00f 100644 --- a/security/manager/ssl/SSLServerCertVerification.cpp +++ b/security/manager/ssl/SSLServerCertVerification.cpp @@ -214,7 +214,7 @@ void StopSSLServerCertVerificationThreads() namespace { void -LogInvalidCertError(TransportSecurityInfo* socketInfo, +LogInvalidCertError(nsNSSSocketInfo* socketInfo, PRErrorCode errorCode, ::mozilla::psm::SSLErrorMessageType errorMessageType) { @@ -236,7 +236,7 @@ class SSLServerCertVerificationResult : public nsRunnable public: NS_DECL_NSIRUNNABLE - SSLServerCertVerificationResult(TransportSecurityInfo* infoObject, + SSLServerCertVerificationResult(nsNSSSocketInfo* infoObject, PRErrorCode errorCode, Telemetry::ID telemetryID = Telemetry::HistogramCount, uint32_t telemetryValue = -1, @@ -245,7 +245,7 @@ public: void Dispatch(); private: - const RefPtr mInfoObject; + const RefPtr mInfoObject; public: const PRErrorCode mErrorCode; const SSLErrorMessageType mErrorMessageType; @@ -258,7 +258,7 @@ class CertErrorRunnable : public SyncRunnableBase public: CertErrorRunnable(const void* fdForLogging, nsIX509Cert* cert, - TransportSecurityInfo* infoObject, + nsNSSSocketInfo* infoObject, PRErrorCode defaultErrorCodeToReport, uint32_t collectedErrors, PRErrorCode errorCodeTrust, @@ -282,7 +282,7 @@ private: const void* const mFdForLogging; // may become an invalid pointer; do not dereference const nsCOMPtr mCert; - const RefPtr mInfoObject; + const RefPtr mInfoObject; const PRErrorCode mDefaultErrorCodeToReport; const uint32_t mCollectedErrors; const PRErrorCode mErrorCodeTrust; @@ -622,7 +622,7 @@ CertErrorRunnable::RunOnTargetThread() CertErrorRunnable* CreateCertErrorRunnable(CertVerifier& certVerifier, PRErrorCode defaultErrorCodeToReport, - TransportSecurityInfo* infoObject, + nsNSSSocketInfo* infoObject, CERTCertificate* cert, const void* fdForLogging, uint32_t providerFlags, @@ -715,7 +715,7 @@ public: // Must be called only on the socket transport thread static SECStatus Dispatch(const RefPtr& certVerifier, const void* fdForLogging, - TransportSecurityInfo* infoObject, + nsNSSSocketInfo* infoObject, CERTCertificate* serverCert, ScopedCERTCertList& peerCertChain, SECItem* stapledOCSPResponse, @@ -728,7 +728,7 @@ private: // Must be called only on the socket transport thread SSLServerCertVerificationJob(const RefPtr& certVerifier, const void* fdForLogging, - TransportSecurityInfo* infoObject, + nsNSSSocketInfo* infoObject, CERTCertificate* cert, CERTCertList* peerCertChain, SECItem* stapledOCSPResponse, @@ -737,7 +737,7 @@ private: PRTime prtime); const RefPtr mCertVerifier; const void* const mFdForLogging; - const RefPtr mInfoObject; + const RefPtr mInfoObject; const ScopedCERTCertificate mCert; ScopedCERTCertList mPeerCertChain; const uint32_t mProviderFlags; @@ -749,7 +749,7 @@ private: SSLServerCertVerificationJob::SSLServerCertVerificationJob( const RefPtr& certVerifier, const void* fdForLogging, - TransportSecurityInfo* infoObject, CERTCertificate* cert, + nsNSSSocketInfo* infoObject, CERTCertificate* cert, CERTCertList* peerCertChain, SECItem* stapledOCSPResponse, uint32_t providerFlags, Time time, PRTime prtime) : mCertVerifier(certVerifier) @@ -1200,7 +1200,7 @@ GatherSuccessfulValidationTelemetry(const ScopedCERTCertList& certList) SECStatus AuthCertificate(CertVerifier& certVerifier, - TransportSecurityInfo* infoObject, + nsNSSSocketInfo* infoObject, CERTCertificate* cert, ScopedCERTCertList& peerCertChain, SECItem* stapledOCSPResponse, @@ -1225,10 +1225,16 @@ AuthCertificate(CertVerifier& certVerifier, SignatureDigestStatus sigDigestStatus = SignatureDigestStatus::NeverChecked; PinningTelemetryInfo pinningTelemetryInfo; + int flags = 0; + if (!infoObject->SharedState().IsOCSPStaplingEnabled() || + !infoObject->SharedState().IsOCSPMustStapleEnabled()) { + flags |= CertVerifier::FLAG_TLS_IGNORE_STATUS_REQUEST; + } + rv = certVerifier.VerifySSLServerCert(cert, stapledOCSPResponse, time, infoObject, infoObject->GetHostNameRaw(), - saveIntermediates, 0, &certList, + saveIntermediates, flags, &certList, &evOidPolicy, &ocspStaplingStatus, &keySizeStatus, &sigDigestStatus, &pinningTelemetryInfo); @@ -1327,7 +1333,7 @@ AuthCertificate(CertVerifier& certVerifier, SSLServerCertVerificationJob::Dispatch( const RefPtr& certVerifier, const void* fdForLogging, - TransportSecurityInfo* infoObject, + nsNSSSocketInfo* infoObject, CERTCertificate* serverCert, ScopedCERTCertList& peerCertChain, SECItem* stapledOCSPResponse, @@ -1662,7 +1668,7 @@ void EnsureServerVerificationInitialized() } SSLServerCertVerificationResult::SSLServerCertVerificationResult( - TransportSecurityInfo* infoObject, PRErrorCode errorCode, + nsNSSSocketInfo* infoObject, PRErrorCode errorCode, Telemetry::ID telemetryID, uint32_t telemetryValue, SSLErrorMessageType errorMessageType) : mInfoObject(infoObject) diff --git a/security/manager/ssl/SharedSSLState.h b/security/manager/ssl/SharedSSLState.h index 396f5fef698..d134a189e2f 100644 --- a/security/manager/ssl/SharedSSLState.h +++ b/security/manager/ssl/SharedSSLState.h @@ -39,12 +39,17 @@ public: { mOCSPStaplingEnabled = staplingEnabled; } + void SetOCSPMustStapleEnabled(bool mustStapleEnabled) + { + mOCSPMustStapleEnabled = mustStapleEnabled; + } // The following methods may be called from any thread bool SocketCreated(); void NoteSocketCreated(); static void NoteCertOverrideServiceInstantiated(); bool IsOCSPStaplingEnabled() const { return mOCSPStaplingEnabled; } + bool IsOCSPMustStapleEnabled() const { return mOCSPMustStapleEnabled; } private: ~SharedSSLState(); @@ -61,6 +66,7 @@ private: Mutex mMutex; bool mSocketCreated; bool mOCSPStaplingEnabled; + bool mOCSPMustStapleEnabled; }; SharedSSLState* PublicSSLState(); diff --git a/security/manager/ssl/nsNSSComponent.cpp b/security/manager/ssl/nsNSSComponent.cpp index bdb0454f9c2..5e058255f20 100644 --- a/security/manager/ssl/nsNSSComponent.cpp +++ b/security/manager/ssl/nsNSSComponent.cpp @@ -847,6 +847,11 @@ void nsNSSComponent::setValidationOptions(bool isInitialSetting, PublicSSLState()->SetOCSPStaplingEnabled(ocspStaplingEnabled); PrivateSSLState()->SetOCSPStaplingEnabled(ocspStaplingEnabled); + bool ocspMustStapleEnabled = Preferences::GetBool("security.ssl.enable_ocsp_must_staple", + false); + PublicSSLState()->SetOCSPMustStapleEnabled(ocspMustStapleEnabled); + PrivateSSLState()->SetOCSPMustStapleEnabled(ocspMustStapleEnabled); + CertVerifier::PinningMode pinningMode = static_cast (Preferences::GetInt("security.cert_pinning.enforcement_level", @@ -1332,6 +1337,7 @@ nsNSSComponent::Observe(nsISupports* aSubject, const char* aTopic, prefName.EqualsLiteral("security.OCSP.GET.enabled") || prefName.EqualsLiteral("security.pki.cert_short_lifetime_in_days") || prefName.EqualsLiteral("security.ssl.enable_ocsp_stapling") || + prefName.EqualsLiteral("security.ssl.enable_ocsp_must_staple") || prefName.EqualsLiteral("security.cert_pinning.enforcement_level") || prefName.EqualsLiteral("security.pki.sha1_enforcement_level")) { MutexAutoLock lock(mutex); diff --git a/security/pkix/include/pkix/Result.h b/security/pkix/include/pkix/Result.h index 091aeaf4e1f..9f90a5a48bb 100644 --- a/security/pkix/include/pkix/Result.h +++ b/security/pkix/include/pkix/Result.h @@ -185,6 +185,8 @@ static const unsigned int FATAL_ERROR_FLAG = 0x800; MOZILLA_PKIX_ERROR_OCSP_RESPONSE_FOR_CERT_MISSING) \ MOZILLA_PKIX_MAP(ERROR_VALIDITY_TOO_LONG, 50, \ MOZILLA_PKIX_ERROR_VALIDITY_TOO_LONG) \ + MOZILLA_PKIX_MAP(ERROR_REQUIRED_TLS_FEATURE_MISSING, 51, \ + MOZILLA_PKIX_ERROR_REQUIRED_TLS_FEATURE_MISSING) \ MOZILLA_PKIX_MAP(FATAL_ERROR_INVALID_ARGS, FATAL_ERROR_FLAG | 1, \ SEC_ERROR_INVALID_ARGS) \ MOZILLA_PKIX_MAP(FATAL_ERROR_INVALID_STATE, FATAL_ERROR_FLAG | 2, \ diff --git a/security/pkix/include/pkix/pkix.h b/security/pkix/include/pkix/pkix.h index 72fedb94c8b..ee6746b0fca 100644 --- a/security/pkix/include/pkix/pkix.h +++ b/security/pkix/include/pkix/pkix.h @@ -147,6 +147,14 @@ Result VerifyEncodedOCSPResponse(TrustDomain& trustDomain, /* optional out */ Time* thisUpdate = nullptr, /* optional out */ Time* validThrough = nullptr); +// Check that the TLSFeature extensions in a given end-entity cert (which is +// assumed to have been already validated with BuildCertChain) are satisfied. +// The only feature which we cancurrently process a requirement for is +// status_request (OCSP stapling) so we reject any extension that specifies a +// requirement for another value. Empty extensions are also rejected. +Result CheckTLSFeaturesAreSatisfied(Input& cert, + const Input* stapledOCSPResponse); + } } // namespace mozilla::pkix #endif // mozilla_pkix_pkix_h diff --git a/security/pkix/include/pkix/pkixnss.h b/security/pkix/include/pkix/pkixnss.h index 8593d7a69b0..414557feb76 100644 --- a/security/pkix/include/pkix/pkixnss.h +++ b/security/pkix/include/pkix/pkixnss.h @@ -84,6 +84,7 @@ enum ErrorCode MOZILLA_PKIX_ERROR_SIGNATURE_ALGORITHM_MISMATCH = ERROR_BASE + 7, MOZILLA_PKIX_ERROR_OCSP_RESPONSE_FOR_CERT_MISSING = ERROR_BASE + 8, MOZILLA_PKIX_ERROR_VALIDITY_TOO_LONG = ERROR_BASE + 9, + MOZILLA_PKIX_ERROR_REQUIRED_TLS_FEATURE_MISSING = ERROR_BASE + 10, }; void RegisterErrorTable(); diff --git a/security/pkix/lib/pkixbuild.cpp b/security/pkix/lib/pkixbuild.cpp index cfaac2335d9..99942cfa13b 100644 --- a/security/pkix/lib/pkixbuild.cpp +++ b/security/pkix/lib/pkixbuild.cpp @@ -188,6 +188,11 @@ PathBuildingStep::Check(Input potentialIssuerDER, } } + rv = CheckTLSFeatures(subject, potentialIssuer); + if (rv != Success) { + return RecordResult(rv, keepGoing); + } + // RFC 5280, Section 4.2.1.3: "If the keyUsage extension is present, then the // subject public key MUST NOT be used to verify signatures on certificates // or CRLs unless the corresponding keyCertSign or cRLSign bit is set." diff --git a/security/pkix/lib/pkixcert.cpp b/security/pkix/lib/pkixcert.cpp index ed6158385c9..1cb452c208e 100644 --- a/security/pkix/lib/pkixcert.cpp +++ b/security/pkix/lib/pkixcert.cpp @@ -219,6 +219,10 @@ BackCert::RememberExtension(Reader& extnID, Input extnValue, static const uint8_t Netscape_certificate_type[] = { 0x60, 0x86, 0x48, 0x01, 0x86, 0xf8, 0x42, 0x01, 0x01 }; + // python DottedOIDToCode.py id-pe-tlsfeature 1.3.6.1.5.5.7.1.24 + static const uint8_t id_pe_tlsfeature[] = { + 0x2b, 0x06, 0x01, 0x05, 0x05, 0x07, 0x01, 0x18 + }; Input* out = nullptr; @@ -263,6 +267,8 @@ BackCert::RememberExtension(Reader& extnID, Input extnValue, out = &inhibitAnyPolicy; } else if (extnID.MatchRest(id_pe_authorityInfoAccess)) { out = &authorityInfoAccess; + } else if (extnID.MatchRest(id_pe_tlsfeature)) { + out = &requiredTLSFeatures; } else if (extnID.MatchRest(id_pkix_ocsp_nocheck) && critical) { // We need to make sure we don't reject delegated OCSP response signing // certificates that contain the id-pkix-ocsp-nocheck extension marked as diff --git a/security/pkix/lib/pkixcheck.cpp b/security/pkix/lib/pkixcheck.cpp index 79d001a911d..3e0f0a6d1cc 100644 --- a/security/pkix/lib/pkixcheck.cpp +++ b/security/pkix/lib/pkixcheck.cpp @@ -830,6 +830,65 @@ CheckExtendedKeyUsage(EndEntityOrCA endEntityOrCA, return Success; } +Result +CheckTLSFeatures(const BackCert& subject, BackCert& potentialIssuer) +{ + const Input* issuerTLSFeatures = potentialIssuer.GetRequiredTLSFeatures(); + if (!issuerTLSFeatures) { + return Success; + } + + const Input* subjectTLSFeatures = subject.GetRequiredTLSFeatures(); + if (issuerTLSFeatures->GetLength() == 0 || + !subjectTLSFeatures || + !InputsAreEqual(*issuerTLSFeatures, *subjectTLSFeatures)) { + return Result::ERROR_REQUIRED_TLS_FEATURE_MISSING; + } + + return Success; +} + +Result +TLSFeaturesSatisfiedInternal(const Input* requiredTLSFeatures, + const Input* stapledOCSPResponse) +{ + if (!requiredTLSFeatures) { + return Success; + } + + // RFC 6066 10.2: ExtensionType status_request + const static uint8_t status_request = 5; + const static uint8_t status_request_bytes[] = { status_request }; + + Reader input(*requiredTLSFeatures); + return der::NestedOf(input, der::SEQUENCE, der::INTEGER, + der::EmptyAllowed::No, [&](Reader& r) { + if (!r.MatchRest(status_request_bytes)) { + return Result::ERROR_REQUIRED_TLS_FEATURE_MISSING; + } + + if (!stapledOCSPResponse) { + return Result::ERROR_REQUIRED_TLS_FEATURE_MISSING; + } + + return Result::Success; + }); +} + +Result +CheckTLSFeaturesAreSatisfied(Input& cert, + const Input* stapledOCSPResponse) +{ + BackCert backCert(cert, EndEntityOrCA::MustBeEndEntity, nullptr); + Result rv = backCert.Init(); + if (rv != Success) { + return rv; + } + + return TLSFeaturesSatisfiedInternal(backCert.GetRequiredTLSFeatures(), + stapledOCSPResponse); +} + Result CheckIssuerIndependentProperties(TrustDomain& trustDomain, const BackCert& cert, diff --git a/security/pkix/lib/pkixcheck.h b/security/pkix/lib/pkixcheck.h index 72b463d2795..c5a31c8ea6e 100644 --- a/security/pkix/lib/pkixcheck.h +++ b/security/pkix/lib/pkixcheck.h @@ -55,6 +55,10 @@ Result ParseValidity(Input encodedValidity, /*optional out*/ Time* notAfterOut = nullptr); Result CheckValidity(Time time, Time notBefore, Time notAfter); +// Check that a subject has TLS Feature (rfc7633) requirements that match its +// potential issuer +Result CheckTLSFeatures(const BackCert& subject, BackCert& potentialIssuer); + } } // namespace mozilla::pkix #endif // mozilla_pkix_pkixcheck_h diff --git a/security/pkix/lib/pkixnss.cpp b/security/pkix/lib/pkixnss.cpp index 19d69323c76..c1228624873 100644 --- a/security/pkix/lib/pkixnss.cpp +++ b/security/pkix/lib/pkixnss.cpp @@ -202,6 +202,8 @@ RegisterErrorTable() "verified." }, { "MOZILLA_PKIX_ERROR_VALIDITY_TOO_LONG", "The server presented a certificate that is valid for too long." }, + { "MOZILLA_PKIX_ERROR_REQUIRED_TLS_FEATURE_MISSING", + "A required TLS feature is missing." }, }; // Note that these error strings are not localizable. // When these strings change, update the localization information too. diff --git a/security/pkix/lib/pkixutil.h b/security/pkix/lib/pkixutil.h index 9179cedc2df..996ffe9b8a2 100644 --- a/security/pkix/lib/pkixutil.h +++ b/security/pkix/lib/pkixutil.h @@ -102,6 +102,10 @@ public: { return MaybeInput(subjectAltName); } + const Input* GetRequiredTLSFeatures() const + { + return MaybeInput(requiredTLSFeatures); + } private: const Input der; @@ -144,6 +148,7 @@ private: Input nameConstraints; Input subjectAltName; Input criticalNetscapeCertificateType; + Input requiredTLSFeatures; Result RememberExtension(Reader& extnID, Input extnValue, bool critical, /*out*/ bool& understood);