Note: There are a few cases of duplicates in user autocompletion which are being worked on.

another stack overflow in mpp_make_prime

RESOLVED FIXED in 3.11.1

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

3.9.5
3.11.1
Sun
Solaris

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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

Comment 1

12 years ago
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

12 years ago
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.
Attachment #197517 - Flags: review+
(Assignee)

Comment 3

12 years ago
Put this back on my radar.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 3.11.1
(Assignee)

Comment 4

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

Requesting second review, for 3.11 branch.
Attachment #197517 - Flags: review?(neil.williams)
(Assignee)

Comment 5

12 years ago
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

12 years ago
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.
Attachment #197517 - Flags: review?(neil.williams) → review+
(Assignee)

Updated

12 years ago
QA Contact: jason.m.reid → libraries
(Assignee)

Comment 7

12 years ago
Wan-Teh, If I check in this fix, does any FIPS testing need to be redone?
(Assignee)

Updated

12 years ago
Attachment #197517 - Attachment description: use heap on all platforms → use heap on all platforms (checked in on trunk only)

Comment 8

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

Comment 9

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

Comment 10

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

Comment 11

12 years ago
Committed on NSS 3.11 branch
Checking in mpi/mpprime.c;  new revision: 1.21.30.1; previous revision: 1.21
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #197517 - Attachment description: use heap on all platforms (checked in on trunk only) → use heap on all platforms (checked in)
You need to log in before you can comment on or make changes to this bug.