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

RESOLVED FIXED in Firefox 48

Status

()

Core
Security: PSM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Tomcat, Assigned: mt)

Tracking

(Blocks: 1 bug, {assertion})

Trunk
mozilla48
assertion
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [psm-assigned], URL)

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8705661 [details]
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:
-> https://netserv20.websiteseguro.com/ecommerce_site/index.php?pg=cadastro&cdg=4424&acao=novo_cadastro&sid=be1lkqcelf2lufkoknm882t5h7-1434923620
--> 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
urity/manager/ssl/nsNSSCallbacks.cpp:1175
#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\message_loop.cc:235)
#30: MessageLoop::RunHandler (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\message_loop.cc:228)
#31: MessageLoop::Run (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\message_loop.cc:202)
#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]
(Reporter)

Comment 1

2 years ago
martin is this also something for you ?
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 2

2 years ago
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)
(Assignee)

Comment 7

2 years ago
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)
(Reporter)

Comment 8

2 years ago
bughunter reports this now for more sites like https://www.chm.tu-dresden.de/tc/intern/index.shtml 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)
(Reporter)

Comment 9

2 years ago
Created attachment 8722464 [details]
stack from todays tip on m-c trunk (windows 7 debug build)
(Assignee)

Comment 10

2 years ago
Created attachment 8722565 [details] [diff] [review]
bug1238001.patch

This should fix it.
Flags: needinfo?(martin.thomson)
Attachment #8722565 - Flags: review?(dkeeler)
Comment on attachment 8722565 [details] [diff] [review]
bug1238001.patch

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)
(Reporter)

Comment 13

2 years ago
so can this land ?
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 14

2 years ago
Comment on attachment 8722565 [details] [diff] [review]
bug1238001.patch

[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
Keywords: sec-high
(Assignee)

Updated

2 years ago
Flags: needinfo?(martin.thomson)
(Reporter)

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2c10aa467bc5
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.