631986 - SSL_ReconfigFD tries to access elements of a null pointer.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[u238590]

http://mxr.mozilla.org/security/source/security/nss/lib/ssl/sslsock.c#1270

1269 PRFileDesc *
1270 SSL_ReconfigFD(PRFileDesc *model, PRFileDesc *fd)
1271 {
1272     sslSocket * sm = NULL, *ss = NULL;
1273     int i;
1274     sslServerCerts * mc = sm->serverCerts;
1275     sslServerCerts * sc = ss->serverCerts;

Comment 1

[u238590]

Attachment: Server program to show core dump

Added is the sample server.cpp.

$cat Makefile
SECURITY_DIR=/share/builds/components/security/SECURITY_3.12.8_20100916/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 server.cpp

TO CREATE NSS DBs : 

$certutil -N -d .
$certutil -S -n Server-Cert -s "CN=test.india.sun.com" -x -t "CT,CT,CT" -d .
$certutil -S -n abc_rsa  -s "CN=www.abc.com" -x -t "CT,CT,CT" -d .
$cat >> sslreq.dat
GET /test.html HTTP/1.0

$

In another window 
$tstclnt -h test.india.sun.com -a www.abc.com -p 333 -n Server-Cert -2 -d  . -v -o -f < sslreq.dat

In one window run this server :
$./a.out 3333

Create VS model socket, add SSL properties, add certificate.

Create Listener model socket, add SSL properties, add certificate.
Registered SNI Callback function for this model



About to call accept.

Reading data from socket...

Reached SNI callback function
going to compare host www.def.com and vsname www.abc.com
27 bytes read [GET /test.html HTTP/1.0

]
Writing data to socket...




About to call accept.

Reading data from socket...

Reached SNI callback function
going to compare host www.abc.com and vsname www.abc.com
Segmentation Fault (core dumped)

core file :
$dbx a.out core.5796
...
t@1 (l@1) program terminated by signal SEGV (no mapping at the fault address)
ck
Current function is SSL_ReconfigFD
 1305           if (mc->serverCert && mc->serverCertChain) {

(dbx) where
current thread: t@1
=>[1] SSL_ReconfigFD(model = 0x24f90, fd = 0x25270), line 1305 in "sslsock.c"
  [2] mySSLSNISocketConfig(fd = 0x25270, sniNameArr = 0x47fd8, sniNameArrSize = 1U, arg = 0x24f90), line 78 in "server.cpp"
  [3] ssl3_HandleClientHello(ss = 0x82aa0, b = 0x8a035 "", length = 0), line 6503 in "ssl3con.c"
  [4] ssl3_HandleHandshakeMessage(ss = 0x82aa0, b = 0x89fe4 "^C^AMQ5\xf1^]y\xcf\xc4^_\xf0\xea^P\xd6\xd8^F\xca\xaamq\x84^^P^N\Cb,\xcfO\x8aZ\xe5", length = 81U), line 8592 in "ssl3con.c"
  [5] ssl3_HandleHandshake(ss = 0x82aa0, origBuf = 0x82cf8), line 8727 in "ssl3con.c"
  [6] ssl3_HandleRecord(ss = 0x82aa0, cText = 0xffbfe7a0, databuf = 0x82cf8), line 9066 in "ssl3con.c"
  [7] ssl3_GatherCompleteHandshake(ss = 0x82aa0, flags = 0), line 209 in "ssl3gthr.c"
  [8] ssl_GatherRecord1stHandshake(ss = 0x82aa0), line 1258 in "sslcon.c"
  [9] ssl_Do1stHandshake(ss = 0x82aa0), line 151 in "sslsecur.c"
  [10] ssl_SecureRecv(ss = 0x82aa0, buf = 0xffbfeaa4 "", len = 1024, flags = 0), line 1141 in "sslsecur.c"
  [11] ssl_SecureRead(ss = 0x82aa0, buf = 0xffbfeaa4 "", len = 1024), line 1160 in "sslsecur.c"
  [12] ssl_Read(fd = 0x25270, buf = 0xffbfeaa4, len = 1024), line 1632 in "sslsock.c"
  [13] PR_Read(fd = 0x25270, buf = 0xffbfeaa4, amount = 1024), line 141 in "priometh.c"
  [14] readDataFromSocket(sslSocket = 0x25270), line 197 in "server.cpp"
  [15] read_write(fd = 0x25270), line 218 in "server.cpp"
  [16] main(argc = 2, argv = 0xffbff06c), line 283 in "server.cpp"
(dbx)
(dbx) p mc
mc = 0x2b4
(dbx) p *mc
dbx: cannot access address 0x2b4
(dbx)

Comment 2

[u238590]

Attachment: Moved two lines below

This is my proposed bug fix on NSS 3.12.9. i.e. 
$export CVSROOT=:pserver:[e-mail]:/cvsroot
$cvs co -r NSPR_4_8_7_RTM NSPR
$cvs co -r NSS_3_12_9_RTM NSS

Tested : this server program (attached in this bug) doesn't dump core anymore with these diffs.

Comment 3

[u238590]

Nelson/Rob can you please verify this bug and review this patch? 

Test results :
HOST=... DOMSUF=india.sun.com ./all.sh |tee all.log

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 4

[Alexei Volkov]

Comment on attachment 511008 [details] [diff] [review]
Moved two lines below

Meena, the place of assignment of mc and sc is correct. But mc, sc can not be declared in the middle of the function.

Comment 5

[u238590]

Attachment: Initially making mc and sc point to NULL, and assigning the correct values later just before for loop

Sorry I had used (C++ compiler) CC by mistake. Now I am using cc(C compiler).

Comment 6

[u238590]

Can I get a super-review for this small change?

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 511653 [details] [diff] [review]
Initially making mc and sc point to NULL, and assigning the correct values later just before for loop

>+    mc = sm->serverCerts;
>+    sc = ss->serverCerts;
>     for (i=kt_null; i < kt_kea_size; i++, mc++, sc++) {
>         if (mc->serverCert && mc->serverCertChain) {
>             if (sc->serverCert) {

This code is an improvement over the previous code, but it is still 
vulnerable to a crash.  Nothing ensures that mc and sc are both non-null
here.  If either is null, it will crash.

Comment 8

[u238590]

Ok I'll fix it. 

If SSL_ConfigureSecureServer is not called on that fd, it will ASSERT some place before. 

Julien had proposed, in SNI client side  bug, a flag that we should have set so that we can create and import model sockets which do not have any certificates (SSL_ConfigureSecureServer is not called). Not sure if it will be useful.

Comment 9

[u238590]

 Can mc and sc be NULL?  They are pointing to an array of sslServerCerts.

    1007 struct sslSocketStr {
    1008     PRFileDesc *    fd;
    ...
    1111     sslServerCerts        serverCerts[kt_kea_size];
    ...


CASE 1 :
~~~~~~~~ 
If I didn't call SSL_ConfigureSecureServer on the original (listener) FD, i.e. have ZERO certificate, it dumps here so I can not test it:

    $mdb core.13038
    Loading modules: [ libc.so.1 libuutil.so.1a ld.so.1 ]
    > ::stack
    libc.so.1`_lwp_kill+8(6, 0, 0, fee2c1f8, ffffffff, 6)
    libc.so.1`abort+0x110(0, 1, fee2c1d4, eea98, feeb3418, 0)
    libnspr4.so`PR_Assert+0xac(ff06ef44, ff06ef68, 2c7, 0, ff3f4910, 0)
    libssl3.so`ssl3_config_match_init+0x3b8(7c100, fefb07c0, ff03b5cc, ...) 
    libssl3.so`ssl2_ConstructCipherSpecs+0x164(7c100, 1, 0, 0, 7b850, 1)
    libssl3.so`ssl2_BeginServerHandshake+0xd4(7c100, 1, 1000, 202, ff3f4248, 1)
    libssl3.so`ssl_Do1stHandshake+0x308(7c100, 0, 0, 1c00, 0, 0)
    libssl3.so`ssl_SecureRecv+0x230(7c100, ffbfeac4, 400, 0, 62e58, ff3c9a74)
    libssl3.so`ssl_SecureRead+0x28(7c100, ffbfeac4, 400, 0, ff3f42f0, 0)
    libssl3.so`ssl_Read+0x114(25230, ffbfeac4, 400, 80808080, 1e, 42)
    libnspr4.so`PR_Read+0x30(25230, ffbfeac4, 400, fffffff8, 0, ffbfeecd)
    __1cSreadDataFromSocket6FpnKPRFileDesc__nK_SECStatus__+0x2c(25230, ...)
    __1cKread_write6FpnKPRFileDesc__nK_SECStatus__+0x1c(25230, 1, ...)
    main+0x188(2, ffbff08c, ffbff098, 22c00, fed20200, 0)
    _start+0x108(0, 0, 0, 0, 0, 0)


CASE 2 :
~~~~~~~~
    If I call SSL_ConfigureSecureServer in the original (listener) fd i.e. add alteast one cert, AND
       do not call SSL_ConfigureSecureServer on the model fd, i.e. have ZERO cert,
      this for loop does nothing which seems right to me.

Here is what happens in the debugger  :

        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1304 in file "sslsock.c"
         1304       mc = sm->serverCerts;
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1305 in file "sslsock.c"
         1305       sc = ss->serverCerts;
        (dbx) p mc
        mc = 0x56894
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1306 in file "sslsock.c"
         1306       for (i=kt_null; i < kt_kea_size; i++, mc++, sc++) {
        (dbx) p *mc
        *mc = {
            serverCert      = (nil)
            serverCertChain = (nil)
            serverKeyPair   = (nil)
            serverKeyBits   = 0
        }
        (dbx) p *sc
        *sc = {
            serverCert      = (nil)
            serverCertChain = (nil)
            serverKeyPair   = (nil)
            serverKeyBits   = 0
        }
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1307 in file "sslsock.c"
         1307           if (mc->serverCert && mc->serverCertChain) {
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1319 in file "sslsock.c"
         1319           if (mc->serverKeyPair) {
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1306 in file "sslsock.c"
         1306       for (i=kt_null; i < kt_kea_size; i++, mc++, sc++) {
        (dbx) p mc
        mc = 0x56894
        (dbx) p *mc
        *mc = {
            serverCert      = (nil)
            serverCertChain = (nil)
            serverKeyPair   = (nil)
            serverKeyBits   = 0
        }
        (dbx) p *sc
        *sc = {
            serverCert      = (nil)
            serverCertChain = (nil)
            serverKeyPair   = (nil)
            serverKeyBits   = 0
        }
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1307 in file "sslsock.c"
         1307           if (mc->serverCert && mc->serverCertChain) {
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1319 in file "sslsock.c"
         1319           if (mc->serverKeyPair) {
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1306 in file "sslsock.c"
         1306       for (i=kt_null; i < kt_kea_size; i++, mc++, sc++) {
        (dbx) p *mc
        *mc = {
            serverCert      = (nil)
            serverCertChain = (nil)
            serverKeyPair   = (nil)
            serverKeyBits   = 0
        }
        (dbx) p *sc
        *sc = {
            serverCert      = 0x79f70
            serverCertChain = 0x7fe30
            serverKeyPair   = 0x38fa8
            serverKeyBits   = 1024U
        }
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1307 in file "sslsock.c"
         1307           if (mc->serverCert && mc->serverCertChain) {
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1319 in file "sslsock.c"
         1319           if (mc->serverKeyPair) {
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1306 in file "sslsock.c"
         1306       for (i=kt_null; i < kt_kea_size; i++, mc++, sc++) {
        (dbx) p *mc
        *mc = {
            serverCert      = (nil)
            serverCertChain = (nil)
            serverKeyPair   = (nil)
            serverKeyBits   = 0
        }
        (dbx) p *sc
        *sc = {
            serverCert      = (nil)
            serverCertChain = (nil)
            serverKeyPair   = (nil)
            serverKeyBits   = 0
        }
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1307 in file "sslsock.c"
         1307           if (mc->serverCert && mc->serverCertChain) {
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1319 in file "sslsock.c"
         1319           if (mc->serverKeyPair) {
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1306 in file "sslsock.c"
         1306       for (i=kt_null; i < kt_kea_size; i++, mc++, sc++) {
        (dbx) p *mc
        *mc = {
            serverCert      = (nil)
            serverCertChain = (nil)
            serverKeyPair   = (nil)
            serverKeyBits   = 0
        }
        (dbx) p *sc
        *sc = {
            serverCert      = (nil)
            serverCertChain = (nil)
            serverKeyPair   = (nil)
            serverKeyBits   = 0
        }
        (dbx) p i
        i = 3
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1307 in file "sslsock.c"
         1307           if (mc->serverCert && mc->serverCertChain) {
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1319 in file "sslsock.c"
         1319           if (mc->serverKeyPair) {
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1306 in file "sslsock.c"
         1306       for (i=kt_null; i < kt_kea_size; i++, mc++, sc++) {
        (dbx) p i
        i = 4
        (dbx) p *mc
        *mc = {
            serverCert      = (nil)
            serverCertChain = (nil)
            serverKeyPair   = (nil)
            serverKeyBits   = 0
        }
        (dbx) p *sc
        *sc = {
            serverCert      = (nil)
            serverCertChain = (nil)
            serverKeyPair   = (nil)
            serverKeyBits   = 0
        }
        (dbx) n
        t@1 (l@1) stopped in SSL_ReconfigFD at line 1327 in file "sslsock.c"
         1327       if (sm->stepDownKeyPair) {
        (dbx)

Comment 10

[u238590]

Should I change 
    mc = sm->serverCerts;
to
    mc = &(sm->serverCerts[0]);
and 
    mc++ 
to 
    &(sm->serverCerts[i]);
and same for sc.

Comment 11

[u238590]

Attachment: Moved mc and sc assignments inside for loop

Comment 12

[Alexei Volkov]

Comment on attachment 514188 [details] [diff] [review]
Moved mc and sc assignments inside for loop

r+

Comment 13

[Alexei Volkov]

Comment on attachment 514188 [details] [diff] [review]
Moved mc and sc assignments inside for loop

Bob, please review.

Comment 14

[Robert Relyea]

Comment on attachment 514188 [details] [diff] [review]
Moved mc and sc assignments inside for loop

r+

Normally I wouldn't like the &val->array[x] format, but in this case the reviewer can easily miss that val->array is an array without it, so I think it's a good choice. Thanks Meena for the patch.

Alexi are you going to check it in?

bob

Comment 15

[Alexei Volkov]

checked into 3.13. Waiting for tinderbox results.

Comment 16

[u238590]

Can you also add them to NSS 3.12.10?

Comment 17

[Alexei Volkov]

Committed into trunk and branch.

Comment 18

[Christophe Ravel]

Assigned target milestone to indicate in which version this bug is fixed (NSS 3.12.10).