1269366 - (non-named-curves) Support for non-named curves

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

We currently have code to support non-named curves that doesn't get compiled. We should either enable this code and make it work or remove it if it's not of interest anymore.

Comment 1

[Eric Rescorla (:ekr)]

Make it gone!

Comment 2

[Franziskus Kiefer [:franziskus]]

Attachment: remove-non-named-curves.patch

It's actually not as much as I first thought. If you think this is more critical, you can pass it on to Bob or Wan-Teh.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8748058 [details] [diff] [review]
remove-non-named-curves.patch

Review of attachment 8748058 [details] [diff] [review]:
-----------------------------------------------------------------

Let's fix the comments in lowkey.c and ssl3ecc.c too, the ones referring to "generic curves".

Comment 4

[Franziskus Kiefer [:franziskus]]

Attachment: non-named-curves-take2.patch

Hi Wan-Teh,
I'm removing the leftovers of non-named curve support. The first patch Tim reviewed is non-critical, but this one removes nsslowkey_ECParamsTemplate, which doesn't seem to be used for anything other than non-named curves. Is it ok to remove this?
Thanks

Comment 5

[Wan-Teh Chang]

Comment on attachment 8748638 [details] [diff] [review]
non-named-curves-take2.patch

Review of attachment 8748638 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. More code should be deleted, as shown by this MXR query:

http://mxr.mozilla.org/nss/search?string=nsslowkey_ECParamsTemplate

In particular, the declaration of nsslowkey_ECParamsTemplate in
lib/softoken/lowkeyti.h should be deleted.

Comment 6

[Wan-Teh Chang]

Comment on attachment 8748638 [details] [diff] [review]
non-named-curves-take2.patch

Review of attachment 8748638 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/softoken/lowkey.c
@@ -98,5 @@
> - * parameters. For now, we only support named curves in which
> - * EC params are simply encoded as an object ID and we don't
> - * use nsslowkey_ECParamsTemplate.
> - */
> -const SEC_ASN1Template nsslowkey_ECParamsTemplate[] = {

Franziskus: an alternative course of action is to update
the comments to reflect the fact that we won't support
generic curves, and start using nsslowkey_ECParamsTemplate
so that we set the |type| field of struct ECParamsStr
while decoding.

This alternative approach is somewhat appealing, but
it has some risk because I don't know if
nsslowkey_ECParamsTemplate really works. So this patch
is safer and less work.

Comment 7

[Franziskus Kiefer [:franziskus]]

removed the declaration of nsslowkey_ECParamsTemplate as well and landed as https://hg.mozilla.org/projects/nss/rev/97803c2b21bc

Comment 8

[Wan-Teh Chang]

Franziskus: please also remove the references to nsslowkey_ECParamsTemplate
lib/softoken/legacydb. You can find them using this MXR query:
http://mxr.mozilla.org/nss/search?string=nsslowkey_ECParamsTemplate

Comment 9

[Franziskus Kiefer [:franziskus]]

uh, I meant remove that... Thanks for pointing it out. I removed the legacydb references in https://hg.mozilla.org/projects/nss/rev/48fe5529358a

Comment 10

[Wan-Teh Chang]

Franziskus: you still missed a comment and an ifdef in
lib/softoken/legacydb/lowkey.c :-)

http://mxr.mozilla.org/nss/search?string=nsslowkey_ECParamsTemplate

Comment 11

[Franziskus Kiefer [:franziskus]]

I searched for it... not sure how I missed that :/ thanks!
https://hg.mozilla.org/projects/nss/rev/ad399d244c4f