From 9e548bd59140bd67736375afbe5e7e98e8a67b43 Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Wed, 11 Dec 2019 12:11:15 +0100 Subject: [PATCH 1/6] Problem: insecure and inefficient strcpy used Solution: use memcpy with known length --- src/ip.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ip.cpp b/src/ip.cpp index 4cefb9a9..81d298a3 100644 --- a/src/ip.cpp +++ b/src/ip.cpp @@ -879,7 +879,7 @@ int zmq::create_ipc_wildcard_address (std::string &path_, std::string &file_) // We need room for tmp_path + trailing NUL std::vector buffer (tmp_path.length () + 1); - strcpy (&buffer[0], tmp_path.c_str ()); + memcpy (&buffer[0], tmp_path.c_str (), tmp_path.length () + 1); #if defined HAVE_MKDTEMP // Create the directory. POSIX requires that mkdtemp() creates the From 30e2398e67dec7aa04ddeea1d3d2c2d6e285fb84 Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Wed, 11 Dec 2019 12:13:34 +0100 Subject: [PATCH 2/6] Problem: WSS-specific members and options are compiled without ZMQ_HAVE_WSS Solution: properly guard members and options --- src/options.cpp | 3 ++- src/session_base.cpp | 9 ++++++++- src/session_base.hpp | 2 ++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/options.cpp b/src/options.cpp index 19a800e1..39d92d1d 100644 --- a/src/options.cpp +++ b/src/options.cpp @@ -786,6 +786,7 @@ int zmq::options_t::setsockopt (int option_, } break; +#ifdef ZMQ_HAVE_WSS case ZMQ_WSS_KEY_PEM: // TODO: check if valid certificate wss_key_pem = std::string ((char *) optval_, optvallen_); @@ -804,7 +805,7 @@ int zmq::options_t::setsockopt (int option_, case ZMQ_WSS_TRUST_SYSTEM: return do_setsockopt_int_as_bool_strict (optval_, optvallen_, &wss_trust_system); - +#endif #endif default: diff --git a/src/session_base.cpp b/src/session_base.cpp index 90d540f5..d6200b15 100644 --- a/src/session_base.cpp +++ b/src/session_base.cpp @@ -114,15 +114,20 @@ zmq::session_base_t::session_base_t (class io_thread_t *io_thread_, _socket (socket_), _io_thread (io_thread_), _has_linger_timer (false), - _addr (addr_), + _addr (addr_) +#ifdef ZMQ_HAVE_WSS + , _wss_hostname (NULL) +#endif { +#ifdef ZMQ_HAVE_WSS if (options_.wss_hostname.length () > 0) { _wss_hostname = static_cast (malloc (options_.wss_hostname.length () + 1)); assert (_wss_hostname); strcpy (_wss_hostname, options_.wss_hostname.c_str ()); } +#endif } const zmq::endpoint_uri_pair_t &zmq::session_base_t::get_endpoint () const @@ -145,8 +150,10 @@ zmq::session_base_t::~session_base_t () if (_engine) _engine->terminate (); +#ifdef ZMQ_HAVE_WSS if (_wss_hostname) free (_wss_hostname); +#endif LIBZMQ_DELETE (_addr); } diff --git a/src/session_base.hpp b/src/session_base.hpp index f4bb88f3..ed2772c1 100644 --- a/src/session_base.hpp +++ b/src/session_base.hpp @@ -192,9 +192,11 @@ class session_base_t : public own_t, public io_object_t, public i_pipe_events // Protocol and address to use when connecting. address_t *_addr; +#ifdef ZMQ_HAVE_WSS // TLS handshake, we need to take a copy when the session is created, // in order to maintain the value at the creation time char *_wss_hostname; +#endif ZMQ_NON_COPYABLE_NOR_MOVABLE (session_base_t) }; From 4c3f1154691de35f1dea2224aee8bdd9544bba28 Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Wed, 11 Dec 2019 12:56:05 +0100 Subject: [PATCH 3/6] Problem: raw malloc used unnecessarily Solution: use std::string instead --- src/session_base.cpp | 19 +++---------------- src/session_base.hpp | 2 +- src/ws_connecter.cpp | 2 +- src/ws_connecter.hpp | 4 ++-- src/ws_listener.cpp | 5 +++-- src/wss_engine.cpp | 12 +++++++----- src/wss_engine.hpp | 2 +- 7 files changed, 18 insertions(+), 28 deletions(-) diff --git a/src/session_base.cpp b/src/session_base.cpp index d6200b15..cbd923d8 100644 --- a/src/session_base.cpp +++ b/src/session_base.cpp @@ -117,17 +117,9 @@ zmq::session_base_t::session_base_t (class io_thread_t *io_thread_, _addr (addr_) #ifdef ZMQ_HAVE_WSS , - _wss_hostname (NULL) + _wss_hostname (options_.wss_hostname) #endif { -#ifdef ZMQ_HAVE_WSS - if (options_.wss_hostname.length () > 0) { - _wss_hostname = - static_cast (malloc (options_.wss_hostname.length () + 1)); - assert (_wss_hostname); - strcpy (_wss_hostname, options_.wss_hostname.c_str ()); - } -#endif } const zmq::endpoint_uri_pair_t &zmq::session_base_t::get_endpoint () const @@ -150,11 +142,6 @@ zmq::session_base_t::~session_base_t () if (_engine) _engine->terminate (); -#ifdef ZMQ_HAVE_WSS - if (_wss_hostname) - free (_wss_hostname); -#endif - LIBZMQ_DELETE (_addr); } @@ -708,8 +695,8 @@ zmq::own_t *zmq::session_base_t::create_connecter_tcp (io_thread_t *io_thread_, zmq::own_t *zmq::session_base_t::create_connecter_ws (io_thread_t *io_thread_, bool wait_) { - return new (std::nothrow) - ws_connecter_t (io_thread_, this, options, _addr, wait_, false, NULL); + return new (std::nothrow) ws_connecter_t (io_thread_, this, options, _addr, + wait_, false, std::string ()); } #endif diff --git a/src/session_base.hpp b/src/session_base.hpp index ed2772c1..42b84e6d 100644 --- a/src/session_base.hpp +++ b/src/session_base.hpp @@ -195,7 +195,7 @@ class session_base_t : public own_t, public io_object_t, public i_pipe_events #ifdef ZMQ_HAVE_WSS // TLS handshake, we need to take a copy when the session is created, // in order to maintain the value at the creation time - char *_wss_hostname; + const std::string _wss_hostname; #endif ZMQ_NON_COPYABLE_NOR_MOVABLE (session_base_t) diff --git a/src/ws_connecter.cpp b/src/ws_connecter.cpp index ca189d63..29b551cc 100644 --- a/src/ws_connecter.cpp +++ b/src/ws_connecter.cpp @@ -74,7 +74,7 @@ zmq::ws_connecter_t::ws_connecter_t (class io_thread_t *io_thread_, address_t *addr_, bool delayed_start_, bool wss_, - const char *tls_hostname_) : + const std::string &tls_hostname_) : stream_connecter_base_t ( io_thread_, session_, options_, addr_, delayed_start_), _connect_timer_started (false), diff --git a/src/ws_connecter.hpp b/src/ws_connecter.hpp index 50d27d0a..20b8b1b9 100644 --- a/src/ws_connecter.hpp +++ b/src/ws_connecter.hpp @@ -47,7 +47,7 @@ class ws_connecter_t : public stream_connecter_base_t address_t *addr_, bool delayed_start_, bool wss_, - const char *tls_hostname_); + const std::string &tls_hostname_); ~ws_connecter_t (); protected: @@ -89,7 +89,7 @@ class ws_connecter_t : public stream_connecter_base_t bool _connect_timer_started; bool _wss; - const char *_hostname; + const std::string &_hostname; ZMQ_NON_COPYABLE_NOR_MOVABLE (ws_connecter_t) }; diff --git a/src/ws_listener.cpp b/src/ws_listener.cpp index d86e49e1..aa53752d 100644 --- a/src/ws_listener.cpp +++ b/src/ws_listener.cpp @@ -294,8 +294,9 @@ void zmq::ws_listener_t::create_engine (fd_t fd_) i_engine *engine = NULL; if (_wss) #ifdef ZMQ_HAVE_WSS - engine = new (std::nothrow) wss_engine_t ( - fd_, options, endpoint_pair, _address, false, _tls_cred, NULL); + engine = new (std::nothrow) + wss_engine_t (fd_, options, endpoint_pair, _address, false, _tls_cred, + std::string ()); #else assert (false); #endif diff --git a/src/wss_engine.cpp b/src/wss_engine.cpp index c3f2b05a..f5a4645d 100644 --- a/src/wss_engine.cpp +++ b/src/wss_engine.cpp @@ -58,7 +58,7 @@ zmq::wss_engine_t::wss_engine_t (fd_t fd_, ws_address_t &address_, bool client_, void *tls_server_cred_, - const char *hostname_) : + const std::string &hostname_) : ws_engine_t (fd_, options_, endpoint_uri_pair_, address_, client_), _established (false), _tls_client_cred (NULL) @@ -88,11 +88,13 @@ zmq::wss_engine_t::wss_engine_t (fd_t fd_, rc = gnutls_init (&_tls_session, GNUTLS_CLIENT | GNUTLS_NONBLOCK); assert (rc == GNUTLS_E_SUCCESS); - if (hostname_) - gnutls_server_name_set (_tls_session, GNUTLS_NAME_DNS, hostname_, - strlen (hostname_)); + if (!hostname_.empty ()) + gnutls_server_name_set (_tls_session, GNUTLS_NAME_DNS, + hostname_.c_str (), hostname_.size ()); - gnutls_session_set_ptr (_tls_session, (void *) hostname_); + gnutls_session_set_ptr ( + _tls_session, + hostname_.empty () ? NULL : const_cast (hostname_.c_str ())); rc = gnutls_credentials_set (_tls_session, GNUTLS_CRD_CERTIFICATE, _tls_client_cred); diff --git a/src/wss_engine.hpp b/src/wss_engine.hpp index 1f744e83..adfaf454 100644 --- a/src/wss_engine.hpp +++ b/src/wss_engine.hpp @@ -46,7 +46,7 @@ class wss_engine_t : public ws_engine_t ws_address_t &address_, bool client_, void *tls_server_cred_, - const char *hostname_); + const std::string &hostname_); ~wss_engine_t (); void out_event (); From 2256bd5b0b687ba06feb38d5b134e2592df6f9af Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Wed, 11 Dec 2019 13:01:26 +0100 Subject: [PATCH 4/6] Problem: unnecessary copying of string literals Solution: just copy the address --- src/ws_engine.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ws_engine.cpp b/src/ws_engine.cpp index 61ef778c..8d12170a 100644 --- a/src/ws_engine.cpp +++ b/src/ws_engine.cpp @@ -118,14 +118,14 @@ zmq::ws_engine_t::~ws_engine_t () void zmq::ws_engine_t::start_ws_handshake () { if (_client) { - char protocol[21]; + const char *protocol; if (_options.mechanism == ZMQ_NULL) - strcpy (protocol, "ZWS2.0/NULL,ZWS2.0"); + protocol = "ZWS2.0/NULL,ZWS2.0"; else if (_options.mechanism == ZMQ_PLAIN) - strcpy (protocol, "ZWS2.0/PLAIN"); + protocol = "ZWS2.0/PLAIN"; #ifdef ZMQ_HAVE_CURVE else if (_options.mechanism == ZMQ_CURVE) - strcpy (protocol, "ZWS2.0/CURVE"); + protocol = "ZWS2.0/CURVE"; #endif else assert (false); From 334e837b88bb62f5db286ecede0e0382813821e1 Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Wed, 11 Dec 2019 13:33:00 +0100 Subject: [PATCH 5/6] Problem: ws_engine uses unsafe strcpy Solution: use strcpy_s instead (define custom if not available) --- CMakeLists.txt | 1 + builds/cmake/platform.hpp.in | 1 + configure.ac | 14 ++++++++++++++ src/ws_engine.cpp | 27 +++++++++++++++++++++++---- 4 files changed, 39 insertions(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index eebdc6bb..ba0cab25 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -460,6 +460,7 @@ if(NOT MSVC) check_cxx_symbol_exists(mkdtemp stdlib.h HAVE_MKDTEMP) check_cxx_symbol_exists(accept4 sys/socket.h HAVE_ACCEPT4) check_cxx_symbol_exists(strnlen string.h HAVE_STRNLEN) + check_cxx_symbol_exists(strlcpy string.h ZMQ_HAVE_STRLCPY) else() set(HAVE_STRNLEN 1) endif() diff --git a/builds/cmake/platform.hpp.in b/builds/cmake/platform.hpp.in index 9f56575f..a95d9d94 100644 --- a/builds/cmake/platform.hpp.in +++ b/builds/cmake/platform.hpp.in @@ -51,6 +51,7 @@ #cmakedefine ZMQ_HAVE_PTHREAD_SET_AFFINITY #cmakedefine HAVE_ACCEPT4 #cmakedefine HAVE_STRNLEN +#cmakedefine ZMQ_HAVE_STRLCPY #cmakedefine ZMQ_HAVE_IPC diff --git a/configure.ac b/configure.ac index f6619cfe..43a74250 100644 --- a/configure.ac +++ b/configure.ac @@ -751,6 +751,20 @@ AC_COMPILE_IFELSE( AC_MSG_RESULT([no]) ]) +# string.h doesn't seem to be included by default in Fedora 30 +AC_MSG_CHECKING([whether strlcpy is available]) +AC_COMPILE_IFELSE( + [AC_LANG_PROGRAM( + [[#include ]], + [[char buf [100]; size_t bar = strlcpy (buf, "foo", 100); (void)bar; return 0;]]) + ],[ + AC_MSG_RESULT([yes]) + AC_DEFINE(ZMQ_HAVE_STRLCPY, [1], + [strlcpy is available]) + ],[ + AC_MSG_RESULT([no]) +]) + # pthread_setname is non-posix, and there are at least 4 different implementations AC_MSG_CHECKING([whether signature of pthread_setname_np() has 1 argument]) AC_COMPILE_IFELSE( diff --git a/src/ws_engine.cpp b/src/ws_engine.cpp index 8d12170a..32a67bac 100644 --- a/src/ws_engine.cpp +++ b/src/ws_engine.cpp @@ -52,6 +52,8 @@ along with this program. If not, see . #endif #endif +#include + #include "tcp.hpp" #include "ws_engine.hpp" #include "session_base.hpp" @@ -71,6 +73,23 @@ along with this program. If not, see . #ifdef ZMQ_HAVE_WINDOWS #define strcasecmp _stricmp +#else +#ifndef ZMQ_HAVE_STRLCPY +static size_t strlcpy (char *dest_, const char *src_, const size_t dest_size_) +{ + size_t remain = dest_size_; + for (; remain && *src_; --remain, ++src_, ++dest_) { + *dest_ = *src_; + } + return dest_size_ - remain; +} +#endif +template +static int strcpy_s (char (&dest_)[size], const char *const src_) +{ + const size_t res = strlcpy (dest_, src_, size); + return res >= size ? ERANGE : 0; +} #endif // OSX uses a different name for this socket option @@ -440,7 +459,7 @@ bool zmq::ws_engine_t::server_handshake () strcasecmp ("upgrade", _header_value) == 0; else if (strcasecmp ("Sec-WebSocket-Key", _header_name) == 0) - strcpy (_websocket_key, _header_value); + strcpy_s (_websocket_key, _header_value); else if (strcasecmp ("Sec-WebSocket-Protocol", _header_name) == 0) { // Currently only the ZWS2.0 is supported @@ -453,7 +472,7 @@ bool zmq::ws_engine_t::server_handshake () p++; if (select_protocol (p)) { - strcpy (_websocket_protocol, p); + strcpy_s (_websocket_protocol, p); break; } @@ -820,11 +839,11 @@ bool zmq::ws_engine_t::client_handshake () strcasecmp ("upgrade", _header_value) == 0; else if (strcasecmp ("Sec-WebSocket-Accept", _header_name) == 0) - strcpy (_websocket_accept, _header_value); + strcpy_s (_websocket_accept, _header_value); else if (strcasecmp ("Sec-WebSocket-Protocol", _header_name) == 0) { if (select_protocol (_header_value)) - strcpy (_websocket_protocol, _header_value); + strcpy_s (_websocket_protocol, _header_value); } _client_handshake_state = client_header_field_cr; } else if (_header_value_position + 1 > MAX_HEADER_VALUE_LENGTH) From 3dbbc28bb87d08ce2b83f26e38d0978dfc868451 Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Mon, 23 Dec 2019 11:06:22 +0100 Subject: [PATCH 6/6] Problem: use of unsafe strcpy Solution: use memcpy with known size instead --- src/err.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/err.cpp b/src/err.cpp index 21117bd1..a73fc113 100644 --- a/src/err.cpp +++ b/src/err.cpp @@ -411,6 +411,7 @@ void zmq::print_backtrace (void) while (unw_step (&cursor) > 0) { unw_word_t offset; unw_proc_info_t p_info; + static const char unknown[] = "?"; const char *file_name; char *demangled_name; char func_name[256] = ""; @@ -422,14 +423,14 @@ void zmq::print_backtrace (void) rc = unw_get_proc_name (&cursor, func_name, 256, &offset); if (rc == -UNW_ENOINFO) - strcpy (func_name, "?"); + memcpy (func_name, unknown, sizeof unknown); addr = (void *) (p_info.start_ip + offset); if (dladdr (addr, &dl_info) && dl_info.dli_fname) file_name = dl_info.dli_fname; else - file_name = "?"; + file_name = unknown; demangled_name = abi::__cxa_demangle (func_name, NULL, NULL, &rc);