351140 - Coverity 995, potential crash in ecgroup_fromNameAndHex

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

In function ecgroup_fromNameAndHex, there is a large if/then/else/if/then 
statement that deals with two cases, params->field == ECField_GFp and
params->field == ECField_GF2m.  There is no final "else" case to deal with
the possibility that params->field is not a known value.  

If params->field is not a known value, and params->text != NULL, 
this code will crash, trying to assign the return value from strdup
to group->text, but group will be NULL.

This code has been changed in the last month to fix bug 303116.  
The change did not fix this problem.

I think the fix is this tiny patch:

	/* set name, if any */
-	if (params->text != NULL) {
+	if (group && params->text != NULL) {

I think this can only happen due to a programming error in NSS.
And in the absense of any proof of such a programming error,
I think this fix can wait, potentially a long time, if necessary.

Comment 1

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.

Patch v1.

Done as suggested in comment #0.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 242658 [details] [diff] [review]
Patch v1.

r=nelson for trunk

Comment 3

[Wan-Teh Chang]

Comment on attachment 242658 [details] [diff] [review]
Patch v1.

r=wtc.  This patch will allow the function
to return NULL without crashing.

This function doesn't set any error code right
now.  If we want to set the error code in the
future, we need a patch like what I'm going to
attach next.

Comment 4

[Wan-Teh Chang]

Attachment: Alternative patch

This patch sets 'res' to MP_UNDEF, which could be mapped
to an NSS error code in the future.  With the current code,
'res' could be MP_OKAY if params->field is an invalid value
and everything else in 'params' is good.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 243258 [details] [diff] [review]
Alternative patch

I vote for committing both of these patches, for belt AND suspenders.

Comment 6

[Wan-Teh Chang]

Attachment: Combined patch

With the alternative patch, you can prove that 'group' cannot
be NULL at that point.  Won't Coverity complain about the
unnecessary group != NULL test?  In any case, I checked this in
on the NSS trunk (NSS 3.12).

Checking in ecl.c;
/cvsroot/mozilla/security/nss/lib/freebl/ecl/ecl.c,v  <--  ecl.c
new revision: 1.12; previous revision: 1.11
done