671060 - Remove imcomplete Ephemeral-Static DH support from libsmime (CMSUtil_EncryptSymKey_ESDH)

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P5)

Description

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

+++ This bug was initially created as a clone of Bug #668397 +++

In particular, remove the code in the cases of the switch statements that call NSS_CMSUtil_*_ESDH and remove the NSS_CMSUtil_*_ESDH functions, as this code can't work and thus hasn't been tested, AFAICT.

Comment 1

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

Attachment: Remove ESDH support

Here is the patch. Notice that this code always failed because NSS_CMSUtil_EncryptSymKey_ESDH unconditionally returned SECFailure. Also, NSS_CMSUtil_DecryptSymKey_ESDH was never called.

Comment 2

[Wan-Teh Chang]

Comment on attachment 545469 [details] [diff] [review]
Remove ESDH support

There are two other SEC_OID_X942_DIFFIE_HELMAN_KEY cases
in security/nss/lib/smime/cmsrecinfo.c.  Should we also
remove them?

Comment 3

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

In bug 676118, David Cooper contributed an ESDH implementation, so I am resolving this as a duplicate of that bug.

Comment 4

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

This code was supposed to implement ephemeral-static non-ECC DH, David's patches are for ephemeral-staic ECC DH. So, it still makes sense to keep this bug open to remove this dead code.

Comment 5

[Robert Relyea]

Comment on attachment 545469 [details] [diff] [review]
Remove ESDH support

r+ with the following caveat....

ECDH and DH should be very similiar (basically differing in some mechanisms and oids).

Most of the underlying mechanical differences should not appear at this layer.

This patch should be coordinated with David Cooper's work on ECC.

bob

Comment 6

[Kai Engert [:KaiE:]]

I think we should not land this patch as is.

This patch removes the empty *_ESDH functions from cmslocal.h and cmspubkey.c - this seems fine.

This patch also removes a block of code from cmsrecinfo.c - we shouldn't do that.
David's patch in bug requires that block of code, and it changes it to handle SEC_OID_ANSIX962_EC_PUBLIC_KEY.

David's patch keeps the call to the _ESDH function, he attempts to make no changes to existing code (handling of other algorithms).

Because there's consensus in this bug here that the _ESDH code isn't working and can be removed, I suggest that we amend David's patch to remove/disable the case that handles SEC_OID_X942_DIFFIE_HELMAN_KEY - after David's patch landed.

Comment 7

[Kai Engert [:KaiE:]]

Now that we have David's other patches landed (ECDH and RSA-OAEP), let's complete this task.

I'll mark the patch as obsolete, and submit a new patch for Bob to review.

Comment 8

[Kai Engert [:KaiE:]]

Attachment: Bug 671060 - Remove incomplete ESDH code. r=rrelyea

Comment 9

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/projects/nss/rev/59cb4785ea5055075136a5421ffddf29ed18b8da
3.100