1215779 - Remove broken (non-EC) DSA keygen code

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[:Cykesiopka]

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.

Comment 1

[:Cykesiopka]

Attachment: MozReview Request: Bug 1215779 - Remove broken (non-EC) DSA keygen code.

Bug 1215779 - Remove broken (non-EC) DSA keygen code.

Comment 2

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5241b81168b
The WebIDLError seems to be Bug 1215755.

Comment 3

[:Cykesiopka]

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

Oops, meant to say EC keygen.

Comment 4

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

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.

Comment 5

[:Cykesiopka]

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.

Comment 6

[:Cykesiopka]

Attachment: bug1215779_rm-dsa-keygen_v2.patch

+ Remove PK11_PQG* functions from config/external/nss/nss.def
+ Update keygen telemetry crash case and message

Comment 7

[:Cykesiopka]

Thanks for the review!

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

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb1068402c01

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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