350771 - SHA1_DestroyContext / MD5_DestroyContext called on unconstructed contexts with SSL2 and PKCS#11 bypass

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect, P3)

Description

[Julien Pierre]

When SSL bypass mode is on in the server, we sometimes load the freebl library from libssl while destroying a socket. The following stack was found during memory leak checks :

=>[1] freebl_LoadDSO(), line 318 in "loader.c"
  [2] PR_CallOnce(once = 0xfefa09a0, func = 0xfef7e5f0 = &`libssl3.so`loader.c`freebl_LoadDSO()), line 809 in "prinit.c"
  [3] freebl_RunLoaderOnce(), line 354 in "loader.c"
  [4] SHA1_DestroyContext(cx = 0xfe538b64, freeit = 0), line 878 in "loader.c"
  [5] ssl3_DestroySSL3Info(ss = 0xfe5385a4), line 8450 in "ssl3con.c"
  [6] ssl_DestroySocketContents(ss = 0xfe5385a4), line 378 in "sslsock.c"
  [7] ssl_FreeSocket(ss = 0x8195e88), line 444 in "sslsock.c"
  [8] ssl_DefClose(ss = 0x8195e88), line 233 in "ssldef.c"
  [9] ssl_SecureClose(ss = 0x8195e88), line 960 in "sslsecur.c"
  [10] ssl_Close(fd = 0x818a108), line 1331 in "sslsock.c"
  [11] PR_Close(fd = 0x818a108), line 136 in "priometh.c"
  [12] handle_connection(tcp_sock = 0x818a108, model_sock = 0x8085100, requestCert = 0), line 1178 in "selfserv.c"
  [13] jobLoop(a = (nil), b = (nil), c = 0), line 518 in "selfserv.c"
  [14] thread_wrapper(arg = 0x816f044), line 486 in "selfserv.c"
  [15] _pt_root(arg = 0x817a400), line 220 in "ptthread.c"
  [16] _thr_setup(0xfe640800), at 0xfebef9be
  [17] _lwp_start(), at 0xfebefca0

The connection is processed correctly before in PKCS#11 mode. But during close, libssl tries to load libfreebl.

There are several issues here.

1) we shouldn't load freebl if we have no actual need for it. The code that initiated the load is in ssl3con.c :

    /* clean up handshake */
    if (ss->opt.bypassPKCS11) {
	SHA1_DestroyContext((SHA1Context *)ss->ssl3.hs.sha_cx, PR_FALSE);
	MD5_DestroyContext((MD5Context *)ss->ssl3.hs.md5_cx, PR_FALSE);
    } 

This is a bug, but probably a mostly harmless one at the moment, since the destroy functions don't seem to have any ill effects.
Rather than merely checking the PKCS#11 bypass option bit on the socket, we need to check if we actually created those contexts before trying to destroy them. There are no pointers for them anymore that can be checked. We probably don't want to check the content of the contexts either. So, we may need a flag indicator in the socket when the contexts are created..

2) is bypass supposed to work for SSL2 ?

If so, our code needs to be fixed, because the whole SSL2 connection gets processed without bypass in the server even when specifying -B for selfserv.

Comment 1

[Julien Pierre]

The test case to obtain this stack is to enable bypass in selfserv with -B, as well as an SSL2 cipher suite, for example -c A .  Put a breakpoint in freebl_LoadDSO from libssl3.so in selfserv . Then just run strsclnt with the same SSL2 cipher suite .

selfserv -B -s -D -p 8443 -d ../../../tests_results/security/monstre.1/server -n monstre.red.iplanet.com -w nss -c A -t 5

strsclnt -q -p 8443 -d ../../../tests_results/security/monstre.1/client -w nss -c 1 -C A monstre.red.iplanet.com

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

There are NO products that use the bypass feature, at all.  
No servers, no browsers, no email clients.  None.
I think this bug should be prioritized accordingly.  

I'm not Sisyphus.  I wouldn't object to removing the bypass testing 
from ssl.sh at this time.  

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I checked.  Bypass feature has no effect on SSL2.  
SSL2 never does bypass, whether it's enabled or not.
Bypass was intended to accelerate SSL3/TLS, not SSL2.
Not much point in scripts testing SSL2 with bypass.  

Yes, this bug should be fixed, but it's not urgent while no products use 
this feature.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Apparently. bugzilla's "modify several bugs at once" feature is setting the
"security group" flag by default :(  So I'm clearing it.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

remove target milestone, since the target was missed.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Will be "fixed" by bug 1303224.