975229 - Remove NSS-based certificate verification

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

We will do this *after* insanity::pkix has shipped. We're hoping to ship in Firefox 30. That means that we can remove this ~3 weeks after Firefox 30 has shipped. Firefox 33 will be in mozilla-central at that time. Since there's no mozilla33 target milestone yet, I set this at mozilla22.

Basically, we need to take all the code that looks at CertVerifier::mImplementation and remove the non-insanity code paths. Also, we need to remove the call to CERT_CacheOCSPResponseFromSideChannel. We also need to remove the non-insanity telemetry. Also, we can probably simplify/remove a lot of configuration stuff that only affects the NSS-based verification in security/certVerifier/ and security/manager/ssl/src/nsNSSComponent.cpp.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

All the "#ifdef NSS_NO_LIBPKIX" should be removed too.

Additionally, all cert verification tests will need to have their non-insanity add_tests_in_mode/run_tests_in_mode calls removed.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 1 - remove NSS_NO_LIBPKIX code

This patch addresses (part of) comment 1.  I gather that there'd be at least
three more parts following on from this:

- Remove everything depending on CertVerifier::classic;
- Redo tests in security/manager/ssl/tests/unit/ to ditch add_tests_in_mode;
- Other cleanup (security/certverifier/ and nsNSSComponent.cpp) that I'm
  probably not qualified to do.

Is that about right?  I gather that this is too early to rip all this code out,
since we haven't released 30 yet, but might as well write the patches while I
figure out how to trim NSS down in size...

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8419547 [details] [diff] [review]
part 1 - remove NSS_NO_LIBPKIX code

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

::: security/certverifier/CertVerifier.cpp
@@ -490,5 @@
>        PORT_SetError(SEC_ERROR_INVALID_ARGS);
>        return SECFailure;
>    }
>  
> -#ifndef NSS_NO_LIBPKIX

This particular chunk cannot be removed without also removing the "classic" stuff too, because this would completely disable the EV indicator for classic mode. (We already disable the EV indicator for B2G, but not for other platforms.) I suggest just writing a patch that removes both the classic and libpkix stuff at the same time, rather than trying to do them separately.

Anyway, this is on the right track.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://tbpl.mozilla.org/?tree=Try&rev=2393454c7b88

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: remove-nss-based-validation.patch

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #4)
> https://tbpl.mozilla.org/?tree=Try&rev=2393454c7b88

That didn't go well. This one seems to be A-OK though:

https://tbpl.mozilla.org/?tree=Try&rev=5f5f19192131

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

mozilla::pkix hasn't actually shipped yet - are you planning on leaving this for 34 or putting it in 33?

Comment 8

[Camilo Viecco (:cviecco)]

This should wait until mid FF 34 once most users are using mozilla::pkix and we have not found and mayor regressions.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #7)
> mozilla::pkix hasn't actually shipped yet - are you planning on leaving this
> for 34 or putting it in 33?

This isn't something I need immediately. However, I think it is likely that I will try to land at least one patch for Firefox 33 that depends on this.

The time has long passed where we consider switching back to the NSS-based validation to be a viable alternative. In particular, there's at least one security bug in the classic code that we're basically WONTFIXing on the expectation that we'll switch to mozilla::pkix in release. Even if something happens in Firefox 31, we would have plenty of time to fix it in Firefox 32 beta. It would be almost impossible for somebody to convince me to disable mozilla::pkix for Firefox 31 *and* leave it off through Firefox 33 or even Firefox 32, especially since turning mozilla::pkix off for Firefox 32 would also require turning off pinning.

That said, if we want to be lazy about this removal, then we can wait until a patch actually forces the issue. But I expect that to happen in the Firefox 33 time frame. If you think that's inappropriate then I'd appreciate hearing your reasoning.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Actually, now that I really think about it, I agree and would rather remove NSS-based verification sooner rather than later. I'll get to this review as soon as I can.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8441464 [details] [diff] [review]
remove-nss-based-validation.patch

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

Well, this is a lot of code, but it's mostly removals and it's all pretty straightforward. Looks good to me. Also, awesome.
I'm assuming turning off building libpkix for all platforms going to be a separate bug?

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1612,3 @@
>                                                  ALPN_ENABLED_DEFAULT));
>      } else if (prefName.EqualsLiteral("security.OCSP.enabled") ||
>                 prefName.EqualsLiteral("security.CRL_download.enabled") ||

Looks like we can also remove this.

@@ +1612,4 @@
>                                                  ALPN_ENABLED_DEFAULT));
>      } else if (prefName.EqualsLiteral("security.OCSP.enabled") ||
>                 prefName.EqualsLiteral("security.CRL_download.enabled") ||
>                 prefName.EqualsLiteral("security.fresh_revocation_info.require") ||

This went away long ago, looks like.

@@ +1612,5 @@
>                                                  ALPN_ENABLED_DEFAULT));
>      } else if (prefName.EqualsLiteral("security.OCSP.enabled") ||
>                 prefName.EqualsLiteral("security.CRL_download.enabled") ||
>                 prefName.EqualsLiteral("security.fresh_revocation_info.require") ||
>                 prefName.EqualsLiteral("security.missing_cert_download.enabled") ||

I think this can be removed too.

::: security/manager/ssl/tests/unit/test_cert_overrides.js
@@ +77,5 @@
> +  add_combo_tests();
> +  add_distrust_tests();
> +
> +  add_test(function () {
> +    certOverrideService.clearValidityOverride("all:temporary-certificates", 0);

I don't think this is necessary anymore - each xpcshell test runs in a separate profile, right? This was only here because we essentially ran the same test twice in the same environment, so we had to clear any temporary certificate exceptions added by the test.

@@ +168,4 @@
>  }
>  
>  function add_distrust_override_test(certFileName, hostName,
> +                                    expectedResult) {

nit: looks like this can all go on one line

::: security/manager/ssl/tests/unit/test_cert_signatures.js
@@ +53,5 @@
>    check_ca("ca-dsa");
>  
>    // mozilla::pkix does not allow CA certs to be validated for end-entity
>    // usages.
> +  let int_usage = 'SSL CA';

Should we make this const as well while we're here to be consistent?

::: security/manager/ssl/tests/unit/test_cert_trust.js
@@ +110,5 @@
>    // Now tests on the SSL trust bit
>    setCertTrust(cert_to_modify_trust, 'p,C,C');
>    check_cert_err_generic(ee_cert, SEC_ERROR_UNTRUSTED_ISSUER,
>                           certificateUsageSSLServer);
> +  

nit: trailing whitespace

::: security/manager/ssl/tests/unit/test_certificate_usages.js
@@ +55,5 @@
>      [ basicEndEntityUsages,
>        basicEndEntityUsages,
>        basicEndEntityUsages,
>        '',
> +      "Status Responder",

Looks like the style in this file is to use single quotes. However, our style in general is to use double-quotes, so I don't think it's a big deal.

::: security/manager/ssl/tests/unit/test_ocsp_no_hsts_upgrade.js
@@ +30,5 @@
>  
> +  // ocsp-stapling-none.example.com does not staple an OCSP response in the
> +  // handshake, so the revocation checking code will attempt to fetch one.
> +  // Since the domain of the certificate's OCSP AIA URI is an HSTS host
> +  // (as added in the setup of this test), a buggy implementation would

nit: the setup is actually below, now, so this comment is a little unclear. Maybe just amend it to:
"(as added in the setup of this test, below)"

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8441464 [details] [diff] [review]
remove-nss-based-validation.patch

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

Thanks for the review!

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1612,5 @@
>                                                  ALPN_ENABLED_DEFAULT));
>      } else if (prefName.EqualsLiteral("security.OCSP.enabled") ||
>                 prefName.EqualsLiteral("security.CRL_download.enabled") ||
>                 prefName.EqualsLiteral("security.fresh_revocation_info.require") ||
>                 prefName.EqualsLiteral("security.missing_cert_download.enabled") ||

Removed, removed, removed.

::: security/manager/ssl/tests/unit/test_cert_overrides.js
@@ +77,5 @@
> +  add_combo_tests();
> +  add_distrust_tests();
> +
> +  add_test(function () {
> +    certOverrideService.clearValidityOverride("all:temporary-certificates", 0);

Removed.

@@ +168,4 @@
>  }
>  
>  function add_distrust_override_test(certFileName, hostName,
> +                                    expectedResult) {

Done.

::: security/manager/ssl/tests/unit/test_cert_signatures.js
@@ +53,5 @@
>    check_ca("ca-dsa");
>  
>    // mozilla::pkix does not allow CA certs to be validated for end-entity
>    // usages.
> +  let int_usage = 'SSL CA';

Done, though that's outside the scope of this bug.

::: security/manager/ssl/tests/unit/test_cert_trust.js
@@ +110,5 @@
>    // Now tests on the SSL trust bit
>    setCertTrust(cert_to_modify_trust, 'p,C,C');
>    check_cert_err_generic(ee_cert, SEC_ERROR_UNTRUSTED_ISSUER,
>                           certificateUsageSSLServer);
> +  

Fixed.

::: security/manager/ssl/tests/unit/test_certificate_usages.js
@@ +55,5 @@
>      [ basicEndEntityUsages,
>        basicEndEntityUsages,
>        basicEndEntityUsages,
>        '',
> +      "Status Responder",

Fixed anyway.

::: security/manager/ssl/tests/unit/test_ocsp_no_hsts_upgrade.js
@@ +30,5 @@
>  
> +  // ocsp-stapling-none.example.com does not staple an OCSP response in the
> +  // handshake, so the revocation checking code will attempt to fetch one.
> +  // Since the domain of the certificate's OCSP AIA URI is an HSTS host
> +  // (as added in the setup of this test), a buggy implementation would

OK.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

See bug 1027241 comment 15. Some of the CMS stuff was using, indirectly, CERT_VerifyCert, so it needed to be removed before it was safe to remove the configuration of the NSS-based validation.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b3ebf7675c7b

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Addons that are trying to use CERT_VerifyCert, CERT_VerifyCertificate, CERT_PKIXVerifyCert, or other NSS functions that call those functions are likely to run into problems after this patch. The solution to those problems is to stop calling those functions. We don't really have an alternative certificate verification mechanism; we basically just aren't providing any certificate validation services to addons.

Comment 16

[DaLiV]

zyxel switches https contain self-signed certs and that is not possible to change - so not possible to access this configuration from mozilla products anymore (as not possible to add exceptions) ...

Comment 17

[Zach Lym]

>we basically just aren't providing any certificate validation services to addons.

The certificate authority system is a mess and removing this API removes our ability to experiment with alternate certificate validation services.

Unless you are planning to follow this up with a pluggable cert auth system, you are killing innovation in a part of the browser that really needs it.

Comment 18

[Zach Lym]

Wait, strike that, we might have a workaround.  :p