338367 - The GF2M_POPULATE and GFP_POPULATE should check the ecCurve_map array index bounds before use

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

In nss/lib/softoken/ecdecode.c, we have code like this:

     case SEC_OID_ANSIX962_EC_C2PNB163V1:
 	/* Populate params for c2pnb163v1 */
 	params->name = ECCurve_X9_62_CHAR2_PNB163V1;
	curveParams = ecCurve_map[params->name];
 	GF2M_POPULATE
 	break;

where GF2M_POPULATE is a macro defined as:

 #define GF2M_POPULATE \
 	if ((params->name < ECCurve_noName) || \
 	    (params->name > ECCurve_pastLastCurve)) goto cleanup; \
 	CHECK_OK(curveParams); \
 	...

So we first take an element in the ecCurve_map array
and then check that the array index params->name is
within bounds.  This order is wrong.

Similarly for the code that uses the GFP_POPULATE macro.

I'm also wondering if the GF2M_POPULATE and GFP_POPULATE
macros can be converted to functions.  The macros are each
21 lines long and are expanded 54 times combined.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

This patch moves the statement

    curveParams = ecCurve_map[params->name];

into the GF2M_POPULATE and GFP_POPULATE macros,
after their array index bound tests.

Comment 2

[Wan-Teh Chang]

I checked in the proposed patch on the NSS trunk (3.12).

Checking in ecdecode.c;
/cvsroot/mozilla/security/nss/lib/softoken/ecdecode.c,v  <--  ecdecode.c
new revision: 1.7; previous revision: 1.6
done

Comment 3

[Robert Relyea]

Comment on attachment 222430 [details] [diff] [review]
Proposed patch

r+=relyea

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Assigning to Wan-Teh, since he wrote the patch

Comment 5

[Wan-Teh Chang]

The remaining work is to investigate whether the
macros GF2M_POPULATE and GFP_POPULATE, which are
each 21 lines long and are expanded 54 times
combined, can be converted to functions.  But I
don't have time to work on that.

Comment 6

[Douglas Stebila]

Attachment: Change GF*_POPULATE macros to functions

(In reply to comment #5)
> The remaining work is to investigate whether the
> macros GF2M_POPULATE and GFP_POPULATE, which are
> each 21 lines long and are expanded 54 times
> combined, can be converted to functions.  But I
> don't have time to work on that.

The attached patch does this.

Comment 7

[Douglas Stebila]

Attachment: Change GF*_POPULATE macros to functions (unified diff)

Comment 8

[Wan-Teh Chang]

Comment on attachment 225923 [details] [diff] [review]
Change GF*_POPULATE macros to functions (unified diff)

r=wtc. Thank you for the patch, Douglas. I like this patch.
Please make the following suggested changes and submit a
new patch.

1. If any lines in gf2m_populate or gfp_populate are
longer than 80 characters, wrap the long lines.  Lines
in NSS source files should not be longer than 80 characters.

2. Consider combining gf2m_populate and gfp_populate
into a function that takes an additional "ECFieldType type"
parameter.  gf2m_populate and gfp_populate only differ
in two lines: the params->fieldID.type assignment and
the hexString2SECItem call that fills in
params->fieldID.u.{poly,prime}.  The new function can
handle params->fieldID.u{poly,prime} with an if statement
as EC_CopyParams does.

3. Remove the 'const' on the "ECCurveName name' parameter
of gf2m_populate and gfp_populate.  'const' is more useful
for the target of a pointer parameter.

4. We don't need to use the CHECK_OK macro on the strcat
calls in gf2m_populate and gfp_populate because the strcat
calls return genenc (the first argument), which cannot be
NULL.

5. In EC_FillParams, the only purpose of the big switch(tag)
statement is to tell us whether 'tag' is a binary curve or
prime curve.  So I suggest you rewrite the code like this:

    ECFieldType type;

    switch (tag) {
    /* Binary curves */
    case SEC_OID_ANSIX962_EC_C2PNB163V1:
    case SEC_OID_ANSIX962_EC_C2PNB163V2:
    ...
    case SEC_OID_SECG_EC_SECT571R1:
        type = ec_field_GF2m;
        break;

    /* Prime curves */
    case SEC_OID_ANSIX962_EC_PRIME192V1:
    case SEC_OID_ANSIX962_EC_PRIME192V2:
    ...
    case SEC_OID_SECG_EC_SECP521R1:
        type = ec_field_GF2m;
        break;

    default:
        goto cleanup;
    }

    CHECK_SEC_OK( gf_populate(type, tag, params) );

The above sample code assumes you combine gf2m_populate
and gfp_populate into the new function gf_populate.  If
you don't do that, you can call gf2m_populate or gfp_populate
directly from the switch(tag) statement.

6. Under the label 'cleanup', you can remove the
"return rv" statement in the if block and just let it
fall off.

Comment 9

[Wan-Teh Chang]

Comment on attachment 225923 [details] [diff] [review]
Change GF*_POPULATE macros to functions (unified diff)

Please ignore item 5 of my previous comment 8.
I misunderstood the purpose of that switch(tag)
statement.  It actually does three things:
- It lists the curves implemented by NSS.
- It maps SECOidTag to ECCurveName. So we can't
  group the cases (binary curve cases, prime
  curve cases) as I suggested.
- It determines if the tag is a binary curve or
  prime curve.

However, it may still be a good idea to just map
the OID tag to the curve name and type in the
switch statement, and call the (combined) gf_populate
(perhaps populate_params?) function after the switch
statement.

Comment 10

[Douglas Stebila]

Attachment: Change GF*_POPULATE macros to functions (unified diff)

With changes as request in comment #8 and #9.

Comment 11

[Wan-Teh Chang]

Comment on attachment 226023 [details] [diff] [review]
Change GF*_POPULATE macros to functions (unified diff)

>+    if (field_type == ec_field_GFp) 
>+    	{
>+	CHECK_OK(hexString2SECItem(params->arena, &params->fieldID.u.prime, 
>+	    curveParams->irr));
>+    	}
>+    else if (field_type == ec_field_GF2m)
>+	{
>+	CHECK_OK(hexString2SECItem(params->arena, &params->fieldID.u.poly, 
>+	    curveParams->irr));
>+	}

In NSS we allow two brace styles.

  if (...) {
      ...
  } else {
      ...
  }

and

  if (...)
  {
      ...
  }
  else
  {
      ...
  }

and we try to use a consistent style within each
file or library.  In lib/softoken/decode.c, you
should use the first style.

I would just use an "else" block to handle the
field_type == ec_field_GF2m case.  Or you can add
an "else" block that goto cleanup.

Comment 12

[Douglas Stebila]

Attachment: Change GF*_POPULATE macros to functions (unified diff)

Changes as suggested in comment #11.

Comment 13

[Wan-Teh Chang]

Comment on attachment 226073 [details] [diff] [review]
Change GF*_POPULATE macros to functions (unified diff)

Douglas, I found that the ecCurve_map array in ecl-curve.h
has the field type information, so we can simplify
gf_populate_params as follows:

static SECStatus
gf_populate_params(ECCurveName name, ECParams *params)
{
    ...
    params->fieldID.type = field_type;
    if (curveParams->field == ECField_GFp) {
	params->fieldID.type = ec_field_GFp;
	CHECK_OK(hexString2SECItem(params->arena, &params->fieldID.u.prime, 
	    curveParams->irr));
    } else {
	params->fieldID.type = ec_field_GF2m;
	CHECK_OK(hexString2SECItem(params->arena, &params->fieldID.u.poly, 
	    curveParams->irr));
    }

Given this, I think it is better to only do the SECOidTag
to ECCurveName mapping in the switch(tag) statement, and
call gf_populate_params after the switch(tag) statement.

    ECCurveName name;

    switch (tag) {
    /* Binary curves */
    case SEC_OID_ANSIX962_EC_C2PNB163V1:
        name = ECCurve_X9_62_CHAR2_PNB163V1;
        break;
    ...

    /* Prime curves */
    case SEC_OID_ANSIX962_EC_PRIME192V1:
        name = ECCurve_X9_62_PRIME_192V1;
        break;
    ...

    default:
        goto cleanup;
    }

    CHECK_SEC_OK( gf_populate_params(name, params) );

Comment 14

[Wan-Teh Chang]

In comment 13, I forgot to delete the statement
    params->fieldID.type = field_type;
from the sample code for gf_populate_params.
params->fieldID.type should only be set inside
the if-else statement.

Comment 15

[Douglas Stebila]

> cvs commit -m "Bugzilla Bug 338367: Turn GF2M_POPULATE and GFP_POPULATE macros into functions. r+=wtchang" ecdecode.c
 
Checking in ecdecode.c;
/cvsroot/mozilla/security/nss/lib/softoken/ecdecode.c,v  <--  ecdecode.c
new revision: 1.8; previous revision: 1.7
done