1183318 - Disable logging of TLS/SSL key material by default in optimized builds

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Maarten Boone]

After having a chat with Otto de Voogd ([e-mail]) about it, I want to file a issue about a potential security issue involving the SSLKEYLOGFILE functionality.
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Key_Log_Format

There seems to be no validation if the file is actually on the client system or someplace else, in Windows a samba share or WebDav url can use used as a target.
Because fopen is used against the content of SSLKEYLOGFILE you can basically supply any type op stream object, on Linux this includes /device/tcp/192.168.1.1:80 but this is just 1 example for the countless of possibilities.

In a lab test we managed to send data to a external system using WebDav, SMB and TCP using netcat as a receiver

And while I know you need to modify the target system to get it to work, from a attacker standpoint this method would be very valuable and most of all extremely easy because it doesn't actually need any malicious code on the target system to work.

While I haven't seen any attackers doing this I do know about malware cases where local logging of key material was used and later on send to the attacker using malware, with the method described above this could be done without any malicious code what so ever and most of all without the user actually knowing it's enabled since there is no notification of it being active in the browser windows at all.

So while in a perfect world it would be best to force the browser to log to local files only I think it would be best if at least some kind of notification is shown to the user to show that its active, preferably showing something that is "very" clear.
Especially since it involves total compromise of any and all cryptographic security of the users connection.

Another option to would be to prevent regular non privileged users from enabling it, but that's probably easier sad then done giving the size of your userbase

To end with is that this doesn't only effect Firefox also Chrome and Chromium and the Tor Browser bundle.
The last one might be a special case given the nature of the product

Regards,
Maarten Boone - Fox-IT

Comment 1

[Maarten Boone]

Since I didn't have a GPG key set on my account it didn't get the content of the 2 mails send about this topic.
If any of the 2 was addressed to me could you please resent them? Otherwise no problem

Comment 2

[Kai Engert (:KaiE:)]

Hi Maarten. Thanks for your report. The emails you received were probably automatic mails from bugzilla notifying you about changes to this bug, such as changes to the CC list (you can see the list of changes in the upper right "History" link on this page, and for every line of change I guess one email was sent).

Comment 3

[Maarten Boone]

Hi Kai,
No problem happy to help.

Btw Opera also seems to be affected

Comment 4

[Kai Engert (:KaiE:)]

So, in order to abuse this debugging facility for logging keys, I understand an attacker must be able to set an environment variable prior to executing an NSS application. This is a hurdle, and requires that the attacker is able to control the machine (at least once).

Nevertheless, I'm surprised that we support logging of the keys in regular, non-debug, non-trace builds.

Looking through commit history, I see the feature was originally inteded to be used only in builds that were compiled with the DEBUG and TRACE variables:
https://hg.mozilla.org/projects/nss/rev/660f72c97d6f

But later, it was changed to be active in all builds in bug 762763:
https://hg.mozilla.org/projects/nss/rev/ae2c06b0fed6

Comment 5

[Maarten Boone]

Yeah I found that as well.
But it seems trace is enabled in the normal build.

I agree with the fact that you have to touch the endpoint once, but in realty this is happening all the time, with malware most of the time though.
Also no special user privileges are required and there is no indication of it being active either

Comment 6

[Kai Engert (:KaiE:)]

Adding Adam, as he had originally suggested to add the patch.

Comment 7

[Maarten Boone]

Well to be clear I have no problem with the functionality.
It's possible to perfectly well use this for legit HTTPS traffic debugging.

But there is no notification for the user that its enabled, if you take Chrome for example, if you start chome with --no-sandbox it will show a yellow bar during the whole session as a warning for the user, something like that should be the least in my opinion.

And even if there would be notification in place I personally thing that it should be possible to log exclusively to local files.
Being able to use stuff like WebDav, SMB, TCP and yes even LPT is just crazy, there are already well documented cases where the file dumping function was used by targeted malware attacks where the malware submits the stolen key material.
Having a option to enable this without any additional code in a fully persistent and stealth way is a very real risk for the end user and lowers the bar for attackers to a absolute minimum

Comment 8

[Kai Engert (:KaiE:)]

Summarizing some thoughts from a discussion between some NSS developers.

It was argued that you cannot protect anyway against someone who has full access to the machine.

Someone who decides to set this environment variable on a victim's machine must obviously have prepared for doing so, by deciding in advance which value to use for the environment variable. That means, instead of setting the env variable, the attacker could equally easily install some backdoor software on the machine.

On first thought it seems like a reasonable idea to limit the logging to local files, but is it really that much better? An attacker could install a simple tool that forwards the contents of the local file to a remote service.

From the perspective of the NSS developers, it should be fine to open up this bug (make it public), because the feature is well documented. There are several HOWTO documents on the web that explain how to use this debugging feature.

From the perspective of the Firefox product, it might make sense to spend some more thoughts on it.

For example, Firefox has locked down the installation of add-ons so much, that an attacker can no longer tell a user to download an arbitrary unsigned add-on and install it, because Firefox will require a signed add-on.

On the other hand, an attacker could tell a naive victim to set this environment variable on their machine, for example, as part of a social engineering attack.

I think it would be reasonable to remove this debugging ability from regular Firefox builds, because of the social engineering risk.

The feature could remain enabled developer builds.

It has been suggested to move this discussion, whether or not to remove this feature from the Firefox product, to a place such as the firefox-dev list.

Comment 9

[Kai Engert (:KaiE:)]

Regardless of the decision in Firefox, I would like to suggest that we introduce a new build time variable, that an application can use to disable the feature, if desired.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

(In reply to Kai Engert (:kaie) from comment #9)
> Regardless of the decision in Firefox, I would like to suggest that we
> introduce a new build time variable, that an application can use to disable
> the feature, if desired.

I think that this would be a good idea.

Comment 11

[Kai Engert (:KaiE:)]

Attachment: patch to allow an application to disable this debug feature at build time

Comment 12

[Martin Thomson [:mt:]]

Comment on attachment 8634900 [details] [diff] [review]
patch to allow an application to disable this debug feature at build time

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

Seems reasonable.

Comment 13

[Maarten Boone]

Do you guys also keep in contact with the Tor Browser project?
I have the idea this might be something they want to disable as well asap

Comment 14

[Kai Engert (:KaiE:)]

Adding Mike Perry from Tor Project to CC list.

Comment 15

[Maarten Boone]

My argument for proper notification actually came from Firefox, it warns the user about invalid SSL certificates, stores passwords for you in a more secure way users would do otherwise cookies the same and even after downloading a TXT file the user is asked if he 'really' wants to open it.
Firefox actually has its own CA store to make sure the end user only gets trusted root certificates, also outdated or vulnerable plugins can be warned against with clearly visible banners in the browser or even disabled for the users own protection.
So while doing all that why not give a user the same type of warning when he's unknowingly sending all his pre master secrets and other highly sensitive material to external attacker.

Other thing to consider is that setting this up compromises not just Firefox but all of the 4 browsers at the same time when present of the user sytstem.
Even if you disable this in the consumer build its clearly a feature that should be handled with great care, if you put all the characteristics together a feature like this would also qualify as a backdoor, not a intentional one but that doesn't change the potential impact

If this technique would be used against a large organization for example you can even enable this using a custom Group Policy

Comment 16

[Daniel Veditz [:dveditz]]

Filed bug 1188657 to make use of DISABLE_SSLKEYLOGFILE in Firefox release builds. Filed bug 1188660 to show a prominent banner in Firefox when this logging is active (though I'm not sure NSS exposes that fact to clients).

Comment 17

[Mike Hommey [:glandium]]

Wrote this in bug 1188657, but this is more relevant here:
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 18

[Mike Perry]

This bug looks like a dup of the public bug at https://bugzilla.mozilla.org/show_bug.cgi?id=908046 from over 2 years ago. Does it need to be embargoed? It's neither novel nor any great secret, really.

I don't really think it is a security issue either, as on Linux if you can inject environment variables, you can grab keys using LD_PRELOAD. The same goes for DYLD_INSERT_LIBRARIES and DYLD_FORCE_FLAT_NAMESPACE on OSX. On Windows, the attacker has about 3 or 4 options: https://en.wikipedia.org/wiki/DLL_injection#Approaches_on_Microsoft_Windows. And all of that stuff doesn't matter, as typically if you can set an env var for a process, you can actually launch a modified one or a different executable instead.

That said: Sure whatever - let's kill the convenience feature here. Maybe in certain hardened/sandboxed/containerized setups of the future, code injection will become harder for some reason. Anyway, if people want to debug HTTPS issues, they should just run an alpha/nightly/dev build.

Comment 19

[Maarten Boone]

Mike,

Every option you give as a example requires either root or administrator level access to the system and/or adding executable code to a target system.
I managed to make the Tor browser send it's keys network/internet without the need to add anything to a system can could potentially be detected.

If you don't see this as a problem then we might differ on the definition of "problem"

Comment 20

[Mike Perry]

What I am saying is that I don't understand what is different about this bug than https://bugzilla.mozilla.org/show_bug.cgi?id=908046.

Are you saying that you have some method of setting environment variables that is possible to do in cases where you cannot already run arbitrary code? Because that would be news.

Otherwise, I recommend duping this bug to Bug #908046, or opening it up publicly.

Comment 21

[Maarten Boone]

Mike,

No I do not, but I showed that it's not just limited to logging to a file if that would be the case at least you would need additional malware to exfiltrate the keys after logging.
Having something like this in the Tor Browser just doesn't seem to be a great idea.

Comment 22

[Georg Koppen]

Could we get that bug opened up (+ the fix with r+ landed, or a different one)? It seems comment 8 and comment 20 give some reasoning why this seems like a good idea.

We are currently preparing the switch to ESR 45 for Tor Browser and would like to take a patch for this bug as kind of a defense-in-depth measure (assuming this does not qualify for an ESR bug fix). And we would link to this bug in case people looking over that commit would like understand the issue better.

Comment 23

[Daniel Veditz [:dveditz]]

Kai: is there anything more that's preventing this reviewed patch from landing?

Comment 24

[Kai Engert (:KaiE:)]

The only why I had not landed this earlier: I was worried some sort of coordination with landing Firefox code might be desirable.

Given that this has been opened up, I'm OK to land this now.

Comment 25

[Kai Engert (:KaiE:)]

changed component to NSS, because the patch is for NSS.

Comment 26

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/a8ed4278b46a

Comment 27

[Kai Engert (:KaiE:)]

While working on a potential patch for Firefox, and thinking about Mike's and Wan-Teh's comments again, maybe the following would be better:

- continue to enable the feature by default in NSS debug builds
- in NSS optimized builds, require opt in

In Firefox, when building a nightly or developer edition build, it could opt in, even when doing an optimized build.

Martin, does this make sense?

Also, we might want to rename the symbol to have a NSS_ prefix.

Comment 28

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #27)
> - continue to enable the feature by default in NSS debug builds
> - in NSS optimized builds, require opt in

As a result, NSS packages shipped by Linux distributions would disable the key logging feature by default.

Comment 29

[Kai Engert (:KaiE:)]

I've backed out, I'd like to not rush this in an incorrect way.
https://hg.mozilla.org/projects/nss/rev/687df1f5ace3

Comment 30

[Kai Engert (:KaiE:)]

Attachment: 1183318-v2.patch

Comment 31

[Martin Thomson [:mt:]]

Comment on attachment 8744113 [details] [diff] [review]
1183318-v2.patch

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

I have one comment, but it's not a big one.

::: lib/ssl/Makefile
@@ +40,5 @@
>  endif
>  
> +# In debug builds, set NSS_ALLOW_SSLKEYLOGFILE by default
> +ifndef BUILD_OPT
> +DEFINES += -DNSS_ALLOW_SSLKEYLOGFILE=1

I think that this would be better as:

# 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

So that someone can override this.

Comment 32

[Martin Thomson [:mt:]]

Comment on attachment 8744113 [details] [diff] [review]
1183318-v2.patch

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

I have one comment, but it's not a big one.

::: lib/ssl/Makefile
@@ +40,5 @@
>  endif
>  
> +# In debug builds, set NSS_ALLOW_SSLKEYLOGFILE by default
> +ifndef BUILD_OPT
> +DEFINES += -DNSS_ALLOW_SSLKEYLOGFILE=1

I think that this would be better as:

# 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

So that someone can override this.

Comment 33

[Kai Engert (:KaiE:)]

Attachment: 1183318-v2b.patch

(In reply to Martin Thomson [:mt:] from comment #32)
> I think that this would be better as:
> ...
> So that someone can override this.

That's a good improvement. Thanks Martin!

Comment 34

[Kai Engert (:KaiE:)]

Comment on attachment 8744251 [details] [diff] [review]
1183318-v2b.patch

https://hg.mozilla.org/projects/nss/rev/68d0b829490f

Comment 35

[Wan-Teh Chang]

Comment on attachment 8744251 [details] [diff] [review]
1183318-v2b.patch

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

::: lib/ssl/Makefile
@@ +40,5 @@
>  endif
>  
> +# 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)

Nit: the value after ?= can be

    $(if $(BUILD_OPT),,1)

which uses the prevalent convention for NSS makefile
variables, which either is undefined/empty or has the
value 1.

@@ +42,5 @@
> +# 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

The =1 part should be omitted. If -DFOO is specified, the compiler
defines FOO as 1.

Comment 36

[Maarten Boone]

So far defense-in-depth.....