Closed Bug 336335 Opened 18 years ago Closed 18 years ago

memory leaks in freebl with ECC and RSA cipher suites (CVE-2006-3127)

Categories

(NSS :: Libraries, defect, P1)

3.11
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.11.1

People

(Reporter: julien.pierre, Assigned: julien.pierre)

Details

(Keywords: regression)

Attachments

(5 files, 2 obsolete files)

Jason found major memory leaks while running Purify on selfserv.
I will attach his purify data file.

Here is one of the most common stacks :

[W] MLK: Memory leak of 2556928 bytes from 9988 blocks allocated in s_mp_alloc [freebl3.dll]
        Distribution of leaked blocks
            2556928 bytes from 9988 blocks of 256 bytes (first block: 0x003e9d60)
        Allocation location
            calloc         [C:\WINDOWS\System32\MSVCRT.dll]
            s_mp_alloc     [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpi.c:2877]
            mp_init_size   [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpi.c:148]
            mp_init        [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpi.c:127]
            ECGroup_new    [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/ecl/ecl.c:65]
            ECGroup_consGFp [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/ecl/ecl.c:93]
            ecgroup_fromNameAndHex [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/ecl/ecl.c:292]
            ECGroup_fromName [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/ecl/ecl.c:370]
            EC_ValidatePublicKey [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/ec.c:492]
            EC_ValidatePublicKey [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/loader.c:1365]
            NSC_DeriveKey  [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/softoken/pkcs11c.c:5508]
            PK11_PubDeriveWithKDF [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/pk11wrap/pk11skey.c:1733]
            ssl3_HandleECDHClientKeyExchange [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3ecc.c:450]
            ssl3_HandleClientKeyExchange [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:6611]
            ssl3_HandleHandshakeMessage [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:7686]
            ssl3_HandleHandshake [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:7757]
            ssl3_HandleRecord [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:8020]
            ssl3_GatherCompleteHandshake [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3gthr.c:206]
            ssl_GatherRecord1stHandshake [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslcon.c:1258]
            ssl_Do1stHandshake [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsecur.c:149]
            ssl_SecureRecv [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsecur.c:1032]
            ssl_SecureRead [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsecur.c:1051]
            ssl_Read       [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsock.c:1393]
            PR_Read        [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/nsprpub/WINNT4.0_DBG.OBJ/pr/src/io/../../../../pr/src/io/priometh.c:141]
            handle_connection [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/cmd/selfserv/selfserv.c:960]
            jobLoop        [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/cmd/selfserv/selfserv.c:518]
            thread_wrapper [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/security/nss/cmd/selfserv/selfserv.c:486]
            PR_NativeRunThread [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/nsprpub/WINNT4.0_DBG.OBJ/pr/src/threads/combined/../../../../../pr/src/threads/combined/pruthr.c:436]
            pr_root        [E:/security/securitytip/builds/20060425.1/blowfish_NT4.0_Win95/mozilla/nsprpub/WINNT4.0_DBG.OBJ/pr/src/md/windows/../../../../../pr/src/md/windows/ntthread.c:198]
Attached file Purify data file
I have seen leaks on Solaris as well. We can't ship any release with ECC unless those memory leaks are resolved.
Severity: normal → blocker
Priority: -- → P1
Target Milestone: --- → 3.11.1
Here is a report from dbx check leaks, for one full handshake, using cipher suite C00E :

Checking for memory leaks...

Actual leaks report    (actual leaks:           12  total size:       3072 bytes)

  Total     Num of  Leaked     Allocation call stack
  Size      Blocks  Block
                    Address
==========  ====== =========== =======================================
       256       1  0x81c2588  calloc < s_mp_alloc < mp_init_copy < mp_div < mp_mod < ec_GFp_enc_mont < ECGroup_consGFp_mont < ecgroup_fromNameAndHex < ECGroup_fromName < ec_points_mul < ECDH_Derive < ECDH_Derive < NSC_DeriveKey < PK11_PubDeriveWithKDF < ssl3_HandleECDHClientKeyExchange < ssl3_HandleClientKeyExchange 
       256       1  0x81c1640  calloc < s_mp_alloc < mp_init_copy < mp_div < mp_mod < ec_GFp_enc_mont < ECGroup_consGFp_mont < ecgroup_fromNameAndHex < ECGroup_fromName < EC_ValidatePublicKey < EC_ValidatePublicKey < NSC_DeriveKey < PK11_PubDeriveWithKDF < ssl3_HandleECDHClientKeyExchange < ssl3_HandleClientKeyExchange < ssl3_HandleHandshakeMessage 
       256       1  0x81c1b40  calloc < s_mp_alloc < mp_init_copy < mp_div < mp_mod < ec_GFp_enc_mont < ECGroup_consGFp_mont < ecgroup_fromNameAndHex < ECGroup_fromName < EC_ValidatePublicKey < EC_ValidatePublicKey < NSC_DeriveKey < PK11_PubDeriveWithKDF < ssl3_HandleECDHClientKeyExchange < ssl3_HandleClientKeyExchange < ssl3_HandleHandshakeMessage 
       256       1  0x81c0808  calloc < s_mp_alloc < mp_init_size < mp_init < GFMethod_new < GFMethod_consGFp < GFMethod_consGFp_mont < ECGroup_consGFp_mont < ecgroup_fromNameAndHex < ECGroup_fromName < EC_ValidatePublicKey < EC_ValidatePublicKey < NSC_DeriveKey < PK11_PubDeriveWithKDF < ssl3_HandleECDHClientKeyExchange < ssl3_HandleClientKeyExchange 
       256       1  0x81c0920  calloc < s_mp_alloc < mp_init_copy < mp_div < mp_mod < ec_GFp_enc_mont < ECGroup_consGFp_mont < ecgroup_fromNameAndHex < ECGroup_fromName < EC_ValidatePublicKey < EC_ValidatePublicKey < NSC_DeriveKey < PK11_PubDeriveWithKDF < ssl3_HandleECDHClientKeyExchange < ssl3_HandleClientKeyExchange < ssl3_HandleHandshakeMessage 
       256       1  0x81c0e98  calloc < s_mp_alloc < mp_init_size < mp_init < ECGroup_new < ECGroup_consGFp_mont < ecgroup_fromNameAndHex < ECGroup_fromName < EC_ValidatePublicKey < EC_ValidatePublicKey < NSC_DeriveKey < PK11_PubDeriveWithKDF < ssl3_HandleECDHClientKeyExchange < ssl3_HandleClientKeyExchange < ssl3_HandleHandshakeMessage < ssl3_HandleHandshake 
       256       1  0x81c26a0  calloc < s_mp_alloc < mp_init_copy < mp_div < mp_mod < ec_GFp_enc_mont < ECGroup_consGFp_mont < ecgroup_fromNameAndHex < ECGroup_fromName < ec_points_mul < ECDH_Derive < ECDH_Derive < NSC_DeriveKey < PK11_PubDeriveWithKDF < ssl3_HandleECDHClientKeyExchange < ssl3_HandleClientKeyExchange 
       256       1  0x81c2c18  calloc < s_mp_alloc < mp_init_copy < mp_div < mp_mod < ec_GFp_enc_mont < ECGroup_consGFp_mont < ecgroup_fromNameAndHex < ECGroup_fromName < ec_points_mul < ECDH_Derive < ECDH_Derive < NSC_DeriveKey < PK11_PubDeriveWithKDF < ssl3_HandleECDHClientKeyExchange < ssl3_HandleClientKeyExchange 
       256       1  0x81c29e8  calloc < s_mp_alloc < mp_init_size < mp_init < ECGroup_new < ECGroup_consGFp_mont < ecgroup_fromNameAndHex < ECGroup_fromName < ec_points_mul < ECDH_Derive < ECDH_Derive < NSC_DeriveKey < PK11_PubDeriveWithKDF < ssl3_HandleECDHClientKeyExchange < ssl3_HandleClientKeyExchange < ssl3_HandleHandshakeMessage 
       256       1  0x81c2b00  calloc < s_mp_alloc < mp_init_size < mp_init < GFMethod_new < GFMethod_consGFp < GFMethod_consGFp_mont < ECGroup_consGFp_mont < ecgroup_fromNameAndHex < ECGroup_fromName < ec_points_mul < ECDH_Derive < ECDH_Derive < NSC_DeriveKey < PK11_PubDeriveWithKDF < ssl3_HandleECDHClientKeyExchange 
       256       1  0x81ca198  calloc < s_mp_alloc < mp_init_copy < mp_div < mp_mod < ec_GFp_enc_mont < ECGroup_consGFp_mont < ecgroup_fromNameAndHex < ECGroup_fromName < EC_ValidatePublicKey < EC_ValidatePublicKey < NSC_DeriveKey < PK11_PubDeriveWithKDF < ssl3_HandleECDHClientKeyExchange < ssl3_HandleClientKeyExchange < ssl3_HandleHandshakeMessage 
       256       1  0x81c5930  calloc < s_mp_alloc < mp_init_copy < mp_div < mp_mod < ec_GFp_enc_mont < ECGroup_consGFp_mont < ecgroup_fromNameAndHex < ECGroup_fromName < ec_points_mul < ECDH_Derive < ECDH_Derive < NSC_DeriveKey < PK11_PubDeriveWithKDF < ssl3_HandleECDHClientKeyExchange < ssl3_HandleClientKeyExchange 
 

Possible leaks report  (possible leaks:          6  total size:       4406 bytes)

  Total     Num of  Leaked     Allocation call stack
  Size      Blocks  Block
                    Address
==========  ====== =========== =======================================
      2071       1  0x81c7fe8  PR_Malloc < PL_ArenaAllocate < PORT_ArenaAlloc < CERT_DupCertList < ssl_DupSocket < ssl_Accept < PR_Accept < do_accepts < server_main < main 
      2071       1  0x81c4ec0  PR_Malloc < PL_ArenaAllocate < PORT_ArenaAlloc < CERT_DupCertList < ssl_DupSocket < ssl_Accept < PR_Accept < do_accepts < server_main < main 
        88       1  0x81b6c48  calloc < PR_Calloc < PR_NewLock < PORT_NewArena < CERT_DupCertList < ssl_DupSocket < ssl_Accept < PR_Accept < do_accepts < server_main < main 
        88       1  0x81a0908  calloc < PR_Calloc < PR_NewLock < PORT_NewArena < CERT_DupCertList < ssl_DupSocket < ssl_Accept < PR_Accept < do_accepts < server_main < main 
        88       2      -      calloc < PR_Calloc < PORT_ZAlloc < PORT_NewArena < CERT_DupCertList < ssl_DupSocket < ssl_Accept < PR_Accept < do_accepts < server_main < main 
It looks like the leaks are all within softoken, in NSC_DeriveKey doing ECDH_Derive to get the SSL pre-master secret.
The problem does not appear to be the SSL code. libssl calls PK11_PubDeriveWithKDF, then calls PK11_FreeSymKey to destroy it once it's done with the value. But that call does not free the softoken/freebl memory associated with the object.
Attached patch fix (obsolete) — Splinter Review
This patch fixes at least the leaks I have observed running cipher suite C00E manually in dbx .I will review the other leak stacks in the purify data file to find if other fixes are necessary.
Assignee: nobody → julien.pierre.bugs
Status: NEW → ASSIGNED
Attachment #220715 - Flags: superreview?(wtchang)
Attachment #220715 - Flags: review?(douglas)
I have reviewed all the suspect leaking stacks from the purify data file, and they all appear related to the bugs fixed by this patch. However, Jason said that he didn't run all the cipher suites once he ran into the leak with C004. I have verified that both C004 and C00E have no leak with the patch. I would like to check it in tonight so that Jason can run purify on tomorrow's build.
Wan-Teh, could you please review it ?

Thanks.
Comment on attachment 220715 [details] [diff] [review]
fix

r=wtc.  I have a question for Douglas and two
suggested changes.

1. Douglas, do we ever set group->constructed or
meth->constructed to MP_NO?  If not, I suggest that
we remove these two fields to simplify our code.
For example, in ECGroup_free we have:

void
ECGroup_free(ECGroup *group)
{
        if (group == NULL)
                return;
        GFMethod_free(group->meth);
        if (group->constructed == MP_NO)
                return;

I don't know if the GFMethod_free call should be
moved after the test for group->constructed == MP_NO.
Compare this code to GFMethod_free, which tests
for meth->constructed == MP_NO first.

2. In ECGroup_new, we need to initialize
group->meth and group->extra_free to NULL before
the first MP_CHECKOK call.  Otherwise we may go
to CLEANUP with uninitialized group->meth and
group->extra_free.

I suggest you do this as follows:
- add "group->meth = NULL;" before
  "group->text = NULL;".
- Move all the MP_CHECKOK calls after
  "group->extra_free = NULL;".

3. In GFMethod_new, change

        MP_CHECKOK(mp_init(&meth->irr));
        meth->extra_free = NULL;

to

        MP_DIGITS(&meth->irr) = 0;
        meth->extra_free = NULL;
        MP_CHECKOK(mp_init(&meth->irr));
Attachment #220715 - Flags: superreview?(wtchang) → superreview+
As checked in to the tip.

Checking in ecl.c;
/cvsroot/mozilla/security/nss/lib/freebl/ecl/ecl.c,v  <--  ecl.c
new revision: 1.10; previous revision: 1.9
done
Checking in ecl_curve.c;
/cvsroot/mozilla/security/nss/lib/freebl/ecl/ecl_curve.c,v  <--  ecl_curve.c
new revision: 1.3; previous revision: 1.2
done
Checking in ecl_gf.c;
/cvsroot/mozilla/security/nss/lib/freebl/ecl/ecl_gf.c,v  <--  ecl_gf.c
new revision: 1.6; previous revision: 1.5
done

3.11 branch :
Checking in ecl.c;
/cvsroot/mozilla/security/nss/lib/freebl/ecl/ecl.c,v  <--  ecl.c
new revision: 1.5.2.4; previous revision: 1.5.2.3
done
Checking in ecl_curve.c;
/cvsroot/mozilla/security/nss/lib/freebl/ecl/ecl_curve.c,v  <--  ecl_curve.c
new revision: 1.2.28.2; previous revision: 1.2.28.1
done
Checking in ecl_gf.c;
/cvsroot/mozilla/security/nss/lib/freebl/ecl/ecl_gf.c,v  <--  ecl_gf.c
new revision: 1.2.28.2; previous revision: 1.2.28.1
done

3.11.1 branch :

Checking in ecl.c;
/cvsroot/mozilla/security/nss/lib/freebl/ecl/ecl.c,v  <--  ecl.c
new revision: 1.5.2.2.2.1; previous revision: 1.5.2.2
done
Checking in ecl_curve.c;
/cvsroot/mozilla/security/nss/lib/freebl/ecl/ecl_curve.c,v  <--  ecl_curve.c
new revision: 1.2.48.1; previous revision: 1.2
done
Checking in ecl_gf.c;
/cvsroot/mozilla/security/nss/lib/freebl/ecl/ecl_gf.c,v  <--  ecl_gf.c
new revision: 1.2.28.1.2.1; previous revision: 1.2.28.1
done
Attachment #220715 - Attachment is obsolete: true
Attachment #220729 - Flags: review?(douglas)
Attachment #220715 - Flags: review?(douglas)
Attachment #220729 - Flags: review+
Attached file new purify leak data
Jason reran the tests. I looked at the new purify result.
Here is still a suspect stack which is not in ECC but in RSA signature :

[W] MLK: Memory leak of 1024 bytes from 2 blocks allocated in s_mp_alloc [freebl3.dll]
        Distribution of leaked blocks
        Allocation location
            calloc         [C:\WINDOWS\System32\MSVCRT.dll]
            s_mp_alloc     [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpi.c:2877]
            mp_init_size   [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpi.c:148]
            mp_exptmod_safe_i [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpmontg.c:949]
            mp_exptmod     [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpmontg.c:1196]
            rsa_PrivateKeyOpCRTCheckedPubKey [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/rsa.c:500]
            rsa_PrivateKeyOp [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/rsa.c:739]
            RSA_PrivateKeyOpDoubleChecked [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/rsa.c:778]
            RSA_PrivateKeyOpDoubleChecked [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/loader.c:394]
            RSA_Sign       [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/softoken/rsawrapr.c:465]
            NSC_Sign       [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/softoken/pkcs11c.c:2067]
            PK11_Sign      [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/pk11wrap/pk11obj.c:766]
            ssl3_SignHashes [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:818]
            ssl3_SendECDHServerKeyExchange [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3ecc.c:832]
            ssl3_SendServerKeyExchange [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:6150]
            ssl3_SendServerHelloSequence [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:5338]
            ssl3_HandleV2ClientHello [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:5947]
            ssl2_HandleClientHelloMessage [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslcon.c:3472]
            ssl_Do1stHandshake [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsecur.c:149]
            ssl_SecureRecv [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsecur.c:1032]
            ssl_SecureRead [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsecur.c:1051]
            ssl_Read       [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsock.c:1393]
            PR_Read        [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/nsprpub/WINNT4.0_DBG.OBJ/pr/src/io/../../../../pr/src/io/priometh.c:141]
            handle_connection [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/cmd/selfserv/selfserv.c:960]
            jobLoop        [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/cmd/selfserv/selfserv.c:518]
            thread_wrapper [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/cmd/selfserv/selfserv.c:486]
            PR_NativeRunThread [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/nsprpub/WINNT4.0_DBG.OBJ/pr/src/threads/combined/../../../../../pr/src/threads/combined/pruthr.c:436]
            pr_root        [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/nsprpub/WINNT4.0_DBG.OBJ/pr/src/md/windows/../../../../../pr/src/md/windows/ntthread.c:198]

It seems odd that there are only 2 blocks.

Here is another suspicious stack, with just one block :

[W] MLK: Memory leak of 512 bytes from 1 block allocated in s_mp_alloc [freebl3.dll]
        Distribution of leaked blocks
        Allocation location
    calloc         [C:\WINDOWS\System32\MSVCRT.dll]
    s_mp_alloc     [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpi.c:2877]
    mp_init_size   [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpi.c:148]
    mp_exptmod_safe_i [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpmontg.c:949]
    mp_exptmod     [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpmontg.c:1196]
    generate_blinding_params [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/rsa.c:552]
    init_blinding_params [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/rsa.c:582]
    get_blinding_params [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/rsa.c:654]
    rsa_PrivateKeyOp [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/rsa.c:727]
    RSA_PrivateKeyOpDoubleChecked [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/rsa.c:778]
    RSA_PrivateKeyOpDoubleChecked [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/loader.c:394]
    RSA_Sign       [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/softoken/rsawrapr.c:465]
    NSC_Sign       [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/softoken/pkcs11c.c:2067]
    PK11_Sign      [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/pk11wrap/pk11obj.c:766]
    ssl3_SignHashes [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:818]
    ssl3_SendECDHServerKeyExchange [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3ecc.c:832]
    ssl3_SendServerKeyExchange [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:6150]
    ssl3_SendServerHelloSequence [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:5338]
    ssl3_HandleV2ClientHello [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:5947]
    ssl2_HandleClientHelloMessage [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslcon.c:3472]
    ssl_Do1stHandshake [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsecur.c:149]
    ssl_SecureRecv [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsecur.c:1032]
    ssl_SecureRead [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsecur.c:1051]
    ssl_Read       [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsock.c:1393]
    PR_Read        [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/nsprpub/WINNT4.0_DBG.OBJ/pr/src/io/../../../../pr/src/io/priometh.c:141]
    handle_connection [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/cmd/selfserv/selfserv.c:960]
    jobLoop        [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/cmd/selfserv/selfserv.c:518]
    thread_wrapper [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/cmd/selfserv/selfserv.c:486]
    PR_NativeRunThread [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/nsprpub/WINNT4.0_DBG.OBJ/pr/src/threads/combined/../../../../../pr/src/threads/combined/pruthr.c:436]
    pr_root        [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/nsprpub/WINNT4.0_DBG.OBJ/pr/src/md/windows/../../../../../pr/src/md/windows/ntthread.c:198]

Another:

[W] MLK: Memory leak of 512 bytes from 2 blocks allocated in s_mp_alloc [freebl3.dll]
        Distribution of leaked blocks
        Allocation location
            calloc         [C:\WINDOWS\System32\MSVCRT.dll]
            s_mp_alloc     [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpi.c:2877]
            mp_init_size   [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpi.c:148]
            mp_exptmod_safe_i [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpmontg.c:949]
            mp_exptmod     [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/mpi/mpmontg.c:1196]
            rsa_PrivateKeyOpCRTNoCheck [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/rsa.c:451]
            rsa_PrivateKeyOpCRTCheckedPubKey [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/rsa.c:496]
            rsa_PrivateKeyOp [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/rsa.c:739]
            RSA_PrivateKeyOpDoubleChecked [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/rsa.c:778]
            RSA_PrivateKeyOpDoubleChecked [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/freebl/loader.c:394]
            RSA_Sign       [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/softoken/rsawrapr.c:465]
            NSC_Sign       [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/softoken/pkcs11c.c:2067]
            PK11_Sign      [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/pk11wrap/pk11obj.c:766]
            ssl3_SignHashes [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:818]
            ssl3_SendECDHServerKeyExchange [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3ecc.c:832]
            ssl3_SendServerKeyExchange [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:6150]
            ssl3_SendServerHelloSequence [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:5338]
            ssl3_HandleV2ClientHello [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/ssl3con.c:5947]
            ssl2_HandleClientHelloMessage [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslcon.c:3472]
            ssl_Do1stHandshake [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsecur.c:149]
            ssl_SecureRecv [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsecur.c:1032]
            ssl_SecureRead [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsecur.c:1051]
            ssl_Read       [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/lib/ssl/sslsock.c:1393]
            PR_Read        [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/nsprpub/WINNT4.0_DBG.OBJ/pr/src/io/../../../../pr/src/io/priometh.c:141]
            handle_connection [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/cmd/selfserv/selfserv.c:960]
            jobLoop        [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/cmd/selfserv/selfserv.c:518]
            thread_wrapper [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/security/nss/cmd/selfserv/selfserv.c:486]
            PR_NativeRunThread [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/nsprpub/WINNT4.0_DBG.OBJ/pr/src/threads/combined/../../../../../pr/src/threads/combined/pruthr.c:436]
            pr_root        [E:/security/securityjes5/builds/20060504.1/blowfish_NT4.0_Win95/mozilla/nsprpub/WINNT4.0_DBG.OBJ/pr/src/md/windows/../../../../../pr/src/md/windows/ntthread.c:198]

These leak stacks were not showing in yesterday's report.
To me, they may indicate that some SSL connections were still not closed at the time the leak report was taken. Jason, do you know if that might have been the case ?

All the other leak stacks are in the builtins (which have known unfixed leaks :() or in 1-time initialization allocation that don't have corresponding frees, such as the RSA stepdown keygen, but those are OK.
All the strsclnt commands had finished. The tstclnt connection may have still
been open when selfserv terminated due to the "GET /stop HTTP/1.0". I noticed
that I have to kill the tstclnts with a ctrl-c.
Jason,

Were you doing session reuse with strsclnt or not during the test ? Perhaps the small number of leaks is explained if a small number of full handshakes occurred.

The RSA signing leak looks legit. But it would happen every time the function is executed. A temporary bignum is created, and never freed. I will create a patch.
Attached patch free temporary bignum (obsolete) — Splinter Review
Attachment #220862 - Flags: review?(wtchang)
Comment on attachment 220862 [details] [diff] [review]
free temporary bignum

r=wtc.  This patch is correct.  But more changes
are needed to make it safe to goto CLEANUP on
error.

Please move the following seven
"MP_DIGITS(&xxx) = 0;" statements to the beginning
of the function:

  MP_DIGITS(&accum1) = 0;
  MP_DIGITS(&accum2) = 0;
  MP_DIGITS(&accum[0]) = 0;
  MP_DIGITS(&accum[1]) = 0;
  MP_DIGITS(&accum[2]) = 0;
  MP_DIGITS(&accum[3]) = 0;
and
  MP_DIGITS(&tmp) = 0;

They need to be before the code that allocates
powersArray.
Attachment #220862 - Flags: review?(wtchang) → review+
Thanks for the quick review, Wan-Teh .

I have checked in the fix to NSS_3_11_1_BRANCH :

Checking in mpi/mpmontg.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mpmontg.c,v  <--  mpmontg.c
new revision: 1.17.2.1.2.1; previous revision: 1.17.2.1
done

NSS_3_11_BRANCH :

Checking in mpi/mpmontg.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mpmontg.c,v  <--  mpmontg.c
new revision: 1.17.2.2; previous revision: 1.17.2.1
done

And the tip :

Checking in mpi/mpmontg.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mpmontg.c,v  <--  mpmontg.c
new revision: 1.19; previous revision: 1.18
done
Attachment #220862 - Attachment is obsolete: true
Attachment #220867 - Flags: review+
Jason,

Please provide more information about how you are running the leak tests.
I'm concerned because the RSA leak showed in thursday's purify report but not in wednesday's report. Unlike the ECC leak, I don't actually have a test case for this leak, although I did not make any effort to create one. I would like to know which exact selfserv and strsclnt commands you ran yesterday that led to finding this RSA leak. Thanks.

Here are the details of my recent purify work. The first time I did the tests, I only ran one strsclnt. Today and yesterday's work match the following writeup.

whitetiger.sfbay.sun.com is running Windows XP Professional and has the
Cygwin environment and Purify installed. Windows XP Professional
version is 5.1.2600 Service Pack 2 Build 2600. The machine has K8T
Master2-FAR motherboard, AMD Opteron processor at roughly 1.6
gigahertz, and 1024 megabytes of memory. Cygwin version is 1.5.19.
Purify version is 2003.06.13.402.000 Build 5352.

Builds taken from: /share/builds/mccrel3/security/securityjes5/ \
builds/<date>.1/blowfish_NT4.0_Win95. Build is placed into a tar file
generated on mace.red.iplanet.com then extracted onto whitetiger into
C:\<release> aka /cygdrive/c/<release> with release being m2 for the
20060503 build, m3 for the 20060504 build, and  m4 for the 20060505
build

The initial environment was generated with the 20060503 build with
common.sh edited so all.sh would run. All the tests passed. For the
subsequent builds, the whitetiger.20 test directory was copied over to
each build's test_results area. The same set of certificates was used
for all the Purify runs.

Tests are run with two Cygwin shell windows with the following environment
enviroment variables:
PWD is /cygdrive/c/<release>/mozilla/tests_results/security/whitetiger.20
 e.g. /cygdrive/c/m4/mozilla/tests_results/security/whitetiger.20
NSS_DISABLE_FREE_ARENA_LIST=1
DOMSUF=sfbay.sun.com
NSS_ENABLE_ECC=1
NSS_STRICT_SHUTDOWN=1
PATH is setup to have <release>/mozilla/dist/WINNT4.0_DBG.OBJ/bin and
<release>/mozilla/dist/WINNT4.0_DBG.OBJ/lib in it.

Window one runs both purify and selfserv.
purify selfserv -D -p 8443 -d ext_server -e whitetiger.sfbay.sun.com-ec \
        -n whitetiger.sfbay.sun.com -w nss -c :C004:C009:C013:C00E -S

Window two runs all the strsclnts and the finishing tstclnt.
strsclnt -p 8443 -d ext_client -w nss -c 10000 -C :C009 -T -N whitetiger.sfbay.sun.com
strsclnt -p 8443 -d ext_client -w nss -c 10000 -C :C013 -T whitetiger.sfbay.sun.com
strsclnt -p 8443 -d ext_client -w nss -c 10000 -C :C004 -N whitetiger.sfbay.sun.com
strsclnt -p 8443 -d ext_client -w nss -c 10000 -C :C013 whitetiger.sfbay.sun.comtstclnt -p 8443 -d ext_client -w nss -c :C004 -h whitetiger.sfbay.sun.com
GET /stop HTTP/1.0
<two carriage returns>
<ctrl-c> (Needed for tstclnt to exit.)

Afterwards, the pfy file is copied via ftp to my workstation for general
dispersal.
Thanks, Jason.

You are running cipher suite C013 with session reuse (no -N argument on strsclnt to force full handshakes), twice.
That is cipher suite TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA .
That means you have 2 full handshake requests with that cipher suites, and 19998 restart handshakes. The 2 full handshake are probably the ones that showed the leak. I will try to verify that between yesterday and today's build, using dbx check leaks.

I reviewed the latest purify data file. Everything looks good. Only the nssckbi (built-ins) known leaks are showing up. I was somewhat surprised not to see the leak for the RSA stepdown keys on this run. The stepdown keygen can only be disabled with the -E argument to selfserv, which you weren't using. Perhaps the stepdown keys actually got freed during shutdown in today's run and not the previous 2 days, but I can't really explain the discrepancy. That is somewhat strange, but lack of this leak on today's run is not a showstopper. I think we are finally good to go for 3.11.1 as far as memory leaks are concerned.
> tstclnt -p 8443 -d ext_client -w nss -c :C004 -h whitetiger.sfbay.sun.com

tstclnt command should use the -f option for https client simulation, 
especially on Windows.

> GET /stop HTTP/1.0
> <two carriage returns>
> <ctrl-c> (Needed for tstclnt to exit.)

control-C will kill tstclnt.  
You want to send it an "end of file", so that it will terminate normally.
That's ctrl-D on Unix or ctrl-Z on windows.  
Comment on attachment 220867 [details] [diff] [review]
patch for RSA 3.11 regression, as checked in

This bug reports a large leak that is seen only with ECC 
cipher suites.  But this patch fixes a leak in an integer 
(bignum) modular exponentiation routine that is used by RSA 
(on some platforms only) and *might* also be used for ECC 
(not sure).  I'd expect this modular exponentiation leak to 
occur equally for all users of this function.  And I'd expect 
the patch to cure that particular leak for all users of this 
function.   

So, if this patch really does fix a leak that is seen only in
ECC tests, I'd like to understand why the leak is not also 
seen in RSA tests.
Nelson,

Unfortunately, the memory leak checks were not run on all the RSA cipher suites, only the ECC ones, as Jason reported. We were under pressure to release, and we had to draw the line somewhere. We had much more leak coverage for 3.11.1 than for 3.11, but we have a long way to go before we have proper leak testing. This is something we have to address for our next point release.

You are correct that all callers of this function are affected equally by the memory leak. All RSA cipher suites are affected on the server-side, since RSA_PrivateKeyOp ends up calling this leaking function.
Keygen is also affected.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Summary: memory leaks in selfserv with ECC cipher suites → memory leaks in selfserv with ECC and RSA cipher suites
The memory leak in the RSA code that Julien reported
in comment 9 is a regression introduced in NSS 3.11
(rev. 1.16 of mpi/mpmontg.c) by Bob's "weaving patch"
(bug 298630).
Keywords: regression
Summary: memory leaks in selfserv with ECC and RSA cipher suites → memory leaks in freebl with ECC and RSA cipher suites
Attachment #220867 - Attachment description: patch as checked in → patch for RSA 3.11 regression, as checked in
Attachment #220729 - Attachment description: fix including Wan-Teh's suggestions → patch for long-standing ECC leak, including Wan-Teh's suggestions
Attachment #220729 - Flags: review?(douglas)
Summary: memory leaks in freebl with ECC and RSA cipher suites → memory leaks in freebl with ECC and RSA cipher suites (CVE-2006-3127)
You need to log in before you can comment on or make changes to this bug.