Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wolfssl: Add SSLKEYLOGFILE support #5327

Closed
wants to merge 3 commits into from

Conversation

@Lekensteyn
Copy link
Member

Lekensteyn commented May 2, 2020

Refactored SSLKEYLOGFILE support in OpenSSL and extended support to WolfSSL.
Tested with OpenSSL 0.9.8zh, 1.0.2u, and 1.1.1f and WolfSSL v4.4.0-stable-44-g3944c8eb7 and TLS 1.0/1.3, see individual commit messages for details.


DONE:

  • Refactor to enable code-reuse with OpenSSL code.
  • Verify things still work with a configuration built for multiple TLS backend. Tested with wolfssl+boringssl and quiche. Tested with NSS and ngtcp2+OpenSSL (NSS does not support the keylog callback but already respect SSLKEYLOGFILE out-of-the-box).
  • Update documentation (not needed)
@Lekensteyn Lekensteyn added the SSL/TLS label May 2, 2020
@Lekensteyn Lekensteyn force-pushed the Lekensteyn:tls-keylog-wolfssl branch from 42ca65a to 9b3883a May 2, 2020
@jay
Copy link
Member

jay commented May 2, 2020

It should probably be refactored to enable code reuse.

Definitely. The SSLKEYLOGFILE code in ngtcp2 needs to be changed for thread safety, you can see an attempt in #4311 that I abandoned. I suggest a separate file in vtls sslkeylogfile.c and then put everything there.

@Lekensteyn Lekensteyn force-pushed the Lekensteyn:tls-keylog-wolfssl branch from 9b3883a to ed3e1aa May 3, 2020
@Lekensteyn
Copy link
Member Author

Lekensteyn commented May 3, 2020

I've added a new keylog.c implementation and adapted OpenSSL and WolfSSL to use it. Test instructions can be found in the commit messages. None of the documentation seem to require updates, I could update libcurl-env.3 with version numbers and backends, but maybe that is too much detail?

I've a patch to add support for ngtcp2 as well but still have to test it. It could be done as part of this PR or a new PR once the patch is ready.

With the multiple TLS backends feature, only one of them can every be loaded it seems. So there is no concern about opening/closing the keylog file multiple times. However with addition of the feature to vquic, Curl_tls_keylog_open/_close can be called by both vtls and vquic. In the proposed implementation, it should still work (f it is already open or closed - nothing to do). Is that a reasonable approach? Or should I add reference counting, and close the file only if all users are done?

@Lekensteyn Lekensteyn marked this pull request as ready for review May 4, 2020
@Lekensteyn Lekensteyn force-pushed the Lekensteyn:tls-keylog-wolfssl branch from ed3e1aa to 55d72f8 May 8, 2020
@Lekensteyn
Copy link
Member Author

Lekensteyn commented May 8, 2020

Rebased on master and tested against ngtcp2 + openssl (built using CMake - patches for this will follow in a separate PR).

👀 are welcome!

@Lekensteyn Lekensteyn force-pushed the Lekensteyn:tls-keylog-wolfssl branch from 55d72f8 to 3c4ed37 May 9, 2020
@Lekensteyn Lekensteyn force-pushed the Lekensteyn:tls-keylog-wolfssl branch 2 times, most recently from b3a7991 to 98f0681 May 23, 2020
@Lekensteyn
Copy link
Member Author

Lekensteyn commented May 25, 2020

Rebased again on master (no changes). The previous tests failed due to ngtcp2 build errors, that should have been fixed in master now.

Feature freeze is coming up :)
(Not a high prio as it is refactoring + small feature, but nevertheless it would clear the queue.)

@bagder
bagder approved these changes May 26, 2020
Create a set of routines for TLS key log file handling to enable reuse
with other TLS backends. Simplify the OpenSSL backend as follows:

 - Drop the ENABLE_SSLKEYLOGFILE macro as it is unconditionally enabled.
 - Do not perform dynamic memory allocation when preparing a log entry.
   Unless the TLS specifications change we can suffice with a reasonable
   fixed-size buffer.
 - Simplify state tracking when SSL_CTX_set_keylog_callback is
   unavailable. My original sslkeylog.c code included this tracking in
   order to handle multiple calls to SSL_connect and detect new keys
   after renegotiation (via SSL_read/SSL_write). For curl however we can
   be sure that a single master secret eventually becomes available
   after SSL_connect, so a simple flag is sufficient. An alternative to
   the flag is examining SSL_state(), but this seems more complex and is
   not pursued. Capturing keys after server renegotiation was already
   unsupported in curl and remains unsupported.

Tested with curl built against OpenSSL 0.9.8zh, 1.0.2u, and 1.1.1f
(`SSLKEYLOGFILE=keys.txt curl -vkso /dev/null https://localhost:4433`)
against an OpenSSL 1.1.1f server configured with:

    # Force non-TLSv1.3, use TLSv1.0 since 0.9.8 fails with 1.1 or 1.2
    openssl s_server -www -tls1
    # Likewise, but fail the server handshake.
    openssl s_server -www -tls1 -Verify 2
    # TLS 1.3 test. No need to test the failing server handshake.
    openssl s_server -www -tls1_3

Verify that all secrets (1 for TLS 1.0, 4 for TLS 1.3) are correctly
written using Wireshark. For the first and third case, expect four
matches per connection (decrypted Server Finished, Client Finished, HTTP
Request, HTTP Response). For the second case where the handshake fails,
expect a decrypted Server Finished only.

    tshark -i lo -pf tcp -otls.keylog_file:keys.txt -Tfields \
        -eframe.number -eframe.time -etcp.stream -e_ws.col.Info \
        -dtls.port==4433,http -ohttp.desegment_body:FALSE \
        -Y 'tls.handshake.verify_data or http'

A single connection can easily be identified via the `tcp.stream` field.
@Lekensteyn
Copy link
Member Author

Lekensteyn commented May 26, 2020

Hmm, a previous build with --enable-all (as is done by CI) succeeds, but I noticed by accident that a build without any options enabled (with no features in wolfssl/options.h) fails because OPENSSL_EXTRA is not defined.

Warnings:

/include/wolfssl/wolfcrypt/settings.h:2089:14: warning: #warning is a GCC extension
/include/wolfssl/wolfcrypt/settings.h:2089:14: warning: #warning "For timing resistance / side-channel attack prevention consider using harden options" [-Wcpp]
lib/vtls/wolfssl.c: In function ‘wolfssl_log_tls12_secret’:
lib/vtls/wolfssl.c:165:10: warning: implicit declaration of function ‘SSL_version’; did you mean ‘curl_version’? [-Wimplicit-function-declaration]
lib/vtls/wolfssl.c:165:10: warning: nested extern declaration of ‘SSL_version’ [-Wnested-externs]
In file included from lib/vtls/wolfssl.c:75:

This is tricky. I see there is wolfSSL_GetVersion which is always available, but it requires wolfSSL 3.13.0 (December 2017). The only reason why this check exists is to skip extraction for TLS 1.3.

Based on the git logs for wolfssl src/tls13.c:

  • v3.11.1-tls13-beta (May 2017) - First preview "release" with TLS 1.3 (draft 18) support.
  • v3.12.0-stable (Aug 2017) - draft 20 support.
  • v3.13.0-stable (Dec 2017) - draft 21 support.
  • v3.15.0-stable (June 2018) - RFC 8446 support.

I'll assume it is acceptable not to support wolfSSL 3.12 where TLS 1.3 is enabled.

lib/vtls/wolfssl.c: In function ‘wolfssl_connect_step2’:
/include/wolfssl/openssl/ssl.h:812:41: warning: implicit declaration of function ‘wolfSSL_want’; did you mean ‘wolfSSL_Init’? [-Wimplicit-function-declaration]
/include/wolfssl/openssl/ssl.h:812:41: warning: nested extern declaration of ‘wolfSSL_want’ [-Wnested-externs]
lib/vtls/wolfssl.c:559:59: error: ‘SSL_NOTHING’ undeclared (first use in this function)

Fixed by using a different API with the same result.

After fixing that, I ran into yet another issue. The public header make SSL_get_keys unconditionally available, but the actual implementation requires OPENSSL_EXTRA to be set... Likewise for the alternative function wolfSSL_SESSION_get_master_key. I guess this keylogfile helper will only be available with that option.

Lekensteyn added 2 commits May 3, 2020
Tested following the same curl and tshark commands as in commit
"vtls: Extract and simplify key log file handling from OpenSSL" using
WolfSSL v4.4.0-stable-128-g5179503e8 from git master built with
`./configure --enable-all --enable-debug CFLAGS=-DHAVE_SECRET_CALLBACK`.

Full support for this feature requires certain wolfSSL build options,
see "Availability note" in lib/vtls/wolfssl.c for details.

Closes #5327
Tested with ngtcp2 built against the OpenSSL library. Additionally
tested with MultiSSL (NSS for TLS and ngtcp2+OpenSSL for QUIC).

The TLS backend (independent of QUIC) may or may not already have opened
the keylog file before. Therefore Curl_tls_keylog_open is always called
to ensure the file is open.
@Lekensteyn Lekensteyn force-pushed the Lekensteyn:tls-keylog-wolfssl branch from 98f0681 to 19f001d May 26, 2020
@Lekensteyn
Copy link
Member Author

Lekensteyn commented May 26, 2020

I amended the existing wolfssl patch with the following and referenced the "Availability note" in the commit message:

diff
--- a/lib/vtls/wolfssl.c
+++ b/lib/vtls/wolfssl.c
@@ -100,12 +100,15 @@ struct ssl_backend_data {
 static Curl_recv wolfssl_recv;
 static Curl_send wolfssl_send;
 
+#ifdef OPENSSL_EXTRA
 /*
  * Availability note:
  * The TLS 1.3 secret callback (wolfSSL_set_tls13_secret_cb) was added in
  * WolfSSL 4.4.0, but requires the -DHAVE_SECRET_CALLBACK build option. If that
  * option is not set, then TLS 1.3 will not be logged.
  * For TLS 1.2 and before, we use wolfSSL_get_keys().
+ * SSL_get_client_random and wolfSSL_get_keys require OPENSSL_EXTRA
+ * (--enable-opensslextra or --enable-all).
  */
 #if defined(HAVE_SECRET_CALLBACK) && defined(WOLFSSL_TLS13)
 static int
@@ -162,17 +165,23 @@ wolfssl_log_tls12_secret(SSL *ssl)
   unsigned char *ms, *sr, *cr;
   unsigned int msLen, srLen, crLen, i, x = 0;
 
-  switch(SSL_version(ssl)) {
-  case SSL3_VERSION:
-  case TLS1_VERSION:
-  case TLS1_1_VERSION:
-  case TLS1_2_VERSION:
+#if LIBWOLFSSL_VERSION_HEX >= 0x0300d000 /* >= 3.13.0 */
+  /* wolfSSL_GetVersion is available since 3.13, we use it instead of
+   * SSL_version since the latter relies on OPENSSL_ALL (--enable-opensslall or
+   * --enable-all). Failing to perform this check could result in an unusable
+   * key log line when TLS 1.3 is actually negotiated. */
+  switch(wolfSSL_GetVersion(ssl)) {
+  case WOLFSSL_SSLV3:
+  case WOLFSSL_TLSV1:
+  case WOLFSSL_TLSV1_1:
+  case WOLFSSL_TLSV1_2:
     break;
   default:
     /* TLS 1.3 does not use this mechanism, the "master secret" returned below
      * is not directly usable. */
     return;
   }
+#endif
 
   if(SSL_get_keys(ssl, &ms, &msLen, &sr, &srLen, &cr, &crLen) != SSL_SUCCESS) {
     return;
@@ -190,6 +199,7 @@ wolfssl_log_tls12_secret(SSL *ssl)
 
   Curl_tls_keylog_write("CLIENT_RANDOM", cr, ms, msLen);
 }
+#endif /* OPENSSL_EXTRA */
 
 static int do_file_type(const char *type)
 {
@@ -477,6 +487,7 @@ wolfssl_connect_step1(struct connectdata *conn,
   }
 #endif /* HAVE_ALPN */
 
+#ifdef OPENSSL_EXTRA
   if(Curl_tls_keylog_enabled()) {
     /* Ensure the Client Random is preserved. */
     wolfSSL_KeepArrays(backend->handle);
@@ -485,6 +496,7 @@ wolfssl_connect_step1(struct connectdata *conn,
                                 wolfssl_tls13_secret_callback, NULL);
 #endif
   }
+#endif /* OPENSSL_EXTRA */
 
   /* Check if there's a cached ID we can/should use here! */
   if(SSL_SET_OPTION(primary.sessionid)) {
@@ -546,23 +558,29 @@ wolfssl_connect_step2(struct connectdata *conn,
 
   ret = SSL_connect(backend->handle);
 
+#ifdef OPENSSL_EXTRA
   if(Curl_tls_keylog_enabled()) {
     /* If key logging is enabled, wait for the handshake to complete and then
      * proceed with logging secrets (for TLS 1.2 or older).
      *
-     * During the handshake (ret==-1), WolfSSL_want returns SSL_READING when it
-     * is waiting for the server response. When the handshake finishes (success
-     * or failure), it returns SSL_NOTHING and we may read the master secret.
-     * Note that OpenSSL SSL_want always returns SSL_READING at this point.
-     * If this ever changes, the worst case is that no key is logged on error.
+     * During the handshake (ret==-1), wolfSSL_want_read() is true as it waits
+     * for the server response. At that point the master secret is not yet
+     * available, so we must not try to read it.
+     * To log the secret on completion with a handshake failure, detect
+     * completion via the observation that there is nothing to read or write.
+     * Note that OpenSSL SSL_want_read() is always true here. If wolfSSL ever
+     * changes, the worst case is that no key is logged on error.
      */
-    if(ret == SSL_SUCCESS || SSL_want(backend->handle) == SSL_NOTHING) {
+    if(ret == SSL_SUCCESS ||
+       (!wolfSSL_want_read(backend->handle) &&
+        !wolfSSL_want_write(backend->handle))) {
       wolfssl_log_tls12_secret(backend->handle);
       /* Client Random and master secrets are no longer needed, erase these.
        * Ignored while the handshake is still in progress. */
       wolfSSL_FreeArrays(backend->handle);
     }
   }
+#endif  /* OPENSSL_EXTRA */
 
   if(ret != 1) {
     char error_buffer[WOLFSSL_MAX_ERROR_SZ];
@@ -870,7 +888,9 @@ static size_t Curl_wolfssl_version(char *buffer, size_t size)
 
 static int Curl_wolfssl_init(void)
 {
+#ifdef OPENSSL_EXTRA
   Curl_tls_keylog_open();
+#endif
   return (wolfSSL_Init() == SSL_SUCCESS);
 }
 
@@ -878,7 +898,9 @@ static int Curl_wolfssl_init(void)
 static void Curl_wolfssl_cleanup(void)
 {
   wolfSSL_Cleanup();
+#ifdef OPENSSL_EXTRA
   Curl_tls_keylog_close();
+#endif
 }

This was tested with a bunch of wolfSSL v4.4.0-stable-128-g5179503e8 configs:

../configure --prefix=/tmp/wolfssl-src/prefix --enable-all --enable-tls13 --enable-debug
../configure --prefix=/tmp/wolfssl-src/prefix --enable-debug
../configure --prefix=/tmp/wolfssl-src/prefix --enable-debug --enable-tls13
../configure --prefix=/tmp/wolfssl-src/prefix --enable-debug --enable-tls13 CFLAGS=-DHAVE_SECRET_CALLBACK
../configure --prefix=/tmp/wolfssl-src/prefix --enable-debug --enable-tls13 CFLAGS=-DHAVE_SECRET_CALLBACK --enable-opensslextra

Does this still look fine to you @bagder?

@bagder
Copy link
Member

bagder commented May 27, 2020

👍 from me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.