857882 - Need runtime check for ECC curve capabilities

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Robert Relyea]

Attachment: switch between full ECC suite and suite-B only based on the ECC key lengths.

We need SSL to switch the curve capabilities based on the capabilities provided by the installed PKCS #11 modules.

Comment 1

[Robert Relyea]

This patch needs a little bit of work, a new one is forthcoming...

Comment 2

[Robert Relyea]

Attachment: switch between full ECC suite and suite-B only based on the ECC key lengths.

Notes about this patch:

1) There is a bug in the old softoken where it was not setting the MechanismInfo key sizes correctly. This patch fixes that bug.
2) The granularity is currently "can only do suiteB" versus "can do everything". We can get a little tighter based on curve size, but we can't distinguish between individual curves of the same size.
3) This fixes a bug in NSS where if ECC ciphers are turned on, but no token can support ECC (currently happens on all RH products), we send the ECC extensions. 

bob

Comment 3

[Ryan Sleevi]

Comment on attachment 733636 [details] [diff] [review]
switch between full ECC suite and suite-B only based on the ECC key lengths.

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

Still putting it at r? - I'm not sure whether I'm being pedantic here or not, but I'm not sure that this is correct, if the 'intent' is as it seems, which is to divorce libssl from knowing how softoken was built.

::: lib/ssl/ssl3ecc.c
@@ +1023,5 @@
> +ssl3_SuiteBOnly(sslSocket *ss)
> +{
> +    /* look to see if we can handle certs less than 256 bits */
> +    PK11SlotInfo *slot = 
> +	PK11_GetBestSlotWithAttributes(CKM_ECDH1_DERIVE, 0, 250, 

nit: Trailing whitespace on these two lines (you can see easier in Splinter)

@@ +1024,5 @@
> +{
> +    /* look to see if we can handle certs less than 256 bits */
> +    PK11SlotInfo *slot = 
> +	PK11_GetBestSlotWithAttributes(CKM_ECDH1_DERIVE, 0, 250, 
> +					ss ? ss->pkcs11PinArg : NULL);

The choice of "250" here seems entirely arbitrary (other than being 'less than 256')

Since we're going to advertise as low as sec163k1, does it make more sense to check for 164 (or is it 163? I'm not fully sure I grokked the PKCS#11 spec on this point)?

It 'seems' wrong that libssl should be using a prefab list in the more-than-suite-b case, since presumably the user may have added other PKCS#11 tokens that support parts, but not all, of the curve space we'll end up advertising. Since we're no longer gated on the #ifdef here (which guaranteed that softokn would support them), it seems like we should "do it right" and actually discover/construct the list by enumerating all the slots that support CKM_ECDH1_DERIVE and querying their min/max sizes to see if any of the curve sizes fit.

@@ +1032,5 @@
> +	PK11_FreeSlot(slot);
> +	return PR_FALSE;
> +    }
> +    /* nope, presume we can only do suite B */
> +    return PR_TRUE;

nit: The ordering of this conditional is a bit odd, given that the function name is ssl3_SuiteBOnly. When I read it, it reads a bit like a double negative.

This is really a minor thing, but it seems structured as:

if (!slot) {
    /* Presume we can only do suite B */
    return PR_TRUE;
}

/* We can handle smaller certs, presume we can do all curves,
 * not just Suite B */
PK11_FreeSlot(slot);
return PR_FALSE;


It's probably fine as is, but hopefully the "nope" plus "PR_TRUE" confusion is clearer with the above.

@@ +1057,5 @@
> +    } else {
> +	ECListSize = sizeof (tlsECList);
> +	ECList = tlsECList;
> +    }
> +   

nit: and on this.

Comment 4

[Robert Relyea]

> I'm not sure whether I'm being pedantic here or not, but I'm not sure that this is correct,
> if the 'intent' is as it seems, which is to divorce libssl from knowing how softoken was
> built.

yes, that's exactly the intent. in comment 2 and specifically acknowledge this. We could do do a finer grain job of it, but not completely find grain. The only information we have is the range of keys that are supported. 

There may be some value in checking if the token can handle the full range of key sizes used in TLS ciphers, assume that it can handle all the ciphers, but if not only select a list of common, ciphers and check the key sizes. It would not be beyond the pale for a token to only support one of the Suite B or NIST curves.

I didn't do this because I didn't need this granulatity at this point, but I did structure the code so someone could improve the granularity if the wanted to.

> The choice of "250" here seems entirely arbitrary (other than being 'less than 256')

It is any value inside the full range of tls key lengths, but outside the suite-B lengths would work. I'm OK with changing it to 163. That would allow NIST curve only tokens to show up was Suite-B (they would have a 192 bit curve), and thus work.

> It 'seems' wrong that libssl should be using a prefab list in the more-than-suite-b case,
> since presumably the user may have added other PKCS#11 tokens that support parts, but not 
> all, of the curve space we'll end up advertising.

yes, we are explicitly punting on this case. This is what I mean by 'it could be more finer grain'. Unfortunately, at some point we have to make some assumptions in any case because we can only test key sizes, not curves. If someone supported all the Prime curves and none of the GF2M curves, we would never be able to detect that.

>nit: The ordering of this conditional is a bit odd, given that the function name is >ssl3_SuiteBOnly. When I read it, it reads a bit like a double negative.

I don't mind changing the ordering. I was mostly worried about making sure the slot was correctly freed, so the current ordering makes more sense for that, but either ordering should be readable by and experienced programmer, so I have no objection to changing the ordering.

> nit: and on this.

I assume you mean ordering? In this case I think the name ssl_SuiteBOnly() implies the order pretty clearly saying !ssl_SuiteBOnly() seems a little like using a double negative to me, doesn't it to you?

Comment 5

[Ryan Sleevi]

Comment on attachment 733636 [details] [diff] [review]
switch between full ECC suite and suite-B only based on the ECC key lengths.

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

r+ , with punting on making the more-than-suite-B detection better, and changing "250" -> "163" (or 164... whichever is the right one)

And the "and on this" was a comment about whitespace, sorry.

Comment 6

[Robert Relyea]

Ok, thanks ryan. I'll make those changes before I check in.

Comment 7

[Robert Relyea]

Attachment: switch between full ECC suite and suite-B only based on the ECC key lengths. - as checked in

Patch as check-in, contains Ryan's review comments.

Comment 8

[Robert Relyea]

changeset:   10730:2380cf2250c4
tag:         tip
user:        Bob Relyea <[e-mail]>
date:        Wed Apr 10 17:02:42 2013 -0700
summary:     Bug 857882 - Need runtime check for ECC curve capabilities

Comment 9

[Wan-Teh Chang]

Comment on attachment 736044 [details] [diff] [review]
switch between full ECC suite and suite-B only based on the ECC key lengths. - as checked in

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

r=wtc. I will attach a patch to make the change I suggested and
fix some typos and style issues.

::: lib/freebl/blapit.h
@@ +85,5 @@
> +#define EC_MAX_KEY		571     /* in bits */
> +#define EC_MIN_KEY		112     /* in bits */
> +#else
> +#define EC_MAX_KEY		521     /* in bits */
> +#define EC_MIN_KEY		256     /* in bits */

I suggest adding _BITS to these macros.

Note the old MAX_ECKEY_LEN macro above, which is a length in bytes.
I found many _BITS macros in blapit.h.

::: lib/ssl/sslimpl.h
@@ +142,5 @@
>  
>  /* Mask of the 25 named curves we support. */
> +#define SSL3_ALL_SUPPORTED_CURVES_MASK 0x3fffffe
> +/* only 3 curves, suite B*/
> +#define SSL3_SUITE_B_SUPPORTED_CURVES_MASK 0x3800000

Nit: Suite B only uses two curves. The three curves in the "Basic ECC" mode
are the curves named in Certicom's IETF IP contribution letter:
https://www.certicom.com/index.php/licensing/ip-contributions

Comment 10

[Wan-Teh Chang]

Attachment: Fix typos in comments and minor variable and function name issues

Comment 11

[Ryan Sleevi]

Comment on attachment 742116 [details] [diff] [review]
Fix typos in comments and minor variable and function name issues

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

I'm not sure what the 'preferred' splinter review is, hence the r-, but I think this is r+ with just one nit about your use of sizeof, unless I've missed something.

::: lib/ssl/derive.c
@@ +761,5 @@
>  
>  		if ( requiredECCbits > signatureKeyStrength ) 
>  		     requiredECCbits = signatureKeyStrength;
>  
> +		ec_curve = ssl3_GetCurveWithECKeyStrength(

The whitespace on this function seems awkward either way, but it seems like as originally written it was aligned (unless I'm missing something in splinter)

::: lib/ssl/ssl3ecc.c
@@ +1056,2 @@
>      } else {
> +	ecListSize = sizeof ECListAll;

NACK on dropping the parens - this is needed for compatibility between gcc, clang, and MSVC.

See http://crbug.com/82385#c52 for the Chromium example of sizeof fixups.

Comment 12

[Wan-Teh Chang]

Comment on attachment 742116 [details] [diff] [review]
Fix typos in comments and minor variable and function name issues

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

Notes on my patch:

::: lib/ssl/ssl3ecc.c
@@ +984,5 @@
>  
>  /* Prefabricated TLS client hello extension, Elliptic Curves List,
>   * offers only 3 curves, the Suite B curves, 23-25 
>   */
> +static const PRUint8 ECListSuiteB[12] = {

The original author seems to capitalize constant names (see "ECPtFmt" below).
So I preserve that style (changed "suiteBECList" to "ECListSuiteB").

The name "ECListSuiteB" actually violates the NSS naming convention, which
only capitalize type and function names. But NSS doesn't have a naming
convention for constants :-(

@@ +1025,3 @@
>      PK11SlotInfo *slot =
>  	PK11_GetBestSlotWithAttributes(CKM_ECDH1_DERIVE, 0, 163,
> +				       ss ? ss->pkcs11PinArg : NULL);

I changed "certs less than 163 bits" to "curves less than 256 bits".
I hope that's correct.

@@ +1044,5 @@
>  			PRBool      append,
>  			PRUint32    maxBytes)
>  {
> +    PRInt32 ecListSize = 0;
> +    const PRUint8 *ecList = NULL;

In NSS we don't capitalize variable names.

@@ +1056,2 @@
>      } else {
> +	ecListSize = sizeof ECListAll;

ECListSuiteB and ECListAll are constants, not types.

Nelson likes to omit parentheses for sizeof when they are not necessary.
That style is prevalent in libSSL.  I can add the parentheses back if we
don't want to preserve that style.

@@ +1072,4 @@
>  }
>  
> +PRUint32
> +ssl3_GetSupportedECCurveMask(sslSocket *ss)

I changed the return value to PRUint32 because that's the type of the
consumer (ss->ssl3.hs.negotiatedECCurves).

I removed a 'C' from "ECCCurve". Actually even "ECCurve" is redundant
(like "PIN number" and "ATM machine"), but "ECCurve" is already commonly
used in NSS.

Comment 13

[Wan-Teh Chang]

Comment on attachment 742116 [details] [diff] [review]
Fix typos in comments and minor variable and function name issues

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

::: lib/ssl/derive.c
@@ +761,5 @@
>  
>  		if ( requiredECCbits > signatureKeyStrength ) 
>  		     requiredECCbits = signatureKeyStrength;
>  
> +		ec_curve = ssl3_GetCurveWithECKeyStrength(

The original code does not have indentation/alignment problem.
I am trying to reduce the vertical span of the code (from four lines
to three lines).

Comment 14

[Wan-Teh Chang]

Attachment: Add _BITS to the EC_MAX_KEY and EC_MIN_KEY macros

I checked in this macro renaming change first because the macros
are defined in a public header (blapit.h). (The macros are
arguably internal to the softoken though.) Ryan approved this
change in comment 11.

https://hg.mozilla.org/projects/nss/rev/a62f0d35d9aa

Comment 15

[Wan-Teh Chang]

Attachment: Fix typos in comments and minor variable and function name issues, v2

The remaining changes are all in nss/lib/ssl.

Please feel free to reject any coding style change.

Comment 16

[Robert Relyea]

Comment on attachment 743385 [details] [diff] [review]
Add _BITS to the EC_MAX_KEY and EC_MIN_KEY macros

r+

Comment 17

[Robert Relyea]

Comment on attachment 743391 [details] [diff] [review]
Fix typos in comments and minor variable and function name issues, v2

r- Most of these changes are innocuous, though some look like bike sheading. A few actually goes backwards, so the r-

Specifically:

ssl3_GetCurveWithECCCurveMask() -> ssl3_GetCurveWithECCurveMask() is an improvement. (Of course 'ECC curve' is redundant like 'DSA algorithm'. This type of accronym abuse is quite common and plainly understood. ECCurve is preferred, though).

suiteBECList -> ECListSuiteB  Yours is a bit of an improvement, though it should be ecListSuiteB since the variable is a static, not a global.

tlsECList -> ECListAll  I would prefer ecListTLSAll. These are far from all the define ECC curves, they are the (current) tls complete list.

ssl3_SuiteBOnly -> ssl3_SuiteBCurvesOnly  seems a bit like bike sheading, I'm ok with the new name, sort of, but 'Curves' is really redundant here. I'm not sure I understand the motivation to add it.

 /* look to see if we can handle certs less than 163 bits */ -> /* look to see if we can handle curves less than 256 bits */  This looks like a bad merge problem. I purposefully changed the 256 bits to 163 bits as part of the code change asked for by Ryan (which I agreed with). If this was intentional, please motivate it with a comment in the bug.

ECListSize, ECList -> ecListSize, ecList  These changes are good. The previous names were motivated by laziness on my part in modifying all the occurrences of ECList and company in this file.

In short, most of the changes are fine. A few a really don't agree with. A few I think are really unnecessary, but not necessarily harmful, a few I think a third choice is more appropriate.

bob

Comment 18

[Wan-Teh Chang]

Re: why I changed "ssl3_SuiteBOnly" to "ssl3_SuiteBCurvesOnly":

Suite B is more than ECC. Suite B also specifies the use of AES-128
and AES-256 for symmetric encryption and SHA-256 and SHA-384 for
hashing. So "ssl3_SuiteBOnly" could be interpreted to mean the SSL
socket is using an AES SHA-2 cipher suite.

Re: the "certs less than 163 bits" to "curves less than 256 bits" change:

The code in question is
    PK11_GetBestSlotWithAttributes(CKM_ECDH1_DERIVE, 0, 163, ...);

This call searches for a slot that supports an EC key size of 163 bits.
Therefore, "less than 163 bits" seems wrong. Do you agree?

Comment 19

[Wan-Teh Chang]

Attachment: Fix typos in comments and minor variable and function name issues, v3

Bob: please review the new patch. I omitted many coding style
fixes based on your review.

Re: ECPtFmt -> ecPtFmt: Nelson added the static constants
'ECList' and 'ECPtFmt' in this CVS revision:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/security/nss/lib/ssl&command=DIFF_FRAMESET&file=ssl3ecc.c&rev2=1.7&rev1=1.6

I believe Nelson used the capitalized names to suggest these
statics are const.  If we don't capitalize 'ECList' (which
became 'suiteBECList' and 'tlsECList'), we should not
capitalize 'ECPtFmt' either.

Comment 20

[Wan-Teh Chang]

Comment on attachment 743975 [details] [diff] [review]
Fix typos in comments and minor variable and function name issues, v3

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

::: lib/ssl/ssl3ecc.c
@@ +1020,5 @@
>   * curves. */
>  static PRBool
>  ssl3_SuiteBOnly(sslSocket *ss)
>  {
> +    /* look to see if we can handle curves less than 256 bits */

Please let me know if you still think "less than 256 bits" is wrong.
("256 bits" refers to the NIST P-256 curve in Suite B.)

Comment 21

[Robert Relyea]

Comment on attachment 743975 [details] [diff] [review]
Fix typos in comments and minor variable and function name issues, v3

r+

Though I would like the following change to the comment 163/256 comment:

/* see if we can support small curves (like 163). If not assume we can only
 * support Suite-B Curves (p256, p384, p521) */

I think the problem with both of our comments is there is a lot of outside context that both of us know, but the next person looking at the code would not. My new suggestion should supply the required context.

bob

Comment 22

[Wan-Teh Chang]

Comment on attachment 743975 [details] [diff] [review]
Fix typos in comments and minor variable and function name issues, v3

Since this bug is already closed, I moved this patch to bug 881427.