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

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

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.

Comment 1

[Wan-Teh Chang]

Attachment: Off-by-one error in duplicating a string

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.

Comment 2

[Wan-Teh Chang]

Attachment: Off-by-one error in the NAF buffer

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.

Comment 3

[Wan-Teh Chang]

Attachment: Off-by-one error in the NAF buffer, alternative patch

If the 'naf' array should have orderBitSize elements,
then this is the right fix.

Comment 4

[Wan-Teh Chang]

Attachment: Don't include BSD Unix header <strings.h>

<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.

Comment 5

[Vipul Gupta]

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.

Comment 6

[Douglas Stebila]

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.

Comment 7

[Douglas Stebila]

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.

Comment 8

[Wan-Teh Chang]

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.