Last Comment Bug 338367 - The GF2M_POPULATE and GFP_POPULATE should check the ecCurve_map array index bounds before use
: The GF2M_POPULATE and GFP_POPULATE should check the ecCurve_map array index b...
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11
: All All
: P2 minor (vote)
: 3.12
Assigned To: Wan-Teh Chang
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-17 15:57 PDT by Wan-Teh Chang
Modified: 2006-07-18 17:15 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (10.74 KB, patch)
2006-05-17 16:07 PDT, Wan-Teh Chang
douglas: review+
rrelyea: superreview+
Details | Diff | Review
Change GF*_POPULATE macros to functions (12.74 KB, patch)
2006-06-16 13:22 PDT, Douglas Stebila
no flags Details | Diff | Review
Change GF*_POPULATE macros to functions (unified diff) (17.25 KB, patch)
2006-06-16 14:46 PDT, Douglas Stebila
wtc: review+
Details | Diff | Review
Change GF*_POPULATE macros to functions (unified diff) (17.61 KB, patch)
2006-06-17 15:59 PDT, Douglas Stebila
no flags Details | Diff | Review
Change GF*_POPULATE macros to functions (unified diff) (17.57 KB, patch)
2006-06-18 13:35 PDT, Douglas Stebila
wtc: review+
Details | Diff | Review

Description Wan-Teh Chang 2006-05-17 15:57:36 PDT
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 2006-05-17 16:07:59 PDT
Created attachment 222430 [details] [diff] [review]
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 2006-05-17 16:59:11 PDT
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 2006-05-19 06:06:53 PDT
Comment on attachment 222430 [details] [diff] [review]
Proposed patch

r+=relyea
Comment 4 Nelson Bolyard (seldom reads bugmail) 2006-06-14 22:48:41 PDT
Assigning to Wan-Teh, since he wrote the patch
Comment 5 Wan-Teh Chang 2006-06-15 19:08:32 PDT
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 2006-06-16 13:22:13 PDT
Created attachment 225907 [details] [diff] [review]
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 2006-06-16 14:46:20 PDT
Created attachment 225923 [details] [diff] [review]
Change GF*_POPULATE macros to functions (unified diff)
Comment 8 Wan-Teh Chang 2006-06-17 07:10:46 PDT
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 2006-06-17 08:59:07 PDT
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 2006-06-17 15:59:06 PDT
Created attachment 226023 [details] [diff] [review]
Change GF*_POPULATE macros to functions (unified diff)

With changes as request in comment #8 and #9.
Comment 11 Wan-Teh Chang 2006-06-17 21:07:06 PDT
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 2006-06-18 13:35:04 PDT
Created attachment 226073 [details] [diff] [review]
Change GF*_POPULATE macros to functions (unified diff)

Changes as suggested in comment #11.
Comment 13 Wan-Teh Chang 2006-06-19 10:15:24 PDT
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 2006-06-19 10:17:42 PDT
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 2006-07-18 17:15:49 PDT
> 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

Note You need to log in before you can comment on or make changes to this bug.