1253912 - (NSS_ECC_MORE_THAN_SUITE_B) Remove NSS_ECC_MORE_THAN_SUITE_B support

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

NSS doesn't support NSS_ECC_MORE_THAN_SUITE_B at the moment without source code changes. If it is not possible to use NSS_ECC_MORE_THAN_SUITE_B with the current code, all related code could be removed.

Comment 1

[Martin Thomson [:mt:]]

Are you able to identify when this broke?  (I know, it means bisecting, but that should be relatively quick.)

Comment 2

[Franziskus Kiefer [:franziskus]]

Bug 335748 removes all the non suite B curves and bug 353896 makes sure to throw an error if NSS_ECC_MORE_THAN_SUITE_B is set. So NSS_ECC_MORE_THAN_SUITE_B is not usable since 3.11.5 (if the target is correct, but definitely since 3.12).

Comment 3

[Eric Rescorla (:ekr)]

Hmmm... So, how are we getting non-SuiteB curves on some builders?

Comment 4

[Franziskus Kiefer [:franziskus]]

We have a build script that reverts ecl-curve.h to 3.11 [1] on some builds [2].

[1] https://hg.mozilla.org/projects/nss/file/tip/automation/buildbot-slave/build.sh#l295
[2] https://bot.nss-crypto.org:8011/builders/fedora-x64-OPT/builds/627/steps/shell/logs/stdio

Comment 5

[Kai Engert (:KaiE:)]

I'm guessing that Julien might want to comment on this bug.

Comment 6

[Julien Pierre]

Kai,

I have been discussing this issue off bugzilla and off the list. Our only requirement at Oracle is that there should still be a way to build with all the curves somehow, whatever the method to do that may be - additional build macros, files, etc.

Comment 7

[Franziskus Kiefer [:franziskus]]

Attachment: ecc-cleanup.patch

This removes NSS_ECC_MORE_THAN_SUITE_B support and all associated code.

The only "observable" differences to the outside world are in ecl-curve.h and ecl-exp.h where I shrank the enums. But since none of those values were usable before, I wouldn't consider this a breaking change.
All OIDs are kept such that other PK11 token can still be used with other curves (in case anyone does this).

This should solve the bustages we currently have and will break the workaround for those additional curves.

Comment 8

[Franziskus Kiefer [:franziskus]]

A try run with this patch https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=bcac2514b9187d5ddd9b926450f5a7f302d7e145

Comment 9

[Martin Thomson [:mt:]]

Franziskus, this looks promising.  It's surprisingly large though.

You need to remove the final column from the table in sslsock.c and the ssl_SuiteBOnly() function from ssl3ecc.c.  There are probably a few other bits of collateral as well.  ssl_InitNamedGroups() could probably just set the value of ss->namedGroups to UINT32_MAX and avoid the looping.  That sort of thing...

Comment 10

[Franziskus Kiefer [:franziskus]]

(In reply to Martin Thomson [:mt:] from comment #9)
> Franziskus, this looks promising.  It's surprisingly large though.

The largest part are files that get removed. And yes, there's a lot of dead code...

> You need to remove the final column from the table in sslsock.c and the
> ssl_SuiteBOnly() function from ssl3ecc.c.  There are probably a few other
> bits of collateral as well.  ssl_InitNamedGroups() could probably just set
> the value of ss->namedGroups to UINT32_MAX and avoid the looping.  That sort
> of thing...

right, I'll fix those things. I'm sure there are some more things we could simplify...

Comment 11

[Wan-Teh Chang]

Comment on attachment 8777327 [details] [diff] [review]
ecc-cleanup.patch

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

Franziskus: I will wait until the NSS team has decided to do this
to review this patch carefully. I took a quick look and I suggest
some changes. See also the patch I emailed to the NSS team today,
which fixes the ssl_gtest.sh test failures with
NSS_ECC_MORE_THAN_SUITE_B=1.

::: external_tests/ssl_gtest/libssl_internals.c
@@ +44,2 @@
>  
>      return PR_MAX(SSL_RSASTRENGTH_TO_ECSTRENGTH(serverKeyBits), minKeaBits);

We should remove the minKeaBits local variable and just
pass the 256U constant to PR_MAX() here.

::: lib/ssl/sslimpl.h
@@ +177,1 @@
>      PRBool suiteb;

We should delete this member. If we can't, I suggest we rename
this member "suitebUnused" or "suitebReserved".

Comment 12

[Wan-Teh Chang]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #7)
> Created attachment 8777327 [details] [diff] [review]
> ecc-cleanup.patch
> 
> This removes NSS_ECC_MORE_THAN_SUITE_B support and all associated code.
> 
> The only "observable" differences to the outside world are in ecl-curve.h
> and ecl-exp.h where I shrank the enums.

ecl-curve.h is a private header.  ecl-exp.h is an exported header, so
your changes to ecl-exp.h need to be carefully reviewed.  We may need
to leave the ECCurveName enum type unchanged.

Comment 13

[Martin Thomson [:mt:]]

Comment on attachment 8777327 [details] [diff] [review]
ecc-cleanup.patch

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

I like the code deleting.

::: cmd/bltest/tests/ecdsa/key0
@@ +1,3 @@
> +AAAACgYIKoZIzj0DAQcAAABBBNGB7n4kH15tKA/SMpetaQVqg6WxIuuUuMQT2tDX
> +NN5jKZfaxD47NsTjTr3x3D5t1qRBYuL6VtdgIuxBIHGG9dcAAAAgaGjyZBL+LN3a
> +7NkGiHJBfqh7XKNH0AnPF3vFWpostIQ=

I assume that this is just renaming key13 to key0, and key14 to key1?

::: lib/freebl/ecl/ecl-exp.h
@@ -31,5 @@
>  
>  	ECCurve_noName = 0,
>  
>  	/* NIST prime curves */
> -	ECCurve_NIST_P192,

As Wan-Teh points out, this is a public file, so we need to keep these.  You should just mark them deprecated.

Comment 14

[Franziskus Kiefer [:franziskus]]

> I assume that this is just renaming key13 to key0, and key14 to key1?

yep

> We may need to leave the ECCurveName enum type unchanged.
> As Wan-Teh points out, this is a public file, so we need to keep these.  You should just mark them deprecated.

Yeah, I was hoping that since those values are not usable we could remove them. But I guess we have to keep and deprecate them.

Comment 15

[Kai Engert (:KaiE:)]

Comment on attachment 8777327 [details] [diff] [review]
ecc-cleanup.patch

I abstain from voting on this.

Comment 16

[Julien Pierre]

For obvious reasons, I'm not in favor of this change. This change basically means Oracle has to fork NSS.

Comment 17

[Martin Thomson [:mt:]]

Comment on attachment 8777327 [details] [diff] [review]
ecc-cleanup.patch

Someone will have to remind me of the exact details of the agreement we came to at the meeting.

Comment 18

[Julien Pierre]

I believe we agreed that the other curves should still continue to work if Oracle/Red Hat only change the content of the ecl directory for their NSS builds.

I pointed out that these extra curves need to continue to work not just for libssl, but also for certutil so that keygen/signing for these other curves continues to work.

Same may be needed in a few other tools as well, but I think this is the most critical one.

I believe the current patch does not address this requirement for certutil. It just removes the extra curves from a hardcoded list. It needs to enumerate the curves supported for keygen and/or signing.

Comment 19

[Franziskus Kiefer [:franziskus]]

I'll update the patch today.
Curves other than the 4 curves currently in ecl-curves.h will only work with a token using something other than freebl (or an old version). Other prime curves can be added to ecl-curves.h if someone wishes to do as a local patch (note that reverting ecl doesn't work anymore as of 4 day ago where the definition of ECCurveParams changed). libssl will continue to support other prime curves if an appropriate token is present, i.e. all curves listed at [1]. The same goes for certutil, it'll support the 4 curves that are present and others if there's an appropriate token, i.e. not freebl without modifications.

[1] https://github.com/nss-dev/nss/blob/c6d94f8f3c493a8cd0b75d3cc65e6eab5612a748/lib/ssl/sslsock.c#L150

Comment 20

[Franziskus Kiefer [:franziskus]]

Attachment: ecc-cleanup.patch

This should remove things as described in comment 19 and hopefully reflect what we discussed.

Comment 21

[Martin Thomson [:mt:]]

Comment on attachment 8794133 [details] [diff] [review]
ecc-cleanup.patch

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

Would you mind throwing this on phabricator for me?

Comment 22

[Martin Thomson [:mt:]]

Ah, belay that: https://nss-dev.phacility.com/D24

Comment 23

[Franziskus Kiefer [:franziskus]]

Attachment: ecc-cleanup.patch

r+ from mt at https://nss-dev.phacility.com/D24

Comment 24

[Robert Relyea]

Comment on attachment 8794829 [details] [diff] [review]
ecc-cleanup.patch

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

Sorry it took so long to get to this. I was a long list, but easier to review than the list seemed to indicate. (mostly I had to verify that yes, indeed this file should be removed).

::: cmd/certutil/certutil.c
@@ +1262,5 @@
>  #ifndef NSS_DISABLE_ECC
>      FPS "%-20s Elliptic curve name (ec only)\n",
>          "   -q curve-name");
> +    FPS "%-20s One of nistp256, nistp384, nistp521, curve25519.\n", "");
> +    FPS "%-20s If a custom token is present, the following curves are also supported:\n", "");

Good!

Comment 25

[Franziskus Kiefer [:franziskus]]

landed as https://hg.mozilla.org/projects/nss/rev/057be83a362092e04c435a6988f4ff22c134305b