Bug 208192 – [Curl] Add TLS debugging feature to log encryption keys

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Takashi Komori]

Chrome has a debugging feature which enables people to record keys which is used for TLS encryption.
In this ticket we will append almost the same feature to Curl port.

[Takashi Komori]

Created attachment 391645 [details]
Add TLS debugging feature.

[Takashi Komori]

This patch adds TLS debugging feature which writes TLS key log to the file which is specified by environment variable "WEBKIT_CURL_TLS_KEY_LOG_FILE"
If we have the key log file and packet dump, we can decrypt the TLS packets which browser sent/received.
Outputted key log file follows the NSS key log format, and it supports less or equal to TLS version 1.2

 https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Key_Log_Format

This feature is enabled when only we enable ENABLE_TLS_DEBUG flag on build time as below.

>perl .\Tools\Scripts\build-webkit --wincairo --cmakeargs="-DENABLE_TLS_DEBUG=1"

Chrome also has the almost the same feature, it outputs the key log file to the path which is specified by environment variable "SSLKEYLOGFILE"

[Takashi Komori]

Usage:
1) Build wincairo with ENABLE_TLS_DEBUG=1
2) Set environment variable WEBKIT_CURL_TLS_KEY_LOG_FILE to a path which you have write privilege.
3) Start WireShark and start capturing packets.
4) Start MiniBrowser and brows some https sites.
5) Stop capturing packets.
6) Open Setting -> preference -> Protocols -> tls on WireShark
7) Set the path you specified to WEBKIT_CURLL_TLS_KEY_LOG_FILE to (Pre)=Master-Secret log filename.
8) You can see decrypted packets on WireShark.

[Fujii Hironori]

Can we include this feature in release builds like Firefox and Chrome?
Firefox and Chrome are using same name env bar. Can we use the same name?

[Fujii Hironori]

Comment on attachment 391645 [details]
Add TLS debugging feature.

View in context: https://bugs.webkit.org/attachment.cgi?id=391645&action=review

> Source/WebCore/platform/network/curl/CurlContext.h:131
> +    bool shouldLogTlsKey() const { return !m_tlsKeyLogFilePath.isEmpty(); }

‘Tls’ seems to violate WebKit coding style. https://webkit.org/code-style-guidelines/#names-basic
TLS is better?

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:123
> +    String line("CLIENT_RANDOM ");

do you need this temporary string? Why don’t you output by using fprintf?

[Fujii Hironori]

(In reply to Fujii Hironori from comment #4)
> Can we include this feature in release builds like Firefox and Chrome?

I meant production builds.

[Takashi Komori]

Created attachment 391724 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #4)
> Can we include this feature in release builds like Firefox and Chrome?
> Firefox and Chrome are using same name env bar. Can we use the same name?

Included in production build and changed the environment variable name to SSLKEYLOGFILE.

[Takashi Komori]

(In reply to Fujii Hironori from comment #5)
> Comment on attachment 391645 [details]
> Add TLS debugging feature.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391645&action=review
> 
> > Source/WebCore/platform/network/curl/CurlContext.h:131
> > +    bool shouldLogTlsKey() const { return !m_tlsKeyLogFilePath.isEmpty(); }
> 
> ‘Tls’ seems to violate WebKit coding style.
> https://webkit.org/code-style-guidelines/#names-basic
> TLS is better?

Fixed.

> 
> > Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:123
> > +    String line("CLIENT_RANDOM ");
> 
> do you need this temporary string? Why don’t you output by using fprintf?

Fixed.

[Fujii Hironori]

Comment on attachment 391724 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391724&action=review

> Source/cmake/OptionsWinCairo.cmake:56
> +WEBKIT_OPTION_DEFINE(ENABLE_TLS_DEBUG "Enable TLS key log support" PRIVATE ON)

Do you want to disable this feature? If not, remove all #if ENABLE(TLS_DEBUG).

[Takashi Komori]

Created attachment 391727 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #10)
> Comment on attachment 391724 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391724&action=review
> 
> > Source/cmake/OptionsWinCairo.cmake:56
> > +WEBKIT_OPTION_DEFINE(ENABLE_TLS_DEBUG "Enable TLS key log support" PRIVATE ON)
> 
> Do you want to disable this feature? If not, remove all #if
> ENABLE(TLS_DEBUG).

Removed.

[Takashi Komori]

(In reply to Takashi Komori from comment #12)
> (In reply to Fujii Hironori from comment #10)
> > Comment on attachment 391724 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=391724&action=review
> > 
> > > Source/cmake/OptionsWinCairo.cmake:56
> > > +WEBKIT_OPTION_DEFINE(ENABLE_TLS_DEBUG "Enable TLS key log support" PRIVATE ON)
> > 
> > Do you want to disable this feature? If not, remove all #if
> > ENABLE(TLS_DEBUG).
> 
> Removed.

Removed but some platform might want disabling option for a security perspective.

[Takashi Komori]

Created attachment 391729 [details]
Patch

[Takashi Komori]

Logging encryption keys feature could be a great security hole.
Some developers who use WebKit for example for some embedded system might concern that there is no way to disable the feature.
Considering that, we would better offer the disabling option.

[Fujii Hironori]

How do Chrome and Firefox deal with the great security hole?

[Takashi Komori]

(In reply to Fujii Hironori from comment #16)
> How do Chrome and Firefox deal with the great security hole?

Other browsers don't seem to have a strong safety guard for this feature.
In other words just setting the environment variable makes browsers start recording encryption keys into local PC.

If the recorded keys is secure and not stolen, the feature itself is secure too.
But we shouldn't assume all systems which use WebKit are implemented right and secure.
So I think offering developers the disabling option is reasonable.

Also I'm concerning browsers don't have any explicit way to reset or remove recorded keys.

[Fujii Hironori]

(In reply to Takashi Komori from comment #17)
> (In reply to Fujii Hironori from comment #16)
> > How do Chrome and Firefox deal with the great security hole?
> 
> Other browsers don't seem to have a strong safety guard for this feature.
> In other words just setting the environment variable makes browsers start
> recording encryption keys into local PC.
> 
> If the recorded keys is secure and not stolen, the feature itself is secure
> too.
> But we shouldn't assume all systems which use WebKit are implemented right
> and secure.
> So I think offering developers the disabling option is reasonable.

If it's possible for someone to steal file from PC, it's impossible to make the browser safe.
 
> Also I'm concerning browsers don't have any explicit way to reset or remove
> recorded keys.

I think it's enough to invoke command `rm $SSLKEYLOGFILE`.

[Alex Christensen]

Comment on attachment 391729 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391729&action=review

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:122
> +    auto fp = fopen(CurlContext::singleton().tlsKeyLogFilePath().utf8().data(), "a");

Are you sure you want all this file I/O on the main thread?
Is there an advantage to using wtf/FileSystem.h?

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:126
> +    fprintf(fp, "CLIENT_RANDOM ");

Do you care about the return code?

> Source/cmake/OptionsWinCairo.cmake:56
> +WEBKIT_OPTION_DEFINE(ENABLE_TLS_DEBUG "Enable TLS key log support" PRIVATE ON)

You probably want to have this off by default.

[Fujii Hironori]

Comment on attachment 391729 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391729&action=review

> ChangeLog:16
> +        * Source/cmake/OptionsWinCairo.cmake:

PlayStation port is also using curl. Don't you change OptionsPlayStation.cmake, too?

[Takashi Komori]

Created attachment 391853 [details]
Patch

[Takashi Komori]

(In reply to Alex Christensen from comment #19)
> Comment on attachment 391729 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391729&action=review
> 
> > Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:122
> > +    auto fp = fopen(CurlContext::singleton().tlsKeyLogFilePath().utf8().data(), "a");
> 
> Are you sure you want all this file I/O on the main thread?
> Is there an advantage to using wtf/FileSystem.h?

No, CurlSSLVerifier::infoCallback is invoked as a callback function in sub thread.

> 
> > Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:126
> > +    fprintf(fp, "CLIENT_RANDOM ");
> 
> Do you care about the return code?
> 

I think it doesn't seem to be necessary caring the return code.
Other parts using fprintf in WebKit also doesn't care.

> > Source/cmake/OptionsWinCairo.cmake:56
> > +WEBKIT_OPTION_DEFINE(ENABLE_TLS_DEBUG "Enable TLS key log support" PRIVATE ON)
> 
> You probably want to have this off by default.

Yes.
Even if the ON is preferable by default, disabling option could be useful for some developers.

[Takashi Komori]

(In reply to Fujii Hironori from comment #20)
> Comment on attachment 391729 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391729&action=review
> 
> > ChangeLog:16
> > +        * Source/cmake/OptionsWinCairo.cmake:
> 
> PlayStation port is also using curl. Don't you change
> OptionsPlayStation.cmake, too?

Added.

[Takashi Komori]

(In reply to Alex Christensen from comment #19)
> Comment on attachment 391729 [details]
> Patch
> 
> Are you sure you want all this file I/O on the main thread?
> Is there an advantage to using wtf/FileSystem.h?
> 

wtf/FileSystem.h doesn't have a mode which appends at tail of a file.
If we use FileSystem.h for logging, we have to keep open PlatformFileHandle as we don't have append mode.
On Windows platform, it means the key log file remains open until the browser exits, and we can't access the log file until  the browser exits.

We might have to consider adding appending mode in another ticket.

[Fujii Hironori]

Comment on attachment 391853 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391853&action=review

LGTM

> Source/cmake/OptionsWinCairo.cmake:58
> +WEBKIT_OPTION_END()

WEBKIT_OPTION_END is already called in OptionsWin.cmake for WinCairo.
It seems that WEBKIT_OPTION_END shouldn't be called twice because FEATURE_DEFINES will get duplicated entries.
I think it should be moved into OptionsWin.cmake.

if (WTF_PLATFORM_WIN_CAIRO)
    WEBKIT_OPTION_DEFINE(ENABLE_TLS_DEBUG "Enable TLS key log support" PRIVATE ON)
endif ()

[Takashi Komori]

Created attachment 391968 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #25)
> WEBKIT_OPTION_END is already called in OptionsWin.cmake for WinCairo.
> It seems that WEBKIT_OPTION_END shouldn't be called twice because
> FEATURE_DEFINES will get duplicated entries.
> I think it should be moved into OptionsWin.cmake.
> 
> if (WTF_PLATFORM_WIN_CAIRO)
>     WEBKIT_OPTION_DEFINE(ENABLE_TLS_DEBUG "Enable TLS key log support"
> PRIVATE ON)
> endif ()

Fixed.

[WebKit Commit Bot]

Comment on attachment 391968 [details]
Patch

Clearing flags on attachment: 391968

Committed r257656: <https://trac.webkit.org/changeset/257656>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59900616>

[Alex Christensen]

Comment on attachment 391968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391968&action=review

> Source/cmake/OptionsWin.cmake:76
> +    WEBKIT_OPTION_DEFINE(ENABLE_TLS_DEBUG "Enable TLS key log support" PRIVATE ON)

Again, this should really be off by default.  Please change it.  If someone who isn't an expert in TLS or WebKit builds WebKit, we want them to be safe.

[Fujii Hironori]

Reopening to attach new patch.

[Fujii Hironori]

Created attachment 392109 [details]
[Patch] Changed the default ENABLE_TLS_DEBUG to OFF

[WebKit Commit Bot]

Comment on attachment 392109 [details]
[Patch] Changed the default ENABLE_TLS_DEBUG to OFF

Clearing flags on attachment: 392109

Committed r257813: <https://trac.webkit.org/changeset/257813>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.