1188657 - Keep SSL/TLS key logging working with all Firefox builds from Mozilla

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Daniel Veditz [:dveditz]]

+++ This bug was initially created as a clone of Bug #1183318 +++

Bug 1183318 is adding a NSS build-time switch DISABLE_SSLKEYLOGFILE. Firefox (and Thunderbird and everything else) should build NSS that way in any build going to large numbers of people (at least Beta and Release). Although the log can be useful diagnosing tricky TLS problems to a small handful of people worldwide, for most people it represents an easy way for a local actor to compromise their cryptographic keys by silently logging them to a file or even a remote system through a simple environment variable setting.

Comment 1

[Wan-Teh Chang]

I think this should be disabled based on the --disable-debug configure
option rather than based on the source code branch.

It may be useful to do a debug build of a Firefox release branch with
this feature enabled.

Comment 2

[Mike Hommey [:glandium]]

Why should this be opt-out and not opt-in? If it's opt-out, you can be sure most Linux distros building with system nss will not opt-out.

Comment 3

[Kai Engert [:KaiE:]]

The argument had been made by some, that this is a worthy debugging feature to have around at any time.

But I agree with Mike, my personal preference is: disable-by-default and require-to-opt-in.

It would be good to disable in all builds that are used by the mass of consumers.

The keylogfile feature could be kept enabled in builds that target developers, such as mozilla-central.

Therefore Wan-Teh's suggestion might be a good way to do it.

Comment 4

[Kai Engert [:KaiE:]]

Attachment: 1188657-v1.patch

Should this work with patch v2 from bug 1183318 ?

Comment 5

[Kai Engert [:KaiE:]]

Wan-Teh, you have suggested that optimized builds should disable the key logging feature.

Because (I believe) all Firefox binaries provided by Mozilla are optmized binaries, this debug mechanism would be removed from all official Firefox binaries, even from the ones targeted at developers (nightly and aurora channels).

The intention of the attached patch is to keep allowing the use of the environment variables with nightly and aurora builds.

What's your opinion? Is this acceptable? Or should we be more strict and require anyone who wants to log to locally build a debug build of Firefox?

Comment 6

[Kai Engert [:KaiE:]]

(FYI, if patch v2 from bug 1183318 were accepted, and if we decided to require a debug build of Firefox to use logging, then no patch for Firefox would be necessary, only an update to the newer version of NSS would be necessary.)

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8744116 [details] [diff] [review]
1188657-v1.patch

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

I agree with Wan-Teh.  We should only enable this for debug builds.  That means no change is necessary for this bug because bug 1188657 covers it.

Comment 8

[Kai Engert [:KaiE:]]

Marking as duplicate of bug 1258375, as the upgrade to NSS 3.24 will have the desired effect.

Comment 9

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1188657 - Enable SSLKEYLOGFILE for non-release builds, r=kaie,glandium

Review commit: https://reviewboard.mozilla.org/r/48953/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48953/

Comment 10

[Martin Thomson [:mt:]]

Apparently this isn't cool: https://groups.google.com/forum/#!topic/mozilla.dev.platform/2o8cnHR4Y84

I think that my patch is better, but I could be wrong.

Comment 11

[Kai Engert [:KaiE:]]

(In reply to Martin Thomson [:mt:] from comment #10)
> Apparently this isn't cool:
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/2o8cnHR4Y84

Ok, good to know. So we're back with the plan to:
- enable in Aurora and Nightly optimized builds
- disable in Release and Beta optimized builds


> I think that my patch is better, but I could be wrong.

My patch imitated other existing code.
I didn't find config/external/nss/Makefile.in when when I made my patch.

If the RELEASE_BUILD variable is defined inside config/external/nss/Makefile.in
then your patch is better/simpler.

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 8745258 [details]
MozReview Request: Bug 1188657 - Enable SSLKEYLOGFILE for non-release builds, r=kaie,glandium

LGTM. Ideally it should be tested with an Aurora and a Beta build.

Comment 13

[Martin Thomson [:mt:]]

My testing extends to running ./mach build-backend on a few branches and checking that the variable was properly set and available to the makefile (`make -p` from the config/external/nss directory specifically).

Comment 14

[Patrick McManus [:mcmanus]]

The networking stack of release builds  need troubleshooting mechanisms too - that's an important part of our drive to quality. Removing key logging from them would hurt that.

Comment 15

[elitest]

Could we ensure that key-logging was available by default in Firefox Developer Edition?  I know the group of people who would make use of this feature is relatively small, however many sysadmins and network engineers are not going to compile FF from source and set a flag.  This isn't just a feature used by developers.  Having a relatively simple way to have these people opt-in to the feature would be great.

FWIW if a user is compromised locally there are many "easy" avenues for an attacker to intercept their traffic.  I do however understand the optics problem of software giving bad guys an easy way to decrypt users traffic, especially these days.

Comment 16

[Mike Savory]

I understand this is being driven by a need to make the defaults as secure as possible. But as someone who needs remote end users to be able to gather diagnostics captures for remote analysis of SSL network issues, having this unavailble causes pain. It's not just developers and techies that need to do this, it would be good to be able to enable this on any build, rather than having the customer install a "diagnostic" version. Maybe just like the browser entering "private browsing" mode, a user could enter "insecure mode" to allow their keys to be logged for remote diagnosis of network issues. Where insecure mode is clearly indicated in the browser. And while I am making a wishlist, the ability to choose cyphers (like avoid DHE) would be useful in this mode.

Comment 17

[Martin Thomson [:mt:]]

I think that I'm reading this feedback as a request to retain the feature.  Can someone in favour of dropping the feature speak?  Because it's actually easier to unconditionally enable the feature.

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8745258 [details]
MozReview Request: Bug 1188657 - Enable SSLKEYLOGFILE for non-release builds, r=kaie,glandium

https://reviewboard.mozilla.org/r/48953/#review46175

Assuming that's what we end up deciding we want to do

Comment 19

[Kai Engert [:KaiE:]]

This seems to wait for Martin to commit and close.

Comment 20

[Martin Thomson [:mt:]]

I'm going to go with the unconditional enabling, based on the feedback we've received (which is non-trivial).  I will request uplift as well so that 48 doesn't miss out.

Comment 21

[Martin Thomson [:mt:]]

Comment on attachment 8745258 [details]
MozReview Request: Bug 1188657 - Enable SSLKEYLOGFILE for non-release builds, r=kaie,glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48953/diff/1-2/

Comment 22

[Martin Thomson [:mt:]]

Comment on attachment 8745258 [details]
MozReview Request: Bug 1188657 - Enable SSLKEYLOGFILE for non-release builds, r=kaie,glandium

Approval Request Comment
[Feature/regressing bug #]: 1188657
[User impact if declined]: NSS 3.24 (in Aurora) disables a feature, this restores it.  This feature makes collecting network diagnostic information easier.
[Describe test coverage new/current, TreeHerder]: Treeherder (ongoing)
[Risks and why]: There are no automated tests for this feature, but it's non-web-facing.
[String/UUID change made/needed]: None

Comment 23

[Patrick McManus [:mcmanus]]

thanks!

Comment 24

[Kai Engert [:KaiE:]]

Martin, in comment 20 you said, you want "unconditional enabling".

But the most recent review request, I see conditional enabling, ifndef RELEASE_BUILD.

Am I using the review tool incorrectly?

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8745258 [details]
MozReview Request: Bug 1188657 - Enable SSLKEYLOGFILE for non-release builds, r=kaie,glandium

OK to uplift. From discussion in comments, sounds like aurora users need this feature.

Comment 26

[Martin Thomson [:mt:]]

Comment on attachment 8745258 [details]
MozReview Request: Bug 1188657 - Enable SSLKEYLOGFILE for non-release builds, r=kaie,glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48953/diff/2-3/

Comment 27

[Martin Thomson [:mt:]]

Sorry Kai, I've been using mercurial for NSS so long that I forgot how git works.  The right change has been uploaded.

Comment 28

[Carsten Book [:Tomcat]]

  https://hg.mozilla.org/releases/mozilla-aurora/rev/e5694384be52

Comment 29

[Kai Engert [:KaiE:]]

Comment on attachment 8745258 [details]
MozReview Request: Bug 1188657 - Enable SSLKEYLOGFILE for non-release builds, r=kaie,glandium

r=kaie for the correctness of the change, but if Firefox doesn't use it, the excercise from bug 1183318 was useless. We made tracing with Linux distributions harder, but everyone else is still exposed. This doesn't match my expectations.

Comment 30

[Kai Engert [:KaiE:]]

Martin, the summary of this bug was incorrect, because it doesn't match what you did.

I'm changing the summary to what has effectively been done, which is "Keep SSL/TLS key logging working with all Firefox builds from Mozilla".

If anyone still wants to lobby for this to be disabled, I guess you need a new bug.

Comment 31

[Wan-Teh Chang]

Martin:

I think Kai is right. Right now what Carsten landed in
mozilla-aurora is a no-op because the NSS in mozilla-aurora
doesn't check the NSS_ALLOW_SSLKEYLOGFILE makefile variable.
When the NSS in mozilla-aurora is upgraded, this will
affect all builds, not just the "non-release builds" that
you intended.

Comment 32

[Wan-Teh Chang]

Kai, Martin:

I think I understand what Martin intended now.

A "release" build has two meanings. The more common
meaning is the opposite of a debug build. But I
guess when Martin said "Enable SSLKEYLOGFILE for
non-release builds", he interpreted a release
build as the opposite of a pre-release build.
Under that definition, mozilla-aurora is a
non-release build.

If I am right, then the summary of this bug should
say "Firefox pre-release builds" instead of
"all Firefox builds".

Comment 33

[Kai Engert [:KaiE:]]

Wan-Teh,

the term "release build" (as used in this bug report) refers to the definition used in Firefox build scripts.

Release builds are:
- final releases
- beta

Non-release builds are:
- aurora
- nightly

This bug ORIGINALLY suggested to:
- disable logging completely in release builds (release and beta),
  but keep it working in non-release builds (aurora and nightly),
  and my initial patch suggested to implement that.

Later Martin made the decision to keep logging working unconditionally.

The patch description that Martin used ("enable ... for non-release builds") doesn't make sense to me, indeed, because he enabled the feature in both release and non-release builds.

But the contents of the patch seems aligned with his comments in this bug, his intention to enable it everywhere.

The comment that Carsten used to land on Aurora doesn't make sense. It shows how confusing this bug is... The patch landed on Aurora enables logging for all Firefox channels.

Wan-Teh, you said, the patch that Carsten landed on aurora is a no-op. I think it isn't a no-op, but it's working as intended (by Martin). Aurora uses NSS 3.24. NSS 3.24 has the code to check for variable NSS_ALLOW_SSLKEYLOGFILE.

Wan-Teh, does this make sense now?

Comment 34

[Kai Engert [:KaiE:]]

Martin, I'm setting needinfo for you, too, so you could confirm that my summary from comment 33 is correctly aligned with your intentions.

I've set needinfo to Wan-Teh simply to get his attention, so he could read my explanation.

Comment 35

[Martin Thomson [:mt:]]

Early in the discussion there was a suggestion that nightly and aurora (dev edition) would be the only versions to retain the feature.  However, as you can see here, and on the thread, there were several people that rely on this.  Since those people are also a large proportion of those who deal with networking issues on a regular basis, I think that the only reasonable conclusion is to restore the function entirely.

The comment on the check-in was an error on my part.  I changed the patch but neglected to update the comment.

Comment 36

[Wan-Teh Chang]

Kai,

I confirm that I understand your comment 33. Thank you
for the explanation.

In my comment 31, I said the patch was a no-op at that
time because the new NSS had not landed in mozilla-aurora
yet. I also said the patch would take effect after the
NSS in mozilla-aurora was upgraded.

Did we do all the work in NSS bug 1183318 for nothing?
Is Fedora going to enable SSLKEYLOGFILE in its NSS
package?

Comment 37

[Kai Engert [:KaiE:]]

(In reply to Wan-Teh Chang from comment #36)
> 
> Did we do all the work in NSS bug 1183318 for nothing?

That's my worry, too.

> Is Fedora going to enable SSLKEYLOGFILE in its NSS
> package?

Undecided.

Comment 38

[Kai Engert [:KaiE:]]

Martin, do you want to fix this edit?
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Key_Log_Format$compare?locale=en-US&to=1047154&from=1046962

Comment 39

[Martin Thomson [:mt:]]

Thanks for the reminder; done.

Comment 40

[Franziskus Kiefer [:franziskus]]

It looks like this never landed in central and thus never made it into any release. We have to a) land this patch in central, and b) uplift where appropriate (right now I think it's not possible to use SSLKEYLOGFILE in any official build of Firefox).

Comment 41

[April King [:April]]

Apparently this feature isn't as commonly used as I had hoped (since it has been broken for a while), but it would be really great if it could get fixed. I currently end up using Chrome every time I need to debug a network issue on a site that uses HTTPS, but that's no good on Firefox specific issues.

Thanks so much!

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b31faccc67c
Enable SSLKEYLOGFILE for non-release builds, r=kaie,glandium

Comment 43

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7380ab03cf390926270fd5d66e17352c8f7d7d9f

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b31faccc67c

Comment 45

[Liz Henry (:lizzard) (relman/hg->git project)]

I'm hoping we can let this ride with 50. Martin,  Dan, is it ok for this to be wontfixed in 49?

Comment 46

[Martin Thomson [:mt:]]

wontfix-49 WFM

Comment 47

[Kenneth Heal]

AFAICT from the bug SSLKEYLOGFILE is disabled for all mozilla-release builds (those commonly downloaded and used by end-users.)
Would also like to see this feature made available again.

Comment 48

[Patrick McManus [:mcmanus]]

(In reply to Kenneth Heal from comment #47)
> AFAICT from the bug SSLKEYLOGFILE is disabled for all mozilla-release builds
> (those commonly downloaded and used by end-users.)
> Would also like to see this feature made available again.

it should work again on >= 50. I'd like it on 49 too, but for risk management it makes sense not to take it at this point.

Comment 49

[Kenneth Heal]

@Patrick Thanks for the clarification, >=50 works well enough for me.