639789 - Possible minor memory leak in SNI code

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[u238590]

export UMEM_DEBUG=default : Started server, ran some tests - taken gcore, ran some more tests took gcore. compared findleaks -d output. Here is the stack that leaks :

umem_alloc_24 leak: 77 buffers, 24 bytes each, 1848 bytes total
     ADDR      BUFADDR   TIMESTAMP      THREAD  CACHE  LASTLOG CONTENTS
     1a7770    481c88    4b24b1c4062a1   35     29c08     0     0

  libumem.so.1`umem_cache_alloc+0x210
  libumem.so.1`umem_alloc+0x60
  libumem.so.1`malloc+0x28
  libnspr4.so`PR_Malloc+0x78
  libnssutil3.so`PORT_Alloc_Util+0x48
  libnssutil3.so`SECITEM_CopyItem_Util+0x88
  libssl3.so`ssl3_NewSessionID+0xc8
  libssl3.so`ssl3_HandleClientHello+0x1dfc
  libssl3.so`ssl3_HandleHandshakeMessage+0x554
  libssl3.so`ssl3_HandleHandshake+0x2d8
  libssl3.so`ssl3_HandleRecord+0xb60
  libssl3.so`ssl3_GatherCompleteHandshake+0x110
  libssl3.so`ssl_GatherRecord1stHandshake+0xd0
  libssl3.so`ssl_Do1stHandshake+0x308
  libssl3.so`ssl_SecureRecv+0x230

-----------------------------------------------------------


Root cause :
~~~~~~~~~~~~
http://mxr.mozilla.org/security/source/security/nss/lib/ssl/ssl3con.c#5829
looks at line 5844
5844   rv = SECITEM_CopyItem(NULL, &sid->u.ssl3.srvName, srvName);

this function calls PORT_Alloc as shown below :
http://mxr.mozilla.org/security/source/security/nss/lib/util/secitem.c#238
245    to->data = (unsigned char*) PORT_Alloc(from->len);

But in ssl_DestroySID, it doesn't free sid->u.ssl3.srvName
http://mxr.mozilla.org/security/source/security/nss/lib/ssl/sslnonce.c#198

it just frees sid->u.ssl3.sessionTicket
223   SECITEM_FreeItem(&sid->u.ssl3.sessionTicket.ticket, PR_FALSE);

Suggested fix : 
~~~~~~~~~~~~~~~
  if (sid->u.ssl3.srvName.data) {
      SECITEM_FreeItem(&sid->u.ssl3.srvName, PR_FALSE);
  }

Comment 1

[Alexei Volkov]

Another minor memory leak in ssl3_ServerHandleSessionTicketXtn:

SessionID is created when ticket parsing is done. After that, if temp cert creation failed, the code jumps into looser, leaving sid.

1210             sid = ssl3_NewSessionID(ss, PR_TRUE);


1248                 sid->peerCert = CERT_NewTempCertificate(ss->dbHandle,
1249                     &parsed_session_ticket->peer_cert, NULL, PR_FALSE, PR_TRUE);
1250                 if (sid->peerCert == NULL) {
1251                     rv = SECFailure;
1252                     goto loser;
1253                 }
1254             }

Comment 2

[u238590]

Attachment: Test case for leak of srvName in ssl3_NewSessionID function

Test case:
~~~~~~~~~~
Create certificate :
    $certutil -S -n Server-Cert -s "CN=test.com" -x -t "CT,CT,CT" -d .
Building server.cpp atatched

    $/usr/dist/share/sunstudio_sparc,v12.0/SUNWspro/bin/CC -g -I -I/export1/NSS/SunOS5.10_DBG.OBJ/include -L/export1/NSS/SunOS5.10_DBG.OBJ/lib -lumem -lnspr4 -lnss3 -lssl3 server.cpp

Enabling debugging features of the umem library (refer man umem_debug for more details)
    $ export UMEM_DEBUG=default

Start server in background
    $ ./a.out 3333  &

Run 4 times :
    $tstclnt -h ... -a www.abc.com -p 3333 -n Server-Cert -2 -d  . -v -o -f < sslreq.dat

Take core dump:
    $ ps -eaf | grep a.out
    $ gcore core.11361

$echo ::findleaks -d | mdb core.11361 | c++filt
CACHE     LEAKED   BUFCTL CALLER
0002bc08       4 0012d248 libnspr4.so`PR_Malloc+0x78
----------------------------------------------------------------------
   Total       4 buffers, 96 bytes

umem_alloc_24 leak: 4 buffers, 24 bytes each, 96 bytes total
    ADDR    BUFADDR    TIMESTAMP    THREAD    CACHE    LASTLOG  CONTENTS
  12d248     125e90  546e091e7fb27       1    2bc08        0    0
    libumem.so.1`umem_cache_alloc+0x210
    libumem.so.1`umem_alloc+0x60
    libumem.so.1`malloc+0x28
    libnspr4.so`PR_Malloc+0x78
    libnssutil3.so`PORT_Alloc_Util+0x48
    libnssutil3.so`SECITEM_CopyItem_Util+0x88
    libssl3.so`ssl3_NewSessionID+0xc8
    libssl3.so`ssl3_HandleClientHello+0x1dfc
    libssl3.so`ssl3_HandleHandshakeMessage+0x554
    libssl3.so`ssl3_HandleHandshake+0x2d8
    libssl3.so`ssl3_HandleRecord+0xb60
    libssl3.so`ssl3_GatherCompleteHandshake+0x110
    libssl3.so`ssl_GatherRecord1stHandshake+0xd0
    libssl3.so`ssl_Do1stHandshake+0x308
    libssl3.so`ssl_SecureRecv+0x230

Comment 3

[u238590]

For my satisfaction, confirming the fact that when SID is being destroyed, it is ok to delete this "sid->u.ssl3.srvName". Figured its being called at PR_Close that should be ok as we wont need srvName beyond PR_Close.

$cat dtrace.d
#!/usr/sbin/dtrace -s
# pragma D option quiet
pid$1::$2:entry
/pid == $1/
{
      ustack();
}

If a.out (attached above) pid is 11453, then 
$./dtrace.d 11453 *ssl_DestroySID*
          libssl3.so`ssl_DestroySID
          libssl3.so`ssl_FreeLockedSID+0x94
          libssl3.so`ssl_FreeSID+0x2c
          libssl3.so`ssl_ResetSecurityInfo+0x240
          libssl3.so`ssl_DestroySecurityInfo+0x10
          libssl3.so`ssl_DestroySocketContents+0x28
          libssl3.so`ssl_FreeSocket+0x13c
          libssl3.so`ssl_DefClose+0xf0
          libssl3.so`ssl_SecureClose+0xb8
          libssl3.so`ssl_Close+0xec
          libnspr4.so`PR_Close+0x18
          a.out`main+0x1c0
          a.out`_start+0x108
^C

Comment 4

[u238590]

Attachment: Deleted srvName in ssl_DestroySID and call ssl_DestroySID in ssl3_ServerHandleSessionTicketXtn function

Please review.


$ cvs co -r NSPR_4_8_7_RTM NSPR
$ cvs co -r NSS_3_12_9_RTM NSS

TEST Results :
SUMMARY:
========
NSS variables:
--------------
HOST=...
DOMSUF=india.sun.com
BUILD_OPT=
USE_64=
NSS_CYCLES=""
NSS_TESTS=""
NSS_SSL_TESTS="crl bypass_normal normal_bypass fips_normal normal_fips iopr"
NSS_SSL_RUN="cov auth stress"
NSS_AIA_PATH=
NSS_AIA_HTTP=
NSS_AIA_OCSP=
IOPR_HOSTADDR_LIST=
PKITS_DATA=

Tests summary:
--------------
Passed:             5732
Failed:             0
Failed with core:   0
Unknown status:     0

Comment 5

[Alexei Volkov]

Comment on attachment 518329 [details] [diff] [review]
Deleted srvName in ssl_DestroySID and call ssl_DestroySID in ssl3_ServerHandleSessionTicketXtn function

For the first part the correct fix would be to goto no_ticket as well as indication, that ticket's cert data was incorrect.
The second part is correct.

Giving r+ since this is minor change that I'll do before integration.

Comment 6

[u238590]

Attachment: Freed sid->u.ssl3.srvName in ssl_DestroySID function. Goto no_ticket instead of goto loser (which frees sid already) in ssl3_ServerHandleSessionTicketXtn function.

Comment 7

[Alexei Volkov]

Comment on attachment 519120 [details] [diff] [review]
Freed sid->u.ssl3.srvName in ssl_DestroySID function. Goto no_ticket instead of goto loser (which frees sid already) in ssl3_ServerHandleSessionTicketXtn function.

r+

Comment 8

[Robert Relyea]

Comment on attachment 519120 [details] [diff] [review]
Freed sid->u.ssl3.srvName in ssl_DestroySID function. Goto no_ticket instead of goto loser (which frees sid already) in ssl3_ServerHandleSessionTicketXtn function.

r- for ssl3ext.c
r+ for sslnonce.c

The ssl3ext.c patch changes the functions semantics. Where we used to return SECFailure, we now return SECSuccess. The better patch would be to move the sid freeing code below the loser label.

bob

Comment 9

[u238590]

Attachment: freeing sid in loser

Comment 10

[Alexei Volkov]

(In reply to comment #8)
> The ssl3ext.c patch changes the functions semantics. Where we used to return
> SECFailure, we now return SECSuccess. 

According to code at http://mxr.mozilla.org/security/source/security/nss/lib/ssl/ssl3ext.c#1381 is does not really matter if an extension handler return failure or not. However, if extension cert was not parsed correctly, I'd prefer that the function would goto no_ticket and change statistic.

Comment 11

[Alexei Volkov]

Comment on attachment 520879 [details] [diff] [review]
freeing sid in loser

patch is fine. Integrating it...

Comment 12

[Robert Relyea]

Comment on attachment 520879 [details] [diff] [review]
freeing sid in loser

r+ rrelyea
Thanks!

bob

Comment 13

[Alexei Volkov]

integrated into the branch and trunk.