From 57ade6d5bbc900bc11ce787018005104e7d2994b Mon Sep 17 00:00:00 2001 From: Pieter Hintjens Date: Thu, 18 Sep 2014 07:32:07 +0200 Subject: [PATCH 1/3] Problem: test_security_curve does't try wrong mechanisms Solution: check that it rejects attempts to connect to a CURVE server using NULL or PLAIN client. --- tests/test_security_curve.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/test_security_curve.cpp b/tests/test_security_curve.cpp index 7b1350db..9e8359c4 100644 --- a/tests/test_security_curve.cpp +++ b/tests/test_security_curve.cpp @@ -198,6 +198,26 @@ int main (void) expect_bounce_fail (server, client); close_zero_linger (client); + // Check CURVE security with NULL client credentials + // This must be caught by the ZAP handler + client = zmq_socket (ctx, ZMQ_DEALER); + assert (client); + rc = zmq_connect (client, "tcp://localhost:9998"); + assert (rc == 0); + expect_bounce_fail (server, client); + close_zero_linger (client); + + // Check CURVE security with PLAIN client credentials + // This must be caught by the ZAP handler + client = zmq_socket (ctx, ZMQ_DEALER); + assert (client); + rc = zmq_setsockopt (client, ZMQ_PLAIN_USERNAME, "admin", 5); + assert (rc == 0); + rc = zmq_setsockopt (client, ZMQ_PLAIN_PASSWORD, "password", 8); + assert (rc == 0); + expect_bounce_fail (server, client); + close_zero_linger (client); + // Shutdown rc = zmq_close (server); assert (rc == 0); From 77f14aad95cdf0d2a244ae9b4a025e5ba0adf01a Mon Sep 17 00:00:00 2001 From: Pieter Hintjens Date: Fri, 19 Sep 2014 19:24:45 +0200 Subject: [PATCH 2/3] Problem: stream_engine.cpp security can be downgraded Solution: accept only the mechanism defined by the socket options. I've not tested this yet, so it's a speculative fix. --- src/stream_engine.cpp | 12 ++++++++---- tests/test_security_curve.cpp | 4 ++-- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/stream_engine.cpp b/src/stream_engine.cpp index 906b0544..ed342150 100644 --- a/src/stream_engine.cpp +++ b/src/stream_engine.cpp @@ -600,13 +600,15 @@ bool zmq::stream_engine_t::handshake () in_batch_size, options.maxmsgsize); alloc_assert (decoder); - if (memcmp (greeting_recv + 12, "NULL\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0", 20) == 0) { + if (options.mechanism == ZMQ_NULL + && memcmp (greeting_recv + 12, "NULL\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0", 20) == 0) { mechanism = new (std::nothrow) null_mechanism_t (session, peer_address, options); alloc_assert (mechanism); } else - if (memcmp (greeting_recv + 12, "PLAIN\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0", 20) == 0) { + if (options.mechanism == ZMQ_PLAIN + && memcmp (greeting_recv + 12, "PLAIN\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0", 20) == 0) { if (options.as_server) mechanism = new (std::nothrow) plain_server_t (session, peer_address, options); @@ -617,7 +619,8 @@ bool zmq::stream_engine_t::handshake () } #ifdef HAVE_LIBSODIUM else - if (memcmp (greeting_recv + 12, "CURVE\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0", 20) == 0) { + if (options.mechanism == ZMQ_CURVE + && memcmp (greeting_recv + 12, "CURVE\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0", 20) == 0) { if (options.as_server) mechanism = new (std::nothrow) curve_server_t (session, peer_address, options); @@ -628,7 +631,8 @@ bool zmq::stream_engine_t::handshake () #endif #ifdef HAVE_LIBGSSAPI_KRB5 else - if (memcmp (greeting_recv + 12, "GSSAPI\0\0\0\0\0\0\0\0\0\0\0\0\0\0", 20) == 0) { + if (options.mechanism == ZMQ_GSSAPI + && memcmp (greeting_recv + 12, "GSSAPI\0\0\0\0\0\0\0\0\0\0\0\0\0\0", 20) == 0) { if (options.as_server) mechanism = new (std::nothrow) gssapi_server_t (session, peer_address, options); diff --git a/tests/test_security_curve.cpp b/tests/test_security_curve.cpp index 9e8359c4..af3925ab 100644 --- a/tests/test_security_curve.cpp +++ b/tests/test_security_curve.cpp @@ -199,7 +199,7 @@ int main (void) close_zero_linger (client); // Check CURVE security with NULL client credentials - // This must be caught by the ZAP handler + // This must be caught by the curve_server class, not passed to ZAP client = zmq_socket (ctx, ZMQ_DEALER); assert (client); rc = zmq_connect (client, "tcp://localhost:9998"); @@ -208,7 +208,7 @@ int main (void) close_zero_linger (client); // Check CURVE security with PLAIN client credentials - // This must be caught by the ZAP handler + // This must be caught by the curve_server class, not passed to ZAP client = zmq_socket (ctx, ZMQ_DEALER); assert (client); rc = zmq_setsockopt (client, ZMQ_PLAIN_USERNAME, "admin", 5); From 0900a489213d74feb86fc0b343308fe7884a2a3c Mon Sep 17 00:00:00 2001 From: Matthew Hawn Date: Fri, 19 Sep 2014 18:07:57 -0600 Subject: [PATCH 3/3] Problem: curve messages can be replayed Solution: ensure message short nonces are strictly increasing and validate them --- src/curve_client.cpp | 20 +++++++++++++++----- src/curve_client.hpp | 1 + src/curve_server.cpp | 17 +++++++++++++---- src/curve_server.hpp | 1 + 4 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/curve_client.cpp b/src/curve_client.cpp index 1dd1dd61..6019c541 100644 --- a/src/curve_client.cpp +++ b/src/curve_client.cpp @@ -34,6 +34,8 @@ zmq::curve_client_t::curve_client_t (const options_t &options_) : mechanism_t (options_), state (send_hello), + cn_nonce(1), + cn_peer_nonce(1), sync() { memcpy (public_key, options_.curve_public_key, crypto_box_PUBLICKEYBYTES); @@ -120,7 +122,7 @@ int zmq::curve_client_t::encode (msg_t *msg_) uint8_t message_nonce [crypto_box_NONCEBYTES]; memcpy (message_nonce, "CurveZMQMESSAGEC", 16); - memcpy (message_nonce + 16, &cn_nonce, 8); + put_uint64 (message_nonce + 16, cn_nonce); const size_t mlen = crypto_box_ZEROBYTES + 1 + msg_->size (); @@ -148,7 +150,7 @@ int zmq::curve_client_t::encode (msg_t *msg_) uint8_t *message = static_cast (msg_->data ()); memcpy (message, "\x07MESSAGE", 8); - memcpy (message + 8, &cn_nonce, 8); + memcpy (message + 8, message_nonce + 16, 8); memcpy (message + 16, message_box + crypto_box_BOXZEROBYTES, mlen - crypto_box_BOXZEROBYTES); @@ -178,6 +180,13 @@ int zmq::curve_client_t::decode (msg_t *msg_) uint8_t message_nonce [crypto_box_NONCEBYTES]; memcpy (message_nonce, "CurveZMQMESSAGES", 16); memcpy (message_nonce + 16, message + 8, 8); + uint64_t nonce = get_uint64(message + 8); + if (nonce <= cn_peer_nonce) { + errno = EPROTO; + return -1; + } + cn_peer_nonce = nonce; + const size_t clen = crypto_box_BOXZEROBYTES + (msg_->size () - 16); @@ -236,7 +245,7 @@ int zmq::curve_client_t::produce_hello (msg_t *msg_) // Prepare the full nonce memcpy (hello_nonce, "CurveZMQHELLO---", 16); - memcpy (hello_nonce + 16, &cn_nonce, 8); + put_uint64 (hello_nonce + 16, cn_nonce); // Create Box [64 * %x0](C'->S) memset (hello_plaintext, 0, sizeof hello_plaintext); @@ -355,7 +364,7 @@ int zmq::curve_client_t::produce_initiate (msg_t *msg_) const size_t mlen = ptr - initiate_plaintext; memcpy (initiate_nonce, "CurveZMQINITIATE", 16); - memcpy (initiate_nonce + 16, &cn_nonce, 8); + put_uint64 (initiate_nonce + 16, cn_nonce); rc = crypto_box (initiate_box, initiate_plaintext, mlen, initiate_nonce, cn_server, cn_secret); @@ -370,7 +379,7 @@ int zmq::curve_client_t::produce_initiate (msg_t *msg_) // Cookie provided by the server in the WELCOME command memcpy (initiate + 9, cn_cookie, 96); // Short nonce, prefixed by "CurveZMQINITIATE" - memcpy (initiate + 105, &cn_nonce, 8); + 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); @@ -399,6 +408,7 @@ int zmq::curve_client_t::process_ready ( memcpy (ready_nonce, "CurveZMQREADY---", 16); memcpy (ready_nonce + 16, msg_data + 6, 8); + cn_peer_nonce = get_uint64(msg_data + 6); int rc = crypto_box_open_afternm (ready_plaintext, ready_box, clen, ready_nonce, cn_precom); diff --git a/src/curve_client.hpp b/src/curve_client.hpp index 14769fda..b5756fbc 100644 --- a/src/curve_client.hpp +++ b/src/curve_client.hpp @@ -102,6 +102,7 @@ namespace zmq // Nonce uint64_t cn_nonce; + uint64_t cn_peer_nonce; int produce_hello (msg_t *msg_); int process_welcome (const uint8_t *cmd_data, size_t data_size); diff --git a/src/curve_server.cpp b/src/curve_server.cpp index 76ed295c..a3c4243b 100644 --- a/src/curve_server.cpp +++ b/src/curve_server.cpp @@ -39,6 +39,7 @@ zmq::curve_server_t::curve_server_t (session_base_t *session_, peer_address (peer_address_), state (expect_hello), cn_nonce (1), + cn_peer_nonce(1), sync() { // Fetch our secret key from socket options @@ -125,7 +126,7 @@ int zmq::curve_server_t::encode (msg_t *msg_) uint8_t message_nonce [crypto_box_NONCEBYTES]; memcpy (message_nonce, "CurveZMQMESSAGES", 16); - memcpy (message_nonce + 16, &cn_nonce, 8); + put_uint64 (message_nonce + 16, cn_nonce); uint8_t flags = 0; if (msg_->flags () & msg_t::more) @@ -155,7 +156,7 @@ int zmq::curve_server_t::encode (msg_t *msg_) uint8_t *message = static_cast (msg_->data ()); memcpy (message, "\x07MESSAGE", 8); - memcpy (message + 8, &cn_nonce, 8); + memcpy (message + 8, message_nonce + 16, 8); memcpy (message + 16, message_box + crypto_box_BOXZEROBYTES, mlen - crypto_box_BOXZEROBYTES); @@ -189,6 +190,12 @@ int zmq::curve_server_t::decode (msg_t *msg_) uint8_t message_nonce [crypto_box_NONCEBYTES]; memcpy (message_nonce, "CurveZMQMESSAGEC", 16); memcpy (message_nonce + 16, message + 8, 8); + uint64_t nonce = get_uint64(message + 8); + if (nonce <= cn_peer_nonce) { + errno = EPROTO; + return -1; + } + cn_peer_nonce = nonce; const size_t clen = crypto_box_BOXZEROBYTES + msg_->size () - 16; @@ -291,6 +298,7 @@ int zmq::curve_server_t::process_hello (msg_t *msg_) memcpy (hello_nonce, "CurveZMQHELLO---", 16); memcpy (hello_nonce + 16, hello + 112, 8); + cn_peer_nonce = get_uint64(hello + 112); memset (hello_box, 0, crypto_box_BOXZEROBYTES); memcpy (hello_box + crypto_box_BOXZEROBYTES, hello + 120, 80); @@ -430,6 +438,7 @@ int zmq::curve_server_t::process_initiate (msg_t *msg_) memcpy (initiate_nonce, "CurveZMQINITIATE", 16); memcpy (initiate_nonce + 16, initiate + 105, 8); + cn_peer_nonce = get_uint64(initiate + 105); rc = crypto_box_open (initiate_plaintext, initiate_box, clen, initiate_nonce, cn_client, cn_secret); @@ -522,7 +531,7 @@ int zmq::curve_server_t::produce_ready (msg_t *msg_) const size_t mlen = ptr - ready_plaintext; memcpy (ready_nonce, "CurveZMQREADY---", 16); - memcpy (ready_nonce + 16, &cn_nonce, 8); + put_uint64 (ready_nonce + 16, cn_nonce); int rc = crypto_box_afternm (ready_box, ready_plaintext, mlen, ready_nonce, cn_precom); @@ -535,7 +544,7 @@ int zmq::curve_server_t::produce_ready (msg_t *msg_) memcpy (ready, "\x05READY", 6); // Short nonce, prefixed by "CurveZMQREADY---" - memcpy (ready + 6, &cn_nonce, 8); + memcpy (ready + 6, ready_nonce + 16, 8); // Box [metadata](S'->C') memcpy (ready + 14, ready_box + crypto_box_BOXZEROBYTES, mlen - crypto_box_BOXZEROBYTES); diff --git a/src/curve_server.hpp b/src/curve_server.hpp index 07eff268..f736ae4e 100644 --- a/src/curve_server.hpp +++ b/src/curve_server.hpp @@ -90,6 +90,7 @@ namespace zmq std::string status_code; uint64_t cn_nonce; + uint64_t cn_peer_nonce; // Our secret key (s) uint8_t secret_key [crypto_box_SECRETKEYBYTES];