[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-commits] [tor-browser] 08/08: fixup! Bug 23247: Communicating security expectations for .onion



This is an automated email from the git hooks/post-receive script.

richard pushed a commit to branch tor-browser-91.13.0esr-11.5-1
in repository tor-browser.

commit a4c279e9f8e5e1ce82c32d4973622b1911f83a2a
Author: Dan Ballard <dan@xxxxxxxxxxxx>
AuthorDate: Tue Aug 23 15:36:44 2022 +0000

    fixup! Bug 23247: Communicating security expectations for .onion
    
    Bug 41075: The Tor Browser is showing caution sign but your document said it won't
---
 browser/base/content/browser-siteIdentity.js       |  6 ++++--
 security/certverifier/CertVerifier.cpp             | 22 ++++++++++++++++++----
 security/manager/ssl/SSLServerCertVerification.cpp | 15 +++++++++++++--
 security/manager/ssl/nsNSSIOLayer.cpp              | 13 ++++++++++---
 security/nss/lib/mozpkix/include/pkix/Result.h     |  2 ++
 security/nss/lib/mozpkix/include/pkix/pkixnss.h    |  1 +
 6 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/browser/base/content/browser-siteIdentity.js b/browser/base/content/browser-siteIdentity.js
index b7d59db3dd34d..e45b65ddac158 100644
--- a/browser/base/content/browser-siteIdentity.js
+++ b/browser/base/content/browser-siteIdentity.js
@@ -767,8 +767,10 @@ var gIdentityHandler = {
     issuerCert = this._secInfo.succeededCertChain[
       this._secInfo.succeededCertChain.length - 1
     ];
-
-    return !issuerCert.isBuiltInRoot;
+    if (issuerCert) {
+      return !issuerCert.isBuiltInRoot;
+    }
+    return false;
   },
 
   /**
diff --git a/security/certverifier/CertVerifier.cpp b/security/certverifier/CertVerifier.cpp
index c427539bd67ea..e513eddb31e0c 100644
--- a/security/certverifier/CertVerifier.cpp
+++ b/security/certverifier/CertVerifier.cpp
@@ -918,6 +918,8 @@ Result CertVerifier::VerifySSLServerCert(
     return Result::ERROR_BAD_CERT_DOMAIN;
   }
 
+  bool errOnionWithSelfSignedCert = false;
+
   // CreateCertErrorRunnable assumes that CheckCertHostname is only called
   // if VerifyCert succeeded.
   Result rv =
@@ -931,9 +933,16 @@ Result CertVerifier::VerifySSLServerCert(
         CertIsSelfSigned(peerCert, pinarg)) {
       // In this case we didn't find any issuer for the certificate and the
       // certificate is self-signed.
-      return Result::ERROR_SELF_SIGNED_CERT;
+      if (StringEndsWith(hostname, ".onion"_ns)) {
+        // Self signed cert over onion is deemed secure, the hidden service provides authentication.
+        // We defer returning this error and keep processing to determine if there are other legitimate
+        // certificate errors (such as expired, wrong domain) that we would like to surface to the user
+        errOnionWithSelfSignedCert = true;
+      } else {
+        return Result::ERROR_SELF_SIGNED_CERT;
+      }
     }
-    if (rv == Result::ERROR_UNKNOWN_ISSUER) {
+    if (rv == Result::ERROR_UNKNOWN_ISSUER && !errOnionWithSelfSignedCert) {
       // In this case we didn't get any valid path for the cert. Let's see if
       // the issuer is the same as the issuer for our canary probe. If yes, this
       // connection is connecting via a misconfigured proxy.
@@ -951,7 +960,9 @@ Result CertVerifier::VerifySSLServerCert(
         return Result::ERROR_MITM_DETECTED;
       }
     }
-    return rv;
+    if (!errOnionWithSelfSignedCert) {
+      return rv;
+    }
   }
 
   if (dcInfo) {
@@ -995,7 +1006,7 @@ Result CertVerifier::VerifySSLServerCert(
   }
   bool isBuiltInRoot;
   rv = IsCertChainRootBuiltInRoot(builtChain, isBuiltInRoot);
-  if (rv != Success) {
+  if (rv != Success && !errOnionWithSelfSignedCert) {
     return rv;
   }
 
@@ -1016,6 +1027,9 @@ Result CertVerifier::VerifySSLServerCert(
     return rv;
   }
 
+  if (errOnionWithSelfSignedCert) {
+    return Result::ERROR_ONION_WITH_SELF_SIGNED_CERT;
+  }
   return Success;
 }
 
diff --git a/security/manager/ssl/SSLServerCertVerification.cpp b/security/manager/ssl/SSLServerCertVerification.cpp
index 0a84aecc6c724..a0c14be276dd6 100644
--- a/security/manager/ssl/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/SSLServerCertVerification.cpp
@@ -299,6 +299,7 @@ SECStatus DetermineCertOverrideErrors(const UniqueCERTCertificate& cert,
     case mozilla::pkix::MOZILLA_PKIX_ERROR_MITM_DETECTED:
     case mozilla::pkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE:
     case mozilla::pkix::MOZILLA_PKIX_ERROR_SELF_SIGNED_CERT:
+    case mozilla::pkix::MOZILLA_PKIX_ERROR_ONION_WITH_SELF_SIGNED_CERT:
     case mozilla::pkix::MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA: {
       collectedErrors = nsICertOverrideService::ERROR_UNTRUSTED;
       errorCodeTrust = defaultErrorCodeToReport;
@@ -984,6 +985,17 @@ PRErrorCode AuthCertificateParseResults(
       gPIPNSSLog, LogLevel::Debug,
       ("[0x%" PRIx64 "] Certificate error was not overridden\n", aPtrForLog));
 
+  // If Onion with self signed cert we want to prioritize any other error
+  if (errorCodeTrust == MOZILLA_PKIX_ERROR_ONION_WITH_SELF_SIGNED_CERT) {
+    if (errorCodeMismatch) {
+      return errorCodeMismatch;
+    } else if (errorCodeTime) {
+      return  errorCodeTime;
+    } else {
+      return MOZILLA_PKIX_ERROR_ONION_WITH_SELF_SIGNED_CERT;
+    }
+  }
+
   // pick the error code to report by priority
   return errorCodeTrust      ? errorCodeTrust
          : errorCodeMismatch ? errorCodeMismatch
@@ -1389,8 +1401,7 @@ SSLServerCertVerificationResult::Run() {
                             std::move(mPeerCertChain),
                             mCertificateTransparencyStatus, mEVStatus,
                             mSucceeded, mIsBuiltCertChainRootBuiltInRoot);
-
-  if (!mSucceeded && mCollectedErrors != 0) {
+  if (!mSucceeded && mCollectedErrors != 0 && mFinalError != MOZILLA_PKIX_ERROR_ONION_WITH_SELF_SIGNED_CERT) {
     mInfoObject->SetStatusErrorBits(mCert, mCollectedErrors);
   }
   mInfoObject->SetCertVerificationResult(mFinalError);
diff --git a/security/manager/ssl/nsNSSIOLayer.cpp b/security/manager/ssl/nsNSSIOLayer.cpp
index 21687447072d4..10d74b9eb3eb4 100644
--- a/security/manager/ssl/nsNSSIOLayer.cpp
+++ b/security/manager/ssl/nsNSSIOLayer.cpp
@@ -411,7 +411,11 @@ void nsNSSSocketInfo::SetCertVerificationResult(PRErrorCode errorCode) {
              "Invalid state transition to cert_verification_finished");
 
   if (mFd) {
-    SECStatus rv = SSL_AuthCertificateComplete(mFd, errorCode);
+    PRErrorCode passCode = errorCode;
+    if (errorCode == MOZILLA_PKIX_ERROR_ONION_WITH_SELF_SIGNED_CERT) {
+      passCode = 0;
+    }
+    SECStatus rv = SSL_AuthCertificateComplete(mFd, passCode);
     // Only replace errorCode if there was originally no error
     if (rv != SECSuccess && errorCode == 0) {
       errorCode = PR_GetError();
@@ -422,12 +426,15 @@ void nsNSSSocketInfo::SetCertVerificationResult(PRErrorCode errorCode) {
     }
   }
 
-  if (errorCode) {
+  if (errorCode &&
+      errorCode != MOZILLA_PKIX_ERROR_ONION_WITH_SELF_SIGNED_CERT) {
     mFailedVerification = true;
     SetCanceled(errorCode);
   }
 
-  if (mPlaintextBytesRead && !errorCode) {
+  if (mPlaintextBytesRead &&
+      (!errorCode ||
+       errorCode == MOZILLA_PKIX_ERROR_ONION_WITH_SELF_SIGNED_CERT)) {
     Telemetry::Accumulate(Telemetry::SSL_BYTES_BEFORE_CERT_CALLBACK,
                           AssertedCast<uint32_t>(mPlaintextBytesRead));
   }
diff --git a/security/nss/lib/mozpkix/include/pkix/Result.h b/security/nss/lib/mozpkix/include/pkix/Result.h
index 29461dc1a510b..b2ad3a383ceb3 100644
--- a/security/nss/lib/mozpkix/include/pkix/Result.h
+++ b/security/nss/lib/mozpkix/include/pkix/Result.h
@@ -188,6 +188,8 @@ static const unsigned int FATAL_ERROR_FLAG = 0x800;
                    SEC_ERROR_LIBRARY_FAILURE)                                  \
   MOZILLA_PKIX_MAP(FATAL_ERROR_NO_MEMORY, FATAL_ERROR_FLAG | 4,                \
                    SEC_ERROR_NO_MEMORY)                                        \
+  MOZILLA_PKIX_MAP(ERROR_ONION_WITH_SELF_SIGNED_CERT, 155,                     \
+                   MOZILLA_PKIX_ERROR_ONION_WITH_SELF_SIGNED_CERT)             \
 /* nothing here */
 
 enum class Result {
diff --git a/security/nss/lib/mozpkix/include/pkix/pkixnss.h b/security/nss/lib/mozpkix/include/pkix/pkixnss.h
index b181ca541e01c..16513a5dfb0b1 100644
--- a/security/nss/lib/mozpkix/include/pkix/pkixnss.h
+++ b/security/nss/lib/mozpkix/include/pkix/pkixnss.h
@@ -88,6 +88,7 @@ enum ErrorCode {
   MOZILLA_PKIX_ERROR_ADDITIONAL_POLICY_CONSTRAINT_FAILED = ERROR_BASE + 13,
   MOZILLA_PKIX_ERROR_SELF_SIGNED_CERT = ERROR_BASE + 14,
   MOZILLA_PKIX_ERROR_MITM_DETECTED = ERROR_BASE + 15,
+  MOZILLA_PKIX_ERROR_ONION_WITH_SELF_SIGNED_CERT = ERROR_BASE + 100,
   END_OF_LIST
 };
 

-- 
To stop receiving notification emails like this one, please contact
the administrator of this repository.
_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits