Closed Bug 1238001 Opened 4 years ago Closed 4 years ago

Assertion failure: ((bool)(!!(!NS_FAILED_impl(infoObject->GetKEAUsed(&KEAUsed))))) && (KEAUsed == cipherInfo.keaType), at c:/Users/mozilla/debug-build/mozilla-central/security/manager/ssl/nsNSSCallbacks.cpp:1175


(Core :: Security: PSM, defect)

Not set



Tracking Status
firefox48 --- fixed


(Reporter: cbook, Assigned: mt)


(Blocks 1 open bug, )


(Keywords: assertion, Whiteboard: [psm-assigned])


(3 files)

Attached file bughunter crash stack
Found this via bughunter and reproduced on a windows 7 trunk debug builds based on m-c tip from today

Steps to reproduce:
--> Assertion failure after a few seconds 

filing as s-s since this is ssl but feel to unmark if this is not a security bug

Assertion failure: ((bool)(!!(!NS_FAILED_impl(infoObject->GetKEAUsed(&KEAUsed))))) && (KEAUsed == cipherInfo.keaType), at c:/Users/mozilla/debug-build/mozilla-central/sec
#01: ssl_FinishHandshake (c:\users\mozilla\debug-build\mozilla-central\security\nss\lib\ssl\sslsecur.c:144)
#02: ssl3_FinishHandshake (c:\users\mozilla\debug-build\mozilla-central\security\nss\lib\ssl\ssl3con.c:11441)
#03: ssl3_HandleFinished (c:\users\mozilla\debug-build\mozilla-central\security\nss\lib\ssl\ssl3con.c:11396)
#04: ssl3_HandleHandshakeMessage (c:\users\mozilla\debug-build\mozilla-central\security\nss\lib\ssl\ssl3con.c:11649)
#05: ssl3_HandleHandshake (c:\users\mozilla\debug-build\mozilla-central\security\nss\lib\ssl\ssl3con.c:11723)
#06: ssl3_HandleRecord (c:\users\mozilla\debug-build\mozilla-central\security\nss\lib\ssl\ssl3con.c:12392)
#07: ssl3_GatherCompleteHandshake (c:\users\mozilla\debug-build\mozilla-central\security\nss\lib\ssl\ssl3gthr.c:378)
#08: ssl3_GatherAppDataRecord (c:\users\mozilla\debug-build\mozilla-central\security\nss\lib\ssl\ssl3gthr.c:451)
#09: DoRecv (c:\users\mozilla\debug-build\mozilla-central\security\nss\lib\ssl\sslsecur.c:616)
#10: ssl_SecureRecv (c:\users\mozilla\debug-build\mozilla-central\security\nss\lib\ssl\sslsecur.c:1237)
#11: ssl_Recv (c:\users\mozilla\debug-build\mozilla-central\security\nss\lib\ssl\sslsock.c:2356)
#12: PSMRecv (c:\users\mozilla\debug-build\mozilla-central\security\manager\ssl\nsnssiolayer.cpp:1419)
#13: nsSSLIOLayerRead (c:\users\mozilla\debug-build\mozilla-central\security\manager\ssl\nsnssiolayer.cpp:1471)
#14: PR_Read (c:\users\mozilla\debug-build\mozilla-central\nsprpub\pr\src\io\priometh.c:109)
#15: nsSocketInputStream::Read (c:\users\mozilla\debug-build\mozilla-central\netwerk\base\nssockettransport2.cpp:395)
#16: mozilla::net::nsHttpConnection::OnWriteSegment (c:\users\mozilla\debug-build\mozilla-central\netwerk\protocol\http\nshttpconnection.cpp:1673)
#17: mozilla::net::nsHttpTransaction::WritePipeSegment (c:\users\mozilla\debug-build\mozilla-central\netwerk\protocol\http\nshttptransaction.cpp:766)
#18: nsPipeOutputStream::WriteSegments (c:\users\mozilla\debug-build\mozilla-central\xpcom\io\nspipe3.cpp:1649)
#19: mozilla::net::nsHttpTransaction::WriteSegments (c:\users\mozilla\debug-build\mozilla-central\netwerk\protocol\http\nshttptransaction.cpp:816)
#20: mozilla::net::nsHttpConnection::OnSocketReadable (c:\users\mozilla\debug-build\mozilla-central\netwerk\protocol\http\nshttpconnection.cpp:1767)
#21: mozilla::net::nsHttpConnection::OnInputStreamReady (c:\users\mozilla\debug-build\mozilla-central\netwerk\protocol\http\nshttpconnection.cpp:2077)
#22: nsSocketInputStream::OnSocketReady (c:\users\mozilla\debug-build\mozilla-central\netwerk\base\nssockettransport2.cpp:289)
#23: nsSocketTransport::OnSocketReady (c:\users\mozilla\debug-build\mozilla-central\netwerk\base\nssockettransport2.cpp:1887)
#24: nsSocketTransportService::DoPollIteration (c:\users\mozilla\debug-build\mozilla-central\netwerk\base\nssockettransportservice2.cpp:1084)
#25: nsSocketTransportService::Run (c:\users\mozilla\debug-build\mozilla-central\netwerk\base\nssockettransportservice2.cpp:867)
#26: nsThread::ProcessNextEvent (c:\users\mozilla\debug-build\mozilla-central\xpcom\threads\nsthread.cpp:989)
#27: NS_ProcessNextEvent (c:\users\mozilla\debug-build\mozilla-central\xpcom\glue\nsthreadutils.cpp:297)
#28: mozilla::ipc::MessagePumpForNonMainThreads::Run (c:\users\mozilla\debug-build\mozilla-central\ipc\glue\messagepump.cpp:326)
#29: MessageLoop::RunInternal (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\
#30: MessageLoop::RunHandler (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\
#31: MessageLoop::Run (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\
#32: nsThread::ThreadFunc (c:\users\mozilla\debug-build\mozilla-central\xpcom\threads\nsthread.cpp:403)
#33: _PR_NativeRunThread (c:\users\mozilla\debug-build\mozilla-central\nsprpub\pr\src\threads\combined\pruthr.c:397)
#34: pr_root (c:\users\mozilla\debug-build\mozilla-central\nsprpub\pr\src\md\windows\w95thred.c:90)
#35: _get_flsindex[MSVCR120 +0x2c01d]
#36: _get_flsindex[MSVCR120 +0x2c001]
#37: BaseThreadInitThunk[kernel32 +0x4ee6c]
#38: RtlInitializeExceptionChain[ntdll +0x63ab3]
#39: RtlInitializeExceptionChain[ntdll +0x63a86]
martin is this also something for you ?
Flags: needinfo?(martin.thomson)
I had a look.  I doubt that this is s-s.  I suspect that the assertion is simply wrong and that this is a result of TLS renegotiation.  It seems strange, but the renegotiated cipher suite doesn't use DHE though the first connection attempt does.

:keeler, wdyt?  Should we just remove the assertion?  Or do you want to prevent this sort of renegotiation somehow?  (That would be a mini project in NSS, I think).
Flags: needinfo?(martin.thomson) → needinfo?(dkeeler)
I had a quick look, and as far as I can tell, the assertion failure is happening on full handshakes, so I don't think it's directly due to renegotiation (unless I'm misunderstanding how that's supposed to work). I'll continue digging.
Flags: needinfo?(dkeeler)
Assigning to David while he digs so we don't have an un-owned security bug. Please assign an appropriate sec- severity keyword or unhide the bug when you know more.
Assignee: nobody → dkeeler
Maybe Martin's right about this not being a problem, but IIRC the original NSS devs were pretty serious about their assertions -- they weren't just warnings, they were conditions they thought weren't supposed to happen.
Keywords: sec-high
It looks like the server is issuing a Hello Request after the first handshake has completed and is using a different cipher suite (in particular, the key exchange algorithm is different). I guess this isn't necessarily bad in and of itself, but it does seem weird. What this assertion failure indicates is that PSM isn't aware that the cipher suite has changed, so we should probably fix that. :mt, is there a callback PSM can register for that libssl exposes for Hello Requests? Or to put it another way, what's the best way to know when this has happened so PSM can reset its state?
Flags: needinfo?(martin.thomson)
Ahh, it looks like my initial assessment was right :(

There isn't anything that would let you detect the start of the handshake.  My recommendation would be to allow PreliminaryHandshakeDone to overwrite values rather than have it shortcut.  Maybe you could switch mPreliminaryHandshakeDone from a boolean to a counter that tracks the number of handshakes.

That suggests an improvement as well, it would be good to have telemetry on how often we see a renegotiation.
Flags: needinfo?(martin.thomson)
bughunter reports this now for more sites like where this assertion failure was reproducible too. David, martin do we know a kind of eta of fixing this ?
Flags: needinfo?(martin.thomson)
Flags: needinfo?(dkeeler)
Attached patch bug1238001.patchSplinter Review
This should fix it.
Flags: needinfo?(martin.thomson)
Attachment #8722565 - Flags: review?(dkeeler)
Comment on attachment 8722565 [details] [diff] [review]

Review of attachment 8722565 [details] [diff] [review]:

Ok - this seems like the right thing to do.
Attachment #8722565 - Flags: review?(dkeeler) → review+
(And this doesn't seem to be security-sensitive.)
Flags: needinfo?(dkeeler)
so can this land ?
Flags: needinfo?(martin.thomson)
Comment on attachment 8722565 [details] [diff] [review]

[Security approval request comment]
This is NOT a security issue, but I don't have the ability to open it up.  It will stop a crash in crash testing caused by an overly aggressive assert and hopefully let us see other more interesting crashes.
Attachment #8722565 - Flags: sec-approval?
Attachment #8722565 - Flags: sec-approval? → sec-approval+
Should this be opened up or kept closed?
Assignee: dkeeler → martin.thomson
Group: crypto-core-security
Component: Libraries → Security: PSM
Product: NSS → Core
Whiteboard: [psm-assigned]
Version: 3.2.1 → Trunk
Flags: needinfo?(martin.thomson)
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.