Remove broken (non-EC) DSA keygen code

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Tracking

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
AFAICT, DSA <keygen> doesn't actually work right now due to the DSA case not being implemented in nsKeygenFormProcessor::GetPublicKey():
https://hg.mozilla.org/mozilla-central/annotate/780ae7350ded/security/manager/ssl/nsKeygenHandler.cpp#l624

This is supported by a Wireshark capture, where RSA and ECDSA keygen POSTs send a public key, but DSA keygen does not.

Bug 1073867 means DSA certs aren't supported in general anyways, so I don't really see the point in keeping code that is there to facilitate generating certificates Firefox doesn't support, especially in light of https://groups.google.com/d/msg/mozilla.dev.security/mr_DoJGOoiA/L1IDmB-sEAAJ.
(Assignee)

Comment 1

3 years ago
Created attachment 8675294 [details]
MozReview Request: Bug 1215779 - Remove broken (non-EC) DSA keygen code.

Bug 1215779 - Remove broken (non-EC) DSA keygen code.
Attachment #8675294 - Flags: review?(dkeeler)
(Assignee)

Comment 2

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5241b81168b
The WebIDLError seems to be Bug 1215755.
(Assignee)

Comment 3

3 years ago
(In reply to Cykesiopka from comment #0)
> [...] ECDSA keygen

Oops, meant to say EC keygen.
Attachment #8675294 - Flags: review?(dkeeler) → review+
Comment on attachment 8675294 [details]
MozReview Request: Bug 1215779 - Remove broken (non-EC) DSA keygen code.

https://reviewboard.mozilla.org/r/22361/#review20027

Great - r=me with comments addressed.

::: security/manager/ssl/nsKeygenHandler.cpp
(Diff revision 1)
> -#include "pk11pqg.h"

Similarly, I think we can remove all PK11_PQG\_\* functions from config/external/nss/nss.def

::: security/manager/ssl/nsKeygenHandler.cpp:441
(Diff revision 1)
>    } else if (keyGenMechanism == CKM_DSA_KEY_PAIR_GEN) {

Let's just change this case to an 'else' and change the MOZ_CRASH message to "Unknown keygen algorithm".

::: security/manager/ssl/nsKeygenHandler.cpp:457
(Diff revision 1)
> -    char *keyparamsString = nullptr, *str = nullptr;
> +    char* keyparamsString = nullptr;

Eh... I feel like we should keep the placement of \* consistent here. Since this code is probably going away in the future anyway, we may as well keep it as 'char \*keyParamsString' so we don't have to modify other nearby cases.
(Assignee)

Comment 5

3 years ago
https://reviewboard.mozilla.org/r/22361/#review20027

> Eh... I feel like we should keep the placement of \* consistent here. Since this code is probably going away in the future anyway, we may as well keep it as 'char \*keyParamsString' so we don't have to modify other nearby cases.

Sure.
(Assignee)

Comment 6

3 years ago
Created attachment 8676038 [details] [diff] [review]
bug1215779_rm-dsa-keygen_v2.patch

+ Remove PK11_PQG* functions from config/external/nss/nss.def
+ Update keygen telemetry crash case and message
Attachment #8675294 - Attachment is obsolete: true
Attachment #8676038 - Flags: review+
(Assignee)

Comment 7

3 years ago
Thanks for the review!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a7c29b2410b
Keywords: checkin-needed

Comment 9

3 years ago
bugherdermerge
https://hg.mozilla.org/mozilla-central/rev/fb1068402c01
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Assignee)

Updated

3 years ago
Blocks: 488059
You need to log in before you can comment on or make changes to this bug.