1320510 - --with-system-nss crashes on Firefox >= 52.0 + NSS == 3.28 with TLS 1.3 disabled

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Jan Beich]

After bug 1295937 Tier3 platforms can no longer use bundled NSS. Nightly 53 currently depends on NSS 3.28 but no release is available yet. Trying to package NSS_3_28_BRANCH snapshot I've bumped into the following crash which affects on Aurora 52 as well, but not Beta 51 or Release 50. NSS 3.29 appears to be fine.

Maybe bump required NSS version in old-configure.in + backport to Aurora 52.

# from mozilla-central changeset b982373cb0e9
Process 22480 stopped
* thread #4: tid = 100242, 0x0000000811022077 libxul.so`nsNSSComponent::nsNSSComponent(this=0x0000000818840070) + 439 at nsNSSComponent.cpp:259, stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
    frame #0: 0x0000000811022077 libxul.so`nsNSSComponent::nsNSSComponent(this=0x0000000818840070) +439 at nsNSSComponent.cpp:259
   256  #endif
   257  {
   258    MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("nsNSSComponent::ctor\n"));
-> 259    MOZ_RELEASE_ASSERT(NS_IsMainThread());
   260
   261    NS_ASSERTION( (0 == mInstanceCount), "nsNSSComponent is a singleton, but instantiated multiple times!");
   262    ++mInstanceCount;
(lldb) bt
* thread #4: tid = 100242, 0x0000000811022077 libxul.so`nsNSSComponent::nsNSSComponent(this=0x0000000818840070) + 439 at nsNSSComponent.cpp:259, stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
  * frame #0: 0x0000000811022077 libxul.so`nsNSSComponent::nsNSSComponent(this=0x0000000818840070) +439 at nsNSSComponent.cpp:259
    frame #1: 0x000000081104437f libxul.so`nsNSSComponentConstructor(aOuter=0x0000000000000000, aIID=0x0000000813354ba0, aResult=0x00007fffdfbfab90) + 207 at nsNSSModule.cpp:173
    frame #2: 0x000000080c7dd0ae libxul.so`mozilla::GenericFactory::CreateInstance(this=0x000000081c1c2920, aOuter=0x0000000000000000, aIID=0x0000000813354ba0, aResult=0x00007fffdfbfab90) + 46 at GenericFactory.cpp:17
    frame #3: 0x000000080c780634 libxul.so`nsComponentManagerImpl::CreateInstanceByContractID(this=0x0000000801a6d900, aContractID="@mozilla.org/psm;1", aDelegate=0x0000000000000000, aIID=0x0000000813354ba0, aResult=0x00007fffdfbfab90) + 292 at nsComponentManager.cpp:1140
    frame #4: 0x000000080c77ccba libxul.so`nsComponentManagerImpl::GetServiceByContractID(this=0x0000000801a6d900, aContractID="@mozilla.org/psm;1", aIID=0x0000000813354ba0, aResult=0x00007fffdfbfac50)+ 778 at nsComponentManager.cpp:1496
    frame #5: 0x000000080c7f3877 libxul.so`CallGetService(aContractID="@mozilla.org/psm;1", aIID=0x0000000813354ba0, aResult=0x00007fffdfbfac50) + 87 at nsComponentManagerUtils.cpp:67
    frame #6: 0x000000080c7f3e88 libxul.so`nsGetServiceByContractID::operator(this=0x00007fffdfbfac68, aIID=0x0000000813354ba0, aInstancePtr=0x00007fffdfbfac50)(nsID const&, void**) const + 40 at nsComponentManagerUtils.cpp:280
    frame #7: 0x000000080c7e01e3 libxul.so`nsCOMPtr_base::assign_from_gs_contractid(this=0x00007fffdfbfacf0, aGS=(mContractID = "@mozilla.org/psm;1"), aIID=0x0000000813354ba0) + 51 at nsCOMPtr.cpp:95
    frame #8: 0x0000000810fd49df libxul.so`nsCOMPtr<nsINSSComponent>::nsCOMPtr(this=0x00007fffdfbfacf0, aGS=(mContractID = "@mozilla.org/psm;1")) + 79 at nsCOMPtr.h:555
    frame #9: 0x0000000811021e0f libxul.so`EnsureNSSInitialized(op=nssEnsure) + 367 at nsNSSComponent.cpp:191
    frame #10: 0x000000081104457b libxul.so`(anonymous namespace)::nsSSLSocketProviderConstructor(aOuter=0x0000000000000000, aIID=0x00000008130a1038, aResult=0x00007fffdfbfaf40) + 139 at nsNSSModule.cpp:186
    frame #11: 0x000000080c7dd0ae libxul.so`mozilla::GenericFactory::CreateInstance(this=0x000000081881d160, aOuter=0x0000000000000000, aIID=0x00000008130a1038, aResult=0x00007fffdfbfaf40) + 46 at GenericFactory.cpp:17
    frame #12: 0x000000080c780634 libxul.so`nsComponentManagerImpl::CreateInstanceByContractID(this=0x0000000801a6d900, aContractID="@mozilla.org/network/socket;2?type=ssl", aDelegate=0x0000000000000000, aIID=0x00000008130a1038, aResult=0x00007fffdfbfaf40) + 292 at nsComponentManager.cpp:1140
    frame #13: 0x000000080c77ccba libxul.so`nsComponentManagerImpl::GetServiceByContractID(this=0x0000000801a6d900, aContractID="@mozilla.org/network/socket;2?type=ssl", aIID=0x00000008130a1038, aResult=0x00007fffdfbfb170) + 778 at nsComponentManager.cpp:1496
    frame #14: 0x000000080c7f3877 libxul.so`CallGetService(aContractID="@mozilla.org/network/socket;2?type=ssl", aIID=0x00000008130a1038, aResult=0x00007fffdfbfb170) + 87 at nsComponentManagerUtils.cpp:67
    frame #15: 0x000000080cbb0286 libxul.so`nsresult CallGetService<nsISocketProvider>(char const*, nsISocketProvider**) + 54
    frame #16: 0x000000080cbaf03b libxul.so`nsSocketProviderService::GetSocketProvider(this=0x000000081881d140, type="ssl", result=0x00007fffdfbfb170) + 171 at nsSocketProviderService.cpp:39
    frame #17: 0x000000080c8fb583 libxul.so`mozilla::net::nsSocketTransport::Init(this=0x0000000818849000, types=0x00007fffdfbfb598, typeCount=1, host=0x0000000818834008, port=443, hostRoute=0x0000000818834020, portRoute=443, givenProxyInfo=0x0000000000000000) + 1587 at nsSocketTransport2.cpp:867
    frame #18: 0x000000080c905b2f libxul.so`mozilla::net::nsSocketTransportService::CreateRoutedTransport(this=0x0000000801a15e00, types=0x00007fffdfbfb598, typeCount=1, host=0x0000000818834008, port=443, hostRoute=0x0000000818834020, portRoute=443, proxyInfo=0x0000000000000000, result=0x00007fffdfbfb580) + 639 at nsSocketTransportService2.cpp:739
    frame #19: 0x000000080cd99341 libxul.so`mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetupStreams(this=0x000000081881f1e0, transport=0x000000081881f228, instream=0x000000081881f238, outstream=0x000000081881f230, isBackup=false) + 849 at nsHttpConnectionMgr.cpp:3033
    frame #20: 0x000000080cd97fb3 libxul.so`mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetupPrimaryStreams(this=0x000000081881f1e0) + 163 at nsHttpConnectionMgr.cpp:3137
    frame #21: 0x000000080cd9555d libxul.so`mozilla::net::nsHttpConnectionMgr::CreateTransport(this=0x000000081c0c4500, ent=0x0000000818840000, trans=0x0000000825c5acc0, caps=1041, speculative=true, isFromPredictor=false, allow1918=false) + 285 at nsHttpConnectionMgr.cpp:2004
    frame #22: 0x000000080cd90027 libxul.so`mozilla::net::nsHttpConnectionMgr::OnMsgSpeculativeConnect(this=0x000000081c0c4500, (null)=0, param=0x0000000824ab77a0) + 695 at nsHttpConnectionMgr.cpp:2927
    frame #23: 0x000000080cdb193a libxul.so`void RefPtr<mozilla::net::nsHttpConnectionMgr>::Proxy<void, int, mozilla::net::ARefBase*>::operator()<int&, RefPtr<mozilla::net::ARefBase>&>(int&&&, RefPtr<mozilla::net::ARefBase>&&&) + 154
    frame #24: 0x000000080cdb1753 libxul.so`mozilla::net::ConnEvent::Run() + 115
    frame #25: 0x000000080c799757 libxul.so`nsThread::ProcessNextEvent(this=0x0000000801a7a180, aMayWait=true, aResult=0x00007fffdfbfb99e) + 1143 at nsThread.cpp:1213
    frame #26: 0x000000080c7fbf1f libxul.so`NS_ProcessNextEvent(aThread=0x0000000801a7a180, aMayWait=true) + 111 at nsThreadUtils.cpp:361
    frame #27: 0x000000080c906560 libxul.so`mozilla::net::nsSocketTransportService::Run(this=0x0000000801a15e00) + 1488 at nsSocketTransportService2.cpp:939
    frame #28: 0x000000080c799757 libxul.so`nsThread::ProcessNextEvent(this=0x0000000801a7a180, aMayWait=false, aResult=0x00007fffdfbfbcde) + 1143 at nsThread.cpp:1213
    frame #29: 0x000000080c7fbf1f libxul.so`NS_ProcessNextEvent(aThread=0x0000000801a7a180, aMayWait=false) + 111 at nsThreadUtils.cpp:361
    frame #30: 0x000000080cfe6636 libxul.so`mozilla::ipc::MessagePumpForNonMainThreads::Run(this=0x0000000818819000, aDelegate=0x0000000818814000) + 646 at MessagePump.cpp:338
    frame #31: 0x000000080cf188b7 libxul.so`MessageLoop::RunInternal(this=0x0000000818814000) + 87 at message_loop.cc:232
    frame #32: 0x000000080cf18855 libxul.so`MessageLoop::RunHandler(this=0x0000000818814000) + 21 atmessage_loop.cc:225
    frame #33: 0x000000080cf1882d libxul.so`MessageLoop::Run(this=0x0000000818814000) + 45 at message_loop.cc:205
    frame #34: 0x000000080c7974e5 libxul.so`nsThread::ThreadFunc(aArg=0x0000000801a7a180) + 389 at nsThread.cpp:467
    frame #35: 0x00000008157a289d libnspr4.so`_pt_root(arg=0x0000000801a72d20) + 333 at ptthread.c:216
    frame #36: 0x0000000801181b95 libthr.so.3`thread_start(curthread=<unavailable>) + 325 at thr_create.c:289

(lldb) l NS_IsMainThread
File: xpcom/threads/nsThreadManager.cpp
   21   using namespace mozilla;
   22
   23   static MOZ_THREAD_LOCAL(bool) sTLSIsMainThread;
   24
   25   bool
   26   NS_IsMainThread()
   27   {
   28     return sTLSIsMainThread.get();
   29   }
   30
   31   void
   32   NS_SetMainThread()
   33   {

Comment 1

[Eric Rescorla (:ekr)]

What reason is there to believe that this bug is caused by NSS? I don't see any NSS functions in the stack trace, just PSM functions.

Comment 2

[Jan Beich]

Unlike GYP build standalone one had only recently enabled TLS 1.3. Perhaps, Firefox misbehaves with NSS 3.28 that has TLS 1.3 disabled.

Comment 3

[Eric Rescorla (:ekr)]

Firefox 52 and above enable TLS 1.3 by default. It appears that the relevant code in PSM to trim the version range doesn't work properly if Firefox's defaults are more expansive than NSS's (i.e., it just picks Firefox's).

See:
http://searchfox.org/mozilla-central/source/security/manager/ssl/nsNSSComponent.cpp#1681
http://searchfox.org/mozilla-central/source/security/manager/ssl/nsNSSComponent.cpp#1401

Needinfoing MT and Kai. It seems like we need to either:

- Enable 1.3 by default in NSS 3.28
- Require that system builds of NSS 3.28 enable 1.3 if they are to be used with Firefox.
- Fix this code in Firefox to work correctly (by setting defaults =
  PR_MAX(defaults.min, nss.min), PR_MIN(defaults.max, nss.max)

Not clear to me why this would lead to a crash, but maybe Firefox isn't prepared for SSL_VersionRangeSetDefault to fail

Comment 4

[Eric Rescorla (:ekr)]

Attachment: range.patch

The following patch trims the compiled in defaults to match NSS. Note that this won't remove the crash if you have a serious mismatch of some kind (like NSS and Firefox don't support the same range at all).

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 8814634 [details] [diff] [review]
range.patch

Review of attachment 8814634 [details] [diff] [review]:
-----------------------------------------------------------------

Keeler, MT: Note we will either need to uplift this patch to 52 or do one of the other things in my comment

Comment 6

[Martin Thomson [:mt:]]

This seems right to me.  r=mt

Clamping the version range works.  The only risk to doing this is when we have completely non-overlapping sets between what Firefox wants and what the system NSS wants to provide; if we're that off, I say that crashing is probably the right thing to do.

I don't know how to make this temporary, but I expect that this is a temporary and unusual situation.  It only exists because we build NSS without TLS 1.3 by default.  I'm not overly happy about that, because we're doing it to shield applications from compatibility risks when they do silly things (like enable the highest available TLS version without knowing what it is!).  When you add the fact that the compatibility risks have been removed in the most recent 3.28 builds, it's a very odd situation.

Of course, all this only applies to 3.28.  3.29 builds TLS 1.3.

Comment 7

[Eric Rescorla (:ekr)]

I defer to Keeler and MT on how/when to land this.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8814634 [details] [diff] [review]
range.patch

Review of attachment 8814634 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like a good solution for the situation we're in. I think it would be nice to follow-up with some more rigorous checks (e.g. the case of no range overlap, like you've said) - this can probably be part of bug 1311753 (basically, if any of these NSS operations fail, we should probably just crash then and there, because our assumptions about the underlying library support are wrong).

(As a side note, I believe the reason this results in a crash is because necko tries to initialize the PSM XPCOM service on the main thread with net_EnsurePSMInit, but it doesn't actually check whether or not it fails in non-debug builds. After that, the next time something wants to do something TLS/security-related, it will attempt to initialize the PSM XPCOM service again, but it might not be on the main thread, which is a fatal failure on release builds.)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8a41d4c6215
clamp the default enabled TLS version range to what NSS supports r=keeler

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d8a41d4c6215

Comment 11

[Jan Beich]

I confirm, Firefox 53 no longer crashes with system NSS 3.28 (TLS 1.3 default OFF) after d8a41d4c6215.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8814634 [details] [diff] [review]
range.patch

Approval Request Comment
[Feature/Bug causing the regression]: enabling TLS 1.3 by default/bug 1310516
[User impact if declined]: versions built with --with-system-nss will crash on startup
[Is this code covered by automated tests?]: yes, but not this specific situation
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: The change is simple and localized. In the vast majority of cases it's basically a no-op.
[String changes made/needed]: none

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8814634 [details] [diff] [review]
range.patch

fix crash for system-nss builds, aurora52+

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/504fe6266836