310145 - another stack overflow in mpp_make_prime

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Bugzilla Bug 115012, filed in December 2001, reported a stack overflow 
trying to generate a prime number for an RSA key generation on OS/2.
The stack size on OS/2 was only 32KB, and mpp_make_prime allocates a
32KB array from the stack.  We changed that function to use malloc on OS/2.  

Now we have reports of the same crash occuring in Java-based servers
that use NSS via JSS, running on Solaris. 

I will change this function to use the heap instead of the stack on 
all platforms.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: use heap on all platforms (checked in)

I won't ask for review until I've done more testing.
Should this be targetted at 3.10.x ?

Comment 2

[Wan-Teh Chang]

Comment on attachment 197517 [details] [diff] [review]
use heap on all platforms (checked in)

Looking at this code carefully, I found two
possible improvements.

1. Since sieve is not a function argument, it
looks bad to use ARGCHK to handle the memory
allocation failure of sieve.  It looks nicer
to write the code like this:

   unsigned char *sieve;

   ARGCHK(start != 0, MP_BADARG);
   ARGCHK(nBits > 16, MP_RANGE);

   sieve = malloc(SIEVE_SIZE);
   if (sieve == NULL)
     return MP_MEM;

2. The zeroization of sieve before freeing may not
be necessary because we don't do that if sieve is
a stack variable.  I don't know what sieve contains,
so Nelson please look into this.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Put this back on my radar.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 197517 [details] [diff] [review]
use heap on all platforms (checked in)

Requesting second review, for 3.11 branch.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 197517 [details] [diff] [review]
use heap on all platforms (checked in)

committed on trunk.
mpi/mpprime.c;  new revision: 1.22; previous revision: 1.21

Comment 6

[Neil Williams]

Comment on attachment 197517 [details] [diff] [review]
use heap on all platforms (checked in)

Patch looks good. I noticed in reviewing it that MP_CHECKOK makes implicit reference to a variable res which seems like a questionable practice. Patch is good though.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, If I check in this fix, does any FIPS testing need to be redone?

Comment 8

[Wan-Teh Chang]

No, the function modified by this patch, mpp_make_prime,
isn't used by any of the FIPS algorithm tests we have done
so far.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Does that mean we haven't yet tested RSA keygen ?

Comment 10

[Wan-Teh Chang]

We have done and received FIPS certificates for the
following tests: AES, Triple DES, SHS, and HMAC.

We have done the following tests but haven't applied
for the certificates: RNG.

We have not done the following tests: DSA, RSA, ECDSA.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Committed on NSS 3.11 branch
Checking in mpi/mpprime.c;  new revision: 1.21.30.1; previous revision: 1.21