doesn't compile against latest BoringSSL #275

Closed
bprodoehl opened this Issue May 18, 2015 · 7 comments

Comments

Projects
None yet
2 participants
@bprodoehl
Contributor

bprodoehl commented May 18, 2015

A change landed in BoringSSL recently that breaks the libcurl build. I'm seeing these errors with boringssl HEAD and libcurl HEAD:

vtls/openssl.c:1810:9: warning: implicit declaration of function 'SSL_CTX_callback_ctrl' is invalid in C99
      [-Wimplicit-function-declaration]
    if(!SSL_CTX_callback_ctrl(connssl->ctx, SSL_CTRL_SET_MSG_CALLBACK,
        ^
vtls/openssl.c:1810:45: error: use of undeclared identifier 'doesnt_exist'
    if(!SSL_CTX_callback_ctrl(connssl->ctx, SSL_CTRL_SET_MSG_CALLBACK,
                                            ^
/Users/bprodoehl/src/boringssl/include/openssl/ssl.h:2405:35: note: expanded from macro 'SSL_CTRL_SET_MSG_CALLBACK'
#define SSL_CTRL_SET_MSG_CALLBACK doesnt_exist
                                  ^
vtls/openssl.c:1814:41: error: use of undeclared identifier 'doesnt_exist'
    else if(!SSL_CTX_ctrl(connssl->ctx, SSL_CTRL_SET_MSG_CALLBACK_ARG, 0,
                                        ^
/Users/bprodoehl/src/boringssl/include/openssl/ssl.h:2406:39: note: expanded from macro
      'SSL_CTRL_SET_MSG_CALLBACK_ARG'
#define SSL_CTRL_SET_MSG_CALLBACK_ARG doesnt_exist
                                      ^

I believe this is the change responsible for breaking things: https://boringssl.googlesource.com/boringssl/+/59015c365b53a855513aaf5f9ff4597df9157ac0%5E!/

@bprodoehl

This comment has been minimized.

Show comment
Hide comment
@bprodoehl

bprodoehl May 19, 2015

Contributor

I don't completely understand the changes, but I think the patch looks something like this:

diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index 16053a7..8febd97 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -1805,6 +1805,11 @@ static CURLcode ossl_connect_step1(struct connectdata *conn, int sockindex)

 #ifdef SSL_CTRL_SET_MSG_CALLBACK
   if(data->set.fdebug && data->set.verbose) {
+#ifdef HAVE_BORINGSSL
+    connssl->ctx->msg_callback = (void (*)(int, int, int, const void *,
+                                           size_t, SSL *, void *))ssl_tls_trace;
+    SSL_CTX_set_msg_callback_arg(connssl->ctx, conn);
+#else
     /* the SSL trace callback is only used for verbose logging so we only
        inform about failures of setting it */
     if(!SSL_CTX_callback_ctrl(connssl->ctx, SSL_CTRL_SET_MSG_CALLBACK,
@@ -1815,6 +1820,7 @@ static CURLcode ossl_connect_step1(struct connectdata *conn, int sockindex)
                           conn)) {
       infof(data, "SSL: couldn't set callback argument!\n");
     }
+#endif
   }
 #endif

Looking through the relevant BoringSSL commits, that seems right, and it gets me building again. I'll have to test more to make sure things actually work with that.

Contributor

bprodoehl commented May 19, 2015

I don't completely understand the changes, but I think the patch looks something like this:

diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index 16053a7..8febd97 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -1805,6 +1805,11 @@ static CURLcode ossl_connect_step1(struct connectdata *conn, int sockindex)

 #ifdef SSL_CTRL_SET_MSG_CALLBACK
   if(data->set.fdebug && data->set.verbose) {
+#ifdef HAVE_BORINGSSL
+    connssl->ctx->msg_callback = (void (*)(int, int, int, const void *,
+                                           size_t, SSL *, void *))ssl_tls_trace;
+    SSL_CTX_set_msg_callback_arg(connssl->ctx, conn);
+#else
     /* the SSL trace callback is only used for verbose logging so we only
        inform about failures of setting it */
     if(!SSL_CTX_callback_ctrl(connssl->ctx, SSL_CTRL_SET_MSG_CALLBACK,
@@ -1815,6 +1820,7 @@ static CURLcode ossl_connect_step1(struct connectdata *conn, int sockindex)
                           conn)) {
       infof(data, "SSL: couldn't set callback argument!\n");
     }
+#endif
   }
 #endif

Looking through the relevant BoringSSL commits, that seems right, and it gets me building again. I'll have to test more to make sure things actually work with that.

@bagder bagder self-assigned this May 19, 2015

@bagder bagder added the SSL/TLS label May 19, 2015

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 19, 2015

Member

From https://twitter.com/davidben__/status/600652652121325568:

"you should be able to call SSL_CTX_set_msg_callback and SSL_CTX_set_msg_callback_arg in OpenSSL too"

Member

bagder commented May 19, 2015

From https://twitter.com/davidben__/status/600652652121325568:

"you should be able to call SSL_CTX_set_msg_callback and SSL_CTX_set_msg_callback_arg in OpenSSL too"

@bprodoehl

This comment has been minimized.

Show comment
Hide comment
@bprodoehl

bprodoehl May 19, 2015

Contributor

Great, I'll try SSL_CTX_set_msg_callback() instead of setting it directly, and will see if that works against OpenSSL, too.

Contributor

bprodoehl commented May 19, 2015

Great, I'll try SSL_CTX_set_msg_callback() instead of setting it directly, and will see if that works against OpenSSL, too.

@bprodoehl

This comment has been minimized.

Show comment
Hide comment
@bprodoehl

bprodoehl May 19, 2015

Contributor

Here's an updated patch, tested against BoringSSL HEAD and Fedora 21's OpenSSL 1.0.1k.

diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index 16053a7..2092411 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -1805,16 +1805,10 @@ static CURLcode ossl_connect_step1(struct connectdata *conn, int sockindex)

 #ifdef SSL_CTRL_SET_MSG_CALLBACK
   if(data->set.fdebug && data->set.verbose) {
-    /* the SSL trace callback is only used for verbose logging so we only
-       inform about failures of setting it */
-    if(!SSL_CTX_callback_ctrl(connssl->ctx, SSL_CTRL_SET_MSG_CALLBACK,
-                               (void (*)(void))ssl_tls_trace)) {
-      infof(data, "SSL: couldn't set callback!\n");
-    }
-    else if(!SSL_CTX_ctrl(connssl->ctx, SSL_CTRL_SET_MSG_CALLBACK_ARG, 0,
-                          conn)) {
-      infof(data, "SSL: couldn't set callback argument!\n");
-    }
+    /* the SSL trace callback is only used for verbose logging */
+    SSL_CTX_set_msg_callback(connssl->ctx, (void (*)(int, int, int, const void *,
+                                           size_t, SSL *, void *))ssl_tls_trace);
+    SSL_CTX_set_msg_callback_arg(connssl->ctx, conn);
   }
 #endif

Contributor

bprodoehl commented May 19, 2015

Here's an updated patch, tested against BoringSSL HEAD and Fedora 21's OpenSSL 1.0.1k.

diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index 16053a7..2092411 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -1805,16 +1805,10 @@ static CURLcode ossl_connect_step1(struct connectdata *conn, int sockindex)

 #ifdef SSL_CTRL_SET_MSG_CALLBACK
   if(data->set.fdebug && data->set.verbose) {
-    /* the SSL trace callback is only used for verbose logging so we only
-       inform about failures of setting it */
-    if(!SSL_CTX_callback_ctrl(connssl->ctx, SSL_CTRL_SET_MSG_CALLBACK,
-                               (void (*)(void))ssl_tls_trace)) {
-      infof(data, "SSL: couldn't set callback!\n");
-    }
-    else if(!SSL_CTX_ctrl(connssl->ctx, SSL_CTRL_SET_MSG_CALLBACK_ARG, 0,
-                          conn)) {
-      infof(data, "SSL: couldn't set callback argument!\n");
-    }
+    /* the SSL trace callback is only used for verbose logging */
+    SSL_CTX_set_msg_callback(connssl->ctx, (void (*)(int, int, int, const void *,
+                                           size_t, SSL *, void *))ssl_tls_trace);
+    SSL_CTX_set_msg_callback_arg(connssl->ctx, conn);
   }
 #endif

@bprodoehl

This comment has been minimized.

Show comment
Hide comment
@bprodoehl

bprodoehl May 19, 2015

Contributor

I can submit an actual PR for this, if you'd like. Just let me know.

Contributor

bprodoehl commented May 19, 2015

I can submit an actual PR for this, if you'd like. Just let me know.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 19, 2015

Member

Yes please do, makes it slightly easier for me.
Den 19 maj 2015 4:47 em skrev "Brian Prodoehl" notifications@github.com:

I can submit an actual PR for this, if you'd like. Just let me know.

Reply to this email directly or view it on GitHub
#275 (comment).

Member

bagder commented May 19, 2015

Yes please do, makes it slightly easier for me.
Den 19 maj 2015 4:47 em skrev "Brian Prodoehl" notifications@github.com:

I can submit an actual PR for this, if you'd like. Just let me know.

Reply to this email directly or view it on GitHub
#275 (comment).

bagder added a commit that referenced this issue May 19, 2015

openssl: Use SSL_CTX_set_msg_callback and SSL_CTX_set_msg_callback_arg
BoringSSL removed support for direct callers of SSL_CTX_callback_ctrl
and SSL_CTX_ctrl, so move to a way that should work on BoringSSL and
OpenSSL.

re #275
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 19, 2015

Member

thanks, fixed merged now

Member

bagder commented May 19, 2015

thanks, fixed merged now

@bagder bagder closed this May 19, 2015

jsonn pushed a commit to jsonn/pkgsrc that referenced this issue Jun 30, 2015

spz
update of curl to version 7.43.0. Upstream RELEASE_NOTES:
Curl and libcurl 7.43.0

 Public curl releases:         147
 Command line options:         176
 curl_easy_setopt() options:   219
 Public functions in libcurl:  58
 Contributors:                 1291

This release includes the following changes:

 o Added CURLOPT_PROXY_SERVICE_NAME[11]
 o Added CURLOPT_SERVICE_NAME[12]
 o New curl option: --proxy-service-name[13]
 o Mew curl option: --service-name [14]
 o New curl option: --data-raw [5]
 o Added CURLOPT_PIPEWAIT [15]
 o Added support for multiplexing transfers using HTTP/2, enable this
   with the new CURLPIPE_MULTIPLEX bit for CURLMOPT_PIPELINING [16]
 o HTTP/2: requires nghttp2 1.0.0 or later
 o scripts: add zsh.pl for generating zsh completion
 o curl.h: add CURL_HTTP_VERSION_2

This release includes the following bugfixes:

 o CVE-2015-3236: lingering HTTP credentials in connection re-use [30]
 o CVE-2015-3237: SMB send off unrelated memory contents [31]
 o nss: fix compilation failure with old versions of NSS [1]
 o curl_easy_getinfo.3: document 'internals' in CURLINFO_TLS_SESSION
 o schannel.c: Fix possible SEC_E_BUFFER_TOO_SMALL error
 o Curl_ossl_init: load builtin modules [2]
 o configure: follow-up fix for krb5-config [3]
 o sasl_sspi: Populate domain from the realm in the challenge [4]
 o netrc: support 'default' token
 o README: convert to UTF-8
 o cyassl: Implement public key pinning
 o nss: implement public key pinning for NSS backend
 o mingw build: add arch -m32/-m64 to LDFLAGS
 o schannel: Fix out of bounds array [6]
 o configure: remove autogenerated files by autoconf
 o configure: remove --automake from libtoolize call
 o acinclude.m4: fix shell test for default CA cert bundle/path
 o schannel: fix regression in schannel_recv [7]
 o openssl: skip trace outputs for ssl_ver == 0 [8]
 o gnutls: properly retrieve certificate status
 o netrc: Read in text mode when cygwin [9]
 o winbuild: Document the option used to statically link the CRT [10]
 o FTP: Make EPSV use the control IP address rather than the original host
 o FTP: fix dangling conn->ip_addr dereference on verbose EPSV
 o conncache: keep bundles on host+port bases, not only host names
 o runtests.pl: use 'h2c' now, no -14 anymore
 o curlver: introducing new version number (checking) macros
 o openssl: boringssl build brekage, use SSL_CTX_set_msg_callback [17]
 o CURLOPT_POSTFIELDS.3: correct variable names [18]
 o curl_easy_unescape.3: update RFC reference [19]
 o gnutls: don't fail on non-fatal alerts during handshake
 o testcurl.pl: allow source to be in an arbitrary directory
 o CURLOPT_HTTPPROXYTUNNEL.3: only works with a HTTP proxy
 o SSPI-error: Change SEC_E_ILLEGAL_MESSAGE description [20]
 o parse_proxy: switch off tunneling if non-HTTP proxy [21]
 o share_init: fix OOM crash
 o perl: remove subdir, not touched in 9 years
 o CURLOPT_COOKIELIST.3: Add example
 o CURLOPT_COOKIE.3: Explain that the cookies won't be modified [22]
 o CURLOPT_COOKIELIST.3: Explain Set-Cookie without a domain [23]
 o FAQ: How do I port libcurl to my OS?
 o openssl: Use TLS_client_method for OpenSSL 1.1.0+
 o HTTP-NTLM: fail auth on connection close instead of looping [24]
 o curl_setup: Add macros for FOPEN_READTEXT, FOPEN_WRITETEXT [25]
 o curl_getdate.3: update RFC reference
 o curl_multi_info_read.3: added example
 o curl_multi_perform.3: added example
 o curl_multi_timeout.3: added example
 o cookie: Stop exporting any-domain cookies [26]
 o openssl: remove dummy callback use from SSL_CTX_set_verify()
 o openssl: remove SSL_get_session()-using code
 o openssl: removed USERDATA_IN_PWD_CALLBACK kludge
 o openssl: removed error string #ifdef
 o openssl: Fix verification of server-sent legacy intermediates [27]
 o docs: man page indentation and syntax fixes
 o docs: Spelling fixes
 o fopen.c: fix a few compiler warnings
 o CURLOPT_OPENSOCKETFUNCTION: return error at once [28]
 o schannel: Add support for optional client certificates
 o build: Properly detect OpenSSL 1.0.2 when using configure
 o urldata: store POST size in state.infilesize too [29]
 o security:choose_mech remove dead code
 o rtsp_do: remove dead code
 o docs: many HTTP URIs changed to HTTPS
 o schannel: schannel_recv overhaul [32]

This release includes the following known bugs:

 o see docs/KNOWN_BUGS (http://curl.haxx.se/docs/knownbugs.html)

This release would not have looked like this without help, code, reports and
advice from friends like these:

  Alessandro Ghedini, Alexander Dyagilev, Anders Bakken, Anthony Avina,
  Ashish Shukla, Bert Huijben, Brian Chrisman, Brian Prodoehl, Chris Araman,
  Dagobert Michelsen, Dan Fandrich, Daniel Melani, Daniel Stenberg,
  Dmitry Eremin-Solenikov, Drake Arconis, Egon Eckert, Frank Meier, Fred Stluka,
  Gisle Vanem, Grant Pannell, Isaac Boukris, Jens Rantil, Joel Depooter,
  Kamil Dudka, Linus Nielsen Feltzing, Linus Nielsen Feltzing Feltzing,
  Liviu Chircu, Marc Hoersken, Michael Osipov, Oren Souroujon, Orgad Shaneh,
  Patrick Monnerat, Patrick Rapin, Paul Howarth, Paul Oliver, Rafayel Mkrtchyan,
  Ray Satiro, Sean Boudreau, Tatsuhiro Tsujikawa, Tomas Tomecek, Viktor Szakáts,
  Ville Skyttä, Yehezkel Horowitz,
  (43 contributors)

        Thanks! (and sorry if I forgot to mention someone)

References to bug reports and discussions on issues:

 [1] = http://curl.haxx.se/mail/lib-2015-04/0095.html
 [2] = curl/curl#206
 [3] = curl/curl@5b66860#commitcomment-10473445
 [4] = curl/curl#141
 [5] = curl/curl#198
 [6] = http://curl.haxx.se/mail/lib-2015-04/0199.html
 [7] = curl/curl#244
 [8] = curl/curl#219
 [9] = curl/curl#258
 [10] = curl/curl#254
 [11] = http://curl.haxx.se/libcurl/c/CURLOPT_PROXY_SERVICE_NAME.html
 [12] = http://curl.haxx.se/libcurl/c/CURLOPT_SERVICE_NAME.html
 [13] = http://curl.haxx.se/docs/manpage.html#--proxy-service-name
 [14] = http://curl.haxx.se/docs/manpage.html#--service-name
 [15] = http://curl.haxx.se/libcurl/c/CURLOPT_PIPEWAIT.html
 [16] = http://curl.haxx.se/libcurl/c/CURLMOPT_PIPELINING.html
 [17] = curl/curl#275
 [18] = curl/curl#281
 [19] = curl/curl#282
 [20] = curl/curl#267
 [21] = http://curl.haxx.se/mail/lib-2015-05/0056.html
 [22] = http://curl.haxx.se/mail/lib-2015-05/0115.html
 [23] = http://curl.haxx.se/mail/lib-2015-05/0137.html
 [24] = curl/curl#256
 [25] = curl/curl#258 (comment)
 [26] = curl/curl#292
 [27] = https://rt.openssl.org/Ticket/Display.html?id=3621&user=guest&pass=guest
 [28] = http://curl.haxx.se/mail/lib-2015-06/0047.html
 [29] = http://curl.haxx.se/mail/lib-2015-06/0019.html
 [30] = http://curl.haxx.se/docs/adv_20150617A.html
 [31] = http://curl.haxx.se/docs/adv_20150617B.html
 [32] = curl/curl#244

jgsogo added a commit to jgsogo/curl that referenced this issue Oct 19, 2015

openssl: Use SSL_CTX_set_msg_callback and SSL_CTX_set_msg_callback_arg
BoringSSL removed support for direct callers of SSL_CTX_callback_ctrl
and SSL_CTX_ctrl, so move to a way that should work on BoringSSL and
OpenSSL.

re #275

@curl curl locked as resolved and limited conversation to collaborators May 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.