1073867 - Remove support for DSS (non-ECC DSA) signatures from mozilla::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

We know from telemetry that DSA certificates are basically never used. My understanding is that Chromium and BoringSSL are removing support for them. In private conversation, Antoine mentioned that it would also be a good idea to remove support for them. This will reduce the amount of code that needs to be audited for correctness, and it also simplifies proofs of correctness of mozilla::pkix.

Comment 1

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

It looks like Cykesiopka is working on this.

Comment 2

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

Telemetry to justify this change:
http://telemetry.mozilla.org/#filter=release%2F32%2FSSL_AUTH_ALGORITHM_FULL
http://telemetry.mozilla.org/#filter=release%2F32%2FSSL_AUTH_DSA_KEY_SIZE_FULL

In Firefox 32, only 66,720 full handshakes negotiated a DSA algorithm. Of those, only 50 (yes, fifty) used a key size larger than 1024 bits.

This is out of ~30 BILLION full handshakes reported (~25 billion RSA handshakes and ~5 billion ECDSA handshakes).

Comment 3

[:Cykesiopka]

Unassigning myself for now - I don't think I'll get this done in a reasonable time frame at the moment due to the other stuff I'm doing unfortunately...

Comment 4

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

Thanks for the update. I think bug 1107787 should be fixed first, because that is a simpler, less intrusive change that should be a good indicator of the impact of also doing this.

Comment 5

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

Attachment: remove-dss-cert-support-from-mozilla-pkix.patch

Comment 6

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8532374 [details] [diff] [review]
remove-dss-cert-support-from-mozilla-pkix.patch

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

Looks good. A couple of the test generators need updating. It looks like you were planning to anyway, but let's wait a bit after bug 1107787 landing to land this, to get a sense of if there will be significant compatibility impact.

::: security/manager/ssl/tests/unit/test_cert_signatures.js
@@ -42,5 @@
>  function run_test() {
>    // Load the ca into mem
>    load_ca("ca-rsa");
>    load_ca("ca-p384");
> -  load_ca("ca-dsa");

test_cert_signatures/generate.py should be updated to no longer generate DSS-signed certificates as well, right? (And then we can also remove the previously-generated files from the tree.)
Same with test_keysize/generate.py, looks like.
(After that cleanup, I imagine at least init_dsa in psm_common_py/CertUtils.py can be removed, and maybe more.)

::: security/pkix/test/gtest/pkixder_pki_types_tests.cpp
@@ +348,5 @@
>        0x2b, 0x0e, 0x03, 0x02, 0x1d },
>      9,
>    },
>  
> +  // id-dsa-with-sha256 ( 2.16.840.1.101.3.4.3.2)

nit: unnecessary space after the '('

Comment 7

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

Attachment: Part 1: Remove DSS certificate support from mozilla::pkix

Addressed nit, carrying over r+.

Comment 8

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

Attachment: Part 2: Remove now-unused DSA test certificates

Comment 9

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8536964 [details] [diff] [review]
Part 2: Remove now-unused DSA test certificates

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

Looks like CertUtils.init_dsa is unused now and can be removed (in fact, it appears that everything dsa-related can be removed from CertUtils). r=me with that done too.

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebae17a7c0e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8dd513ac780

Comment 11

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

(In reply to David Keeler (:keeler) [use needinfo?] from comment #9)
> Looks like CertUtils.init_dsa is unused now and can be removed (in fact, it
> appears that everything dsa-related can be removed from CertUtils). r=me
> with that done too.

I made this change.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ebae17a7c0e3
https://hg.mozilla.org/mozilla-central/rev/d8dd513ac780

Comment 13

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

Re-opening. I found out that DSS end-entity certificates are still accepted, due to an omission in the patch.

Comment 14

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

Attachment: Part 3: Reject DSS end-entity certificates

keeler is on vacation, so it would be great if you could review this.

Comment 15

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

Attachment: Part 4: Test that DSS end-entity certificates are rejected

If you run this test without Part 3 applied, then the test will fail because the call to BuildCertChain will wrongly return Success. With Part 3 applied, the test passes, but it is very slow, because NSS's DSS parameter generation is *very* slow.

Comment 16

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

Attachment: Part 5: Make DSS test faster by using precomputed PQG parameters

This makes the test run in a reasonable amount of time, by hard-coding the PQG parameters generated in a previous run.

Comment 17

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

Attachment: Part 4: Test that DSS end-entity certificates are rejected [v2]

Fixed some typos.

Comment 18

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

Attachment: Part 5: Make DSS test faster by using precomputed PQG parameters [v2]

Rebased on top of v2 of Part 4.

Comment 19

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8541765 [details] [diff] [review]
Part 4: Test that DSS end-entity certificates are rejected [v2]

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

::: security/pkix/test/gtest/pkixbuild_tests.cpp
@@ +390,5 @@
> +  {
> +    return Success;
> +  }
> +
> +  virtual Result VerifySignedData(const SignedDataWithSignature& signedData,

Please be consistent including variable names in function signatures, here and elsewhere.

Comment 20

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8541766 [details] [diff] [review]
Part 5: Make DSS test faster by using precomputed PQG parameters [v2]

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

I'm not crazy about changes that decrease readability. How much faster is it? If this change is important, please document in the comments how you generated the params, in case anyone ever needs to change them in the future.

Comment 21

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

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #20)
> I'm not crazy about changes that decrease readability. How much faster is
> it? If this change is important, please document in the comments how you
> generated the params, in case anyone ever needs to change them in the future.

OK, I will add such documentation. The difference is ~25 seconds vs. <0.1 seconds, IIRC, so it really is a substantial difference--the difference between the test suite taking an acceptable amount of time to run repeatedly during development, and taking an unreasonable amount of time to run during development.

Comment 22

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

Part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/f7c107708b99
Part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/415c5a505868

Thanks for the quick reviews! I will post a new version of Part 5 soon.

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f7c107708b99
https://hg.mozilla.org/mozilla-central/rev/415c5a505868

Comment 24

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

Attachment: Part 5: Make DSS test faster by using precomputed PQG parameters [v3]

Comment 25

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8544857 [details] [diff] [review]
Part 5: Make DSS test faster by using precomputed PQG parameters [v3]

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

All right, makes sense to me. Thanks!

::: security/pkix/test/lib/pkixtestnss.cpp
@@ +247,5 @@
>  
> +  // XXX: Since it takes over 20 seconds to generate the parameters, we've
> +  // taken the parameters generated during one run and hard-coded them for use
> +  // in all runs. The parameters were generated from this code:
> +  // 

whitespace

Comment 26

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ccaa9fe3423

Thanks for the quick review!

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ccaa9fe3423

Comment 28

[Kohei Yoshino [:kohei]]

Added to the compat doc: https://developer.mozilla.org/en-US/Firefox/Releases/37/Site_Compatibility

Comment 29

[Lawrence Mandel [:lmandel] (use needinfo)]

relnoted as "Removed support for DSA in certificates and TLS".