978637 - Always build libssl with ECC support enabled, dynamically detecting ECC support at runtime

Status: ASSIGNED

(NSS :: Libraries, enhancement, P5)

Description

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

It seems straightforward to do this by just removing the #ifndef NSS_DISABLE_ECC everywhere in lib/* except for lib/freebl and lib/softoken. However, I think we must also do one or both of the following:

1. Modify ssl3_SuiteBOnly to return PR_TRUE if libssl wasn't built with NSS_ECC_MORE_THAN_SUITE_B.

2. Add an option to let the application explicitly choose which curves to support, defaulted to the Suite B curves.

In particular, an application that isn't explicitly configuring ECC support shouldn't automatically be using non-Suite-B curves, even if NSS and/or whatever other tokens supports additional curves.

Comment 1

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

Attachment: always-enable-ECC-outside-of-freebl-and-softoken.patch

Wan-Teh, I think you have the most experience with running this kind of potentially-mixed combination where libssl has ECC enabled and softoken/freebl may or may not. Is there anything else we have to do, besides remove these #ifndefs?

Comment 2

[Wan-Teh Chang]

Comment on attachment 8384400 [details] [diff] [review]
always-enable-ECC-outside-of-freebl-and-softoken.patch

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

Thanks a lot for writing this patch. It is correct except
for two problems.

1. There are three const arrays that will need to have
a non-ECC variant. Since that will require more changes,
I'd like to review a new patch. So I gave this review-.

2. In ssl3con,c, the code inside one #ifdef NSS_DISABLE_ECC
(not #ifndef) should be deleted.

::: lib/ssl/ssl3con.c
@@ -228,2 @@
>      ct_ECDSA_sign,
> -#endif /* NSS_DISABLE_ECC */

We will need two variants of this array, one with ECC and one without.

@@ -242,2 @@
>      tls_hash_sha256, tls_sig_ecdsa,
> -#endif

We will need two variants of this array, one with ECC and one without.

@@ +718,1 @@
>  	    svrAuth = ss->serverCerts + exchKeyType;

I think this line should be removed, right?

::: lib/ssl/ssl3ext.c
@@ -2223,4 @@
>  	tls_hash_sha256, tls_sig_ecdsa,
>  	tls_hash_sha384, tls_sig_ecdsa,
>  	tls_hash_sha1,   tls_sig_ecdsa,
> -#endif

IMPORTANT: we will need two variants of this array, one
with ECC and one without.

Comment 3

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

Attachment: remove-NSS_DISABLE_ECC.patch

Wan-Teh, I think this patch may still need some work. In particular, I am not sure ssl3_CheckECCEnabled is right. But, I think that otherwise this is the right way of going about things, and I will double-check ssl3_CheckECCEnabled later and submit a follow-up patch for it if necessary.

Comment 4

[Wan-Teh Chang]

Comment on attachment 8385183 [details] [diff] [review]
remove-NSS_DISABLE_ECC.patch

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

r=wtc. Note that the patch has bugs. Please review the comments marked
with "IMPORTANT" or "BUG" carefully.

The only nontrivial change I suggested is to figure out how to
re-evaluate ss->ssl3.isECCEnabled after a ssl3_DisableECCSuites call.

::: lib/ssl/ssl3con.c
@@ +5061,5 @@
>  	if (total_exten_len > 0)
>  	    total_exten_len += 2;
>      }
>  
> +    if (!ss->ssl3.isECCEnabled) {

IMPORTANT: This test is different from the original code. The original
code tests if we are sending any extensions.

So this change seems wrong. If we have a token capable of doig ECC, but
only SSL 3.0 is enabled, we should still disable ECC cipher suites.

@@ +8850,5 @@
> +    } else {
> +      certTypes = nonECCCertificateTypes;
> +      certTypesLength = sizeof nonECCCertificateTypes;
> +      sigAlgs = nonECCSupportedSignatureAlgorithms;
> +      sigAlgsLength = sizeof nonECCSupportedSignatureAlgorithms;

Nit: it seems that the "if" block uses spaces but the "else" block
uses tabs.

::: lib/ssl/ssl3ecc.c
@@ +1003,5 @@
>      }
>  }
>  
>  /* Ask: is ANY ECC cipher suite enabled on this socket? */
>  /* Order(N^2).  Yuk.  Also, this ignores export policy. */

1. Nit: document that this function sets ss->ssl3.isECCEnabled and ss->ssl3.isECCSuiteBOnly.

2. IMPORTANT: this function shows after we call ssl3_DisableECCSuites, we need to check if
ss->ssl3.isECCEnabled should be set to PR_FALSE. Since ssl3_DisableECCSuites may disable
some but not all of the ECC cipher suites, we cannot blindly set ss->ssl3.isECCEnabled to
PR_FALSE. It may be cheaper to do this check in the callers of ssl3_DisableECCSuites rather
than in ssl3_DisableECCSuites.

@@ +1036,5 @@
> +    /* XXX: If we can find an implementation of a small curve (163) then assume
> +     * all the non-Suite-B cutves we know of are supported. */
> +    slot =
> +      PK11_GetBestSlotWithAttributes(CKM_ECDH1_DERIVE, 0, 163,
> +                                     ss ? ss->pkcs11PinArg : NULL);

1. Nit: format this call like this:

    slot = PK11_GetBestSlotWithAttributes(
        CKM_ECDH1_DERIVE, 0, 163, ss ? ss->pkcs11PinArg : NULL);

2. It's not clear what the "XXX" on line 1036 means. I guess you
are unhappy with this test. If so, you should say it.

3. Typo: cutves => curves

@@ +1040,5 @@
> +                                     ss ? ss->pkcs11PinArg : NULL);
> +    if (slot) {
> +        ss->ssl3.isECCSuiteBOnly = PR_FALSE;
> +        PK11_FreeSlot(slot);
> +    }

I think the "Suite B only" test should only be performed
when ss->ssl3.isECCEnabled is true.

I would probably do the three tests in the following order:

1. Do the for loop to see if any ECC cipher suites is enabled.

2. If yes, call PK11_GetBestSlot. This determines ss->ssl3.isECCEnabled.

3. If yes again, call PK11_GetBestSlotWithAttributes. This
determines ss->ssl3.isECCSuiteBOnly.

::: lib/ssl/ssl3ext.c
@@ +2240,5 @@
> +        sigAlgs = allSignatureAlgorithms;
> +        sigAlgsLength = sizeof(allSignatureAlgorithms);
> +    } else {
> +        sigAlgs = noECCSignatureAlgorithms;
> +        sigAlgsLength = sizeof(allSignatureAlgorithms);

BUG: allSignatureAlgorithms => noECCSignatureAlgorithms.