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.
Created attachment 391645 [details] Add TLS debugging feature.
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"
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.
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?
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?
(In reply to Fujii Hironori from comment #4) > Can we include this feature in release builds like Firefox and Chrome? I meant production builds.
Created attachment 391724 [details] Patch
(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.
(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.
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).
Created attachment 391727 [details] Patch
(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.
(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.
Created attachment 391729 [details] Patch
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.
How do Chrome and Firefox deal with the great security hole?
(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.
(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`.
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.
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?
Created attachment 391853 [details] Patch
(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.
(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.
(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.
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 ()
Created attachment 391968 [details] Patch
(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.
Comment on attachment 391968 [details] Patch Clearing flags on attachment: 391968 Committed r257656: <https://trac.webkit.org/changeset/257656>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59900616>
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.
Reopening to attach new patch.
Created attachment 392109 [details] [Patch] Changed the default ENABLE_TLS_DEBUG to OFF
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>