Closed Bug 1188657 Opened 9 years ago Closed 8 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox48 fixed, firefox49 wontfix, firefox50 fixed, firefox51 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- fixed
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: dveditz, Assigned: mt)

References

Details

(Keywords: csectype-disclosure, sec-want, Whiteboard: [adv-main48-][adv-main50-])

Attachments

(2 files)

+++ 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.
See Also: → 1188660
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.
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.
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.
Group: core-security → firefox-core-security
Group: firefox-core-security
Keywords: sec-moderatesec-want
Attached patch 1188657-v1.patchSplinter Review
Should this work with patch v2 from bug 1183318 ?
Attachment #8744116 - Flags: superreview?(mh+mozilla)
Attachment #8744116 - Flags: review?(martin.thomson)
Summary: Release and Beta Firefox should build NSS with DISABLE_SSLKEYLOGFILE → Release and Beta Firefox should disable the SSL/TLS key logging feature in NSS
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?
(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 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.
Attachment #8744116 - Flags: review?(martin.thomson) → review-
Attachment #8744116 - Flags: superreview?(mh+mozilla)
Marking as duplicate of bug 1258375, as the upgrade to NSS 3.24 will have the desired effect.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(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 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.
Attachment #8745258 - Flags: review?(kaie) → review+
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).
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.
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.
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.
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 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
Attachment #8745258 - Flags: review?(mh+mozilla) → review+
This seems to wait for Martin to commit and close.
Assignee: nobody → martin.thomson
Flags: needinfo?(martin.thomson)
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.
Flags: needinfo?(martin.thomson)
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/
Attachment #8745258 - Attachment description: MozReview Request: Bug 1188657 - Enable SSLKEYLOGFILE for non-release builds, r?kaie,glandium → MozReview Request: Bug 1188657 - Enable SSLKEYLOGFILE for non-release builds, r=kaie,glandium
Attachment #8745258 - Flags: review+ → review?(kaie)
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
Attachment #8745258 - Flags: approval-mozilla-aurora?
thanks!
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 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.
Attachment #8745258 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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/
Sorry Kai, I've been using mercurial for NSS so long that I forgot how git works.  The right change has been uploaded.
  https://hg.mozilla.org/releases/mozilla-aurora/rev/e5694384be52
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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.
Attachment #8745258 - Flags: review?(kaie) → review+
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.
Summary: Release and Beta Firefox should disable the SSL/TLS key logging feature in NSS → Keep SSL/TLS key logging working with all Firefox builds from Mozilla
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.
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".
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?
Flags: needinfo?(wtc)
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.
Flags: needinfo?(martin.thomson)
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.
Flags: needinfo?(martin.thomson)
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?
Flags: needinfo?(wtc)
(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.
Thanks for the reminder; done.
Flags: needinfo?(martin.thomson)
Whiteboard: [adv-main48-]
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).
Status: RESOLVED → REOPENED
Flags: needinfo?(martin.thomson)
Resolution: FIXED → ---
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!
Pushed by franziskuskiefer@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b31faccc67c
Enable SSLKEYLOGFILE for non-release builds, r=kaie,glandium
https://hg.mozilla.org/releases/mozilla-aurora/rev/7380ab03cf390926270fd5d66e17352c8f7d7d9f
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Flags: needinfo?(martin.thomson)
Resolution: --- → FIXED
Target Milestone: Firefox 48 → Firefox 50
I'm hoping we can let this ride with 50. Martin,  Dan, is it ok for this to be wontfixed in 49?
Flags: needinfo?(martin.thomson)
Flags: needinfo?(dveditz)
wontfix-49 WFM
Flags: needinfo?(martin.thomson)
Flags: needinfo?(dveditz)
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.
(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.
@Patrick Thanks for the clarification, >=50 works well enough for me.
Whiteboard: [adv-main48-] → [adv-main48-][adv-main50-]
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 50 → mozilla50
You need to log in before you can comment on or make changes to this bug.