Last Comment Bug 310145 - another stack overflow in mpp_make_prime
: another stack overflow in mpp_make_prime
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.9.5
: Sun Solaris
: P2 normal (vote)
: 3.11.1
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-26 22:44 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2006-04-05 21:18 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
use heap on all platforms (checked in) (1.41 KB, patch)
2005-09-26 22:51 PDT, Nelson Bolyard (seldom reads bugmail)
wtc: review+
neil.williams: review+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2005-09-26 22:44:05 PDT
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) 2005-09-26 22:51:56 PDT
Created attachment 197517 [details] [diff] [review]
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 2005-09-27 12:10:26 PDT
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) 2006-03-09 13:17:01 PST
Put this back on my radar.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2006-03-18 21:03:59 PST
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) 2006-03-18 21:10:53 PST
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 2006-03-20 10:51:03 PST
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) 2006-04-04 00:24:12 PDT
Wan-Teh, If I check in this fix, does any FIPS testing need to be redone?
Comment 8 Wan-Teh Chang 2006-04-04 14:06:13 PDT
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) 2006-04-04 14:34:53 PDT
Does that mean we haven't yet tested RSA keygen ?
Comment 10 Wan-Teh Chang 2006-04-04 14:56:35 PDT
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) 2006-04-05 21:17:56 PDT
Committed on NSS 3.11 branch
Checking in mpi/mpprime.c;  new revision: 1.21.30.1; previous revision: 1.21

Note You need to log in before you can comment on or make changes to this bug.