Closed Bug 1215779 Opened 5 years ago Closed 5 years ago

Remove broken (non-EC) DSA keygen code

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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.
Bug 1215779 - Remove broken (non-EC) DSA keygen code.
Attachment #8675294 - Flags: review?(dkeeler)
(In reply to Cykesiopka from comment #0)
> [...] ECDSA keygen

Oops, meant to say EC keygen.
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.
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.
+ 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+
https://hg.mozilla.org/mozilla-central/rev/fb1068402c01
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 488059
You need to log in before you can comment on or make changes to this bug.