1303648 - Only enable EC groups that are supported by the token

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Martin Thomson [:mt:]]

We discovered that the only part of softtoken that knows about keys is the bit that imports a key.  Therefore, if we import a key of a specific type, then it will succeed if the group is supported and fail otherwise.  We can use this to detect what groups are available to us.

Unfortunately, this is a little involved and probably slow, so it's probably best to only do this once.

This can be replaced by something less kludgy if PKCS#11 ever gets a better way to enumerate support for named groups.

Comment 1

[Martin Thomson [:mt:]]

Attachment: bug1303648-2.patch

Not sure why we are getting some of the curves show up as being present when they shouldn't be.  Investigation required, but that shouldn't stop this from getting a first pass.

Comment 2

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8792461 [details] [diff] [review]
bug1303648-2.patch

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

lgtm

::: lib/ssl/filterec.c
@@ +9,5 @@
> +#include "pk11func.h"
> +#include "ssl.h"
> +#include "sslimpl.h"
> +
> +/* This table contains valid public keys for each of the support EC groups we

... supported EC groups we support.

@@ +17,5 @@
> +    SSLNamedGroup name;
> +    const PRUint8 data[145];
> +    unsigned int len;
> +} ssl_test_public_keys[SSL_NAMED_GROUP_COUNT] = {
> +    { ssl_grp_ec_secp256r1,

add 25519

::: lib/ssl/ssl3ecc.c
@@ +64,5 @@
>  
> +    PORT_Assert(params);
> +    PORT_Assert(ecGroup);
> +    PORT_Assert(ecGroup->type == group_type_ec);
> +    if ((oidData = SECOID_FindOIDByTag(ecGroup->oidTag)) == NULL) {

I think we should keep those null checks. opt builds can run in a null ecGroup here otherwise.

Comment 3

[Martin Thomson [:mt:]]

Reviewed here: https://nss-dev.phacility.com/D6

https://hg.mozilla.org/projects/nss/rev/0f1a09d967b3

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8792461 [details] [diff] [review]
bug1303648-2.patch

Overtaken by events.  This isn't that great.