Keep returning ERROR_ONION_WITH_SELF_SIGNED_CERT only for .onion sites whose cert throws ERROR_UNKNOWN_ISSUER

assigned to @pierov

changed title from Keep return ERROR_ONION_WITH_SELF_SIGNED_CERT only for .onion sites whose cert throws ERROR_UNKNOWN_ISSUER to Keep returning ERROR_ONION_WITH_SELF_SIGNED_CERT only for .onion sites whose cert throws ERROR_UNKNOWN_ISSUER

Quickly jumping here... sorry in advance if this comment does not make any sense :)

If I understood this problem statement, the issue happens when Firefox user initially trusts a self-signed certificate A for site example.com but is presented with a self-signed certificate B for the same site, then an overridable error should be thrown, so the user is informed and can decide. This could prevent some certificate impersonation attacks for self-signed sites. Is this understanding correct?

If that's the case, it seems (in a first take of the matter) that for onionsites this behavior would not protect against impersonation at the TLS level, so yeah, this would not improve things for .onion.

In the order hand, it seems harmless for onionsites, the only drawback being the inability to use many different self-signed certificates for the same service. And it looks like an improvement for regular sites.

I wonder whether keeping this change would break anything for onionsites (except for those relying on multiple self-signed certs). I haven't tested it myself to check unwanted cases where ERROR_ONION_WITH_SELF_SIGNED_CERT is returned.

Wait, maybe we want to be more strict with onion services, if I understand correctly:

if the issuer/subject field of a self-signed cert C matches the subject of a cert R in the trust store (and R's public key is different from C's), then our path validation logic will treat C as having an invalid signature from R. This is a non-overridable error.

Is the subject of C something like gitlab.torproject.org, or is it something like Let's Encrypt R3?

If it is a CA, then we might want a non-overridable error here, because someone is definitely abusing our system of allowing any self-signed certificate.

I haven't tested it myself to check unwanted cases where ERROR_ONION_WITH_SELF_SIGNED_CERT is returned.

This is an error we created to distinguish the case from other self-signed certs, and eventually it isn't handled as an error.

if the issuer/subject field of a self-signed cert C matches the subject of a cert R in the trust store (and R's public key is different from C's), then our path validation logic will treat C as having an invalid signature from R. This is a non-overridable error.

Is the subject of C something like gitlab.torproject.org, or is it something like Let's Encrypt R3?

The original statement (well, the whole bug / patch narrative) is quite confusing: it says "issuer/subject field" (singular), and AFAIK the issuer is Let's Encrypt R3 while the subject is gitlab.torproject.org (two separate fields).

OTOH, how do they tell it's self signed if the issuer is in the trust store?

OTOH, how do they tell it's self signed if the issuer is in the trust store?

Answering to myself: they very likely mean "a certificate with the same issuer and subject" (like an explicitly accepted self-signed cert) is in the cert store, not that the issuer is trusted.

Back to this bug, I do think we should keep everything as it was for onion sites, i.e.:

diff --git a/security/certverifier/CertVerifier.cpp b/security/certverifier/CertVerifier.cpp
index a3f832c58b11b..4f81a6ea17c09 100644
--- a/security/certverifier/CertVerifier.cpp
+++ b/security/certverifier/CertVerifier.cpp
@@ -870,7 +870,7 @@ Result CertVerifier::VerifySSLServerCert(
         // 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;
+        errOnionWithSelfSignedCert = rv == Result::ERROR_UNKNOWN_ISSUER;
       } else {
         return Result::ERROR_SELF_SIGNED_CERT;
       }

The original statement (well, the whole bug / patch narrative) is quite confusing: it says "issuer/subject field" (singular), and AFAIK the issuer is Let's Encrypt R3 while the subject is gitlab.torproject.org (two separate fields).

It depends on the certificate you're looking at.

The subject is gitlab.torproject.org for the certificate the server is using, but it's singed by someone. You can see that someone in another certificate, whose subject is the CA.

OTOH, how do they tell it's self signed if the issuer is in the trust store?

For regular Firefox you can save the exception when you get the self-signed cert the first time.

Back to this bug, I do think we should keep everything as it was for onion sites, i.e.:

I thought to a slightly different thing, that has a subtle difference (the early return):

diff --git a/security/certverifier/CertVerifier.cpp b/security/certverifier/CertVerifier.cpp
index a3f832c58b11..71908496e985 100644
--- a/security/certverifier/CertVerifier.cpp
+++ b/security/certverifier/CertVerifier.cpp
@@ -864,7 +864,8 @@ Result CertVerifier::VerifySSLServerCert(
       // In this case we didn't find any issuer for the certificate, or we did
       // find other certificates with the same subject but different keys, and
       // the certificate is self-signed.
-      if (StringEndsWith(hostname, ".onion"_ns)) {
+      if (StringEndsWith(hostname, ".onion"_ns) &&
+          rv == Result::ERROR_UNKNOWN_ISSUER) {
         // 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

I thought to a slightly different thing, that has a subtle difference (the early return):

The difference is not that subtle if I understand the flow correctly: my patch keeps BAD_SIGNATURE and INADEQUATE_KEY as non-overridable errors for .onion sites, while yours takes the new more relaxed stance introduced by Mozilla for normal sites.

We just need to decide what's better for us (I'll be conservative on this until we do).