Coverity 995, potential crash in ecgroup_fromNameAndHex

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P3
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Ryan Jones)

Tracking

({coverity})

3.11
3.12
coverity

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CID 995], URL)

Attachments

(1 attachment, 2 obsolete attachments)

932 bytes, patch
Details | Diff | Splinter Review
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.
(Reporter)

Updated

11 years ago
Priority: -- → P3
Whiteboard: [CID 995]
(Assignee)

Comment 1

11 years ago
Created attachment 242658 [details] [diff] [review]
Patch v1.

Patch v1.

Done as suggested in comment #0.
Assignee: nobody → sciguyryan+bugzilla
Status: NEW → ASSIGNED
Attachment #242658 - Flags: superreview?(wtchang)
Attachment #242658 - Flags: review?(nelson)
(Reporter)

Comment 2

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

r=nelson for trunk
Attachment #242658 - Flags: review?(nelson) → review+

Comment 3

11 years ago
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.
Attachment #242658 - Flags: superreview?(wtchang) → superreview+

Comment 4

11 years ago
Created attachment 243258 [details] [diff] [review]
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.
Attachment #243258 - Flags: review?(nelson)
(Reporter)

Comment 5

11 years ago
Comment on attachment 243258 [details] [diff] [review]
Alternative patch

I vote for committing both of these patches, for belt AND suspenders.
Attachment #243258 - Flags: review?(nelson) → review+

Comment 6

11 years ago
Created attachment 243272 [details] [diff] [review]
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
Attachment #242658 - Attachment is obsolete: true
Attachment #243258 - Attachment is obsolete: true

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.