Last Comment Bug 351140 - Coverity 995, potential crash in ecgroup_fromNameAndHex
: Coverity 995, potential crash in ecgroup_fromNameAndHex
Status: RESOLVED FIXED
[CID 995]
: coverity
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11
: All All
: P3 normal (vote)
: 3.12
Assigned To: Ryan Jones
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-02 03:56 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2006-10-23 17:07 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1. (728 bytes, patch)
2006-10-18 11:02 PDT, Ryan Jones
nelson: review+
wtc: superreview+
Details | Diff | Splinter Review
Alternative patch (854 bytes, patch)
2006-10-23 15:41 PDT, Wan-Teh Chang
nelson: review+
Details | Diff | Splinter Review
Combined patch (932 bytes, patch)
2006-10-23 17:06 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2006-09-02 03:56:05 PDT
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 2006-10-18 11:02:21 PDT
Created attachment 242658 [details] [diff] [review]
Patch v1.

Patch v1.

Done as suggested in comment #0.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2006-10-18 13:06:56 PDT
Comment on attachment 242658 [details] [diff] [review]
Patch v1.

r=nelson for trunk
Comment 3 Wan-Teh Chang 2006-10-23 15:39:13 PDT
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 2006-10-23 15:41:40 PDT
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.
Comment 5 Nelson Bolyard (seldom reads bugmail) 2006-10-23 15:49:20 PDT
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 2006-10-23 17:06:07 PDT
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

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