675876 - Crash in PORT_Assert(!ss->xtnData.sniNameArr); in debug build only

Reporter

Description

•

13 years ago

Attached file Server code to generate the core dump — Details

I can reproduce a crash in PORT_Assert(!ss->xtnData.sniNameArr); 
http://mxr.mozilla.org/mozilla/source/security/nss/lib/ssl/sslsock.c#441

Attached is file my.cpp. 

To build it use the following Makefile :
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
$cat Makefile 
SECURITY_DIR=/share/builds/components/security/SECURITY_3.12.10_20110506/SunOS5.8_DBG.OBJ/
all:
        /usr/dist/share/sunstudio_sparc,v12.0/SUNWspro/bin/CC -g -I$(INCL_DIR) -I$(SECURITY_DIR)/include -L$(SECURITY_DIR)/lib -lnspr4 -lnss3 -lssl3 my.cpp

To create certs :
~~~~~~~~~~~~~~~
$certutil -N -d sql:.
$certutil -S -d sql:. -n ecccert -s "CN=www.ecc.com" -t "Cu,Cu,Cu" -k ec -q nistp256 -x -m 1343 -v 120 -5
$certutil -S -d sql:. -n rsacert -s "CN=www.rsa.com" -t "Cu,Cu,Cu" -x -v 120 -5

When it asks for input type
1     5      9    y

To run tests :
~~~~~~~~~~~~~~~
Server :
$./a.out 3333

Client :
$tstclnt -c :C00A -h hostname -d sql:. -o -p 3333 -2 -a www.abc.com < sni-abc.req 
tstclnt: write to SSL socket failed: Cannot communicate securely with peer: no common encryption algorithm(s).

Question is why when I have enabled that cipher in the server program?
cipher TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA = { 0xC0, 0x0A } i.e. -c :C00A  

$cat sni-abc.req
GET /sec.html HTTP/1.1
Host: www.abc.com
Connection: close


core dump :
~~~~~~~~~~~~
$dbx a.out core.23855
t@1 (l@1) program terminated by signal ABRT (Abort)
0xfee4c95c: __lwp_kill+0x0008:  bcc,a,pt  %icc,__lwp_kill+0x18  ! 0xfee4c96c
Current function is PR_Assert
(dbx) where                                                                  
current thread: t@1
  [1] __lwp_kill(0x0, 0x6, 0x0, 0x6, 0xffbffeff, 0x0), at 0xfee4c95c 
  [2] raise(0x6, 0x0, 0x0, 0xfee2c1f8, 0xffffffff, 0x6), at 0xfede5d3c 
  [3] abort(0x0, 0x1, 0xfee2c1d4, 0xeea98, 0xfeeb3418, 0x0), at 0xfedc1a2c 
=>[4] PR_Assert(s = 0xff07a51c "!ss->xtnData.sniNameArr", file = 0xff07a534 "sslsock.c", ln = 441), line 577 in "prlog.c"
  [5] ssl_DestroySocketContents(ss = 0xffbfc8b0), line 441 in "sslsock.c"
  [6] ssl_FreeSocket(ss = 0x9d8c0), line 478 in "sslsock.c"
  [7] ssl_DefClose(ss = 0x9d8c0), line 233 in "ssldef.c"
  [8] ssl_SecureClose(ss = 0x9d8c0), line 1078 in "sslsecur.c"
  [9] ssl_Close(fd = 0x6af58), line 1572 in "sslsock.c"
  [10] PR_Close(fd = 0x6af58), line 136 in "priometh.c"
  [11] cleanup(fd = 0x6af58), line 228 in "my.cpp"
  [12] main(argc = 2, argv = 0xffbff95c), line 292 in "my.cpp"

u238590

Reporter

Comment 1

•

13 years ago

Note that the same problem exists if only 1 ECC cert is added first on the socket. And client requests for RSA cert. That RSA cert is suposed to be returned by SNI Callback using SSL_ConfigureSecureServer. 

Here is what I think is happening : 
When a socket is configured with only one either ECC/RSA cert . Client sends request for the other one RSA/ECC.  We dynamically add the certificate in SNI callback using SSL_ConfigureSecureServer. But I think its too late. Even before that tstclnt gets the error.

u238590

Reporter

Comment 2

•

13 years ago

Reason why tstclnt is getting "no common cipher ..." error is  : 

 In ssl3_HandleClientHello in file "ssl3con.c", NSS is calling this function  

    /* Disable any ECC cipher suites for which we have no cert. */  
    ssl3_FilterECCipherSuitesByServerCerts(ss);

see http://mxr.mozilla.org/mozilla/source/security/nss/lib/ssl/ssl3con.c#6190

Which disables all ECC ciphers if no ECC certs are registered with the socket  yet. 

We are not registering default ECC cert with this socket in the beginning. 

We add ECC cert when SNI callback is called later which is too late.

So for SNI to work, we must mandate that the users have to add atleast one cert of each type as the default cert in the beginning for all the certs of that type to work in SNI callback.

u238590

Reporter

Comment 3

•

13 years ago

Also an assumption is made in function ssl3_config_match_init() that if the server cert of that type is already registered as a default certificate for that socket, then only set suite->isPresent flag true. 

http://mxr.mozilla.org/mozilla/source/security/nss/lib/ssl/ssl3con.c#695

 694         svrAuth = ss->serverCerts + exchKeyType;
...
 700         suite->isPresent = (PRBool)
 701         (((exchKeyType == kt_null) ||
 702            ((!isServer || (svrAuth->serverKeyPair &&
 703                            svrAuth->SERVERKEY &&
 704                    svrAuth->serverCertChain)) &&
 705             PK11_TokenExists(kea_alg_defs[exchKeyType]))) &&
 706         ((cipher_alg == calg_null) || PK11_TokenExists(cipher_mech)));

u238590

Reporter

Comment 4

•

13 years ago

As far as core dump is concerned, 

    5966 ssl3_HandleClientHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
calls   ssl3_HandleHelloExtensions() where xtnData is populated as shown below :
 (dbx) p *xtnData
  *xtnData = {
     serverSenders           = (
     {
         sniNameArr              = 0x690a8
        sniNameArrSize          = 1U
     }
 (dbx) p *xtnData->sniNameArr
  *xtnData->sniNameArr = {
      type = siBuffer
      data = 0xa46c2 "www.abc.com"
      len  = 11U
 }
but in this case my program goes to alert_loser  
http://mxr.mozilla.org/mozilla/source/security/nss/lib/ssl/ssl3con.c#6287
    6286     errCode = SSL_ERROR_NO_CYPHER_OVERLAP;
    6287     goto alert_loser;

Since it goes to alert_loser i.e. line 6655 , it skips lines where we are freeing this sniNameArr :

    6603         if (ss->xtnData.sniNameArr) {
    6604             PORT_Free(ss->xtnData.sniNameArr);
    6605             ss->xtnData.sniNameArr = NULL;
    6606             ss->xtnData.sniNameArrSize = 0;
    6607         }

in the end in my program, PR_Close()  calls ssl_DestroySocketContents() which calls  ssl_DestroyGather(). As you can see ssl_DestroyGather sets "www.abc.com" to "".

http://mxr.mozilla.org/mozilla/source/security/nss/lib/ssl/ssl3con.c#6287 
(dbx) n
t@1 (l@1) stopped in ssl_DestroySocketContents at line 411 in file "sslsock.c"
  411       ssl_DestroyGather(&ss->gs);
j = <not active>
*ss->xtnData.sniNameArr = {
    type = siBuffer
    data = 0xa46c2 "www.abc.com"
    len  = 11U
}
ss->xtnData.sniNameArrSize = 1U
(dbx) n
t@1 (l@1) stopped in ssl_DestroySocketContents at line 413 in file "sslsock.c"
  413       if (ss->peerID != NULL)
j = <not active>
*ss->xtnData.sniNameArr = {
    type = siBuffer
    data = 0xa46c2 ""
    len  = 11U
}
and then it dumps core at

t@1 (l@1) stopped in ssl_DestroySocketContents at line 441 in file "sslsock.c"
  441       PORT_Assert(!ss->xtnData.sniNameArr);

u238590

Reporter

Comment 5

•

13 years ago

Attached patch Attaching rough patch given by Alexei (not ready for review) — Details — Splinter Review

Attaching rough patch given by Alexei (not ready for review). There is one bug in this patch as given in https://bugzilla.mozilla.org/show_bug.cgi?id=693274#c2

Alexei Volkov

Comment 6

•

13 years ago

Attached patch Patch v1 - set up socket for the negotiated name before checking cache — Details — Splinter Review

Assertion happens during the restart due to incorrect setup of the socket when session info is taken from cache. The solution is to pre-configure socket with the sni callback before restring session info from the cache. It has minimum performance impact to restart.

This fix will also resolve the bug 693274

Attachment #567391 - Flags: review?(nelson)

u238590

Reporter

Comment 7

•

13 years ago

Thanx Alexei for working in the weekend.

Julien Pierre

Updated

•

13 years ago

OS: Solaris → All

Hardware: Sun → All

u238590

Reporter

Comment 8

•

13 years ago

Can you tell me which build tags should I use to check out NSS and NSPR to test this patch?

From what I see this patch is built on cvs revision 1.51
    https://bug675876.bugzilla.mozilla.org/attachment.cgi?id=567391&action=diff&collapsed=&context=patch&format=raw&headers=1
    See at the top.

From cvs stats it has one more version 1.52
    $cvs stat ssl3con.c
    ...
    revision 1.152
    date: 2011/10/01 03:59:54; author: bsmith%mozilla.com; state: Exp; lines: +68 -28   
    Bug 665814: Prevent chosen plaintext attacks on SSL 3.0 and TLS 1.0 connections, r=wtc, sr=rrelyea   
    ---------------------------- 
    revision 1.151    
    date: 2011/07/26 02:13:37;  author: wtc%google.com;  state: Exp; lines: +3 -4  
    Bug 673477: Expose the error code set by CERT_ExtractPublicKey in ssl3_VerifySignedHashes and ssl3_SendClientKeyExchange.  r=wtc.      
    ----------------------------

As per the graph http://bonsai.mozilla.org/cvsgraph.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c
1.51 was checked into NSS_3_13_BETA1
and 
1.52 was checked into NSS_3_13_RTM, NSS_3_13_RC0, NSS_3_13_BETA2, HEAD.

u238590

Reporter

Comment 9

•

13 years ago

Alexei,
I patched these diffs on NSS 3.12 as shown below.

cvs -q -z3 co -P -r NSPR_4_8_9_RTM mozilla/nsprpub
cvs -q -z3 co -P -r NSS_3_12_BRANCH mozilla/dbm mozilla/security/dbm mozilla/security/coreconf mozilla/security/nss
cvs -q -z3 co -P -r NSS_3_11_1_RTM mozilla/security/nss/lib/freebl/ecl/ecl-curve.h
cd mozilla/security/nss
gmake nss_build_all NSS_ENABLE_ECC=1 NSS_ECC_MORE_THAN_SUITE_B=1 

Assert failures are gone. But the test program attached is failing. Can you please take a look? I am still getting in the client :

tstclnt: write to SSL socket failed: Cannot communicate securely with peer: no common encryption algorithm(s).

u238590

Reporter

Comment 10

•

13 years ago

Alexei,
    Could this failure be due to the fact that  client uses v2 hello? if yes, how can I change the client code for testing? 
Regards, 
Meena

u238590

Reporter

Comment 11

•

13 years ago

I tested with our server and tstclnt its working. So looks like the test program I attached was buggy. Please ignore my last two comments.

u238590

Reporter

Comment 12

•

13 years ago

Alexei,
    This doesn't fix the reconfig Bug 693274 - "If SNI callback function returns a new cert for the same SNI name, SSL session cache returns the older cert by mistake"

u238590

Reporter

Comment 13

•

13 years ago

I debugged what is happening in my test program, I saw

Its disabling ECC suites first sslecc.cpp file function ssl3_HandleSupportedCurvesXtn

1184 ssl3_DisableECCSuites(ss, ecdhe_ecdsa_suites);

STACK : 

(gdb) where
#0  ssl3_HandleSupportedCurvesXtn (ss=0x6a6280, ex_type=10, data=0x7fffffffd820) at ssl3ecc.c:1184
#1  0x00002aaaab0b18af in ssl3_HandleHelloExtensions (ss=0x6a6280, b=0x7fffffffd880,   length=0x7fffffffd87c) at ssl3ext.c:1384
#2  0x00002aaaab09e44b in ssl3_HandleClientHello (ss=0x6a6280, b=0x6adc6d "", length=6)  at ssl3con.c:6192
#3  0x00002aaaab0a3855 in ssl3_HandleHandshakeMessage (ss=0x6a6280,   b=0x6adbf4 "\003\001N\242\263h\313\372u0\263\234g\374\061\236\254\377\071+\260\224\373\315\036\224\300\273\027C\375I\330#", length=127) at ssl3con.c:8586
#4  0x00002aaaab0a3cee in ssl3_HandleHandshake (ss=0x6a6280, origBuf=0x6a65e8) at ssl3con.c:8721
#5  0x00002aaaab0a48ca in ssl3_HandleRecord (ss=0x6a6280, cText=0x7fffffffdbf0, databuf=0x6a65e8)   at ssl3con.c:9060
#6  0x00002aaaab0a5a65 in ssl3_GatherCompleteHandshake (ss=0x6a6280, flags=0) at ssl3gthr.c:209
#7  0x00002aaaab0a8694 in ssl_GatherRecord1stHandshake (ss=0x6a6280) at sslcon.c:1258
#8  0x00002aaaab0b4203 in ssl_Do1stHandshake (ss=0x6a6280) at sslsecur.c:151
#9  0x00002aaaab0b61cb in ssl_SecureRecv (ss=0x6a6280, buf=0x7fffffffe190 "", len=1024, flags=0)
    at sslsecur.c:1150
#10 0x00002aaaab0b6297 in ssl_SecureRead (ss=0x6a6280, buf=0x7fffffffe190 "", len=1024)   at sslsecur.c:1169
#11 0x00002aaaab0be4c2 in ssl_Read (fd=0x695250, buf=0x7fffffffe190, len=1024) at sslsock.c:1639
#12 0x00002aaaaaabc8e2 in PR_Read (fd=0x695250, buf=0x7fffffffe190, amount=1024)   at ../../../../pr/src/io/priometh.c:141
#13 0x0000000000401449 in readDataFromSocket (sslSocket=0x695250) at my.cpp:195
#14 0x00000000004014f7 in read_write (fd=0x695250) at my.cpp:228
#15 0x0000000000401bfa in main (argc=2, argv=0x7fffffffe778) at my.cpp:293

....

Later In
5874 static int
5875 ssl3_sniSocketConfig(sslSocket *ss, SSL3AlertDescription *desc,
5876                      int *errCode)

It calls SNI Callback function of my test program which returns 0 which is the index of SNI Names array

so it goes to
5928         } else if (ret < ss->xtnData.sniNameArrSize) {

It calls ssl3_config_match_init :

5964	    configedCiphers = ssl3_config_match_init(ss);

In this function I printed the cipher suite set unfortunately it contains only 5 enabled ciphers : 

 {{cipher_suite = 49162, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49172, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 136, policy = 1, enabled = 0,  isPresent = 0}, 
 {cipher_suite = 135, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 57, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 56, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49167, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49157, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 132, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 53, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49159, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49161, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49169,  policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49171, policy = 1, enabled = 0,  isPresent = 0}, 
 {cipher_suite = 69, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 68, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 102, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 51, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 50, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49164, policy = 1,  enabled = 0, isPresent = 0}, 
 {cipher_suite = 49166, policy = 1, enabled = 0, isPresent = 0},
 {cipher_suite = 49154, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49156, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 150, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 65, policy = 1, enabled = 0, isPresent = 0}, 
{cipher_suite = 4, policy = 1, enabled = 1, isPresent = 0}, 
 {cipher_suite = 5, policy = 1,  enabled = 0, isPresent = 0}, 
 {cipher_suite = 47, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49160, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49170,  policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 22, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 19, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49165, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49155, policy = 1, enabled = 0, isPresent = 0}, 
{cipher_suite = 65279, policy = 1, enabled = 1, isPresent = 0}, 
{cipher_suite = 10, policy = 1, enabled = 1, isPresent = 0}, 
 {cipher_suite = 21, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 18, policy = 1, enabled = 0, isPresent = 0}, 
{cipher_suite = 65278, policy = 1, enabled = 1, isPresent = 0}, 
{cipher_suite = 9, policy = 1, enabled = 1, isPresent = 0}, 
 {cipher_suite = 100, policy = 1,  enabled = 0, isPresent = 0},
 {cipher_suite = 98, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 3, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 6, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49158, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49168, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49163,  policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 49153, policy = 1, enabled = 0,  isPresent = 0}, 
 {cipher_suite = 2, policy = 1, enabled = 0, isPresent = 0}, 
 {cipher_suite = 1, policy = 1, enabled = 0, isPresent = 0}}

When I printed out the details of these ciphers I got : 
cipher_suite #   bulk_cipher_alg   mac_alg       key_exchange_alg
             4        cipher_rc4   ssl_mac_md5   kea_rsa
         65279       cipher_3des   ssl_mac_sha   kea_rsa_fips
            10       cipher_3des   ssl_mac_sha   kea_rsa
         65278        cipher_des   ssl_mac_sha   kea_rsa_fips
             9        cipher_des   ssl_mac_sha   kea_rsa

As you can see no ECC ciphers are enabled in the above set so it eventually fails in

6423	    errCode = SSL_ERROR_NO_CYPHER_OVERLAP;
6424	    goto alert_loser;

of lines ssl3_HandleClientHello.

Hope this helps to debug the reason why my test program doesn't work.

u238590

Reporter

Comment 14

•

13 years ago

My guess is the cipher in my test case and curve do not match. I will fix my test case and let you know.

u238590

Reporter

Comment 15

•

13 years ago

When I try the reverse case ECC in listen socket and RSA in SNI VS and passing -c c (RSA cipher) in the test client. That works. :-)

Even without SNI my test program crashes  in
    Assertion failure: numPresent > 0 || numEnabled == 0, at ssl3con.c:711
    Aborted
So confirms my belief about curve and cipher.

But when I test in Web Server, the same tstclnt, same NSS libraries, this works. wonder how. 

Here is ssltap output :
    --> [
    (136 bytes of 131)
    SSLRecord { [Mon Oct 24 08:36:09 2011]
       type    = 22 (handshake)
       version = { 3,1 }
       length  = 131 (0x83)
       handshake {
          type = 1 (client_hello)
          length = 127 (0x00007f)
             ClientHelloV3 {
                client_version = {3, 1}
   ...
                cipher_suites[2] = {
                    (0x00ff) TLS_EMPTY_RENEGOTIATION_INFO_SCSV
                    (0xc00a) TLS/ECDHE-ECDSA/AES256-CBC/SHA
                }
                compression[1] = {
                    (00) NULL
                }
                extensions[82] = {
                  extension type server_name, length [16] = {
       0: 00 0e 00 00  0b 77 77 77  2e 61 62 63  2e 63 6f 6d  | .....www.abc.com
                  }
                  extension type elliptic_curves, length [52] = {
       0: 00 32 00 01  00 02 00 03  00 04 00 05  00 06 00 07  | .2..............
      10: 00 08 00 09  00 0a 00 0b  00 0c 00 0d  00 0e 00 0f  | ................
      20: 00 10 00 11  00 12 00 13  00 14 00 15  00 16 00 17  | ................
      30: 00 18 00 19                                         | ....
                  }
                  extension type ec_point_formats, length [2] = {
       0: 01 00                                               | ..
                  }
                }
             }
       }
    }
    ]
    <-- [
    (7 bytes of 2)
    SSLRecord { [Mon Oct 24 08:36:09 2011]
       type    = 21 (alert)
       version = { 3,1 }
       length  = 2 (0x2)
       fatal: handshake_failure
    }
    ]
...

My cert is :

    $certutil -L -d sql:. -n ecccert
    Certificate:
        Data:
            Version: 3 (0x2)
            Serial Number: 1343 (0x53f)
            Signature Algorithm: X9.62 ECDSA signature with SHA-1
            Issuer: "CN=www.abc.com"
            Validity:  Not Before: ...  Not After : ...
            Subject: "CN=www.abc.com"
            Subject Public Key Info:
                Public Key Algorithm: X9.62 elliptic curve public key
                    Args:   06:08:2a:86:48:ce:3d:03:01:07
                EC Public Key:
                    PublicValue:
                        04:fd:77:40:1e:8f:82:53:c2:b3:51:38:b1:dd:4f:14:
                        3f:96:e9:a8:ef:ae:f4:f4:cb:3f:63:38:5d:68:69:12:
                        33:a0:ce:ac:21:0c:96:e7:4b:b3:d2:c0:29:db:8c:92:
                        32:cc:72:f0:06:73:7b:ec:90:d4:04:35:e4:cf:0c:b3:
                        e2
                    Curve: ANSI X9.62 elliptic curve prime256v1 (aka secp256r1, NIST P-256)
            Signed Extensions:  Name: Certificate Type   Critical: True   Data: <SSL Server,SSL CA>
        Signature Algorithm: X9.62 ECDSA signature with SHA-1
        Signature:
  ...

Julien Pierre

Comment 16

•

13 years ago

Meena,

Exactly how did you trigger another assertion ? Please detail what the test case is.

Julien Pierre

Comment 17

•

13 years ago

Comment on attachment 566166 [details] [diff] [review]
Attaching rough patch given by Alexei (not ready for review)

Review of attachment 566166 [details] [diff] [review]:
-----------------------------------------------------------------

This patch was reportedly not working.

Attachment #566166 - Flags: review-

Julien Pierre

Comment 18

•

13 years ago

I found a  problem.

The test program uses SSL_CipherPrefSetDefault to create the mode socket. If the listener only has an RSA cert, NSS disables the ECC cipher suites early on during handshake processing.
The ECC suites don't get re-enabled again after SNI processing.

If the program is modified to use SSL_CipherPrefSet instead, then things work as expected.

Julien Pierre

Comment 19

•

13 years ago

Bob, Wan-Teh,

We need a reviewer for the latest patch attached to this bug. Alexei is no longer with Oracle, and Nelson doesn't have the time. Would either of you be able to take care of it this week ?

u238590

Reporter

Comment 20

•

13 years ago

If I modify this test program I attached and remove all Virtual Server and SNI part and have just ECC certificate set on listener and with cipher set as follows:

  secStatus = SSL_CipherPrefSetDefault(TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, PR_TRUE);

Run this command
/export/home/meena/security/nss-sni-fix-3.12/Linux2.6_64_DBG.OBJ/bin/tstclnt -c :C00A -h hostname -d sql:. -o -p 3333 -2 -a www.abc.com < req.txt

My test program crashes as shown below

$./a.out 3333
Create Listener model socket, add SSL properties, add certificate.
About to call accept.
Reading data from socket...
Assertion failure: numPresent > 0 || numEnabled == 0, at ssl3con.c:711
Aborted

$gdb a.out core
...
(gdb) p numPresent
$1 = 0
(gdb) p numEnabled
$2 = 5
(gdb) where
#0  0x0000003beea30265 in raise () from /lib64/libc.so.6
#1  0x0000003beea31d10 in abort () from /lib64/libc.so.6
#2  0x00002b128a28fa9e in PR_Assert (s=0x2b128a897f80 "numPresent > 0 || numEnabled == 0",
    file=0x2b128a897f3e "ssl3con.c", ln=711) at ../../../../pr/src/io/prlog.c:577
#3  0x00002b128a861334 in ssl3_config_match_init (ss=0x97e4510) at ssl3con.c:711
#4  0x00002b128a875412 in ssl2_ConstructCipherSpecs (ss=0x97e4510) at sslcon.c:206
#5  0x00002b128a87d574 in ssl2_BeginServerHandshake (ss=0x97e4510) at sslcon.c:3800
#6  0x00002b128a883203 in ssl_Do1stHandshake (ss=0x97e4510) at sslsecur.c:151
#7  0x00002b128a8851cb in ssl_SecureRecv (ss=0x97e4510, buf=0x7fff4be7f500 "", len=1024, flags=0) at sslsecur.c:1150
#8  0x00002b128a885297 in ssl_SecureRead (ss=0x97e4510, buf=0x7fff4be7f500 "", len=1024) at sslsecur.c:1169
#9  0x00002b128a88d4c2 in ssl_Read (fd=0x97de2a0, buf=0x7fff4be7f500, len=1024) at sslsock.c:1639
#10 0x00002b128a28b8e2 in PR_Read (fd=0x97de2a0, buf=0x7fff4be7f500, amount=1024) at ../../../../pr/src/io/priometh.c:141
#11 0x0000000000401269 in readDataFromSocket (sslSocket=0x97de2a0) at my.cpp:126
#12 0x0000000000401317 in read_write (fd=0x97de2a0) at my.cpp:159
#13 0x0000000000401804 in main (argc=2, argv=0x7fff4be7fad8) at my.cpp:219
(gdb)

u238590

Reporter

Comment 21

•

13 years ago

Attached file assert.cpp test program for crash as reported above — Details

assert.cpp test program for crash as reported above. 
cat Makefile:
SECURITY_DIR=/bucket2/home/meena/security/old/64/
all:
	g++ -g -I$(SECURITY_DIR)/include -L$(SECURITY_DIR)/lib -lnspr4 -lnss3 -lssl3 my.cpp

u238590

Reporter

Comment 22

•

13 years ago

Attached file NSS DBs for testing above crash — Details

u238590

Reporter

Comment 23

•

13 years ago

When I replace      

SSL_CipherPrefSetDefault(TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, PR_TRUE); 

by

SSL_CipherPrefSet(sock, TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, PR_TRUE);

it fixes this crash.

So we are not having any crashes now.

Julien Pierre

Comment 24

•

13 years ago

Meena,

That's because your test program is calling SSL_CipherPrefSetDefault after it calls SSL_ImportFD. The default setting only applies to new sockets. If you move your SSL_CipherPrefSetDefault up, before SSL_ImportFD, then it works as expected - there is no assertion. I don't know if NSS should really asset for this case, as it's actually an application bug if no ciphers have been configured on the socket at all.

IMO, the ssl3con.c assertion should be changed. It should not assert that numEnabled is greater than 0 . Even without SNI involved, this is incorrect - it's still an application configuration error, not an NSS bug, so NSS should just return SSL_ERROR_NO_CIPHERS_SUPPORTED .

Nelson Bolyard (seldom reads bugmail)

Comment 25

•

13 years ago

Julien, You're right that it's an application configuration error when no cipher
suites are enabled, and that we don't generally assert for application errors.
This is an exception to that general rule. There's a reason for this exception.

Before that assertion was put in, the most common complaint that the NSS team 
would receive in bug reports from the field was that "the server doesn't work".
They'd misconfigure it, and instead of the server failing in configuration, 
it would simply fail to complete any handshakes.  This was due, in part, to the 
server apps not doing sufficient sanity checking on their own configurations,
and even ignoring error codes returned by some calls.  something that the NSS 
team was unable to get applications to correct.

The "server doesn't work" reports were frequent, time consuming.  Now, the 
assertion failure is trivial to spot and understand, and will not be ignored.

Alexei Volkov

Updated

•

12 years ago

Attachment #567391 - Flags: review?(nelson) → review?(bsmith)

Alexei Volkov

Updated

•

12 years ago

Blocks: 693274

Brian Smith (:briansmith, :bsmith, use NEEDINFO?)

Comment 26

•

10 years ago

Comment on attachment 567391 [details] [diff] [review]
Patch v1 - set up socket for the negotiated name before checking cache

Review of attachment 567391 [details] [diff] [review]:
-----------------------------------------------------------------

Dropping this old review request. I'm not as familiar with the server-side stuff as others are.

Attachment #567391 - Flags: review?(brian)

BMO Automation

Updated

•

2 years ago

Severity: normal → S3

Benjamin Beurdouche [:beurdouche]

Updated

•

6 months ago

Severity: S3 → S4

Status: NEW → RESOLVED

Closed: 6 months ago

Priority: -- → P5

Resolution: --- → INACTIVE

Server code to generate the core dump 13 years ago u238590 8.75 KB, text/plain		Details
Attaching rough patch given by Alexei (not ready for review) 13 years ago u238590 14.84 KB, patch	julien.pierre : review-	Details \| Diff \| Splinter Review
Patch v1 - set up socket for the negotiated name before checking cache 13 years ago Alexei Volkov 17.21 KB, patch		Details \| Diff \| Splinter Review
assert.cpp test program for crash as reported above 13 years ago u238590 6.39 KB, text/plain		Details
NSS DBs for testing above crash 13 years ago u238590 9.90 KB, application/octet-stream		Details