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)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12
People
(Reporter: wtc, Assigned: wtc)
Details
Attachments
(2 files, 3 obsolete files)
10.74 KB,
patch
|
douglas
:
review+
rrelyea
:
superreview+
|
Details | Diff | Splinter Review |
17.57 KB,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
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)
Updated•18 years ago
|
Attachment #222430 -
Flags: review?(douglas) → review+
Assignee | ||
Comment 2•18 years ago
|
||
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 3•18 years ago
|
||
Comment on attachment 222430 [details] [diff] [review] Proposed patch r+=relyea
Attachment #222430 -
Flags: superreview?(rrelyea) → superreview+
Comment 4•18 years ago
|
||
Assigning to Wan-Teh, since he wrote the patch
Assignee: nobody → wtchang
Status: ASSIGNED → NEW
Assignee | ||
Comment 5•18 years ago
|
||
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
Comment 6•18 years ago
|
||
(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)
Comment 7•18 years ago
|
||
Attachment #225907 -
Attachment is obsolete: true
Attachment #225923 -
Flags: review?(wtchang)
Attachment #225907 -
Flags: review?(wtchang)
Assignee | ||
Comment 8•18 years ago
|
||
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+
Assignee | ||
Comment 9•18 years ago
|
||
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•18 years ago
|
||
With changes as request in comment #8 and #9.
Attachment #225923 -
Attachment is obsolete: true
Attachment #226023 -
Flags: review?(wtchang)
Assignee | ||
Comment 11•18 years ago
|
||
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, ¶ms->fieldID.u.prime, >+ curveParams->irr)); >+ } >+ else if (field_type == ec_field_GF2m) >+ { >+ CHECK_OK(hexString2SECItem(params->arena, ¶ms->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•18 years ago
|
||
Changes as suggested in comment #11.
Attachment #226023 -
Attachment is obsolete: true
Attachment #226073 -
Flags: review?(wtchang)
Attachment #226023 -
Flags: review?(wtchang)
Assignee | ||
Comment 13•18 years ago
|
||
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, ¶ms->fieldID.u.prime, curveParams->irr)); } else { params->fieldID.type = ec_field_GF2m; CHECK_OK(hexString2SECItem(params->arena, ¶ms->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+
Assignee | ||
Comment 14•18 years ago
|
||
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•18 years ago
|
||
> 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.
Description
•