Closed Bug 303116 Opened 15 years ago Closed 15 years ago

Two off-by-one errors in lib/freebl/ecl

Categories

(NSS :: Libraries, defect)

3.10
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: wtc)

Details

Attachments

(4 files)

I found two off-by-one errors in our ECC code in
mozilla/security/nss/lib/freebl/ecl.  I will attach
patches to show where they are.
Here is a common bug that the duplicate string buffer
doesn't have room for the terminating null byte.  The
code also may call strcpy on a NULL group->text buffer.

I fixed it by using the strdup function.
Attachment #191368 - Flags: review?(vipul.gupta)
I don't know how to fix this off-by-one error.
The correct fix depends on whether the 'naf' array
should have orderBitSize or orderBitSize+1 elements.

The code in ec_GFp_pt_mul_jm_wNAF and ec_compute_wNAF
uses the 'naf' array with indices 0...orderBitSize, which
is why I guess the 'naf' array should have orderBitSize+1
elements.  But it is also possible that the code in those
two functions should use the 'naf' array with indices
0...orderBitSize-1 (most C arrays are used this way), in
which case the array would have orderBitSize elements.
Vipul, Douglas, please advise.
Attachment #191369 - Flags: review?(vipul.gupta)
If the 'naf' array should have orderBitSize elements,
then this is the right fix.
<strings.h> includes <string.h> and declares additional
BSD Unix functions: bcmp, bcopy, bzero, index, rindex, ffs,
strcasecmp, and strncasecmp.

ecl_mult.c doesn't use any of these BSD Unix functions and
doesn't even use any of the str* and mem* functions declared
in <string.h>.	So the <strings.h> include can simply be removed.

This patch is required to build with ECC on Windows.
Attachment #191375 - Flags: review?(rrelyea)
Douglas will start another internship at Sun on Mon, Aug 8
and he'd be the best person to answer questions about the
code in the ecl subdirectory.

vipul

(In reply to comment #3)
> Created an attachment (id=191371) [edit]
> Off-by-one error in the NAF buffer, alternative patch
> 
> If the 'naf' array should have orderBitSize elements,
> then this is the right fix.
Attachment #191368 - Flags: review?(vipul.gupta) → review?(mozilla.org)
Attachment #191369 - Flags: review?(vipul.gupta) → review?(mozilla.org)
Attachment #191368 - Flags: review?(mozilla.org) → review+
Attachment #191375 - Flags: review?(rrelyea) → review+
Comment on attachment 191371 [details] [diff] [review]
Off-by-one error in the NAF buffer, alternative patch

Patch causes ecp_test to fail.	The values of point multiplication are changed
by the changing of the size of the for loop in the patch to ecp_jm.c giving
incorrect results.
Attachment #191371 - Flags: review-
Comment on attachment 191369 [details] [diff] [review]
Off-by-one error in the NAF buffer

Appears to correct the off-by-one error.  Tests in .../freebl/ecl/tests passed.
 This patch should be used instead of the alternate patch #191371.
Attachment #191369 - Flags: review?(douglas) → review+
Thanks a lot for the code review, Douglas and Bob.
I checked in the three reviewed patches on the NSS
trunk for NSS 3.11.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.11
You need to log in before you can comment on or make changes to this bug.