[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

@@ -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 += \

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"

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 <uint8_t *> (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);
+    int rc = msg_->init_size (200);
+    errno_assert (rc == 0);
 
-    //  Create Box [64 * %x0](C'->S)
-    memset (hello_plaintext, 0, sizeof hello_plaintext);
+    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!)
 
-    int rc = crypto_box (hello_box, hello_plaintext,
-                         sizeof hello_plaintext,
-                         hello_nonce, server_key, cn_secret);
-    if (rc == -1)
+        // msg_->close ();
         return -1;
-
-    rc = msg_->init_size (200);
-    errno_assert (rc == 0);
-    uint8_t *hello = static_cast <uint8_t *> (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);
+    }
 
     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) {
-        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];
+    int rc = tools.process_welcome (msg_data, msg_size);
 
-    //  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) {
+    if (rc == -1) {
         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;
-
-    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;
+    const size_t metadata_length = ptr - metadata_plaintext;
 
-    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 <uint8_t *> (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;

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;

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;
 

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);

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

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)