From 75d4f50be32690769d16361c1c1b73cd6a99088f Mon Sep 17 00:00:00 2001 From: Pieter Hintjens Date: Sat, 9 Aug 2014 10:24:26 +0200 Subject: [PATCH] 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 | 6 ++++- doc/zmq_setsockopt.txt | 32 +++++++++++++++++-------- src/options.cpp | 44 +++++++++++++++++++++++++++++++---- tests/test_security_curve.cpp | 32 ++++++++++++------------- 4 files changed, 82 insertions(+), 32 deletions(-) diff --git a/doc/zmq_getsockopt.txt b/doc/zmq_getsockopt.txt index 6961cc7a..3732cd24 100644 --- a/doc/zmq_getsockopt.txt +++ b/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 diff --git a/doc/zmq_setsockopt.txt b/doc/zmq_setsockopt.txt index 1deab4bf..be01fe13 100644 --- a/doc/zmq_setsockopt.txt +++ b/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 diff --git a/src/options.cpp b/src/options.cpp index 22409ae7..84b2a7b1 100644 --- a/src/options.cpp +++ b/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_); - as_server = 0; 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; diff --git a/tests/test_security_curve.cpp b/tests/test_security_curve.cpp index d3850eaa..7b1350db 100644 --- a/tests/test_security_curve.cpp +++ b/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);