Closed Bug 1515236 Opened 5 years ago Closed 5 years ago

NSS_ALLOW_SSLKEYLOGFILE appears to default to enabled in release

Categories

(NSS :: Libraries, defect, P1)

defect

Tracking

(firefox-esr60 disabled, firefox64 wontfix, firefox65+ disabled, firefox66+ fixed)

RESOLVED FIXED
Tracking Status
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
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...
(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?
Flags: needinfo?(franziskuskiefer)
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
Assignee: nobody → nobody
Group: firefox-core-security → crypto-core-security
Component: Security → Libraries
Flags: needinfo?(franziskuskiefer)
Product: Firefox → NSS
QA Contact: jjones
Target Milestone: --- → 3.42
Version: unspecified → other
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.
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.
Flags: needinfo?(jjones)

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

Flags: needinfo?(jjones)

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.

Flags: needinfo?(erahm)
Depends on: 1519209
Flags: needinfo?(erahm)

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: nobody → jjones
Status: NEW → ASSIGNED
See Also: → 1522735
Blocks: 1522929
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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.

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

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.

Attachment #9068146 - Attachment is obsolete: true
  • Don't always build ssl_keylog_unittest.cc in Makefile builds

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

Backs out backout 92e62d0731bd6ce961359b4c438448f2cf548bf2

Note: Some parts were included in rev 6f442f3ba2239b857c8fd540468f9f83c84b7c0b

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

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.

Group: crypto-core-security
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Blocks: 1558549
You need to log in before you can comment on or make changes to this bug.