361089 - memory leak in mp_bdivmod

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

Memory leak testing revealed a leak from this stack.

[1] calloc() at 0xa152f633
[2] s_mp_alloc() at line 2877 in "mpi.c"
[3] mp_init_copy() at line 178 in "mpi.c"
[4] mp_bdivmod() at line 499 in "mp_gf2m.c"
[5] ec_GF2m_div() at line 1030 in "ecl_gf.c"
[6] ec_GF2m_pt_add_aff() at line 101 in "ec2_aff.c"
[7] ec_pts_mul_basic() at line 138 in "ecl_mult.c"
[8] ECPoints_mul() at line 347 in "ecl_mult.c"
[9] ec_points_mul() at line 184 in "ec.c"
[10] ECDSA_VerifyDigest() at line 999 in "ec.c"
[11] ECDSA_VerifyDigest() at line 1388 in "loader.c"
[12] sftk_fips_ECDSA_Test() at line 1607 in "fipstest.c"
[13] sftk_fips_ECDSA_PowerUpSelfTest() at line 1672 in "fipstest.c"
[14] sftk_fipsPowerUpSelfTest() at line 1983 in "fipstest.c"
[15] FC_Initialize() at line 465 in "fipstokn.c"
[16] secmod_ModuleInit() at line 150 in "pk11load.c" 

mp_bdivmod leaks its local bignum variables every time it is called. 

But it seems that function only gets called once, and then only in 
FIPS mode tests, where it gets called in the FIPS power up self test.

This function is the general divide function for binary polynomial curves.
Are there any FIPS approved binary polynomial curves?  
I thought all the FIPS curves were the so-called "prime" curves.
If that is correct, if FIPS curves are all prime curves, then why is the
FIPS mode self test doing binary polynomial division?

If this function is used as rarely as it appears to be, if this function
really is not called in the course of normal TLS ECC processing, then this
leak is not high priority, and can be treated as a "one time" leak.  
OTOH, if this function gets called once per some common softoken ECC 
function, then it's worthy of higher priority.

Comment 1

[Wan-Teh Chang]

Ten out of the fifteen NIST-recommended curves are over binary fields.
Our softoken's power-up self-tests only test NIST-recommended curves
(P-256 and K-283).  Curve K-283 is over a binary field.

Comment 2

[Wan-Teh Chang]

Attachment: Proposed patch

Douglas, Bob, please review this patch.

Christophe, could you test this patch in dbx?

I reviewed all the mp_init_copy calls in NSS and the whole mp_gf2m.c
file to make sure there are no similar memory leaks.

Douglas, if you suspect any other file that might have similar memory
leaks, please let me know.  I'll review them.

Comment 3

[Christophe Ravel]

I have tested the patch with an extended ECC build for memory leaks with dbx. There are no memory leak in mp_bdivmod anymore.
To confirm the result, I ran the same test without the patch and the memory leaks are there.

Comment 4

[Douglas Stebila]

(In reply to comment #0)
> If this function is used as rarely as it appears to be, if this function
> really is not called in the course of normal TLS ECC processing, then this
> leak is not high priority, and can be treated as a "one time" leak.  
> OTOH, if this function gets called once per some common softoken ECC 
> function, then it's worthy of higher priority.

While it's true that binary curves are not (presently) widely deployed compared to prime curves, I will note that this is high priority in the context of binary curves.  In particular, this modular division routine is called 1 time for each n-bit binary curve point multiplication.  (See line 159 in ecl/ec2_mont.c; the enclosing routine Mxy is called at line 257.)

Douglas

Comment 5

[Wan-Teh Chang]

I checked in the patch on the NSS trunk (NSS 3.12).

Checking in mp_gf2m.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mp_gf2m.c,v  <--  mp_gf2m.c
new revision: 1.5; previous revision: 1.4
done

Comment 6

[Robert Relyea]

Comment on attachment 246086 [details] [diff] [review]
Proposed patch

r+=relyea

mp_ints must be initialized before any possible CLEANUP calls. This is happening correctly.

Comment 7

[Slavomir Katuscak]

This leak still occures in NSS_3_11_BRANCH. Will be good to check in fix also there.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

This fix should have gone into 3.11.6!

Wan-Teh, when the 3_11 branch reopens, please commit this fix to the 3.11 
branch.  Thanks.

Comment 9

[Wan-Teh Chang]

I checked in the patch on the NSS_3_11_BRANCH for NSS 3.11.7.

This checkin requires revalidation of the ECDSA algorithm implementation.

Checking in mp_gf2m.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mp_gf2m.c,v  <--  mp_gf2m.c
new revision: 1.3.30.3; previous revision: 1.3.30.2
done