Problem: ZMQ_CURVE_SECRETKEY reads beyond end of Z85 data

Solution: change setsockopts on printable keys to expect 41, nor 40
bytes. Code still accepts 40 bytes for compatibility, and copies the
key to a well-terminated string before using it.

Fixes #1148

doc/zmq_getsockopt.txt

@@ -85,6 +85,8 @@ ZMQ_CURVE_SECRETKEY: Retrieve current CURVE secret key
 Retrieves the current long term secret key for the socket. You can
 provide either a 32 byte buffer, to retrieve the binary key value, or
 a 41 byte buffer, to retrieve the key in a printable Z85 format.
+NOTE: to fetch a printable key, the buffer must be 41 bytes large
+to hold the 40-char key value and one null byte.
 
 [horizontal]
 Option value type:: binary data or Z85 text string
@@ -98,7 +100,9 @@ ZMQ_CURVE_SERVERKEY: Retrieve current CURVE server key
 
 Retrieves the current server key for the client socket. You can
 provide either a 32 byte buffer, to retrieve the binary key value, or
-a 40 byte buffer, to retrieve the key in a printable Z85 format.
+a 41-byte buffer, to retrieve the key in a printable Z85 format.
+NOTE: to fetch a printable key, the buffer must be 41 bytes large
+to hold the 40-char key value and one null byte.
 
 [horizontal]
 Option value type:: binary data or Z85 text string

doc/zmq_setsockopt.txt

@@ -112,13 +112,17 @@ ZMQ_CURVE_PUBLICKEY: Set CURVE public key
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 Sets the socket's long term public key. You must set this on CURVE client
 sockets, see linkzmq:zmq_curve[7]. You can provide the key as 32 binary
-bytes, or as a 40-character string encoded in the Z85 encoding format.
-The public key must always be used with the matching secret key. To
-generate a public/secret key pair, use linkzmq:zmq_curve_keypair[3].
+bytes, or as a 40-character string encoded in the Z85 encoding format and
+terminated in a null byte. The public key must always be used with the
+matching secret key. To generate a public/secret key pair, use
+linkzmq:zmq_curve_keypair[3].
+
+NOTE: an option value size of 40 is supported for backwards compatibility,
+though is deprecated.
 
 [horizontal]
 Option value type:: binary data or Z85 text string
-Option value size:: 32 or 40
+Option value size:: 32 or 41
 Default value:: NULL
 Applicable socket types:: all, when using TCP transport
 
@@ -128,12 +132,15 @@ ZMQ_CURVE_SECRETKEY: Set CURVE secret key
 Sets the socket's long term secret key. You must set this on both CURVE
 client and server sockets, see linkzmq:zmq_curve[7]. You can provide the
 key as 32 binary bytes, or as a 40-character string encoded in the Z85
-encoding format. To generate a public/secret key pair, use
-linkzmq:zmq_curve_keypair[3].
+encoding format and terminated in a null byte. To generate a public/secret
+key pair, use linkzmq:zmq_curve_keypair[3].
+
+NOTE: an option value size of 40 is supported for backwards compatibility,
+though is deprecated.
 
 [horizontal]
 Option value type:: binary data or Z85 text string
-Option value size:: 32 or 40
+Option value size:: 32 or 41
 Default value:: NULL
 Applicable socket types:: all, when using TCP transport
 
@@ -160,12 +167,17 @@ ZMQ_CURVE_SERVERKEY: Set CURVE server key
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 Sets the socket's long term server key. You must set this on CURVE client
 sockets, see linkzmq:zmq_curve[7]. You can provide the key as 32 binary
-bytes, or as a 40-character string encoded in the Z85 encoding format.
-This key must have been generated together with the server's secret key.
+bytes, or as a 40-character string encoded in the Z85 encoding format and
+terminated in a null byte. This key must have been generated together with
+the server's secret key. To generate a public/secret key pair, use
+linkzmq:zmq_curve_keypair[3].
+
+NOTE: an option value size of 40 is supported for backwards compatibility,
+though is deprecated.
 
 [horizontal]
 Option value type:: binary data or Z85 text string
-Option value size:: 32 or 40
+Option value size:: 32 or 41
 Default value:: NULL
 Applicable socket types:: all, when using TCP transport
 

src/options.cpp

@@ -366,17 +366,30 @@ int zmq::options_t::setsockopt (int option_, const void *optval_,
             break;
 
         case ZMQ_CURVE_PUBLICKEY:
+            //  TODO: refactor repeated code for these three options
+            //  into set_curve_key (destination, optval, optlen) method
+            //  ==> set_curve_key (curve_public_key, optval_, optvallen_);
             if (optvallen_ == CURVE_KEYSIZE) {
                 memcpy (curve_public_key, optval_, CURVE_KEYSIZE);
                 mechanism = ZMQ_CURVE;
                 return 0;
             }
             else
-            if (optvallen_ == CURVE_KEYSIZE_Z85) {
+            if (optvallen_ == CURVE_KEYSIZE_Z85 + 1) {
                 zmq_z85_decode (curve_public_key, (char *) optval_);
                 mechanism = ZMQ_CURVE;
                 return 0;
             }
+            else
+            //  Deprecated, not symmetrical with zmq_getsockopt
+            if (optvallen_ == CURVE_KEYSIZE_Z85) {
+                char z85_key [41];
+                memcpy (z85_key, (char *) optval_, CURVE_KEYSIZE_Z85);
+                z85_key [CURVE_KEYSIZE_Z85] = 0;
+                zmq_z85_decode (curve_public_key, z85_key);
+                mechanism = ZMQ_CURVE;
+                return 0;
+            }
             break;
 
         case ZMQ_CURVE_SECRETKEY:
@@ -386,25 +399,46 @@ int zmq::options_t::setsockopt (int option_, const void *optval_,
                 return 0;
             }
             else
-            if (optvallen_ == CURVE_KEYSIZE_Z85) {
+            if (optvallen_ == CURVE_KEYSIZE_Z85 + 1) {
                 zmq_z85_decode (curve_secret_key, (char *) optval_);
                 mechanism = ZMQ_CURVE;
                 return 0;
             }
+            else
+            //  Deprecated, not symmetrical with zmq_getsockopt
+            if (optvallen_ == CURVE_KEYSIZE_Z85) {
+                char z85_key [41];
+                memcpy (z85_key, (char *) optval_, CURVE_KEYSIZE_Z85);
+                z85_key [CURVE_KEYSIZE_Z85] = 0;
+                zmq_z85_decode (curve_secret_key, z85_key);
+                mechanism = ZMQ_CURVE;
+                return 0;
+            }
             break;
 
         case ZMQ_CURVE_SERVERKEY:
             if (optvallen_ == CURVE_KEYSIZE) {
                 memcpy (curve_server_key, optval_, CURVE_KEYSIZE);
-                as_server = 0;
                 mechanism = ZMQ_CURVE;
+                as_server = 0;
                 return 0;
             }
             else
-            if (optvallen_ == CURVE_KEYSIZE_Z85) {
+            if (optvallen_ == CURVE_KEYSIZE_Z85 + 1) {
                 zmq_z85_decode (curve_server_key, (char *) optval_);
+                mechanism = ZMQ_CURVE;
                 as_server = 0;
+                return 0;
+            }
+            else
+            //  Deprecated, not symmetrical with zmq_getsockopt
+            if (optvallen_ == CURVE_KEYSIZE_Z85) {
+                char z85_key [41];
+                memcpy (z85_key, (char *) optval_, CURVE_KEYSIZE_Z85);
+                z85_key [CURVE_KEYSIZE_Z85] = 0;
+                zmq_z85_decode (curve_server_key, z85_key);
                 mechanism = ZMQ_CURVE;
+                as_server = 0;
                 return 0;
             }
             break;

tests/test_security_curve.cpp

@@ -111,7 +111,7 @@ int main (void)
     int as_server = 1;
     rc = zmq_setsockopt (server, ZMQ_CURVE_SERVER, &as_server, sizeof (int));
     assert (rc == 0);
-    rc = zmq_setsockopt (server, ZMQ_CURVE_SECRETKEY, server_secret, 40);
+    rc = zmq_setsockopt (server, ZMQ_CURVE_SECRETKEY, server_secret, 41);
     assert (rc == 0);
     rc = zmq_setsockopt (server, ZMQ_IDENTITY, "IDENT", 6);
     assert (rc == 0);
@@ -121,11 +121,11 @@ int main (void)
     //  Check CURVE security with valid credentials
     void *client = zmq_socket (ctx, ZMQ_DEALER);
     assert (client);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_SERVERKEY, server_public, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_SERVERKEY, server_public, 41);
     assert (rc == 0);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_PUBLICKEY, client_public, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_PUBLICKEY, client_public, 41);
     assert (rc == 0);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_SECRETKEY, client_secret, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_SECRETKEY, client_secret, 41);
     assert (rc == 0);
     rc = zmq_connect (client, "tcp://localhost:9998");
     assert (rc == 0);
@@ -138,11 +138,11 @@ int main (void)
     char garbage_key [] = "0000111122223333444455556666777788889999";
     client = zmq_socket (ctx, ZMQ_DEALER);
     assert (client);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_SERVERKEY, garbage_key, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_SERVERKEY, garbage_key, 41);
     assert (rc == 0);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_PUBLICKEY, client_public, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_PUBLICKEY, client_public, 41);
     assert (rc == 0);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_SECRETKEY, client_secret, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_SECRETKEY, client_secret, 41);
     assert (rc == 0);
     rc = zmq_connect (client, "tcp://localhost:9998");
     assert (rc == 0);
@@ -153,11 +153,11 @@ int main (void)
     //  This will be caught by the curve_server class, not passed to ZAP
     client = zmq_socket (ctx, ZMQ_DEALER);
     assert (client);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_SERVERKEY, server_public, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_SERVERKEY, server_public, 41);
     assert (rc == 0);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_PUBLICKEY, garbage_key, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_PUBLICKEY, garbage_key, 41);
     assert (rc == 0);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_SECRETKEY, client_secret, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_SECRETKEY, client_secret, 41);
     assert (rc == 0);
     rc = zmq_connect (client, "tcp://localhost:9998");
     assert (rc == 0);
@@ -168,11 +168,11 @@ int main (void)
     //  This will be caught by the curve_server class, not passed to ZAP
     client = zmq_socket (ctx, ZMQ_DEALER);
     assert (client);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_SERVERKEY, server_public, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_SERVERKEY, server_public, 41);
     assert (rc == 0);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_PUBLICKEY, client_public, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_PUBLICKEY, client_public, 41);
     assert (rc == 0);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_SECRETKEY, garbage_key, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_SECRETKEY, garbage_key, 41);
     assert (rc == 0);
     rc = zmq_connect (client, "tcp://localhost:9998");
     assert (rc == 0);
@@ -187,11 +187,11 @@ int main (void)
 
     client = zmq_socket (ctx, ZMQ_DEALER);
     assert (client);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_SERVERKEY, server_public, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_SERVERKEY, server_public, 41);
     assert (rc == 0);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_PUBLICKEY, bogus_public, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_PUBLICKEY, bogus_public, 41);
     assert (rc == 0);
-    rc = zmq_setsockopt (client, ZMQ_CURVE_SECRETKEY, bogus_secret, 40);
+    rc = zmq_setsockopt (client, ZMQ_CURVE_SECRETKEY, bogus_secret, 41);
     assert (rc == 0);
     rc = zmq_connect (client, "tcp://localhost:9998");
     assert (rc == 0);