1286087 - Adds NIST known answer tests

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Watson Ladd]

Attachment: Adds known answer tests

This adds known answer tests from NIST to ecperf.

Comment 1

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8769922 [details] [diff] [review]
Adds known answer tests

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

In general great to have those test cases. But I think this is the wrong file for them. This should be a performance test (even if we don't check the actual performance on CI here yet).
Please add a new test case (something like ectest) and move this code over to the new test. You can look at the patch I sent you yesterday to see how to do that.

::: cmd/ecperf/ecperf.c
@@ -292,4 @@
>          return NULL;
>      }
>  
> -    /* skip leading 00's unless the hex string is "00" */

do the other tests still run with this change?

@@ +577,5 @@
> +    arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
> +    if (!arena) {
> +        return SECFailure;
> +    }
> +    

nit: whitespaces (more below)

::: cmd/ecperf/testvecs.h
@@ +1,1 @@
> +static KAT ecdh_testvecs[] = {

those numbers are pretty long. Can you break them to make it look a bit nicer and make them easier to read?

Comment 2

[Watson Ladd]

Attachment: Patch which includes test integration, different command

Comment 3

[Watson Ladd]

https://codereview.appspot.com/300710043/

Comment 4

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8770247 [details] [diff] [review]
Patch which includes test integration, different command

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

lgtm. Can you upload a new version with those nits fixed?

::: cmd/ectest/Makefile
@@ +1,2 @@
> +#! gmake
> +# 

nit: ws

::: cmd/ectest/ectest.c
@@ +26,5 @@
> + * Initializes a SECItem from a hexadecimal string
> + *
> + */
> +static SECItem *
> +hexString2SECItem(PLArenaPool *arena, SECItem *item, const char *str)

we should really put this into util at some point...

@@ +144,5 @@
> +    PORT_FreeArena(ecPriv->ecParams.arena, PR_FALSE);
> +    return rv;
> +}
> +
> +/* Performs tests of elliptic curve cryptography over prime fields If

bit: ... fields. If ...

::: cmd/ectest/manifest.mn
@@ +12,5 @@
> +INCLUDES += -I$(CORE_DEPTH)/nss/lib/softoken
> +
> +# This next line is used by .mk files
> +# and gets translated into $LINCS in manifest.mnw
> +REQUIRES = dbm seccmd

do we really need those? at least dbm is something we probably don't need.

::: tests/ec/ectest.sh
@@ +42,5 @@
> +
> +ectest_init
> +ECTEST_OUT=`ectest`
> +ECTEST_OUT=`echo $ECTEST_OUT | grep -i "not okay"`
> +# TODO: expose individual tests and failures instead of overall

yeah, that would be great to identify failures. but I'd be ok with keeping it this way for now.

Comment 5

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8770247 [details] [diff] [review]
Patch which includes test integration, different command

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

::: cmd/ectest/ectest.c
@@ +163,5 @@
> +    while (ecdh_testvecs[numkats].curve != ECCurve_pastLastCurve) {
> +        numkats++;
> +    }
> +    printf("1..%d\n", numkats);
> +    for (int i = 0; ecdh_testvecs[i].curve != ECCurve_pastLastCurve; i++) {

oh, I missed this. This initialisation doesn't work everywhere.

Comment 6

[Watson Ladd]

Attachment: Fixes nits from r+ review

Comment 7

[Watson Ladd]

Attachment: Fixes an ASAN leak

Comment 8

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/20cf849a52fc