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.
zeromq · Sep 19, 2014 · 77f14aa · 77f14aa
1 parent 57ade6d
commit 77f14aa
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 6 deletions.
diff --git 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
@@ -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);