Revocations through the blocklist should set error to SEC_ERROR_REVOKED_CERTIFICATE

RESOLVED DUPLICATE of bug 1024809

Status

()

Core
Security: PSM
RESOLVED DUPLICATE of bug 1024809
4 years ago
2 years ago

People

(Reporter: hpathak, Assigned: hpathak)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

4 years ago
As part of Bug1024809, we piggyback intermediate certificate revocations in the existing blocklist. The current error code for such distrusted entities is SEC_ERROR_UNTRUSTED_ISSUER, which needs to be changed to a new error code with an appropriate message.
(Assignee)

Updated

4 years ago
Assignee: nobody → hpathak
(Assignee)

Comment 1

4 years ago
Created attachment 8456401 [details] [diff] [review]
Bug1038861_blocked_intermediate_psm_errorcode.diff
Attachment #8456401 - Flags: review?(dkeeler)
Comment on attachment 8456401 [details] [diff] [review]
Bug1038861_blocked_intermediate_psm_errorcode.diff

Review of attachment 8456401 [details] [diff] [review]:
-----------------------------------------------------------------

You'll also have to change the test that gets added when the actual cert blocklisting patch lands.
I pointed out a few nits. Also, I'd like you to answer the question in the first patch comment. r- for now.

::: security/certverifier/CertVerifier.cpp
@@ +398,5 @@
>        PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
>        return SECFailure;
>    }
>  
> +  // For intermediate certificate's distrusted by the blocklist, we set a

nit: "certificates"
So, here's the thing: this actually always changes SEC_ERROR_UNTRUSTED_ISSUER into PSM_ERROR_BLOCKED_CERTIFICATE, regardless of if the certificate was on the blocklist or just marked as distrusted by the user (which is uncommon). Given that that's the case, what's the motivation for adding this error code?

::: security/manager/ssl/src/NSSErrorsService.cpp
@@ +20,5 @@
>      "The server uses key pinning (HPKP) but no trusted certificate chain "
>      "could be constructed that matches the pinset. Key pinning violations "
> +    "cannot be overridden." },
> +  { "PSM_ERROR_BLOCKED_CERTIFICATE",
> +    "The website's authenticity could not be determined. It's certificate is "

How about something like: "The server uses a certificate signed by a certificate that has been distrusted. This error cannot be overridden."

Also, a grammar note: "it's" is the contraction of "it is". "its" is the possessive form of the pronoun "it".

::: security/manager/ssl/tests/unit/head_psm.js
@@ +58,5 @@
>  const SSL_ERROR_BAD_CERT_DOMAIN                         = SSL_ERROR_BASE +  12;
>  
>  const PSM_ERROR_KEY_PINNING_FAILURE                     = PSM_ERROR_BASE +   0;
>  
> +// Error code for certificate's blocked by the blocklist.

No need for this comment. Also, don't have a blank line between the two PSM errors.

::: security/manager/ssl/tests/unit/test_add_preexisting_cert.js
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Intermediate certificate's which get actively distrusted should return
> +// the error code PSM_ERROR_BLOCKED_CERTIFICATE.

I'm not sure this comment is necessary.

@@ +35,5 @@
>    let verifiedChain = {};
>    equal(Cr.NS_OK, certDB.verifyCertNow(ee, certificateUsageSSLServer,
>                                         NO_FLAGS, verifiedChain, hasEVPolicy));
> +  // Change the intermediate certificate's trust using addCertFromBase64().
> +  // We use findCertByNickname first to ensure that it already exists.

note sure this comment needs to change

@@ +41,5 @@
>    ok(int_cert);
>    let base64_cert = btoa(getDERString(int_cert));
>    certDB.addCertFromBase64(base64_cert, "p,p,p", "ignored_argument");
> +  equal(PSM_ERROR_BLOCKED_CERTIFICATE,
> +        certDB.verifyCertNow(ee,

nit: these arguments can probably be re-aligned (keep lines as close to 80 chars as possible without going over)
Attachment #8456401 - Flags: review?(dkeeler) → review-
(Assignee)

Comment 3

4 years ago
Created attachment 8456589 [details] [diff] [review]
Bug1038861_blocked_intermediate_psm_errorcode.diff
Attachment #8456401 - Attachment is obsolete: true
Attachment #8456589 - Flags: review?(dkeeler)
(Assignee)

Comment 4

4 years ago
(In reply to David Keeler (:keeler) [use needinfo?] from comment #2)
> So, here's the thing: this actually always changes
> SEC_ERROR_UNTRUSTED_ISSUER into PSM_ERROR_BLOCKED_CERTIFICATE, regardless of
> if the certificate was on the blocklist or just marked as distrusted by the
> user (which is uncommon). Given that that's the case, what's the motivation
> for adding this error code?
Yes. It is uncommon for a user to add an intermediate (more so than an EE) as distrusted, and maybe only a few would be doing it. But if we happen to push intermediates frequently as distrusted it would be confusing to display a message which says "issuer has been marked as not trusted by the user". We could change the error text of SEC_ERROR_UNRTUSTED_ISSUER and make it non-overridable. It would be awesome if we could somehow have both the error codes specific to each scenario, but AFAIK that might need new (dis)trust flags. I have removed the comment for now.
Why not just use SEC_ERROR_REVOKED_CERTIFICATE for revocations that come from the blocklist? Why do end-users need to distinguish the source of the revocation?
(Assignee)

Comment 6

4 years ago
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #5)
> Why not just use SEC_ERROR_REVOKED_CERTIFICATE for revocations that come
> from the blocklist? Why do end-users need to distinguish the source of the
> revocation?
That is what we had thought of initially and we could make that change in pkixcheck. I suppose its okay as long as we don't say "not trusted by the user". Someone who adds an intermediate as distrusted would probably know whats going on.
Comment on attachment 8456589 [details] [diff] [review]
Bug1038861_blocked_intermediate_psm_errorcode.diff

As discussed on IRC, this is probably not the approach we're going to take.
Attachment #8456589 - Flags: review?(dkeeler) → review-
(Assignee)

Updated

4 years ago
Summary: Add new PSM error code for blocked intermediate certificates. → Revocations through the blocklist should set error to SEC_ERROR_REVOKED_CERTIFICATE
(Assignee)

Comment 8

4 years ago
Created attachment 8461086 [details] [diff] [review]
Bug1038861_change_errorcode_for_blocklisted_certs.diff
Attachment #8456589 - Attachment is obsolete: true
Attachment #8461086 - Flags: review?(dkeeler)
Comment on attachment 8461086 [details] [diff] [review]
Bug1038861_change_errorcode_for_blocklisted_certs.diff

Review of attachment 8461086 [details] [diff] [review]:
-----------------------------------------------------------------

Has this patch been rebased on top of the blocklisting patch? It should modify the test that that patch adds, right?
I'd like to see that change and what I've mentioned below, so r- for now.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +190,5 @@
>        endEntityOrCA == EndEntityOrCA::MustBeCA ? CERTDB_TRUSTED_CA
>                                                 : CERTDB_TRUSTED;
> +
> +    // For certificates distrusted via the blocklist, we set flags as p,p,p. But
> +    // when a user imports a certificate as explicitly distrusted via the UI, the

I don't think this is quite true. What is true is that it's not possible to set the trust flags of a certificate to "p,p,p" via the UI, so it's safe to use to indicate a blocked certificate. It could interfere with addons, though... (I think it's okay for now, since we're going to improve on this shortly.)

@@ +195,5 @@
> +    // flags are p,,. This condition differentiates between the two and sets the
> +    // appropriate error message.
> +    if (((flags & (relevantTrustBit|CERTDB_TERMINAL_RECORD))
> +        == CERTDB_TERMINAL_RECORD) && (trust.sslFlags & trust.emailFlags
> +                                       & trust.objectSigningFlags)) {

What we want here is (trust.sslFlags == trust.emailFlags && trust.sslFlags == trust.objectSigningFlags), right? (Otherwise, I think the latter half of the conjunction could be true with flags that aren't identical.)

::: security/manager/ssl/tests/unit/test_add_preexisting_cert.js
@@ +38,5 @@
>    let int_cert = certDB.findCertByNickname(null, "int-limited-depth");
>    ok(int_cert);
>    let base64_cert = btoa(getDERString(int_cert));
>    certDB.addCertFromBase64(base64_cert, "p,p,p", "ignored_argument");
> +  equal(SEC_ERROR_REVOKED_CERTIFICATE,

This isn't really the place for this - just change this test to use "p,," and don't add the extra test.

::: security/manager/ssl/tests/unit/test_cert_trust.js
@@ +56,5 @@
>    check_cert_err_generic(ee_cert, SEC_ERROR_INADEQUATE_CERT_TYPE,
>                           certificateUsageStatusResponder); //expected
>  
>  
>    // Test of active distrust. No usage should pass.

Add a comment that we consider complete active distrust to be "revoked via blocklist".

@@ +75,5 @@
>                           certificateUsageVerifyCA);
>    // In mozilla::pkix (but not classic verification), certificate chain
>    // properties are checked before the end-entity. Thus, if we're using
>    // mozilla::pkix and the root certificate has been distrusted, the error
>    // will be "untrusted issuer" and not "inadequate cert type".

nit: update this comment (also, remove the reference to "classic verification")
Attachment #8461086 - Flags: review?(dkeeler) → review-
(Assignee)

Comment 10

4 years ago
Created attachment 8462850 [details] [diff] [review]
Bug1038861_change_errorcode_for_blocklisted_certs.diff

Hopefully, this one is good enough. A bit of a tricky way, but allows us to differentiate errors with minimal changes. The related changes to the blocklist and its test case are in that bug's patch.
Attachment #8461086 - Attachment is obsolete: true
Attachment #8462850 - Flags: review?(dkeeler)
(Assignee)

Comment 11

4 years ago
Created attachment 8463596 [details] [diff] [review]
Bug1038861_change_errorcode_for_blocklisted_certs.diff

Callback sets the error code to "revoked_certificate". Reverts the add_preexisting_cert changes.
Attachment #8462850 - Attachment is obsolete: true
Attachment #8462850 - Flags: review?(dkeeler)
Attachment #8463596 - Flags: review?(dkeeler)
Comment on attachment 8463596 [details] [diff] [review]
Bug1038861_change_errorcode_for_blocklisted_certs.diff

Review of attachment 8463596 [details] [diff] [review]:
-----------------------------------------------------------------

Cancelling review based on conversation over IRC.
Attachment #8463596 - Flags: review?(dkeeler)

Comment 13

2 years ago
AFAICT, this was fixed/not an issue when the final OneCRL implementation landed in Bug 1024809.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1024809
You need to log in before you can comment on or make changes to this bug.