From d5e4319edcc984e4b03f16249378ae9eca67fc01 Mon Sep 17 00:00:00 2001 From: Simon Giesecke Date: Tue, 15 Aug 2017 16:28:24 +0200 Subject: [PATCH] [WIP, do not merge] Problem: insufficient tests for ZMTP-CURVE protocol errors (#2680) * Extracted connect_vanilla_socket function * Problem: no tests for ZMTP-CURVE protocol errors Solution: added two test cases with erroneous HELLO commands * Problem: insufficient tests for ZMTP-CURVE protocol errors Solution: added two test cases with erroneous HELLO command version * Problem: test HELLO message is invalid apart from deliberate errors Solution: create cryptographically correct HELLO message add tweetnacl.c to test_security_curve * Problem: nonce is incorrect, build fails with GCC Solution: use correct non prefix * Problem: make builds are failing Solution: transfer CMake changes to (auto)make files * Problem: nonce is incorrect, build fails with GCC Solution: use correct non prefix * Problem: make builds are failing Solution: transfer CMake changes to (auto)make files * Problem: no test with INITIATE command with invalid length Solution: added test case * Problem: code duplication between test_security_curve.cpp and curve_client.cpp Solution: extracted parts of zmq::curve_client_t::produce_hello into reusable function * Problem: code duplication between test_security_curve.cpp and curve_client.cpp Solution: extracted further parts of zmq::curve_client_t into reusable functions added missing file * Problem: mechanism_t::add_property can be declared static Solution: declare mechanism_t::add_property static * Problem: intermediate crypto data needs to be passed between static function calls to curve_client_tools_t Solution: add non-static member functions * Problem: msg_t instance may be closed twice Solution: remove offending close * Problem: prepare_hello uses static curve_client_tools_t::produce_hello Solution: Use non-static curve_client_tools_t::produce_hello * Problem: no test with invalid command name where INITIATE command is expected Solution: added test case * Problem: make builds are failing due to curve_client_tools.hpp not being found Solution: add curve_client_tools.hpp to list of source files * Problem: wrong initializer order in zmq::curve_client_t Solution: reorder * Problem: under non-Windows systems, test fails because random_open was not called Solution: call random_open/random_close within test * Problem: conflict between custom function htonll and macro definition on Darwin Solution: define htonll function only if not defined as a macro * Problem: nullptr not defined on all platforms Solution: replace nullptr by NULL * Problem: libsodium builds not working Solution: adapt compile and link file sets for libsodium builds * Problem: Makefile.am broken Solution: Fix syntax * Problem: no tests for garbage encrypted cookie or content in INITIATE Solution: added test cases * Problem: test cases accidentally excluded from build Solution: remove #if/#endif * Solution: some error cases are unreachable Problem: for the time being, added some comments without changing the code * Added comments on hard-to-test cases --- Makefile.am | 36 ++- configure.ac | 1 + src/curve_client.cpp | 177 ++++----------- src/curve_client.hpp | 40 +--- src/curve_client_tools.hpp | 307 ++++++++++++++++++++++++++ src/curve_server.cpp | 27 +++ src/mechanism.cpp | 2 +- src/mechanism.hpp | 6 +- tests/CMakeLists.txt | 20 +- tests/test_security_curve.cpp | 397 +++++++++++++++++++++++++++++++++- 10 files changed, 829 insertions(+), 184 deletions(-) create mode 100644 src/curve_client_tools.hpp diff --git a/Makefile.am b/Makefile.am index f27bb24e..d0b9483e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -37,6 +37,7 @@ src_libzmq_la_SOURCES = \ src/ctx.hpp \ src/curve_client.cpp \ src/curve_client.hpp \ + src/curve_client_tools.hpp \ src/curve_server.cpp \ src/curve_server.hpp \ src/dbuffer.hpp \ @@ -373,7 +374,6 @@ test_apps = \ tests/test_ctx_destroy \ tests/test_security_null \ tests/test_security_plain \ - tests/test_security_curve \ tests/test_iov \ tests/test_spec_req \ tests/test_spec_rep \ @@ -520,9 +520,6 @@ tests_test_security_null_LDADD = src/libzmq.la tests_test_security_plain_SOURCES = tests/test_security_plain.cpp tests_test_security_plain_LDADD = src/libzmq.la -tests_test_security_curve_SOURCES = tests/test_security_curve.cpp -tests_test_security_curve_LDADD = src/libzmq.la - tests_test_spec_req_SOURCES = tests/test_spec_req.cpp tests_test_spec_req_LDADD = src/libzmq.la @@ -619,6 +616,37 @@ tests_test_base85_LDADD = src/libzmq.la tests_test_sodium_SOURCES = tests/test_sodium.cpp tests_test_sodium_LDADD = src/libzmq.la +if HAVE_CURVE + +test_apps += \ + tests/test_security_curve + +tests_test_security_curve_SOURCES = \ + tests/test_security_curve.cpp \ + tests/testutil.hpp \ + src/curve_client_tools.hpp \ + src/clock.hpp \ + src/clock.cpp \ + src/random.hpp \ + src/random.cpp \ + src/err.hpp \ + src/err.cpp + +if USE_TWEETNACL +tests_test_security_curve_SOURCES += \ + src/tweetnacl.c +endif + +tests_test_security_curve_LDADD = \ + src/libzmq.la + +if USE_LIBSODIUM +tests_test_security_curve_LDADD += \ + ${sodium_LIBS} +endif + +endif + if !ON_MINGW if !ON_CYGWIN test_apps += \ diff --git a/configure.ac b/configure.ac index 47a888f4..7551f9a3 100644 --- a/configure.ac +++ b/configure.ac @@ -498,6 +498,7 @@ AM_CONDITIONAL(ENABLE_CURVE_KEYGEN, test "x$enable_curve" = "xyes" -a "x$zmq_ena AM_CONDITIONAL(USE_LIBSODIUM, test "$curve_library" = "libsodium") AM_CONDITIONAL(USE_TWEETNACL, test "$curve_library" = "tweetnacl") +AM_CONDITIONAL(HAVE_CURVE, test "x$curve_library" != "x") # build using pgm have_pgm_library="no" diff --git a/src/curve_client.cpp b/src/curve_client.cpp index b251ac7d..f79f16ec 100644 --- a/src/curve_client.cpp +++ b/src/curve_client.cpp @@ -37,21 +37,17 @@ #include "err.hpp" #include "curve_client.hpp" #include "wire.hpp" +#include "curve_client_tools.hpp" zmq::curve_client_t::curve_client_t (const options_t &options_) : mechanism_t (options_), state (send_hello), - cn_nonce(1), - cn_peer_nonce(1) + tools (options_.curve_public_key, + options_.curve_secret_key, + options_.curve_server_key), + cn_nonce (1), + cn_peer_nonce (1) { - int rc; - memcpy (public_key, options_.curve_public_key, crypto_box_PUBLICKEYBYTES); - memcpy (secret_key, options_.curve_secret_key, crypto_box_SECRETKEYBYTES); - memcpy (server_key, options_.curve_server_key, crypto_box_PUBLICKEYBYTES); - - // Generate short-term key pair - rc = crypto_box_keypair (cn_public, cn_secret); - zmq_assert (rc == 0); } zmq::curve_client_t::~curve_client_t () @@ -87,13 +83,13 @@ int zmq::curve_client_t::process_handshake_command (msg_t *msg_) const size_t msg_size = msg_->size (); int rc = 0; - if (msg_size >= 8 && !memcmp (msg_data, "\7WELCOME", 8)) + if (curve_client_tools_t::is_handshake_command_welcome (msg_data, msg_size)) rc = process_welcome (msg_data, msg_size); - else - if (msg_size >= 6 && !memcmp (msg_data, "\5READY", 6)) + else if (curve_client_tools_t::is_handshake_command_ready (msg_data, + msg_size)) rc = process_ready (msg_data, msg_size); - else - if (msg_size >= 6 && !memcmp (msg_data, "\5ERROR", 6)) + else if (curve_client_tools_t::is_handshake_command_error (msg_data, + msg_size)) rc = process_error (msg_data, msg_size); else { errno = EPROTO; @@ -137,8 +133,8 @@ int zmq::curve_client_t::encode (msg_t *msg_) uint8_t *message_box = static_cast (malloc (mlen)); alloc_assert (message_box); - int rc = crypto_box_afternm (message_box, message_plaintext, - mlen, message_nonce, cn_precom); + int rc = crypto_box_afternm (message_box, message_plaintext, mlen, + message_nonce, tools.cn_precom); zmq_assert (rc == 0); rc = msg_->close (); @@ -199,8 +195,8 @@ int zmq::curve_client_t::decode (msg_t *msg_) memcpy (message_box + crypto_box_BOXZEROBYTES, message + 16, msg_->size () - 16); - int rc = crypto_box_open_afternm (message_plaintext, message_box, - clen, message_nonce, cn_precom); + int rc = crypto_box_open_afternm (message_plaintext, message_box, clen, + message_nonce, tools.cn_precom); if (rc == 0) { rc = msg_->close (); zmq_assert (rc == 0); @@ -240,78 +236,34 @@ zmq::mechanism_t::status_t zmq::curve_client_t::status () const int zmq::curve_client_t::produce_hello (msg_t *msg_) { - uint8_t hello_nonce [crypto_box_NONCEBYTES]; - uint8_t hello_plaintext [crypto_box_ZEROBYTES + 64]; - uint8_t hello_box [crypto_box_BOXZEROBYTES + 80]; - - // Prepare the full nonce - memcpy (hello_nonce, "CurveZMQHELLO---", 16); - put_uint64 (hello_nonce + 16, cn_nonce); - - // Create Box [64 * %x0](C'->S) - memset (hello_plaintext, 0, sizeof hello_plaintext); - - int rc = crypto_box (hello_box, hello_plaintext, - sizeof hello_plaintext, - hello_nonce, server_key, cn_secret); - if (rc == -1) - return -1; - - rc = msg_->init_size (200); + int rc = msg_->init_size (200); errno_assert (rc == 0); - uint8_t *hello = static_cast (msg_->data ()); - memcpy (hello, "\x05HELLO", 6); - // CurveZMQ major and minor version numbers - memcpy (hello + 6, "\1\0", 2); - // Anti-amplification padding - memset (hello + 8, 0, 72); - // Client public connection key - memcpy (hello + 80, cn_public, crypto_box_PUBLICKEYBYTES); - // Short nonce, prefixed by "CurveZMQHELLO---" - memcpy (hello + 112, hello_nonce + 16, 8); - // Signature, Box [64 * %x0](C'->S) - memcpy (hello + 120, hello_box + crypto_box_BOXZEROBYTES, 80); + rc = tools.produce_hello (msg_->data (), cn_nonce); + if (rc == -1) { + // TODO this is somewhat inconsistent: we call init_size, but we may + // not close msg_; i.e. we assume that msg_ is initialized but empty + // (if it were non-empty, calling init_size might cause a leak!) + + // msg_->close (); + return -1; + } cn_nonce++; return 0; } -int zmq::curve_client_t::process_welcome ( - const uint8_t *msg_data, size_t msg_size) +int zmq::curve_client_t::process_welcome (const uint8_t *msg_data, + size_t msg_size) { - if (msg_size != 168) { + int rc = tools.process_welcome (msg_data, msg_size); + + if (rc == -1) { errno = EPROTO; return -1; } - uint8_t welcome_nonce [crypto_box_NONCEBYTES]; - uint8_t welcome_plaintext [crypto_box_ZEROBYTES + 128]; - uint8_t welcome_box [crypto_box_BOXZEROBYTES + 144]; - - // Open Box [S' + cookie](C'->S) - memset (welcome_box, 0, crypto_box_BOXZEROBYTES); - memcpy (welcome_box + crypto_box_BOXZEROBYTES, msg_data + 24, 144); - - memcpy (welcome_nonce, "WELCOME-", 8); - memcpy (welcome_nonce + 8, msg_data + 8, 16); - - int rc = crypto_box_open (welcome_plaintext, welcome_box, - sizeof welcome_box, - welcome_nonce, server_key, cn_secret); - if (rc != 0) { - errno = EPROTO; - return -1; - } - - memcpy (cn_server, welcome_plaintext + crypto_box_ZEROBYTES, 32); - memcpy (cn_cookie, welcome_plaintext + crypto_box_ZEROBYTES + 32, 16 + 80); - - // Message independent precomputation - rc = crypto_box_beforenm (cn_precom, cn_server, cn_secret); - zmq_assert (rc == 0); - state = send_initiate; return 0; @@ -319,40 +271,12 @@ int zmq::curve_client_t::process_welcome ( int zmq::curve_client_t::produce_initiate (msg_t *msg_) { - uint8_t vouch_nonce [crypto_box_NONCEBYTES]; - uint8_t vouch_plaintext [crypto_box_ZEROBYTES + 64]; - uint8_t vouch_box [crypto_box_BOXZEROBYTES + 80]; - - // Create vouch = Box [C',S](C->S') - memset (vouch_plaintext, 0, crypto_box_ZEROBYTES); - memcpy (vouch_plaintext + crypto_box_ZEROBYTES, cn_public, 32); - memcpy (vouch_plaintext + crypto_box_ZEROBYTES + 32, server_key, 32); - - memcpy (vouch_nonce, "VOUCH---", 8); - randombytes (vouch_nonce + 8, 16); - - int rc = crypto_box (vouch_box, vouch_plaintext, - sizeof vouch_plaintext, - vouch_nonce, cn_server, secret_key); - if (rc == -1) - return -1; - // Assume here that metadata is limited to 256 bytes - uint8_t initiate_nonce [crypto_box_NONCEBYTES]; - uint8_t initiate_plaintext [crypto_box_ZEROBYTES + 128 + 256]; - uint8_t initiate_box [crypto_box_BOXZEROBYTES + 144 + 256]; - - // Create Box [C + vouch + metadata](C'->S') - memset (initiate_plaintext, 0, crypto_box_ZEROBYTES); - memcpy (initiate_plaintext + crypto_box_ZEROBYTES, - public_key, 32); - memcpy (initiate_plaintext + crypto_box_ZEROBYTES + 32, - vouch_nonce + 8, 16); - memcpy (initiate_plaintext + crypto_box_ZEROBYTES + 48, - vouch_box + crypto_box_BOXZEROBYTES, 80); + // FIXME see https://github.com/zeromq/libzmq/issues/2681 + uint8_t metadata_plaintext [256]; // Metadata starts after vouch - uint8_t *ptr = initiate_plaintext + crypto_box_ZEROBYTES + 128; + uint8_t *ptr = metadata_plaintext; // Add socket type property const char *socket_type = socket_type_string (options.type); @@ -360,35 +284,24 @@ int zmq::curve_client_t::produce_initiate (msg_t *msg_) strlen (socket_type)); // Add identity property - if (options.type == ZMQ_REQ - || options.type == ZMQ_DEALER - || options.type == ZMQ_ROUTER) + if (options.type == ZMQ_REQ || options.type == ZMQ_DEALER + || options.type == ZMQ_ROUTER) ptr += add_property (ptr, ZMQ_MSG_PROPERTY_IDENTITY, options.identity, options.identity_size); - const size_t mlen = ptr - initiate_plaintext; + const size_t metadata_length = ptr - metadata_plaintext; - memcpy (initiate_nonce, "CurveZMQINITIATE", 16); - put_uint64 (initiate_nonce + 16, cn_nonce); - - rc = crypto_box (initiate_box, initiate_plaintext, - mlen, initiate_nonce, cn_server, cn_secret); - if (rc == -1) - return -1; - - rc = msg_->init_size (113 + mlen - crypto_box_BOXZEROBYTES); + size_t msg_size = 113 + 128 + crypto_box_BOXZEROBYTES + metadata_length; + int rc = msg_->init_size (msg_size); errno_assert (rc == 0); - uint8_t *initiate = static_cast (msg_->data ()); + if (-1 + == tools.produce_initiate (msg_->data (), msg_size, cn_nonce, + metadata_plaintext, metadata_length)) { + // TODO see comment in produce_hello + return -1; + } - memcpy (initiate, "\x08INITIATE", 9); - // Cookie provided by the server in the WELCOME command - memcpy (initiate + 9, cn_cookie, 96); - // Short nonce, prefixed by "CurveZMQINITIATE" - memcpy (initiate + 105, initiate_nonce + 16, 8); - // Box [C + vouch + metadata](C'->S') - memcpy (initiate + 113, initiate_box + crypto_box_BOXZEROBYTES, - mlen - crypto_box_BOXZEROBYTES); cn_nonce++; return 0; @@ -417,7 +330,7 @@ int zmq::curve_client_t::process_ready ( cn_peer_nonce = get_uint64(msg_data + 6); int rc = crypto_box_open_afternm (ready_plaintext, ready_box, - clen, ready_nonce, cn_precom); + clen, ready_nonce, tools.cn_precom); if (rc != 0) { errno = EPROTO; diff --git a/src/curve_client.hpp b/src/curve_client.hpp index 57ea3356..066cd36e 100644 --- a/src/curve_client.hpp +++ b/src/curve_client.hpp @@ -32,22 +32,9 @@ #ifdef ZMQ_HAVE_CURVE -#if defined (ZMQ_USE_TWEETNACL) -# include "tweetnacl.h" -#elif defined (ZMQ_USE_LIBSODIUM) -# include "sodium.h" -#endif - -#if crypto_box_NONCEBYTES != 24 \ -|| crypto_box_PUBLICKEYBYTES != 32 \ -|| crypto_box_SECRETKEYBYTES != 32 \ -|| crypto_box_ZEROBYTES != 32 \ -|| crypto_box_BOXZEROBYTES != 16 -# error "CURVE library not built properly" -#endif - #include "mechanism.hpp" #include "options.hpp" +#include "curve_client_tools.hpp" namespace zmq { @@ -83,29 +70,8 @@ namespace zmq // Current FSM state state_t state; - // Our public key (C) - uint8_t public_key [crypto_box_PUBLICKEYBYTES]; - - // Our secret key (c) - uint8_t secret_key [crypto_box_SECRETKEYBYTES]; - - // Our short-term public key (C') - uint8_t cn_public [crypto_box_PUBLICKEYBYTES]; - - // Our short-term secret key (c') - uint8_t cn_secret [crypto_box_SECRETKEYBYTES]; - - // Server's public key (S) - uint8_t server_key [crypto_box_PUBLICKEYBYTES]; - - // Server's short-term public key (S') - uint8_t cn_server [crypto_box_PUBLICKEYBYTES]; - - // Cookie received from server - uint8_t cn_cookie [16 + 80]; - - // Intermediary buffer used to speed up boxing and unboxing. - uint8_t cn_precom [crypto_box_BEFORENMBYTES]; + // CURVE protocol tools + curve_client_tools_t tools; // Nonce uint64_t cn_nonce; diff --git a/src/curve_client_tools.hpp b/src/curve_client_tools.hpp new file mode 100644 index 00000000..eaf74cf7 --- /dev/null +++ b/src/curve_client_tools.hpp @@ -0,0 +1,307 @@ +/* + Copyright (c) 2007-2016 Contributors as noted in the AUTHORS file + + This file is part of libzmq, the ZeroMQ core engine in C++. + + libzmq is free software; you can redistribute it and/or modify it under + the terms of the GNU Lesser General Public License (LGPL) as published + by the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + As a special exception, the Contributors give you permission to link + this library with independent modules to produce an executable, + regardless of the license terms of these independent modules, and to + copy and distribute the resulting executable under terms of your choice, + provided that you also meet, for each linked independent module, the + terms and conditions of the license of that module. An independent + module is a module which is not derived from or based on this library. + If you modify this library, you must extend this exception to your + version of the library. + + libzmq is distributed in the hope that it will be useful, but WITHOUT + ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public + License for more details. + + You should have received a copy of the GNU Lesser General Public License + along with this program. If not, see . +*/ + +#ifndef __ZMQ_CURVE_CLIENT_TOOLS_HPP_INCLUDED__ +#define __ZMQ_CURVE_CLIENT_TOOLS_HPP_INCLUDED__ + +#ifdef ZMQ_HAVE_CURVE + +#if defined(ZMQ_USE_TWEETNACL) +#include "tweetnacl.h" +#elif defined(ZMQ_USE_LIBSODIUM) +#include "sodium.h" +#endif + +#if crypto_box_NONCEBYTES != 24 || crypto_box_PUBLICKEYBYTES != 32 \ + || crypto_box_SECRETKEYBYTES != 32 || crypto_box_ZEROBYTES != 32 \ + || crypto_box_BOXZEROBYTES != 16 +#error "CURVE library not built properly" +#endif + +#include "wire.hpp" +#include "err.hpp" + +namespace zmq +{ +struct curve_client_tools_t +{ + static int produce_hello (void *data, + const uint8_t *server_key, + const uint64_t cn_nonce, + const uint8_t *cn_public, + const uint8_t *cn_secret) + { + uint8_t hello_nonce[crypto_box_NONCEBYTES]; + uint8_t hello_plaintext[crypto_box_ZEROBYTES + 64]; + uint8_t hello_box[crypto_box_BOXZEROBYTES + 80]; + + // Prepare the full nonce + memcpy (hello_nonce, "CurveZMQHELLO---", 16); + put_uint64 (hello_nonce + 16, cn_nonce); + + // Create Box [64 * %x0](C'->S) + memset (hello_plaintext, 0, sizeof hello_plaintext); + + int rc = crypto_box (hello_box, hello_plaintext, sizeof hello_plaintext, + hello_nonce, server_key, cn_secret); + if (rc == -1) + return -1; + + uint8_t *hello = static_cast (data); + + memcpy (hello, "\x05HELLO", 6); + // CurveZMQ major and minor version numbers + memcpy (hello + 6, "\1\0", 2); + // Anti-amplification padding + memset (hello + 8, 0, 72); + // Client public connection key + memcpy (hello + 80, cn_public, crypto_box_PUBLICKEYBYTES); + // Short nonce, prefixed by "CurveZMQHELLO---" + memcpy (hello + 112, hello_nonce + 16, 8); + // Signature, Box [64 * %x0](C'->S) + memcpy (hello + 120, hello_box + crypto_box_BOXZEROBYTES, 80); + + return 0; + } + + static int process_welcome (const uint8_t *msg_data, + size_t msg_size, + const uint8_t *server_key, + const uint8_t *cn_secret, + uint8_t *cn_server, + uint8_t *cn_cookie, + uint8_t *cn_precom) + { + if (msg_size != 168) { + errno = EPROTO; + return -1; + } + + uint8_t welcome_nonce[crypto_box_NONCEBYTES]; + uint8_t welcome_plaintext[crypto_box_ZEROBYTES + 128]; + uint8_t welcome_box[crypto_box_BOXZEROBYTES + 144]; + + // Open Box [S' + cookie](C'->S) + memset (welcome_box, 0, crypto_box_BOXZEROBYTES); + memcpy (welcome_box + crypto_box_BOXZEROBYTES, msg_data + 24, 144); + + memcpy (welcome_nonce, "WELCOME-", 8); + memcpy (welcome_nonce + 8, msg_data + 8, 16); + + int rc = + crypto_box_open (welcome_plaintext, welcome_box, sizeof welcome_box, + welcome_nonce, server_key, cn_secret); + if (rc != 0) { + errno = EPROTO; + return -1; + } + + memcpy (cn_server, welcome_plaintext + crypto_box_ZEROBYTES, 32); + memcpy (cn_cookie, welcome_plaintext + crypto_box_ZEROBYTES + 32, + 16 + 80); + + // Message independent precomputation + rc = crypto_box_beforenm (cn_precom, cn_server, cn_secret); + zmq_assert (rc == 0); + + return 0; + } + + static int produce_initiate (void *data, + size_t size, + const uint64_t cn_nonce, + const uint8_t *server_key, + const uint8_t *public_key, + const uint8_t *secret_key, + const uint8_t *cn_public, + const uint8_t *cn_secret, + const uint8_t *cn_server, + const uint8_t *cn_cookie, + const uint8_t *metadata_plaintext, + const size_t metadata_length) + { + const size_t max_metadata_length = 256; + zmq_assert (metadata_length < max_metadata_length); + + uint8_t vouch_nonce[crypto_box_NONCEBYTES]; + uint8_t vouch_plaintext[crypto_box_ZEROBYTES + 64]; + uint8_t vouch_box[crypto_box_BOXZEROBYTES + 80]; + + // Create vouch = Box [C',S](C->S') + memset (vouch_plaintext, 0, crypto_box_ZEROBYTES); + memcpy (vouch_plaintext + crypto_box_ZEROBYTES, cn_public, 32); + memcpy (vouch_plaintext + crypto_box_ZEROBYTES + 32, server_key, + 32); + + memcpy (vouch_nonce, "VOUCH---", 8); + randombytes (vouch_nonce + 8, 16); + + int rc = crypto_box (vouch_box, vouch_plaintext, sizeof vouch_plaintext, + vouch_nonce, cn_server, secret_key); + if (rc == -1) + return -1; + + uint8_t initiate_nonce[crypto_box_NONCEBYTES]; + uint8_t + initiate_box[crypto_box_BOXZEROBYTES + 144 + max_metadata_length]; + uint8_t + initiate_plaintext[crypto_box_ZEROBYTES + 128 + max_metadata_length]; + + // Create Box [C + vouch + metadata](C'->S') + memset (initiate_plaintext, 0, crypto_box_ZEROBYTES); + memcpy (initiate_plaintext + crypto_box_ZEROBYTES, public_key, + 32); + memcpy (initiate_plaintext + crypto_box_ZEROBYTES + 32, vouch_nonce + 8, + 16); + memcpy (initiate_plaintext + crypto_box_ZEROBYTES + 48, + vouch_box + crypto_box_BOXZEROBYTES, 80); + memcpy (initiate_plaintext + crypto_box_ZEROBYTES + 48 + 80, + metadata_plaintext, metadata_length); + + memcpy (initiate_nonce, "CurveZMQINITIATE", 16); + put_uint64 (initiate_nonce + 16, cn_nonce); + + rc = crypto_box (initiate_box, initiate_plaintext, + crypto_box_ZEROBYTES + 128 + metadata_length, + initiate_nonce, cn_server, cn_secret); + if (rc == -1) + return -1; + + uint8_t *initiate = static_cast (data); + + zmq_assert (size + == 113 + 128 + crypto_box_BOXZEROBYTES + metadata_length); + + memcpy (initiate, "\x08INITIATE", 9); + // Cookie provided by the server in the WELCOME command + memcpy (initiate + 9, cn_cookie, 96); + // Short nonce, prefixed by "CurveZMQINITIATE" + memcpy (initiate + 105, initiate_nonce + 16, 8); + // Box [C + vouch + metadata](C'->S') + memcpy (initiate + 113, initiate_box + crypto_box_BOXZEROBYTES, + 128 + metadata_length + crypto_box_BOXZEROBYTES); + + return 0; + } + + static bool is_handshake_command_welcome (const uint8_t *msg_data, + const size_t msg_size) + { + return is_handshake_command (msg_data, msg_size, "\7WELCOME"); + } + + static bool is_handshake_command_ready (const uint8_t *msg_data, + const size_t msg_size) + { + return is_handshake_command (msg_data, msg_size, "\5READY"); + } + + static bool is_handshake_command_error (const uint8_t *msg_data, + const size_t msg_size) + { + return is_handshake_command (msg_data, msg_size, "\5ERROR"); + } + + // non-static functions + curve_client_tools_t ( + const uint8_t (&curve_public_key)[crypto_box_PUBLICKEYBYTES], + const uint8_t (&curve_secret_key)[crypto_box_SECRETKEYBYTES], + const uint8_t (&curve_server_key)[crypto_box_PUBLICKEYBYTES]) + { + int rc; + memcpy (public_key, curve_public_key, crypto_box_PUBLICKEYBYTES); + memcpy (secret_key, curve_secret_key, crypto_box_SECRETKEYBYTES); + memcpy (server_key, curve_server_key, crypto_box_PUBLICKEYBYTES); + + // Generate short-term key pair + rc = crypto_box_keypair (cn_public, cn_secret); + zmq_assert (rc == 0); + } + + int produce_hello (void *data, const uint64_t cn_nonce) const + { + return produce_hello (data, server_key, cn_nonce, cn_public, cn_secret); + } + + int process_welcome (const uint8_t *msg_data, size_t msg_size) + { + return process_welcome (msg_data, msg_size, server_key, cn_secret, + cn_server, cn_cookie, cn_precom); + } + + int produce_initiate (void *data, + size_t size, + const uint64_t cn_nonce, + const uint8_t *metadata_plaintext, + const size_t metadata_length) + { + return produce_initiate ( + data, size, cn_nonce, server_key, public_key, secret_key, cn_public, + cn_secret, cn_server, cn_cookie, metadata_plaintext, metadata_length); + } + + // Our public key (C) + uint8_t public_key[crypto_box_PUBLICKEYBYTES]; + + // Our secret key (c) + uint8_t secret_key[crypto_box_SECRETKEYBYTES]; + + // Our short-term public key (C') + uint8_t cn_public[crypto_box_PUBLICKEYBYTES]; + + // Our short-term secret key (c') + uint8_t cn_secret[crypto_box_SECRETKEYBYTES]; + + // Server's public key (S) + uint8_t server_key[crypto_box_PUBLICKEYBYTES]; + + // Server's short-term public key (S') + uint8_t cn_server[crypto_box_PUBLICKEYBYTES]; + + // Cookie received from server + uint8_t cn_cookie[16 + 80]; + + // Intermediary buffer used to speed up boxing and unboxing. + uint8_t cn_precom[crypto_box_BEFORENMBYTES]; + + + private: + template + static bool is_handshake_command (const uint8_t *msg_data, + const size_t msg_size, + const char (&prefix)[N]) + { + return msg_size >= (N - 1) && !memcmp (msg_data, prefix, N - 1); + } +}; +} + +#endif + +#endif diff --git a/src/curve_server.cpp b/src/curve_server.cpp index 00af6e0f..7807795d 100644 --- a/src/curve_server.cpp +++ b/src/curve_server.cpp @@ -102,6 +102,12 @@ int zmq::curve_server_t::process_handshake_command (msg_t *msg_) rc = process_initiate (msg_); break; default: + // TODO I think this is not a case reachable with a misbehaving + // client. It is not an "invalid handshake command", but would be + // trying to process a handshake command in an invalid state, + // which is purely under control of this peer. + // Therefore, it should be changed to zmq_assert (false); + // CURVE I: invalid handshake command current_error_detail = zmtp; errno = EPROTO; @@ -242,6 +248,9 @@ int zmq::curve_server_t::decode (msg_t *msg_) int zmq::curve_server_t::zap_msg_available () { + // TODO I don't think that it is possible that this is called in any + // state other than expect_zap_reply. It should be changed to + // zmq_assert (state == expect_zap_reply); if (state != expect_zap_reply) { errno = EFSM; return -1; @@ -371,6 +380,13 @@ int zmq::curve_server_t::produce_welcome (msg_t *msg_) rc = crypto_box (welcome_ciphertext, welcome_plaintext, sizeof welcome_plaintext, welcome_nonce, cn_client, secret_key); + + // TODO I think we should change this back to zmq_assert (rc == 0); + // as it was before https://github.com/zeromq/libzmq/pull/1832 + // The reason given there was that secret_key might be 0ed. + // But if it were, we would never get this far, since we could + // not have opened the client's hello box with a 0ed key. + if (rc == -1) return -1; @@ -426,6 +442,9 @@ int zmq::curve_server_t::process_initiate (msg_t *msg_) // Check cookie plain text is as expected [C' + s'] if (memcmp (cookie_plaintext + crypto_secretbox_ZEROBYTES, cn_client, 32) || memcmp (cookie_plaintext + crypto_secretbox_ZEROBYTES + 32, cn_secret, 32)) { + // TODO this case is very hard to test, as it would require a modified + // client that knows the server's secret temporary cookie key + // CURVE I: client INITIATE cookie is not valid current_error_detail = encryption; errno = EPROTO; @@ -483,6 +502,9 @@ int zmq::curve_server_t::process_initiate (msg_t *msg_) // What we decrypted must be the client's short-term public key if (memcmp (vouch_plaintext + crypto_box_ZEROBYTES, cn_client, 32)) { + // TODO this case is very hard to test, as it would require a modified + // client that knows the server's secret short-term key + // CURVE I: invalid handshake from client (public key) current_error_detail = encryption; errno = EPROTO; @@ -581,6 +603,11 @@ int zmq::curve_server_t::produce_error (msg_t *msg_) const int zmq::curve_server_t::send_zap_request (const uint8_t *key) { + // TODO I don't think the rc can be -1 anywhere below. + // It might only be -1 if the HWM was exceeded, but on the ZAP socket, + // the HWM is disabled. They should be changed to zmq_assert (rc == 0); + // The method's return type can be changed to void then. + int rc; msg_t msg; diff --git a/src/mechanism.cpp b/src/mechanism.cpp index a27b99a6..ad6dea16 100644 --- a/src/mechanism.cpp +++ b/src/mechanism.cpp @@ -83,7 +83,7 @@ const char *zmq::mechanism_t::socket_type_string (int socket_type) const } size_t zmq::mechanism_t::add_property (unsigned char *ptr, const char *name, - const void *value, size_t value_len) const + const void *value, size_t value_len) { const size_t name_len = strlen (name); zmq_assert (name_len <= 255); diff --git a/src/mechanism.hpp b/src/mechanism.hpp index e49787d0..12295eb7 100644 --- a/src/mechanism.hpp +++ b/src/mechanism.hpp @@ -107,8 +107,10 @@ namespace zmq // property in the wire protocol. const char *socket_type_string (int socket_type) const; - size_t add_property (unsigned char *ptr, const char *name, - const void *value, size_t value_len) const; + static size_t add_property (unsigned char *ptr, + const char *name, + const void *value, + size_t value_len); // Parses a metadata. // Metadata consists of a list of properties consisting of diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 2ffa97ee..383770a7 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -34,7 +34,6 @@ set(tests test_ctx_destroy test_security_null test_security_plain - test_security_curve test_iov test_spec_req test_spec_rep @@ -69,6 +68,10 @@ set(tests test_bind_after_connect_tcp test_sodium ) +if(ZMQ_HAVE_CURVE) + list(APPEND tests + test_security_curve) +endif() if(NOT WIN32) list(APPEND tests test_monitor @@ -170,7 +173,20 @@ foreach(test ${tests}) endforeach() #override timeout for these tests -set_tests_properties(test_security_curve PROPERTIES TIMEOUT 60) +if(ZMQ_HAVE_CURVE) + set_tests_properties(test_security_curve PROPERTIES TIMEOUT 60) +endif() + +#add additional required files +if(ZMQ_HAVE_CURVE) + target_sources(test_security_curve PRIVATE + "../src/tweetnacl.c" + "../src/err.cpp" + "../src/random.cpp" + "../src/clock.cpp" + ) + target_compile_definitions(test_security_curve PRIVATE "-DZMQ_USE_TWEETNACL") +endif() #Check whether all tests in the current folder are present file(READ "${CMAKE_CURRENT_LIST_FILE}" CURRENT_LIST_FILE_CONTENT) diff --git a/tests/test_security_curve.cpp b/tests/test_security_curve.cpp index 6bcab165..16e3902f 100644 --- a/tests/test_security_curve.cpp +++ b/tests/test_security_curve.cpp @@ -40,6 +40,10 @@ # include #endif +#include "../src/tweetnacl.h" +#include "../src/curve_client_tools.hpp" +#include "../src/random.hpp" + // We'll generate random test keys at startup static char valid_client_public [41]; static char valid_client_secret [41]; @@ -550,7 +554,6 @@ void expect_zmtp_failure (void *client, char *my_endpoint, void *server, void *s expect_bounce_fail (server, client); close_zero_linger (client); - #ifdef ZMQ_BUILD_DRAFT_API expect_monitor_event_multiple (server_mon, ZMQ_EVENT_HANDSHAKE_FAILED_ZMTP); #endif @@ -584,13 +587,10 @@ void test_curve_security_with_plain_client_credentials (void *ctx, expect_zmtp_failure (client, my_endpoint, server, server_mon); } -void test_curve_security_unauthenticated_message (char *my_endpoint, - void *server, - int timeout) +int connect_vanilla_socket (char *my_endpoint) { - // Unauthenticated messages from a vanilla socket shouldn't be received - struct sockaddr_in ip4addr; int s; + struct sockaddr_in ip4addr; unsigned short int port; int rc = sscanf (my_endpoint, "tcp://127.0.0.1:%hu", &port); @@ -607,6 +607,15 @@ void test_curve_security_unauthenticated_message (char *my_endpoint, s = socket (AF_INET, SOCK_STREAM, IPPROTO_TCP); rc = connect (s, (struct sockaddr *) &ip4addr, sizeof (ip4addr)); assert (rc > -1); + return s; +} + +void test_curve_security_unauthenticated_message (char *my_endpoint, + void *server, + int timeout) +{ + // Unauthenticated messages from a vanilla socket shouldn't be received + int s = connect_vanilla_socket(my_endpoint); // send anonymous ZMTP/1.0 greeting send (s, "\x01\x00", 2, 0); // send sneaky message that shouldn't be received @@ -621,6 +630,323 @@ void test_curve_security_unauthenticated_message (char *my_endpoint, close (s); } +void send_all (int fd, const char *data, size_t size) +{ + while (size > 0) { + int res = send (fd, data, size, 0); + assert (res > 0); + size -= res; + data += res; + } +} + +template void send (int fd, const char (&data) [N]) +{ + send_all (fd, data, N - 1); +} + +void send_greeting(int s) +{ + send (s, "\xff\0\0\0\0\0\0\0\0\x7f"); // signature + send (s, "\x03\x00"); // version 3.0 + send (s, "CURVE\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"); // mechanism CURVE + send (s, "\0"); // as-server == false + send (s, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"); +} + +void test_curve_security_invalid_hello_wrong_length (char *my_endpoint, + void *server, + void *server_mon, + int timeout) +{ + int s = connect_vanilla_socket (my_endpoint); + + // send GREETING + send_greeting (s); + + // send CURVE HELLO of wrong size + send(s, "\x04\x05HELLO"); + +#ifdef ZMQ_BUILD_DRAFT_API + expect_monitor_event_multiple (server_mon, ZMQ_EVENT_HANDSHAKE_FAILED_ZMTP, + EPROTO); +#endif + + close (s); +} + +const size_t hello_length = 200; +const size_t welcome_length = 168; + +zmq::curve_client_tools_t make_curve_client_tools () +{ + uint8_t valid_client_secret_decoded[32]; + uint8_t valid_client_public_decoded[32]; + + zmq_z85_decode (valid_client_public_decoded, valid_client_public); + zmq_z85_decode (valid_client_secret_decoded, valid_client_secret); + + uint8_t valid_server_public_decoded[32]; + zmq_z85_decode (valid_server_public_decoded, valid_server_public); + + return zmq::curve_client_tools_t (valid_client_public_decoded, + valid_client_secret_decoded, + valid_server_public_decoded); +} + +#ifndef htonll +uint64_t htonll (uint64_t value) +{ + // The answer is 42 + static const int num = 42; + + // Check the endianness + if (*reinterpret_cast (&num) == num) { + const uint32_t high_part = htonl (static_cast (value >> 32)); + const uint32_t low_part = + htonl (static_cast (value & 0xFFFFFFFFLL)); + + return (static_cast (low_part) << 32) | high_part; + } else { + return value; + } +} +#endif + +template void send_command (int s, char (&command)[N]) +{ + if (N < 256) { + send(s, "\x04"); + char len = (char)N; + send_all(s, &len, 1); + } else { + send(s, "\x06"); + uint64_t len = htonll (N); + send_all (s, (char*)&len, 8); + } + send_all (s, command, N); +} + +void test_curve_security_invalid_hello_command_name (char *my_endpoint, + void *server, + void *server_mon, + int timeout) +{ + int s = connect_vanilla_socket (my_endpoint); + + send_greeting (s); + + zmq::curve_client_tools_t tools = make_curve_client_tools (); + + // send CURVE HELLO with a misspelled command name (but otherwise correct) + char hello[hello_length]; + int rc = tools.produce_hello (hello, 0); + assert (rc == 0); + hello[5] = 'X'; + + send_command(s, hello); + +#ifdef ZMQ_BUILD_DRAFT_API + expect_monitor_event_multiple (server_mon, ZMQ_EVENT_HANDSHAKE_FAILED_ZMTP, + EPROTO); +#endif + + close (s); +} + +void test_curve_security_invalid_hello_version (char *my_endpoint, + void *server, + void *server_mon, + int timeout) +{ + int s = connect_vanilla_socket (my_endpoint); + + send_greeting (s); + + zmq::curve_client_tools_t tools = make_curve_client_tools (); + + // send CURVE HELLO with a wrong version number (but otherwise correct) + char hello[hello_length]; + int rc = tools.produce_hello (hello, 0); + assert (rc == 0); + hello[6] = 2; + + send_command (s, hello); + +#ifdef ZMQ_BUILD_DRAFT_API + expect_monitor_event_multiple (server_mon, ZMQ_EVENT_HANDSHAKE_FAILED_ZMTP, + EPROTO); +#endif + + close (s); +} + +void flush_read(int fd) +{ + int res; + char buf[256]; + + while ((res = recv (fd, buf, 256, 0)) == 256) { + } + assert (res != -1); +} + +void recv_all(int fd, uint8_t *data, size_t len) +{ + size_t received = 0; + while (received < len) + { + int res = recv(fd, (char*)data, len, 0); + assert(res > 0); + + data += res; + received += res; + } +} + +void recv_greeting (int fd) +{ + uint8_t greeting[64]; + recv_all (fd, greeting, 64); + // TODO assert anything about the greeting received from the server? +} + +int connect_exchange_greeting_and_send_hello (char *my_endpoint, + zmq::curve_client_tools_t &tools) +{ + int s = connect_vanilla_socket (my_endpoint); + + send_greeting (s); + recv_greeting (s); + + // send valid CURVE HELLO + char hello[hello_length]; + int rc = tools.produce_hello (hello, 0); + assert (rc == 0); + + send_command (s, hello); + return s; +} + +void test_curve_security_invalid_initiate_length (char *my_endpoint, + void *server, + void *server_mon, + int timeout) +{ + zmq::curve_client_tools_t tools = make_curve_client_tools (); + + int s = connect_exchange_greeting_and_send_hello (my_endpoint, tools); + + // receive but ignore WELCOME + flush_read (s); + +#ifdef ZMQ_BUILD_DRAFT_API + int res = get_monitor_event_with_timeout (server_mon, NULL, NULL, timeout); + assert (res == -1); +#endif + + send(s, "\x04\x08INITIATE"); + +#ifdef ZMQ_BUILD_DRAFT_API + expect_monitor_event_multiple (server_mon, ZMQ_EVENT_HANDSHAKE_FAILED_ZMTP, + EPROTO); +#endif + + close (s); +} + +int connect_exchange_greeting_and_hello_welcome ( + char *my_endpoint, + void *server_mon, + int timeout, + zmq::curve_client_tools_t &tools) +{ + int s = connect_exchange_greeting_and_send_hello ( + my_endpoint, tools); + + // receive but ignore WELCOME + uint8_t welcome[welcome_length + 2]; + recv_all (s, welcome, welcome_length + 2); + + int res = tools.process_welcome (welcome + 2, welcome_length); + assert (res == 0); + +#ifdef ZMQ_BUILD_DRAFT_API + res = get_monitor_event_with_timeout (server_mon, NULL, NULL, timeout); + assert (res == -1); +#endif + + return s; +} + +void test_curve_security_invalid_initiate_command_name (char *my_endpoint, + void *server, + void *server_mon, + int timeout) +{ + zmq::curve_client_tools_t tools = make_curve_client_tools (); + int s = connect_exchange_greeting_and_hello_welcome ( + my_endpoint, server_mon, timeout, tools); + + char initiate [257]; + tools.produce_initiate (initiate, 257, 1, NULL, 0); + // modify command name + initiate[5] = 'X'; + + send_command (s, initiate); + +#ifdef ZMQ_BUILD_DRAFT_API + expect_monitor_event_multiple (server_mon, ZMQ_EVENT_HANDSHAKE_FAILED_ZMTP, + EPROTO); +#endif + + close (s); +} + +void test_curve_security_invalid_initiate_command_encrypted_cookie ( + char *my_endpoint, void *server, void *server_mon, int timeout) +{ + zmq::curve_client_tools_t tools = make_curve_client_tools (); + int s = connect_exchange_greeting_and_hello_welcome ( + my_endpoint, server_mon, timeout, tools); + + char initiate [257]; + tools.produce_initiate (initiate, 257, 1, NULL, 0); + // make garbage from encrypted cookie + initiate[30] = !initiate[30]; + + send_command (s, initiate); + +#ifdef ZMQ_BUILD_DRAFT_API + expect_monitor_event_multiple ( + server_mon, ZMQ_EVENT_HANDSHAKE_FAILED_ENCRYPTION, EPROTO); +#endif + + close (s); +} + +void test_curve_security_invalid_initiate_command_encrypted_content ( + char *my_endpoint, void *server, void *server_mon, int timeout) +{ + zmq::curve_client_tools_t tools = make_curve_client_tools (); + int s = connect_exchange_greeting_and_hello_welcome ( + my_endpoint, server_mon, timeout, tools); + + char initiate [257]; + tools.produce_initiate (initiate, 257, 1, NULL, 0); + // make garbage from encrypted content + initiate[150] = !initiate[150]; + + send_command (s, initiate); + +#ifdef ZMQ_BUILD_DRAFT_API + expect_monitor_event_multiple ( + server_mon, ZMQ_EVENT_HANDSHAKE_FAILED_ENCRYPTION, EPROTO); +#endif + + close (s); +} + void test_curve_security_zap_unsuccessful (void *ctx, char *my_endpoint, void *server, @@ -682,6 +1008,8 @@ int main (void) return 0; } + zmq::random_open (); + // Generate new keypairs for these tests int rc = zmq_curve_keypair (valid_client_public, valid_client_secret); assert (rc == 0); @@ -849,10 +1177,67 @@ int main (void) shutdown_context_and_server_side (ctx, zap_thread, server, server_mon, handler); + fprintf (stderr, "test_curve_security_invalid_hello_wrong_length\n"); + setup_context_and_server_side (&ctx, &handler, &zap_thread, &server, + &server_mon, my_endpoint); + test_curve_security_invalid_hello_wrong_length (my_endpoint, server, + server_mon, timeout); + shutdown_context_and_server_side (ctx, zap_thread, server, server_mon, + handler); + fprintf (stderr, "test_curve_security_invalid_hello_command_name\n"); + setup_context_and_server_side (&ctx, &handler, &zap_thread, &server, + &server_mon, my_endpoint); + test_curve_security_invalid_hello_command_name (my_endpoint, server, + server_mon, timeout); + shutdown_context_and_server_side (ctx, zap_thread, server, server_mon, + handler); + + fprintf (stderr, "test_curve_security_invalid_hello_command_version\n"); + setup_context_and_server_side (&ctx, &handler, &zap_thread, &server, + &server_mon, my_endpoint); + test_curve_security_invalid_hello_version (my_endpoint, server, server_mon, + timeout); + shutdown_context_and_server_side (ctx, zap_thread, server, server_mon, + handler); + + fprintf (stderr, "test_curve_security_invalid_initiate_command_length\n"); + setup_context_and_server_side (&ctx, &handler, &zap_thread, &server, + &server_mon, my_endpoint); + test_curve_security_invalid_initiate_length (my_endpoint, server, + server_mon, timeout); + shutdown_context_and_server_side (ctx, zap_thread, server, server_mon, + handler); + + fprintf (stderr, "test_curve_security_invalid_initiate_command_name\n"); + setup_context_and_server_side (&ctx, &handler, &zap_thread, &server, + &server_mon, my_endpoint); + test_curve_security_invalid_initiate_command_name (my_endpoint, server, + server_mon, timeout); + shutdown_context_and_server_side (ctx, zap_thread, server, server_mon, + handler); + + fprintf (stderr, "test_curve_security_invalid_initiate_command_encrypted_cookie\n"); + setup_context_and_server_side (&ctx, &handler, &zap_thread, &server, + &server_mon, my_endpoint); + test_curve_security_invalid_initiate_command_encrypted_cookie ( + my_endpoint, server, server_mon, timeout); + shutdown_context_and_server_side (ctx, zap_thread, server, server_mon, + handler); + + fprintf (stderr, "test_curve_security_invalid_initiate_command_encrypted_content\n"); + setup_context_and_server_side (&ctx, &handler, &zap_thread, &server, + &server_mon, my_endpoint); + test_curve_security_invalid_initiate_command_encrypted_content ( + my_endpoint, server, server_mon, timeout); + shutdown_context_and_server_side (ctx, zap_thread, server, server_mon, + handler); + ctx = zmq_ctx_new (); test_curve_security_invalid_keysize (ctx); rc = zmq_ctx_term (ctx); assert (rc == 0); + zmq::random_close (); + return 0; }