NSS crashes when generating NIST P512 EC keys.

RESOLVED FIXED in 3.11.1

Status

P1
normal
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: rrelyea, Assigned: wtc)

Tracking

3.9.7
3.11.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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
(Reporter)

Comment 1

13 years ago
Created attachment 205103 [details] [diff] [review]
Use standard mpi_initialization on precomputed array

Here's my potential patch. Note this adds several mallocs and frees to this code path.
Attachment #205103 - Flags: review?(vipul.gupta)

Comment 2

13 years ago
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.
> 
(Assignee)

Comment 3

13 years ago
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.
Attachment #205103 - Flags: review?(vipul.gupta) → review-
(Assignee)

Comment 4

13 years ago
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.
(Reporter)

Updated

13 years ago
Attachment #205103 - Flags: review- → review?(wtchang)
(Reporter)

Comment 5

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

I'd like douglas's review as well.
Attachment #205103 - Flags: superreview?(douglas)
(Reporter)

Comment 6

13 years ago
BTW ecl.h should not be part of the patch.

bob

Comment 7

13 years ago
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)
Attachment #205103 - Flags: superreview?(douglas) → superreview+
(Reporter)

Comment 8

13 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12
(Assignee)

Comment 9

13 years ago
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?
(Reporter)

Comment 10

13 years ago
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.
(Reporter)

Comment 11

13 years ago
Setting target to 3.11, requires 2 reviews.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 3.12 → 3.11.1
(Assignee)

Comment 12

13 years ago
Created attachment 206147 [details] [diff] [review]
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
Attachment #205103 - Attachment is obsolete: true
Attachment #206147 - Flags: review+
Attachment #205103 - Flags: review?(wtchang)
(Assignee)

Updated

13 years ago
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Priority: -- → P1
Resolution: --- → FIXED
(Assignee)

Comment 13

13 years ago
*** Bug 325495 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.