Closed
Bug 1238001
Opened 8 years ago
Closed 8 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
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: cbook, Assigned: mt)
References
()
Details
(Keywords: assertion, Whiteboard: [psm-assigned])
Attachments
(3 files)
115.26 KB,
text/plain
|
Details | |
4.93 KB,
text/plain
|
Details | |
2.50 KB,
patch
|
keeler
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
martin is this also something for you ?
Flags: needinfo?(martin.thomson)
Assignee | ||
Comment 2•8 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)
Comment 4•8 years ago
|
||
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
Comment 5•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
Assignee | ||
Comment 10•8 years ago
|
||
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)
Assignee | ||
Comment 14•8 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?
Updated•8 years ago
|
Attachment #8722565 -
Flags: sec-approval? → sec-approval+
Comment 15•8 years ago
|
||
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•8 years ago
|
Flags: needinfo?(martin.thomson)
Reporter | ||
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2c10aa467bc5
Status: NEW → RESOLVED
Closed: 8 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.
Description
•