NSS_ALLOW_SSLKEYLOGFILE appears to default to enabled in release
Categories
(NSS :: Libraries, defect, P1)
Tracking
(firefox-esr60 disabled, firefox64 wontfix, firefox65+ disabled, firefox66+ fixed)
People
(Reporter: erahm, Assigned: jcj)
References
Details
(Keywords: csectype-disclosure, sec-moderate)
Attachments
(4 files, 1 obsolete file)
While investigating some shutdown hangs in the 64.0 release related to http shutdown (bug 1514028) I noticed that we're crashing due to being blocked on writing out to a file in `ssl3_RecordKeyLog` [1]. See bp-dbfb8fc5-2517-40b6-9060-9d97b0181213 and bp-bbc0c346-f717-484b-831a-045010181218 for examples (look at the socket thread stack). My understanding is that this code should only be enabled in debug (or possibly dev-edition?). It's possible we intentionally enabled this in release and this is a non-issue or it's possible we accidentally enabled it by switching to using gyp [2] which defaults to enabled rather than Make [3] which defaults to disabled when building NSS. Crash reports indicate this is happening on such sites as web.whatsapp.com and mail.google.com which is a bit sketchy. Note that even with the code enabled we shouldn't actually write anything unless the `SSLKEYLOGFILE` env var is set [4], so these stacks indicate that is probably set. [1] https://hg.mozilla.org/releases/mozilla-release/annotate/8337ebb86a425a1c65467fc68eb7c26b9046159e/security/nss/lib/ssl/ssl3con.c#l11118 [2] https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/security/nss/lib/ssl/ssl.gyp#97 [3] https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/security/nss/lib/ssl/Makefile#44 [4] https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/security/nss/lib/ssl/sslsock.c#3667-3670
Comment 1•5 years ago
|
||
Odd... https://dxr.mozilla.org/mozilla-release/source/security/nss/lib/ssl/Makefile#46 looks right: # Enable key logging by default in debug builds, but not opt builds. # Logging still needs to be enabled at runtime through env vars. NSS_ALLOW_SSLKEYLOGFILE ?= $(if $(BUILD_OPT),0,1) ifeq (1,$(NSS_ALLOW_SSLKEYLOGFILE)) DEFINES += -DNSS_ALLOW_SSLKEYLOGFILE=1 endif If BUILD_OPT is wrong here, we have other issues...
Comment 2•5 years ago
|
||
(In reply to Eric Rahm [:erahm] (ni? for review in phab) from comment #0) > It's possible we intentionally enabled this in > release and this is a non-issue or it's possible we accidentally enabled it > by switching to using gyp [2] Oh. This seems likely to me... Franziskus?
Comment 3•5 years ago
|
||
This is enabled by default, and should be, see bug 1188657 for a discussion. But this shouldn't write anything unless the env is set as Eric said. The write function bails [1] when `ssl_keylog_iob` isn't set, which set only by the `SSLKEYLOGFILE` env var [3]. But it looks like it's not initialised to NULL [2] such that the condition at [1] doesn't trigger properly. Moving this to NSS. [1] https://searchfox.org/nss/rev/7bc70a3317b800aac07bad83e74b6c79a9ec5bff/lib/ssl/ssl3con.c#11082 [2] https://searchfox.org/nss/rev/7bc70a3317b800aac07bad83e74b6c79a9ec5bff/lib/ssl/sslsock.c#124 [3] https://searchfox.org/nss/rev/7bc70a3317b800aac07bad83e74b6c79a9ec5bff/lib/ssl/sslsock.c#3670
Comment 4•5 years ago
|
||
I don't know how often this is going to happen, but calling it sec-moderate because uninitentionally leaking keys is going to make people upset. Even if it's only local.
Reporter | ||
Comment 5•5 years ago
|
||
Ignoring the sec implications, this is responsible for a non-trivial amount of shutdown hangs (and probably busted network connections), do we plan on fixing this any time soon or should I file a separate bug to disable NSS_ALLOW_SSLKEYLOGFILE by default for Firefox until a proper fix can be worked out? That would be pretty easy to uplift to beta in the short term at the expense of disabling this feature for a release or two.
Assignee | ||
Comment 6•5 years ago
|
||
I won't be able to fix NSS before the RC builds next week.
Comment 7•5 years ago
|
||
Eric, can you please file a bug for disabling NSS_ALLOW_SSLKEYLOGFILE on Firefox 65? Per IRC discussion with jcj, we want to leave it enabled on Nightly because it provides useful diagnostics, but we can still accept a Beta-only patch for 65.
Reporter | ||
Updated•5 years ago
|
Comment 8•5 years ago
|
||
Bug 1519209 disabled this functionality for RELEASE_OR_BETA.
Assignee | ||
Comment 9•5 years ago
|
||
There is a new test here because the keylog unittest sets the environment
variable for SSLKEYLOG and NSPR provides no mechanism (cross-platform-wise) to
actually delete an environment variable, so I made another file for the base
uninitialized case.
The fix here is for undefined behavior. That said, I can only repro on OSX and
Linux64 using ASAN, and that's not exactly a reproduction. I'm hoping that this
fix really catches the shutdown crashes we're seeing.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 10•5 years ago
|
||
Landed in trunk:
https://hg.mozilla.org/projects/nss/rev/3e26ed39924175d8dd7e4fc3904907cae8c0689b
https://hg.mozilla.org/projects/nss/rev/1313deb37daf581f5795d5b1e406238a47f77f5d
https://hg.mozilla.org/projects/nss/rev/32ad1532c67acc8974160880e7d79d22b0d0f25a
Assignee | ||
Comment 11•5 years ago
|
||
Assignee | ||
Comment 12•5 years ago
|
||
Backed out from trunk for ASAN failures in Bug 1522929
https://hg.mozilla.org/projects/nss/rev/31656990b5d0
https://hg.mozilla.org/projects/nss/rev/92e62d0731bd
Comment 13•5 years ago
|
||
Was just in a meeting with some Anti-virus folks (on another issue) and they mentioned that they enable this to help them to do traffic interception. So that's probably why we're seeing this in release builds.
Assignee | ||
Comment 14•5 years ago
|
||
Update the keylog gtest to avoid a stack overflow if the labels map is empty,
as happens on opt asan builds
Assignee | ||
Comment 15•5 years ago
|
||
Build-script updates re-landed in 3.45 and in NSS_3_44_BRANCH, to go along with any point releases we make there.
default: https://hg.mozilla.org/projects/nss/rev/4a08af60c2b7b4d8c69a936bd95dc83f50dcc7e2
NSS_3_44_BRANCH: https://hg.mozilla.org/projects/nss/rev/21fdbf4c1c44324116a9dd51b5c675ebcf5fffcc
The tests which were backed out in 92e62d0731bd6ce961359b4c438448f2cf548bf2 will be a separate commit.
Furthermore, since the crash fix is released, this bug can probably be made public, which I will plan to do tomorrow unless I hear dissent.
Updated•5 years ago
|
Assignee | ||
Comment 16•5 years ago
|
||
- Don't always build ssl_keylog_unittest.cc in Makefile builds
Assignee | ||
Comment 17•5 years ago
|
||
Fixup: https://hg.mozilla.org/projects/nss/rev/6f442f3ba2239b857c8fd540468f9f83c84b7c0b
(Due to test failures, the NSS_3_44_BRANCH commit in Comment 15 was not landed.)
Assignee | ||
Comment 18•5 years ago
|
||
Backs out backout 92e62d0731bd6ce961359b4c438448f2cf548bf2
Note: Some parts were included in rev 6f442f3ba2239b857c8fd540468f9f83c84b7c0b
original phab link: https://phabricator.services.mozilla.com/D17380
Assignee | ||
Comment 19•5 years ago
|
||
New tests: https://hg.mozilla.org/projects/nss/rev/43a7fb4f994a31222c308113b0fccdd5480d5b8e
I'll back-port to 3.44 tomorrow.
Assignee | ||
Comment 20•5 years ago
|
||
Assignee | ||
Comment 21•5 years ago
|
||
This bug was fixed in Firefox 66 and should have been left as Resolved (comment 12) at that point, with the test failures handled in a different patch. Per also Comment 15, making this public, marking 66 as fixed, resolving it, and the final test cleanup patch (and the backports) can land in the clear.
Assignee | ||
Comment 22•5 years ago
|
||
Assignee | ||
Comment 23•5 years ago
|
||
Description
•