Bug 1533640 - Attempt to parse empty OCSP responses and let the parse attempt signal malformedness, instead of letting an empty response's |Vector<uint8_t>::begin() == nullptr| be the trigger of that signal. r=keeler

Differential Revision: https://phabricator.services.mozilla.com/D22656 --HG-- extra : rebase_source : 47afff90c0a07330664b95fbdd7d5cc7e8b5bb4d
2019-03-07 15:28:00 -08:00 · 2019-03-07 15:28:00 -08:00 · 44f0e9ca5f
--- a/security/certverifier/NSSCertDBTrustDomain.cpp
+++ b/security/certverifier/NSSCertDBTrustDomain.cpp
@ -541,76 +541,57 @@ Result NSSCertDBTrustDomain::CheckRevocation(
    return Success;
  }

-  // Only request a response if we didn't have a cached indication of failure
-  // (don't keep requesting responses from a failing server).
-  bool attemptedRequest;
-  Vector<uint8_t> ocspResponse;
-  Input response;
  if (cachedResponseResult == Success ||
      cachedResponseResult == Result::ERROR_OCSP_UNKNOWN_CERT ||
      cachedResponseResult == Result::ERROR_OCSP_OLD_RESPONSE) {
-    uint8_t ocspRequestBytes[OCSP_REQUEST_MAX_LENGTH];
-    size_t ocspRequestLength;
-    rv = CreateEncodedOCSPRequest(*this, certID, ocspRequestBytes,
-                                  ocspRequestLength);
-    if (rv != Success) {
-      return rv;
-    }
-    Vector<uint8_t> ocspRequest;
-    if (!ocspRequest.append(ocspRequestBytes, ocspRequestLength)) {
-      return Result::FATAL_ERROR_NO_MEMORY;
-    }
-    Result tempRV =
-        DoOCSPRequest(aiaLocation, mOriginAttributes, std::move(ocspRequest),
-                      GetOCSPTimeout(), ocspResponse);
-    if (tempRV != Success) {
-      rv = tempRV;
-    } else if (response.Init(ocspResponse.begin(), ocspResponse.length()) !=
-               Success) {
-      rv = Result::ERROR_OCSP_MALFORMED_RESPONSE;  // too big
-    }
-    attemptedRequest = true;
-  } else {
-    rv = cachedResponseResult;
-    attemptedRequest = false;
+    // Only send a request to, and process a response from, the server if we
+    // didn't have a cached indication of failure.  Also, ddon't keep requesting
+    // responses from a failing server.
+    return SynchronousCheckRevocationWithServer(
+        certID, aiaLocation, time, maxOCSPLifetimeInDays, cachedResponseResult,
+        stapledOCSPResponseResult);
  }

-  if (response.GetLength() == 0) {
-    Result error = rv;
-    if (attemptedRequest) {
-      Time timeout(time);
-      if (timeout.AddSeconds(ServerFailureDelaySeconds) != Success) {
-        return Result::FATAL_ERROR_LIBRARY_FAILURE;  // integer overflow
-      }
-      rv = mOCSPCache.Put(certID, mOriginAttributes, error, time, timeout);
-      if (rv != Success) {
-        return rv;
-      }
-    }
-    if (mOCSPFetching != FetchOCSPForDVSoftFail) {
-      MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
-              ("NSSCertDBTrustDomain: returning SECFailure after "
-               "OCSP request failure"));
-      return error;
-    }
-    if (cachedResponseResult == Result::ERROR_OCSP_UNKNOWN_CERT) {
-      MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
-              ("NSSCertDBTrustDomain: returning SECFailure from cached "
-               "response after OCSP request failure"));
-      return cachedResponseResult;
-    }
-    if (stapledOCSPResponseResult != Success) {
-      MOZ_LOG(
-          gCertVerifierLog, LogLevel::Debug,
-          ("NSSCertDBTrustDomain: returning SECFailure from expired/invalid "
-           "stapled response after OCSP request failure"));
-      return stapledOCSPResponseResult;
+  return HandleOCSPFailure(cachedResponseResult, stapledOCSPResponseResult,
+                           cachedResponseResult);
+}
+
+Result NSSCertDBTrustDomain::SynchronousCheckRevocationWithServer(
+    const CertID& certID, const nsCString& aiaLocation, Time time,
+    uint16_t maxOCSPLifetimeInDays, const Result cachedResponseResult,
+    const Result stapledOCSPResponseResult) {
+  uint8_t ocspRequestBytes[OCSP_REQUEST_MAX_LENGTH];
+  size_t ocspRequestLength;
+
+  Result rv = CreateEncodedOCSPRequest(*this, certID, ocspRequestBytes,
+                                       ocspRequestLength);
+  if (rv != Success) {
+    return rv;
+  }
+
+  Vector<uint8_t> ocspResponse;
+  Input response;
+  rv = DoOCSPRequest(aiaLocation, mOriginAttributes, ocspRequestBytes,
+                     ocspRequestLength, GetOCSPTimeout(), ocspResponse);
+  if (rv == Success &&
+      response.Init(ocspResponse.begin(), ocspResponse.length()) != Success) {
+    rv = Result::ERROR_OCSP_MALFORMED_RESPONSE;  // too big
+  }
+
+  if (rv != Success) {
+    Time timeout(time);
+    if (timeout.AddSeconds(ServerFailureDelaySeconds) != Success) {
+      return Result::FATAL_ERROR_LIBRARY_FAILURE;  // integer overflow
    }

-    MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
-            ("NSSCertDBTrustDomain: returning SECSuccess after "
-             "OCSP request failure"));
-    return Success;  // Soft fail -> success :(
+    Result cacheRV =
+        mOCSPCache.Put(certID, mOriginAttributes, rv, time, timeout);
+    if (cacheRV != Success) {
+      return cacheRV;
+    }
+
+    return HandleOCSPFailure(cachedResponseResult, stapledOCSPResponseResult,
+                             rv);
  }

  // If the response from the network has expired but indicates a revoked
@ -621,9 +602,9 @@ Result NSSCertDBTrustDomain::CheckRevocation(
                                              maxOCSPLifetimeInDays, response,
                                              ResponseIsFromNetwork, expired);
  if (rv == Success || mOCSPFetching != FetchOCSPForDVSoftFail) {
-    MOZ_LOG(
-        gCertVerifierLog, LogLevel::Debug,
-        ("NSSCertDBTrustDomain: returning after VerifyEncodedOCSPResponse"));
+    MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
+            ("NSSCertDBTrustDomain: returning after "
+             "VerifyEncodedOCSPResponse"));
    return rv;
  }

@ -631,6 +612,7 @@ Result NSSCertDBTrustDomain::CheckRevocation(
      rv == Result::ERROR_REVOKED_CERTIFICATE) {
    return rv;
  }
+
  if (stapledOCSPResponseResult != Success) {
    MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
            ("NSSCertDBTrustDomain: returning SECFailure from expired/invalid "
@ -644,6 +626,36 @@ Result NSSCertDBTrustDomain::CheckRevocation(
  return Success;  // Soft fail -> success :(
 }

+Result NSSCertDBTrustDomain::HandleOCSPFailure(
+    const Result cachedResponseResult, const Result stapledOCSPResponseResult,
+    const Result error) {
+  if (mOCSPFetching != FetchOCSPForDVSoftFail) {
+    MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
+            ("NSSCertDBTrustDomain: returning SECFailure after OCSP request "
+             "failure"));
+    return error;
+  }
+
+  if (cachedResponseResult == Result::ERROR_OCSP_UNKNOWN_CERT) {
+    MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
+            ("NSSCertDBTrustDomain: returning SECFailure from cached response "
+             "after OCSP request failure"));
+    return cachedResponseResult;
+  }
+
+  if (stapledOCSPResponseResult != Success) {
+    MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
+            ("NSSCertDBTrustDomain: returning SECFailure from expired/invalid "
+             "stapled response after OCSP request failure"));
+    return stapledOCSPResponseResult;
+  }
+
+  MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
+          ("NSSCertDBTrustDomain: returning SECSuccess after OCSP request "
+           "failure"));
+  return Success;  // Soft fail -> success :(
+}
+
 Result NSSCertDBTrustDomain::VerifyAndMaybeCacheEncodedOCSPResponse(
    const CertID& certID, Time time, uint16_t maxLifetimeInDays,
    Input encodedResponse, EncodedResponseSource responseSource,
--- a/security/certverifier/NSSCertDBTrustDomain.h
+++ b/security/certverifier/NSSCertDBTrustDomain.h
@ -202,6 +202,15 @@ class NSSCertDBTrustDomain : public mozilla::pkix::TrustDomain {
      EncodedResponseSource responseSource, /*out*/ bool& expired);
  TimeDuration GetOCSPTimeout() const;

+  Result SynchronousCheckRevocationWithServer(
+      const mozilla::pkix::CertID& certID, const nsCString& aiaLocation,
+      mozilla::pkix::Time time, uint16_t maxOCSPLifetimeInDays,
+      const Result cachedResponseResult,
+      const Result stapledOCSPResponseResult);
+  Result HandleOCSPFailure(const Result cachedResponseResult,
+                           const Result stapledOCSPResponseResult,
+                           const Result error);
+
  const SECTrustType mCertDBTrustType;
  const OCSPFetching mOCSPFetching;
  OCSPCache& mOCSPCache;  // non-owning!
--- a/security/manager/ssl/nsNSSCallbacks.cpp
+++ b/security/manager/ssl/nsNSSCallbacks.cpp
@ -14,6 +14,7 @@
 #include "mozilla/Assertions.h"
 #include "mozilla/Casting.h"
 #include "mozilla/RefPtr.h"
+#include "mozilla/Span.h"
 #include "mozilla/Telemetry.h"
 #include "mozilla/Unused.h"
 #include "nsContentUtils.h"
@ -65,9 +66,10 @@ const uint32_t KEA_NOT_SUPPORTED = 1;

 class OCSPRequest final : public nsIStreamLoaderObserver, public nsIRunnable {
 public:
-  OCSPRequest(const nsCString& aiaLocation,
+  OCSPRequest(const nsACString& aiaLocation,
              const OriginAttributes& originAttributes,
-              Vector<uint8_t>&& ocspRequest, TimeDuration timeout);
+              const uint8_t (&ocspRequest)[OCSP_REQUEST_MAX_LENGTH],
+              size_t ocspRequestLength, TimeDuration timeout);

  NS_DECL_THREADSAFE_ISUPPORTS
  NS_DECL_NSISTREAMLOADEROBSERVER
@ -104,7 +106,7 @@ class OCSPRequest final : public nsIStreamLoaderObserver, public nsIRunnable {
  nsCOMPtr<nsIStreamLoader> mLoader;
  const nsCString mAIALocation;
  const OriginAttributes mOriginAttributes;
-  const Vector<uint8_t> mPOSTData;
+  const mozilla::Span<const char> mPOSTData;
  const TimeDuration mTimeout;
  nsCOMPtr<nsITimer> mTimeoutTimer;
  TimeStamp mStartTime;
@ -114,20 +116,23 @@ class OCSPRequest final : public nsIStreamLoaderObserver, public nsIRunnable {

 NS_IMPL_ISUPPORTS(OCSPRequest, nsIStreamLoaderObserver, nsIRunnable)

-OCSPRequest::OCSPRequest(const nsCString& aiaLocation,
+OCSPRequest::OCSPRequest(const nsACString& aiaLocation,
                         const OriginAttributes& originAttributes,
-                         Vector<uint8_t>&& ocspRequest, TimeDuration timeout)
+                         const uint8_t (&ocspRequest)[OCSP_REQUEST_MAX_LENGTH],
+                         size_t ocspRequestLength, TimeDuration timeout)
    : mMonitor("OCSPRequest.mMonitor"),
      mNotifiedDone(false),
      mLoader(nullptr),
      mAIALocation(aiaLocation),
      mOriginAttributes(originAttributes),
-      mPOSTData(std::move(ocspRequest)),
+      mPOSTData(reinterpret_cast<const char*>(ocspRequest), ocspRequestLength),
      mTimeout(timeout),
      mTimeoutTimer(nullptr),
      mStartTime(),
      mResponseResult(NS_ERROR_FAILURE),
-      mResponseBytes() {}
+      mResponseBytes() {
+  MOZ_ASSERT(ocspRequestLength <= OCSP_REQUEST_MAX_LENGTH);
+}

 nsresult OCSPRequest::DispatchToMainThreadAndWait() {
  MOZ_ASSERT(!NS_IsMainThread());
@ -279,10 +284,7 @@ OCSPRequest::Run() {
  }

  nsCOMPtr<nsIInputStream> uploadStream;
-  rv = NS_NewByteInputStream(
-      getter_AddRefs(uploadStream),
-      MakeSpan(reinterpret_cast<const char*>(mPOSTData.begin()),
-               mPOSTData.length()));
+  rv = NS_NewByteInputStream(getter_AddRefs(uploadStream), mPOSTData);
  if (NS_FAILED(rv)) {
    return NotifyDone(rv, lock);
  }
@ -425,16 +427,19 @@ void OCSPRequest::OnTimeout(nsITimer* timer, void* closure) {
  self->NotifyDone(NS_ERROR_NET_TIMEOUT, lock);
 }

-mozilla::pkix::Result DoOCSPRequest(const nsCString& aiaLocation,
-                                    const OriginAttributes& originAttributes,
-                                    Vector<uint8_t>&& ocspRequest,
-                                    TimeDuration timeout,
-                                    /*out*/ Vector<uint8_t>& result) {
+mozilla::pkix::Result DoOCSPRequest(
+    const nsCString& aiaLocation, const OriginAttributes& originAttributes,
+    uint8_t (&ocspRequest)[OCSP_REQUEST_MAX_LENGTH], size_t ocspRequestLength,
+    TimeDuration timeout, /*out*/ Vector<uint8_t>& result) {
  MOZ_ASSERT(!NS_IsMainThread());
  if (NS_IsMainThread()) {
    return mozilla::pkix::Result::ERROR_OCSP_UNKNOWN_CERT;
  }

+  if (ocspRequestLength > OCSP_REQUEST_MAX_LENGTH) {
+    return mozilla::pkix::Result::FATAL_ERROR_LIBRARY_FAILURE;
+  }
+
  result.clear();
  MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
          ("DoOCSPRequest to '%s'", aiaLocation.get()));
@ -455,8 +460,8 @@ mozilla::pkix::Result DoOCSPRequest(const nsCString& aiaLocation,
    return mozilla::pkix::Result::FATAL_ERROR_INVALID_STATE;
  }

-  RefPtr<OCSPRequest> request(new OCSPRequest(aiaLocation, originAttributes,
-                                              std::move(ocspRequest), timeout));
+  RefPtr<OCSPRequest> request(new OCSPRequest(
+      aiaLocation, originAttributes, ocspRequest, ocspRequestLength, timeout));
  rv = request->DispatchToMainThreadAndWait();
  if (NS_FAILED(rv)) {
    return mozilla::pkix::Result::FATAL_ERROR_LIBRARY_FAILURE;
--- a/security/manager/ssl/nsNSSCallbacks.h
+++ b/security/manager/ssl/nsNSSCallbacks.h
@ -14,6 +14,7 @@
 #include "nspr.h"
 #include "nsString.h"
 #include "pk11func.h"
+#include "mozpkix/pkix.h"
 #include "mozpkix/pkixtypes.h"

 using mozilla::OriginAttributes;
@ -28,10 +29,10 @@ void HandshakeCallback(PRFileDesc* fd, void* client_data);
 SECStatus CanFalseStartCallback(PRFileDesc* fd, void* client_data,
                                PRBool* canFalseStart);

-mozilla::pkix::Result DoOCSPRequest(const nsCString& aiaLocation,
-                                    const OriginAttributes& originAttributes,
-                                    Vector<uint8_t>&& ocspRequest,
-                                    TimeDuration timeout,
-                                    /*out*/ Vector<uint8_t>& result);
+mozilla::pkix::Result DoOCSPRequest(
+    const nsCString& aiaLocation, const OriginAttributes& originAttributes,
+    uint8_t (&ocspRequest)[mozilla::pkix::OCSP_REQUEST_MAX_LENGTH],
+    size_t ocspRequestLength, TimeDuration timeout,
+    /*out*/ Vector<uint8_t>& result);

 #endif  // nsNSSCallbacks_h