Closed Bug 338367 Opened 18 years ago Closed 18 years ago

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

Categories

(NSS :: Libraries, defect, P2)

3.11

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: wtc)

Details

Attachments

(2 files, 3 obsolete files)

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.
Attached patch Proposed patchSplinter Review
This patch moves the statement

    curveParams = ecCurve_map[params->name];

into the GF2M_POPULATE and GFP_POPULATE macros,
after their array index bound tests.
Attachment #222430 - Flags: superreview?(rrelyea)
Attachment #222430 - Flags: review?(douglas)
Attachment #222430 - Flags: review?(douglas) → review+
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
Severity: normal → minor
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 3.12
Comment on attachment 222430 [details] [diff] [review]
Proposed patch

r+=relyea
Attachment #222430 - Flags: superreview?(rrelyea) → superreview+
Assigning to Wan-Teh, since he wrote the patch
Assignee: nobody → wtchang
Status: ASSIGNED → NEW
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.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
(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.
Attachment #225907 - Flags: review?(wtchang)
Attachment #225907 - Attachment is obsolete: true
Attachment #225923 - Flags: review?(wtchang)
Attachment #225907 - Flags: review?(wtchang)
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.
Attachment #225923 - Flags: review?(wtchang) → review+
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.
With changes as request in comment #8 and #9.
Attachment #225923 - Attachment is obsolete: true
Attachment #226023 - Flags: review?(wtchang)
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.
Changes as suggested in comment #11.
Attachment #226023 - Attachment is obsolete: true
Attachment #226073 - Flags: review?(wtchang)
Attachment #226023 - Flags: review?(wtchang)
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) );
Attachment #226073 - Flags: review?(wtchang) → review+
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.
> 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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: