629299 - core dump when tls session tickets are enabled and session cache is disabled

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[u238590]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.237 Safari/534.10
Build Identifier: 3.12.8

core dump when tls session tickets are enabled and session cache is disabled

Reproducible: Always

Steps to Reproduce:
1. Create NSS Db and a self signed server cert named "Server-Cert"

2. When tls session tickets are enabled and ssl session cache is disabled (-N option), in selfserv 

$/share/builds/components/security/SECURITY_3.12.8_20100916/SunOS5.8_DBG.OBJ/bin/selfserv -p 4443 -d . -D -B -s -n Serevr-Cert -u -N
Segmentation Fault (core dumped)

3. In another window  run this stress client: $/share/builds/components/security/SECURITY_3.12.8_20100916/SunOS5.8_DBG.OBJ/bin/strsclnt -p 4443 -c 2 -o -d . -u -2 testhostname
strsclnt: -- SSL: Server Certificate Validated.
strsclnt: PR_Send returned error -5938:
Encountered end of file.
Actual Results:  
$mdb core.20033
> ::stack
libssl3.so`sslMutex_Lock+0xc(8, 0, 0, 0, 0, 0)
libssl3.so`LockSidCacheLock+0x2c(0, 0, 0, 0, 0, 0)
libssl3.so`ssl_GetSessionTicketKeys+0x50(ff38f9cc, ff38f9d8, ff38f9f8, 0, ffffffe0, 50d35)
libssl3.so`ssl3_GenerateSessionTicketKeys+0x74(0, 1, 0, 0, fe0f8a04, 0)
libnspr4.so`PR_CallOnce+0x80(ff38f9bc, ff335c00, 10, fe0f8af4, 0, fe763afc)
libssl3.so`ssl3_GetSessionTicketKeys+0x48(fe0f8a74, fe0f8a6c, fe0f8a70, fe0f8a68, 0, 0)
libssl3.so`ssl3_SendNewSessionTicket+0x2a0(9dbb8, 23, c, fe0f8b88, 0, fe0f8c05)
libssl3.so`ssl3_HandleFinished+0x51c(9dbb8, a50fc, c, fe0f8c28, 0, 20)
libssl3.so`ssl3_HandleHandshakeMessage+0x8d0(9dbb8, a50fc, c, 10, 0, 20)
libssl3.so`ssl3_HandleHandshake+0x2d8(9dbb8, 9de10, 10, 301, fe0f8d48, a50f8)
libssl3.so`ssl3_HandleRecord+0xb60(a50f8, 10, fe0f8d74, 1c00, 0, 4aec8)
libssl3.so`ssl3_GatherCompleteHandshake+0x110(9dbb8, 0, c90081, 9, 1, 0)
libssl3.so`ssl_GatherRecord1stHandshake+0xd0(9dbb8, fe0f9028, 1000, 202, ff3f4248, ff3f1978)
libssl3.so`ssl_Do1stHandshake+0x308(9dbb8, 1, fe763a00, ff3f4910, fedc729c, ff3f6ae8)
libssl3.so`ssl_SecureRecv+0x230(9dbb8, fe0f9408, 27ff, 0, ff3f4910, 821)
libssl3.so`ssl_SecureRead+0x28(9dbb8, fe0f9408, 27ff, 0, ff3f42f0, 0)
libssl3.so`ssl_Read+0x114(4c328, fe0f9408, 27ff, 0, 0, 0)
libnspr4.so`PR_Read+0x30(4c328, fe0f9408, 27ff, 0, fe763a00, 0)
handle_connection+0x39c(4c328, 4c0e8, 0, 1c00, 0, 4aec8)
jobLoop+0x1a8(0, 0, 0, 0, fe763a00, 0)
thread_wrapper+0x48(7da1c, 7df80, 0, 0, fe763a00, 0)
libnspr4.so`_pt_root+0x1bc(7df80, fe0fc000, 0, 0, fefebe28, 0)
libc.so.1`_lwp_start(0, 0, 0, 0, 0, 0)

Expected Results:  
no core dumps.

Comment 1

[u238590]

Question is whether "TLS session ticket extension" supposed to work without "session cache" or not?

Comment 2

[Alexei Volkov]

It suppose to work, but you will not be able to share server ticket encryption keys in between the processes.

Comment 3

[Alexei Volkov]

server crashes since the session cache is not initialized. I plan to allow key generation in this case, but without future storing or milti-process sharing.

Comment 4

[Alexei Volkov]

Attachment: Patch v1: Bug fix

Do not store keys in the cache if it was not initialize. Just generate the key and return them. Patch fixes bypass and non-bypass modes.

Comment 5

[Robert Relyea]

Comment on attachment 511878 [details] [diff] [review]
Patch v1: Bug fix

r+

I would have preferred the Bypass case to have the name Bypass in it. I almost r- because I saw bare keys in what I thought was the normal case.

That isn't a problem with this patch, however, it's a problem with the original session ticket code.

bob

Comment 6

[Wan-Teh Chang]

Comment on attachment 511878 [details] [diff] [review]
Patch v1: Bug fix

r=wtc.  Please make the following changes before you check this in.

1. lib/freebl/blapit.h

Change AES_xxx_KEY_SIZE to AES_xxx_KEY_LENGTH for consistency with
the existing DES_KEY_LENGTH and SEED_KEY_LENGTH macros in this file.

2. lib/ssl/sslsnce.c

>+    uint8 ticketKeyNameSuffix[SESS_TICKET_KEY_VAR_NAME_LEN];
>+    uint8 *ticketKeyNameSuffixPtr;

Consider renaming the stack buffer and the pointer variable as follows:

>+    uint8 ticketKeyNameSuffixLocal[SESS_TICKET_KEY_VAR_NAME_LEN];
>+    uint8 *ticketKeyNameSuffix;

This allows the more commonly used variable, the pointer, to
have a shorter name.

This convention is used in NSPR.  I can't find such code in NSS.
I don't know why.
 
Similarly for the stack buffers and pointer variables you added
to ssl_GetSessionTicketKeys.

Comment 7

[Alexei Volkov]

Attachment: Patch as committed.

Thanks for reviews. The patch is committed on trunk and branch.