1515236 - NSS_ALLOW_SSLKEYLOGFILE appears to default to enabled in release

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Eric Rahm [:erahm]]

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

[:Gijs (out for now; he/him)]

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

[:Gijs (out for now; he/him)]

(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

[Franziskus Kiefer [:franziskus]]

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

[Daniel Veditz [:dveditz]]

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.

Comment 5

[Eric Rahm [:erahm]]

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.

Comment 6

[J.C. Jones [:jcj] (he/they)]

I won't be able to fix NSS before the RC builds next week.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Bug 1519209 disabled this functionality for RELEASE_OR_BETA.

Comment 9

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1515236 - Initialize debug environment variables before use r?mt

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.

Comment 10

[J.C. Jones [:jcj] (he/they)]

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

Comment 11

[J.C. Jones [:jcj] (he/they)]

Uplifted to NSS_3_42_BRANCH:
https://hg.mozilla.org/projects/nss/rev/d4545c5d52c35410f61f18b9f2774637717ef88b
https://hg.mozilla.org/projects/nss/rev/80aed136d48c8db4d975959990db6b1cd8760342
https://hg.mozilla.org/projects/nss/rev/a00bce9553dbcadb9afd2c7b3ca4bd0367dd6cb7

Comment 12

[J.C. Jones [:jcj] (he/they)]

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

[Daniel Veditz [:dveditz]]

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.

Comment 14

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1515236 - Fixup for 4a08af60c2b7 r?KevinJacobs

Update the keylog gtest to avoid a stack overflow if the labels map is empty,
as happens on opt asan builds

Comment 15

[J.C. Jones [:jcj] (he/they)]

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.

Comment 16

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1515236 - Fixup for 4a08af60c2b7 r?KevinJacobs

• Don't always build ssl_keylog_unittest.cc in Makefile builds

Comment 17

[J.C. Jones [:jcj] (he/they)]

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.)

Comment 18

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1515236 - Add a test that SSLKEYLOGFILE and SSLDEBUGFILE init properly r=kevinJacobs

Backs out backout 92e62d0731bd6ce961359b4c438448f2cf548bf2

Note: Some parts were included in rev 6f442f3ba2239b857c8fd540468f9f83c84b7c0b

original phab link: https://phabricator.services.mozilla.com/D17380

Comment 19

[J.C. Jones [:jcj] (he/they)]

New tests: https://hg.mozilla.org/projects/nss/rev/43a7fb4f994a31222c308113b0fccdd5480d5b8e

I'll back-port to 3.44 tomorrow.

Comment 20

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1515236 - Don't run keylog deathtest on ASAN builds r?kevinJacobs

Comment 21

[J.C. Jones [:jcj] (he/they)]

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.

Comment 22

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/43a7fb4f994a31222c308113b0fccdd5480d5b8e
https://hg.mozilla.org/projects/nss/rev/8771716345ee0585b7183fe9d730fadf3c08041d

Comment 23

[J.C. Jones [:jcj] (he/they)]

NSS_3_44_BRANCH

• https://hg.mozilla.org/projects/nss/rev/21fdbf4c1c44324116a9dd51b5c675ebcf5fffcc
• https://hg.mozilla.org/projects/nss/rev/9215df3395e672b0686aff556bfa4e1ef02a8aa6
• https://hg.mozilla.org/projects/nss/rev/46606f961ed10335c32918b772fcb8d77573e37c