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

RESOLVED FIXED in mozilla37

Status

()

Core
Security: PSM
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

({dev-doc-complete, site-compat})

Trunk
mozilla37
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 37+)

Details

Attachments

(5 attachments, 4 obsolete attachments)

11.45 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
15.11 KB, patch
keeler
: review+
Details | Diff | Splinter Review
1.36 KB, patch
mmc
: review+
Details | Diff | Splinter Review
7.21 KB, patch
mmc
: review+
Details | Diff | Splinter Review
6.01 KB, patch
mmc
: review+
Details | Diff | Splinter Review
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.
Assignee: brian → nobody
Target Milestone: mozilla35 → ---

Updated

3 years ago
Depends on: 1085170
It looks like Cykesiopka is working on this.
Assignee: nobody → cykesiopka.bmo
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).

Updated

3 years ago
No longer depends on: 1085170
See Also: → bug 1107787
Summary: Remove support for non-ECC DSA signatures from mozilla::pkix → Remove support for DSS (non-ECC DSA) signatures from mozilla::pkix

Comment 3

3 years ago
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...
Assignee: cykesiopka.bmo → nobody
Status: ASSIGNED → NEW
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.
Depends on: 1107787
Assignee: nobody → brian
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla37
Created attachment 8532374 [details] [diff] [review]
remove-dss-cert-support-from-mozilla-pkix.patch
Attachment #8532374 - Flags: review?(dkeeler)
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 '('
Attachment #8532374 - Flags: review?(dkeeler) → review+
See Also: → bug 833996
See Also: → bug 834001
See Also: → bug 833986
Created attachment 8536963 [details] [diff] [review]
Part 1: Remove DSS certificate support from mozilla::pkix

Addressed nit, carrying over r+.
Attachment #8532374 - Attachment is obsolete: true
Attachment #8536963 - Flags: review+
Created attachment 8536964 [details] [diff] [review]
Part 2: Remove now-unused DSA test certificates
Attachment #8536964 - Flags: review?(dkeeler)
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.
Attachment #8536964 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebae17a7c0e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8dd513ac780
(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.
Depends on: 1114295
https://hg.mozilla.org/mozilla-central/rev/ebae17a7c0e3
https://hg.mozilla.org/mozilla-central/rev/d8dd513ac780
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Re-opening. I found out that DSS end-entity certificates are still accepted, due to an omission in the patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 8541359 [details] [diff] [review]
Part 3: Reject DSS end-entity certificates

keeler is on vacation, so it would be great if you could review this.
Attachment #8541359 - Flags: review?(mmc)
Created attachment 8541360 [details] [diff] [review]
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.
Attachment #8541360 - Flags: review?(mmc)
Created attachment 8541361 [details] [diff] [review]
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.
Attachment #8541361 - Flags: review?(mmc)
Attachment #8541359 - Attachment description: Reject DSS end-entity certificates → Part 3: Reject DSS end-entity certificates
Created attachment 8541765 [details] [diff] [review]
Part 4: Test that DSS end-entity certificates are rejected [v2]

Fixed some typos.
Attachment #8541360 - Attachment is obsolete: true
Attachment #8541360 - Flags: review?(mmc)
Attachment #8541765 - Flags: review?(mmc)
Created attachment 8541766 [details] [diff] [review]
Part 5: Make DSS test faster by using precomputed PQG parameters [v2]

Rebased on top of v2 of Part 4.
Attachment #8541361 - Attachment is obsolete: true
Attachment #8541361 - Flags: review?(mmc)
Attachment #8541766 - Flags: review?(mmc)
Attachment #8541359 - Flags: review?(mmc) → review+
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.
Attachment #8541765 - Flags: review?(mmc) → review+
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.
Attachment #8541766 - Flags: review?(mmc) → review-
(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.
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.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/f7c107708b99
https://hg.mozilla.org/mozilla-central/rev/415c5a505868
Created attachment 8544857 [details] [diff] [review]
Part 5: Make DSS test faster by using precomputed PQG parameters [v3]
Attachment #8541766 - Attachment is obsolete: true
Attachment #8544857 - Flags: review?(mmc)
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
Attachment #8544857 - Flags: review?(mmc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ccaa9fe3423

Thanks for the quick review!
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/6ccaa9fe3423
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Added to the compat doc: https://developer.mozilla.org/en-US/Firefox/Releases/37/Site_Compatibility
Keywords: dev-doc-complete
relnoted as "Removed support for DSA in certificates and TLS".
relnote-firefox: --- → 37+

Updated

2 years ago
Depends on: 1200577

Updated

2 years ago
No longer depends on: 1200577
You need to log in before you can comment on or make changes to this bug.