319252 - NSS crashes when generating NIST P512 EC keys.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Robert Relyea]

NSS will crash if you try to generate a keypair on the NIST P512 Eliptic Curve. The crash happens in VerifyKeyPair when trying to Verify a signature.

The problem happens ec_pts_mul_simul_w2 (which is used by p512 curves but not smaller pXXX curves). In ec_pts_mul_simul_w2, the code creates a bunch of mp_int structures with fixed mp_digit values from a stack array. For most operations, these mp_int structures never have to grow, so everything silently works. When we Verify the signature, however, one of these values need to grow from 20 mp_digits to 64 mp_digits, causing the code to allocate a fresh buffer and free the old buffer. Free panics while freeing stack memory.

I have a patch which fixes the problem by using the standard mp_int initialization. Other fixes could include not using simul_w2 for this curve (I don't know what other curves use this function, they could also run into problems).

bob

Comment 1

[Robert Relyea]

Attachment: Use standard mpi_initialization on precomputed array

Here's my potential patch. Note this adds several mallocs and frees to this code path.

Comment 2

[Vipul Gupta]

Douglas is the right person to review this patch.

vipul

(In reply to comment #1)
> Created an attachment (id=205103) [edit]
> Use standard mpi_initialization on precomputed array
> 
> Here's my potential patch. Note this adds several mallocs and frees to this
> code path.
> 

Comment 3

[Wan-Teh Chang]

Comment on attachment 205103 [details] [diff] [review]
Use standard mpi_initialization on precomputed array

The change to ecl.h should be removed.

There is an unnecessary whitespace change in ecl_mult.c.

But the real problem is that we need to first use
MP_DIGITS to initialize these mp_int's.  Otherwise
if any of the mp_init_size calls fails and we go to
CLEANUP, we may call mp_clear on an uninitialized
mp_int.

See dsa.c for an example of MP_DIGITS followed by
mp_init.

Comment 4

[Wan-Teh Chang]

Comment on attachment 205103 [details] [diff] [review]
Use standard mpi_initialization on precomputed array

Bob, we need to add the following nested for loop before
the existing nested for loop:

>+	for (i = 0; i < 4; i++) {
>+		for (j = 0; j < 4; j++) {
>+			 MP_DIGITS(&precomp[i][j][0]) = 0;
>+			 MP_DIGITS(&precomp[i][j][1]) = 0;
> 		}
> 	}

This will initialize all the mp_int's so that we can call
mp_clear on them safely.  Unfortunately we can't combine
the two nested for loops into one.

Comment 5

[Robert Relyea]

Comment on attachment 205103 [details] [diff] [review]
Use standard mpi_initialization on precomputed array

I'd like douglas's review as well.

Comment 6

[Robert Relyea]

BTW ecl.h should not be part of the patch.

bob

Comment 7

[Douglas Stebila]

Comment on attachment 205103 [details] [diff] [review]
Use standard mpi_initialization on precomputed array

Fine with me, assuming ecl.h is not touched (comment #6) and the whitespace change is removed (comment #3)

Comment 8

[Robert Relyea]

checked into trunk (3.12)
Checking in ecl_mult.c;
/cvsroot/mozilla/security/nss/lib/freebl/ecl/ecl_mult.c,v  <--  ecl_mult.c
new revision: 1.4; previous revision: 1.3
done
 I've set the target to 3.12, I'm presuming we do not want this patch in 3.11,
so I'm closing it as well.

Comment 9

[Wan-Teh Chang]

Douglas, the same technique is used in lib/freebl/ecl/ec2_proj.c,
function ec_GF2m_pt_mul_proj.  Should we fix that, too?

I don't understand the code, so this may be a stupid question:
does this bug mean the value of ECL_MAX_FIELD_SIZE_DIGITS is too
small?  If so, can we fix this bug by making ECL_MAX_FIELD_SIZE_DIGITS
bigger?

Comment 10

[Robert Relyea]

Yes, it means the value is too small for certain MPI operations. Knowing how big that value should be requires knowlege about how MPI works, which may change in the future.

Comment 11

[Robert Relyea]

Setting target to 3.11, requires 2 reviews.

Comment 12

[Wan-Teh Chang]

Attachment: Use standard mpi_initialization on precomputed array (as checked in)

This is what Bob actually checked in on the NSS trunk (3.12).
r=wtc.  Douglas gave his r+ to the previous version of
this patch.

I checked in this patch on the NSS_3_11_BRANCH (3.11.1).

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

Comment 13

[Wan-Teh Chang]

*** Bug 325495 has been marked as a duplicate of this bug. ***