From 77ef79e3b565f120172c6d1c30fabec6185553da Mon Sep 17 00:00:00 2001 From: Pieter Hintjens Date: Fri, 5 Dec 2014 09:07:37 +0100 Subject: [PATCH] Problem: issue #1273, protocol downgrade attack Solution: backport fix from libzmq master. Also backported test cases. --- NEWS | 4 +- src/session_base.cpp | 8 +++ src/session_base.hpp | 3 +- src/stream_engine.cpp | 15 ++++++ tests/test_security_curve.cpp | 66 ++++++++++++++++++++--- tests/test_security_null.cpp | 121 ++++++++++++++++++++++++++---------------- tests/test_security_plain.cpp | 37 ++++++++++++- 7 files changed, 197 insertions(+), 57 deletions(-) diff --git a/src/session_base.cpp b/src/session_base.cpp index 537dcb3..0b58436 100644 --- a/src/session_base.cpp +++ b/src/session_base.cpp @@ -323,6 +323,14 @@ int zmq::session_base_t::zap_connect () return 0; } +bool zmq::session_base_t::zap_enabled () +{ + return ( + options.mechanism != ZMQ_NULL || + (options.mechanism == ZMQ_NULL && options.zap_domain.length() > 0) + ); +} + void zmq::session_base_t::process_attach (i_engine *engine_) { zmq_assert (engine_ != NULL); diff --git a/src/session_base.hpp b/src/session_base.hpp index 2ef7dc5..63e16bd 100644 --- a/src/session_base.hpp +++ b/src/session_base.hpp @@ -68,7 +68,8 @@ namespace zmq int push_msg (msg_t *msg_); int zap_connect (); - + bool zap_enabled (); + // Fetches a message. Returns 0 if successful; -1 otherwise. // The caller is responsible for freeing the message when no // longer used. diff --git a/src/stream_engine.cpp b/src/stream_engine.cpp index 4d252d8..3d84d8f 100644 --- a/src/stream_engine.cpp +++ b/src/stream_engine.cpp @@ -464,6 +464,11 @@ bool zmq::stream_engine_t::handshake () // Is the peer using ZMTP/1.0 with no revision number? // If so, we send and receive rest of identity message if (greeting_recv [0] != 0xff || !(greeting_recv [9] & 0x01)) { + if (session->zap_enabled ()) { + // Reject ZMTP 1.0 connections if ZAP is enabled + error (); + return false; + } encoder = new (std::nothrow) v1_encoder_t (out_batch_size); alloc_assert (encoder); @@ -505,6 +510,11 @@ bool zmq::stream_engine_t::handshake () } else if (greeting_recv [revision_pos] == ZMTP_1_0) { + if (session->zap_enabled ()) { + // Reject ZMTP 1.0 connections if ZAP is enabled + error (); + return false; + } encoder = new (std::nothrow) v1_encoder_t ( out_batch_size); alloc_assert (encoder); @@ -515,6 +525,11 @@ bool zmq::stream_engine_t::handshake () } else if (greeting_recv [revision_pos] == ZMTP_2_0) { + if (session->zap_enabled ()) { + // Reject ZMTP 1.0 connections if ZAP is enabled + error (); + return false; + } encoder = new (std::nothrow) v2_encoder_t (out_batch_size); alloc_assert (encoder); diff --git a/tests/test_security_curve.cpp b/tests/test_security_curve.cpp index a24466f..e99a4b3 100644 --- a/tests/test_security_curve.cpp +++ b/tests/test_security_curve.cpp @@ -18,12 +18,23 @@ */ #include "testutil.hpp" +#if defined (ZMQ_HAVE_WINDOWS) +# include +# include +# include +# define close closesocket +#else +# include +# include +# include +# include +#endif // We'll generate random test keys at startup -static char client_public [40]; -static char client_secret [40]; -static char server_public [40]; -static char server_secret [40]; +static char client_public [41]; +static char client_secret [41]; +static char server_public [41]; +static char server_secret [41]; // -------------------------------------------------------------------------- // This methods receives and validates ZAP requestes (allowing or denying @@ -46,7 +57,7 @@ static void zap_handler (void *handler) int size = zmq_recv (handler, client_key, 32, 0); assert (size == 32); - char client_key_text [40]; + char client_key_text [41]; zmq_z85_encode (client_key_text, client_key, 32); assert (streq (version, "1.0")); @@ -181,8 +192,8 @@ int main (void) // Check CURVE security with bogus client credentials // This must be caught by the ZAP handler - char bogus_public [40]; - char bogus_secret [40]; + char bogus_public [41]; + char bogus_secret [41]; zmq_curve_keypair (bogus_public, bogus_secret); client = zmq_socket (ctx, ZMQ_DEALER); @@ -217,7 +228,46 @@ int main (void) assert (rc == 0); expect_bounce_fail (server, client); close_zero_linger (client); - + + // Unauthenticated messages from a vanilla socket shouldn't be received + struct sockaddr_in ip4addr; + int s; + + ip4addr.sin_family = AF_INET; + ip4addr.sin_port = htons (9998); + inet_pton (AF_INET, "127.0.0.1", &ip4addr.sin_addr); + + s = socket (AF_INET, SOCK_STREAM, IPPROTO_TCP); + rc = connect (s, (struct sockaddr*) &ip4addr, sizeof (ip4addr)); + assert (rc > -1); + // send anonymous ZMTP/1.0 greeting + send (s, "\x01\x00", 2, 0); + // send sneaky message that shouldn't be received + send (s, "\x08\x00sneaky\0", 9, 0); + int timeout = 150; + zmq_setsockopt (server, ZMQ_RCVTIMEO, &timeout, sizeof (timeout)); + char *buf = s_recv (server); + if (buf != NULL) { + printf ("Received unauthenticated message: %s\n", buf); + assert (buf == NULL); + } + close (s); + + // Check return codes for invalid buffer sizes + client = zmq_socket (ctx, ZMQ_DEALER); + assert (client); + errno = 0; + rc = zmq_setsockopt (client, ZMQ_CURVE_SERVERKEY, server_public, 123); + assert (rc == -1 && errno == EINVAL); + errno = 0; + rc = zmq_setsockopt (client, ZMQ_CURVE_PUBLICKEY, client_public, 123); + assert (rc == -1 && errno == EINVAL); + errno = 0; + rc = zmq_setsockopt (client, ZMQ_CURVE_SECRETKEY, client_secret, 123); + assert (rc == -1 && errno == EINVAL); + rc = zmq_close (client); + assert (rc == 0); + // Shutdown rc = zmq_close (server); assert (rc == 0); diff --git a/tests/test_security_null.cpp b/tests/test_security_null.cpp index 8a55632..6b74e8c 100644 --- a/tests/test_security_null.cpp +++ b/tests/test_security_null.cpp @@ -1,5 +1,5 @@ /* - Copyright (c) 2007-2013 Contributors as noted in the AUTHORS file + Copyright (c) 2007-2014 Contributors as noted in the AUTHORS file This file is part of 0MQ. @@ -18,6 +18,17 @@ */ #include "testutil.hpp" +#if defined (ZMQ_HAVE_WINDOWS) +# include +# include +# include +# define close closesocket +#else +# include +# include +# include +# include +#endif static void zap_handler (void *handler) @@ -27,6 +38,7 @@ zap_handler (void *handler) char *version = s_recv (handler); if (!version) break; // Terminating + char *sequence = s_recv (handler); char *domain = s_recv (handler); char *address = s_recv (handler); @@ -57,7 +69,7 @@ zap_handler (void *handler) free (identity); free (mechanism); } - zmq_close (handler); + close_zero_linger (handler); } int main (void) @@ -76,72 +88,89 @@ int main (void) void *zap_thread = zmq_threadstart (&zap_handler, handler); // We bounce between a binding server and a connecting client + + // We first test client/server with no ZAP domain + // Libzmq does not call our ZAP handler, the connect must succeed void *server = zmq_socket (ctx, ZMQ_DEALER); assert (server); void *client = zmq_socket (ctx, ZMQ_DEALER); assert (client); - - // We first test client/server with no ZAP domain - // Libzmq does not call our ZAP handler, the connect must succeed rc = zmq_bind (server, "tcp://127.0.0.1:9000"); assert (rc == 0); - rc = zmq_connect (client, "tcp://localhost:9000"); + rc = zmq_connect (client, "tcp://127.0.0.1:9000"); assert (rc == 0); bounce (server, client); - zmq_unbind (server, "tcp://127.0.0.1:9000"); - zmq_disconnect (client, "tcp://localhost:9000"); - + close_zero_linger (client); + close_zero_linger (server); + // Now define a ZAP domain for the server; this enables // authentication. We're using the wrong domain so this test // must fail. - // ************************************************************** - // PH: the following causes libzmq to get confused, so that the - // next step fails. To reproduce, uncomment this block. Note that - // even creating a new client/server socket pair, the behaviour - // does not change. - // ************************************************************** - // Destroying the old sockets and creating new ones isn't needed, - // but it shows that the problem isn't related to specific sockets. - //close_zero_linger (client); - //close_zero_linger (server); - //server = zmq_socket (ctx, ZMQ_DEALER); - //assert (server); - //client = zmq_socket (ctx, ZMQ_DEALER); - //assert (client); - //// The above code should not be required - //rc = zmq_setsockopt (server, ZMQ_ZAP_DOMAIN, "WRONG", 5); - //assert (rc == 0); - //rc = zmq_bind (server, "tcp://127.0.0.1:9001"); - //assert (rc == 0); - //rc = zmq_connect (client, "tcp://localhost:9001"); - //assert (rc == 0); - //expect_bounce_fail (server, client); - //zmq_unbind (server, "tcp://127.0.0.1:9001"); - //zmq_disconnect (client, "tcp://localhost:9001"); - + server = zmq_socket (ctx, ZMQ_DEALER); + assert (server); + client = zmq_socket (ctx, ZMQ_DEALER); + assert (client); + rc = zmq_setsockopt (server, ZMQ_ZAP_DOMAIN, "WRONG", 5); + assert (rc == 0); + rc = zmq_bind (server, "tcp://127.0.0.1:9001"); + assert (rc == 0); + rc = zmq_connect (client, "tcp://127.0.0.1:9001"); + assert (rc == 0); + expect_bounce_fail (server, client); + close_zero_linger (client); + close_zero_linger (server); + // Now use the right domain, the test must pass + server = zmq_socket (ctx, ZMQ_DEALER); + assert (server); + client = zmq_socket (ctx, ZMQ_DEALER); + assert (client); rc = zmq_setsockopt (server, ZMQ_ZAP_DOMAIN, "TEST", 4); assert (rc == 0); rc = zmq_bind (server, "tcp://127.0.0.1:9002"); assert (rc == 0); - rc = zmq_connect (client, "tcp://localhost:9002"); + rc = zmq_connect (client, "tcp://127.0.0.1:9002"); assert (rc == 0); - // ************************************************************** - // PH: it fails here; though the ZAP reply is 200 OK, and - // null_mechanism.cpp correctly parses that, the connection - // never succeeds and the test hangs. - // ************************************************************** bounce (server, client); - zmq_unbind (server, "tcp://127.0.0.1:9002"); - zmq_disconnect (client, "tcp://localhost:9002"); - - // Shutdown close_zero_linger (client); close_zero_linger (server); - rc = zmq_ctx_term (ctx); + + // Unauthenticated messages from a vanilla socket shouldn't be received + server = zmq_socket (ctx, ZMQ_DEALER); + assert (server); + rc = zmq_setsockopt (server, ZMQ_ZAP_DOMAIN, "WRONG", 5); assert (rc == 0); + rc = zmq_bind (server, "tcp://127.0.0.1:9003"); + assert (rc == 0); + + struct sockaddr_in ip4addr; + int s; + + ip4addr.sin_family = AF_INET; + ip4addr.sin_port = htons(9003); + inet_pton(AF_INET, "127.0.0.1", &ip4addr.sin_addr); - // Wait until ZAP handler terminates. + s = socket (AF_INET, SOCK_STREAM, IPPROTO_TCP); + rc = connect (s, (struct sockaddr*) &ip4addr, sizeof ip4addr); + assert (rc > -1); + // send anonymous ZMTP/1.0 greeting + send (s, "\x01\x00", 2, 0); + // send sneaky message that shouldn't be received + send (s, "\x08\x00sneaky\0", 9, 0); + int timeout = 150; + zmq_setsockopt (server, ZMQ_RCVTIMEO, &timeout, sizeof (timeout)); + char *buf = s_recv (server); + if (buf != NULL) { + printf ("Received unauthenticated message: %s\n", buf); + assert (buf == NULL); + } + close (s); + close_zero_linger (server); + + // Shutdown + rc = zmq_ctx_term (ctx); + assert (rc == 0); + // Wait until ZAP handler terminates zmq_threadclose (zap_thread); return 0; diff --git a/tests/test_security_plain.cpp b/tests/test_security_plain.cpp index 74973fd..c257840 100644 --- a/tests/test_security_plain.cpp +++ b/tests/test_security_plain.cpp @@ -1,5 +1,5 @@ /* - Copyright (c) 2007-2013 Contributors as noted in the AUTHORS file + Copyright (c) 2007-2014 Contributors as noted in the AUTHORS file This file is part of 0MQ. @@ -18,6 +18,17 @@ */ #include "testutil.hpp" +#if defined (ZMQ_HAVE_WINDOWS) +# include +# include +# include +# define close closesocket +#else +# include +# include +# include +# include +#endif static void zap_handler (void *ctx) @@ -137,6 +148,30 @@ int main (void) expect_bounce_fail (server, client); close_zero_linger (client); + // Unauthenticated messages from a vanilla socket shouldn't be received + struct sockaddr_in ip4addr; + int s; + + ip4addr.sin_family = AF_INET; + ip4addr.sin_port = htons (9998); + inet_pton (AF_INET, "127.0.0.1", &ip4addr.sin_addr); + + s = socket (AF_INET, SOCK_STREAM, IPPROTO_TCP); + rc = connect (s, (struct sockaddr*) &ip4addr, sizeof (ip4addr)); + assert (rc > -1); + // send anonymous ZMTP/1.0 greeting + send (s, "\x01\x00", 2, 0); + // send sneaky message that shouldn't be received + send (s, "\x08\x00sneaky\0", 9, 0); + int timeout = 150; + zmq_setsockopt (server, ZMQ_RCVTIMEO, &timeout, sizeof (timeout)); + char *buf = s_recv (server); + if (buf != NULL) { + printf ("Received unauthenticated message: %s\n", buf); + assert (buf == NULL); + } + close (s); + // Shutdown rc = zmq_close (server); assert (rc == 0);