3party/libzmq
0
0
Fork 0
mirror of https://github.com/zeromq/libzmq.git synced 2025-01-14 01:37:56 +08:00
Code Issues Projects Releases Wiki Activity

Problem: Possible buffer overruns related to metadata in various mechanisms (#2683)

Browse Source 
* Problem: no test case with CURVE encryption and large identity

Solution: added test case (currently crashing)

* Problem: possible buffer overflow in mechanism_t::add_property

Solution: add target buffer length parameter and check the buffer is sufficiently large

* Problem: test cases accidentally excluded from build

Solution: remove #if/#endif

* Problem: possible buffer overruns related to metadata at various locations

Solution: allocate buffer large enough for actual metadata, reduce code duplication

* Problem: syntax error related to pointer type conversion

Solution: change argument type of make_command_with_basic_properties to const char *

* Problem: large metadata may cause an assertion in produce_initiate

Solution: Allow metadata of arbitrary size in produce_initiate
This commit is contained in:
Simon Giesecke 2017-08-15 19:42:31 +02:00 committed by Luca Boccassi
parent d5e4319edc
commit 4a18f6204c
9 changed files with 162 additions and 132 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
src/curve_client.cpp
View File

@ -271,33 +271,23 @@ int zmq::curve_client_t::process_welcome (const uint8_t *msg_data,
int zmq::curve_client_t::produce_initiate (msg_t *msg_)
{
// Assume here that metadata is limited to 256 bytes
// FIXME see https://github.com/zeromq/libzmq/issues/2681
uint8_t metadata_plaintext [256];
const size_t metadata_length = basic_properties_len ();
unsigned char *metadata_plaintext =
(unsigned char *) malloc (metadata_length);
alloc_assert (metadata_plaintext);
// Metadata starts after vouch
uint8_t *ptr = metadata_plaintext;
// Add socket type property
const char *socket_type = socket_type_string (options.type);
ptr += add_property (ptr, ZMQ_MSG_PROPERTY_SOCKET_TYPE, socket_type,
strlen (socket_type));
// Add identity property
if (options.type == ZMQ_REQ || options.type == ZMQ_DEALER
|| options.type == ZMQ_ROUTER)
ptr += add_property (ptr, ZMQ_MSG_PROPERTY_IDENTITY, options.identity,
options.identity_size);
const size_t metadata_length = ptr - metadata_plaintext;
add_basic_properties (metadata_plaintext, metadata_length);
size_t msg_size = 113 + 128 + crypto_box_BOXZEROBYTES + metadata_length;
int rc = msg_->init_size (msg_size);
errno_assert (rc == 0);
if (-1
== tools.produce_initiate (msg_->data (), msg_size, cn_nonce,
metadata_plaintext, metadata_length)) {
rc = tools.produce_initiate (msg_->data (), msg_size, cn_nonce,
metadata_plaintext, metadata_length);
free (metadata_plaintext);
if (-1 == rc) {
// TODO see comment in produce_hello
return -1;
}
@ -318,8 +308,11 @@ int zmq::curve_client_t::process_ready (
const size_t clen = (msg_size - 14) + crypto_box_BOXZEROBYTES;
uint8_t ready_nonce [crypto_box_NONCEBYTES];
uint8_t ready_plaintext [crypto_box_ZEROBYTES + 256];
uint8_t ready_box [crypto_box_BOXZEROBYTES + 16 + 256];
uint8_t *ready_plaintext = (uint8_t *) malloc (crypto_box_ZEROBYTES + clen);
alloc_assert (ready_plaintext);
uint8_t *ready_box =
(uint8_t *) malloc (crypto_box_BOXZEROBYTES + 16 + clen);
alloc_assert (ready_box);
memset (ready_box, 0, crypto_box_BOXZEROBYTES);
memcpy (ready_box + crypto_box_BOXZEROBYTES,
@ -331,6 +324,7 @@ int zmq::curve_client_t::process_ready (
int rc = crypto_box_open_afternm (ready_plaintext, ready_box,
clen, ready_nonce, tools.cn_precom);
free (ready_box);
if (rc != 0) {
errno = EPROTO;
@ -339,6 +333,8 @@ int zmq::curve_client_t::process_ready (
rc = parse_metadata (ready_plaintext + crypto_box_ZEROBYTES,
clen - crypto_box_ZEROBYTES);
free (ready_plaintext);
if (rc == 0)
state = connected;

16
src/curve_client_tools.hpp
View File

@ -146,9 +146,6 @@ struct curve_client_tools_t
const uint8_t *metadata_plaintext,
const size_t metadata_length)
{
const size_t max_metadata_length = 256;
zmq_assert (metadata_length < max_metadata_length);
uint8_t vouch_nonce[crypto_box_NONCEBYTES];
uint8_t vouch_plaintext[crypto_box_ZEROBYTES + 64];
uint8_t vouch_box[crypto_box_BOXZEROBYTES + 80];
@ -168,10 +165,12 @@ struct curve_client_tools_t
return -1;
uint8_t initiate_nonce[crypto_box_NONCEBYTES];
uint8_t
initiate_box[crypto_box_BOXZEROBYTES + 144 + max_metadata_length];
uint8_t
initiate_plaintext[crypto_box_ZEROBYTES + 128 + max_metadata_length];
uint8_t *initiate_box = (uint8_t *) malloc (
crypto_box_BOXZEROBYTES + 144 + metadata_length);
alloc_assert (initiate_box);
uint8_t *initiate_plaintext =
(uint8_t *) malloc (crypto_box_ZEROBYTES + 128 + metadata_length);
alloc_assert (initiate_plaintext);
// Create Box [C + vouch + metadata](C'->S')
memset (initiate_plaintext, 0, crypto_box_ZEROBYTES);
@ -190,6 +189,8 @@ struct curve_client_tools_t
rc = crypto_box (initiate_box, initiate_plaintext,
crypto_box_ZEROBYTES + 128 + metadata_length,
initiate_nonce, cn_server, cn_secret);
free (initiate_plaintext);
if (rc == -1)
return -1;
@ -206,6 +207,7 @@ struct curve_client_tools_t
// Box [C + vouch + metadata](C'->S')
memcpy (initiate + 113, initiate_box + crypto_box_BOXZEROBYTES,
128 + metadata_length + crypto_box_BOXZEROBYTES);
free (initiate_box);
return 0;
}

31
src/curve_server.cpp
View File

@ -542,35 +542,33 @@ int zmq::curve_server_t::process_initiate (msg_t *msg_)
int zmq::curve_server_t::produce_ready (msg_t *msg_)
{
const size_t metadata_length = basic_properties_len ();
uint8_t ready_nonce [crypto_box_NONCEBYTES];
uint8_t ready_plaintext [crypto_box_ZEROBYTES + 256];
uint8_t ready_box [crypto_box_BOXZEROBYTES + 16 + 256];
uint8_t *ready_plaintext =
(uint8_t *) malloc (crypto_box_ZEROBYTES + metadata_length);
alloc_assert (ready_plaintext);
// Create Box [metadata](S'->C')
memset (ready_plaintext, 0, crypto_box_ZEROBYTES);
uint8_t *ptr = ready_plaintext + crypto_box_ZEROBYTES;
// Add socket type property
const char *socket_type = socket_type_string (options.type);
ptr += add_property (ptr, ZMQ_MSG_PROPERTY_SOCKET_TYPE, socket_type,
strlen (socket_type));
// Add identity property
if (options.type == ZMQ_REQ
|| options.type == ZMQ_DEALER
|| options.type == ZMQ_ROUTER)
ptr += add_property (ptr, ZMQ_MSG_PROPERTY_IDENTITY, options.identity,
options.identity_size);
ptr += add_basic_properties (ptr, metadata_length);
const size_t mlen = ptr - ready_plaintext;
memcpy (ready_nonce, "CurveZMQREADY---", 16);
put_uint64 (ready_nonce + 16, cn_nonce);
int rc = crypto_box_afternm (ready_box, ready_plaintext,
mlen, ready_nonce, cn_precom);
uint8_t *ready_box =
(uint8_t *) malloc (crypto_box_BOXZEROBYTES + 16 + metadata_length);
alloc_assert (ready_box);
int rc = crypto_box_afternm (ready_box, ready_plaintext, mlen, ready_nonce,
cn_precom);
zmq_assert (rc == 0);
free (ready_plaintext);
rc = msg_->init_size (14 + mlen - crypto_box_BOXZEROBYTES);
errno_assert (rc == 0);
@ -582,6 +580,7 @@ int zmq::curve_server_t::produce_ready (msg_t *msg_)
// Box [metadata](S'->C')
memcpy (ready + 14, ready_box + crypto_box_BOXZEROBYTES,
mlen - crypto_box_BOXZEROBYTES);
free (ready_box);
cn_nonce++;

80
src/mechanism.cpp
View File

@ -82,11 +82,28 @@ const char *zmq::mechanism_t::socket_type_string (int socket_type) const
return names [socket_type];
}
size_t zmq::mechanism_t::add_property (unsigned char *ptr, const char *name,
const void *value, size_t value_len)
static size_t property_len (size_t name_len, size_t value_len)
{
return 1 + name_len + 4 + value_len;
}
static size_t name_len (const char *name)
{
const size_t name_len = strlen (name);
zmq_assert (name_len <= 255);
return name_len;
}
size_t zmq::mechanism_t::add_property (unsigned char *ptr,
size_t ptr_capacity,
const char *name,
const void *value,
size_t value_len)
{
const size_t name_len = ::name_len (name);
const size_t total_len = ::property_len (name_len, value_len);
zmq_assert (total_len <= ptr_capacity);
*ptr++ = static_cast <unsigned char> (name_len);
memcpy (ptr, name, name_len);
ptr += name_len;
@ -95,11 +112,66 @@ size_t zmq::mechanism_t::add_property (unsigned char *ptr, const char *name,
ptr += 4;
memcpy (ptr, value, value_len);
return 1 + name_len + 4 + value_len;
return total_len;
}
size_t zmq::mechanism_t::property_len (const char *name, size_t value_len)
{
return ::property_len (name_len (name), value_len);
}
size_t zmq::mechanism_t::add_basic_properties (unsigned char *buf,
size_t buf_capacity) const
{
unsigned char *ptr = buf;
// Add socket type property
const char *socket_type = socket_type_string (options.type);
ptr += add_property (ptr, buf_capacity,
ZMQ_MSG_PROPERTY_SOCKET_TYPE, socket_type,
strlen (socket_type));
// Add identity property
if (options.type == ZMQ_REQ || options.type == ZMQ_DEALER
|| options.type == ZMQ_ROUTER)
ptr += add_property (ptr, buf_capacity - (ptr - buf),
ZMQ_MSG_PROPERTY_IDENTITY, options.identity,
options.identity_size);
return ptr - buf;
}
size_t zmq::mechanism_t::basic_properties_len() const
{
const char *socket_type = socket_type_string (options.type);
return property_len (ZMQ_MSG_PROPERTY_SOCKET_TYPE, strlen (socket_type))
+ ((options.type == ZMQ_REQ || options.type == ZMQ_DEALER
|| options.type == ZMQ_ROUTER)
? property_len (ZMQ_MSG_PROPERTY_IDENTITY,
options.identity_size)
: 0);
}
void zmq::mechanism_t::make_command_with_basic_properties (
msg_t *msg_, const char *prefix, size_t prefix_len) const
{
const size_t command_size = prefix_len + basic_properties_len ();
const int rc = msg_->init_size (command_size);
errno_assert (rc == 0);
unsigned char *ptr = (unsigned char *) msg_->data ();
// Add prefix
memcpy (ptr, prefix, prefix_len);
ptr += prefix_len;
ptr += add_basic_properties (
ptr, command_size - (ptr - (unsigned char *) msg_->data ()));
}
int zmq::mechanism_t::parse_metadata (const unsigned char *ptr_,
size_t length_, bool zap_flag)
size_t length_,
bool zap_flag)
{
size_t bytes_left = length_;

10
src/mechanism.hpp
View File

@ -108,9 +108,19 @@ namespace zmq
const char *socket_type_string (int socket_type) const;
static size_t add_property (unsigned char *ptr,
size_t ptr_capacity,
const char *name,
const void *value,
size_t value_len);
static size_t property_len (const char *name,
size_t value_len);
size_t add_basic_properties (unsigned char *ptr, size_t ptr_capacity) const;
size_t basic_properties_len () const;
void make_command_with_basic_properties (msg_t *msg_,
const char *prefix,
size_t prefix_len) const;
// Parses a metadata.
// Metadata consists of a list of properties consisting of

27
src/null_mechanism.cpp
View File

@ -98,32 +98,7 @@ int zmq::null_mechanism_t::next_handshake_command (msg_t *msg_)
return 0;
}
unsigned char *const command_buffer = (unsigned char *) malloc (512);
alloc_assert (command_buffer);
unsigned char *ptr = command_buffer;
// Add mechanism string
memcpy (ptr, "\5READY", 6);
ptr += 6;
// Add socket type property
const char *socket_type = socket_type_string (options.type);
ptr += add_property (ptr, ZMQ_MSG_PROPERTY_SOCKET_TYPE, socket_type,
strlen (socket_type));
// Add identity property
if (options.type == ZMQ_REQ
|| options.type == ZMQ_DEALER
|| options.type == ZMQ_ROUTER)
ptr += add_property (ptr, ZMQ_MSG_PROPERTY_IDENTITY, options.identity,
options.identity_size);
const size_t command_size = ptr - command_buffer;
const int rc = msg_->init_size (command_size);
errno_assert (rc == 0);
memcpy (msg_->data (), command_buffer, command_size);
free (command_buffer);
make_command_with_basic_properties (msg_, "\5READY", 6);
ready_command_sent = true;

27
src/plain_client.cpp
View File

@ -159,32 +159,7 @@ int zmq::plain_client_t::process_welcome (
int zmq::plain_client_t::produce_initiate (msg_t *msg_) const
{
unsigned char * const command_buffer = (unsigned char *) malloc (512);
alloc_assert (command_buffer);
unsigned char *ptr = command_buffer;
// Add mechanism string
memcpy (ptr, "\x08INITIATE", 9);
ptr += 9;
// Add socket type property
const char *socket_type = socket_type_string (options.type);
ptr += add_property (ptr, ZMQ_MSG_PROPERTY_SOCKET_TYPE, socket_type,
strlen (socket_type));
// Add identity property
if (options.type == ZMQ_REQ
|| options.type == ZMQ_DEALER
|| options.type == ZMQ_ROUTER)
ptr += add_property (ptr, ZMQ_MSG_PROPERTY_IDENTITY, options.identity,
options.identity_size);
const size_t command_size = ptr - command_buffer;
const int rc = msg_->init_size (command_size);
errno_assert (rc == 0);
memcpy (msg_->data (), command_buffer, command_size);
free (command_buffer);
make_command_with_basic_properties (msg_, "\x08INITIATE", 9);
return 0;
}

27
src/plain_server.cpp
View File

@ -239,32 +239,7 @@ int zmq::plain_server_t::process_initiate (msg_t *msg_)
int zmq::plain_server_t::produce_ready (msg_t *msg_) const
{
unsigned char * const command_buffer = (unsigned char *) malloc (512);
alloc_assert (command_buffer);
unsigned char *ptr = command_buffer;
// Add command name
memcpy (ptr, "\x05READY", 6);
ptr += 6;
// Add socket type property
const char *socket_type = socket_type_string (options.type);
ptr += add_property (ptr, ZMQ_MSG_PROPERTY_SOCKET_TYPE, socket_type,
strlen (socket_type));
// Add identity property
if (options.type == ZMQ_REQ
|| options.type == ZMQ_DEALER
|| options.type == ZMQ_ROUTER)
ptr += add_property (ptr, ZMQ_MSG_PROPERTY_IDENTITY, options.identity,
options.identity_size);
const size_t command_size = ptr - command_buffer;
const int rc = msg_->init_size (command_size);
errno_assert (rc == 0);
memcpy (msg_->data (), command_buffer, command_size);
free (command_buffer);
make_command_with_basic_properties (msg_, "\5READY", 6);
return 0;
}

34
tests/test_security_curve.cpp
View File

@ -44,6 +44,15 @@
#include "../src/curve_client_tools.hpp"
#include "../src/random.hpp"
const char large_identity[] = "0123456789012345678901234567890123456789"
"0123456789012345678901234567890123456789"
"0123456789012345678901234567890123456789"
"0123456789012345678901234567890123456789"
"0123456789012345678901234567890123456789"
"0123456789012345678901234567890123456789"
"012345678901234";
// We'll generate random test keys at startup
static char valid_client_public [41];
static char valid_client_secret [41];
@ -154,7 +163,9 @@ enum zap_protocol_t
zap_too_many_parts
};
static void zap_handler_generic (void *ctx, zap_protocol_t zap_protocol)
static void zap_handler_generic (void *ctx,
zap_protocol_t zap_protocol,
const char *expected_identity = "IDENT")
{
void *control = zmq_socket (ctx, ZMQ_REQ);
assert (control);
@ -205,7 +216,7 @@ static void zap_handler_generic (void *ctx, zap_protocol_t zap_protocol)
assert (streq (version, "1.0"));
assert (streq (mechanism, "CURVE"));
assert (streq (identity, "IDENT"));
assert (streq (identity, expected_identity));
s_sendmore (handler, zap_protocol == zap_wrong_version
? "invalid_version"
@ -265,6 +276,11 @@ static void zap_handler (void *ctx)
zap_handler_generic (ctx, zap_ok);
}
static void zap_handler_large_identity (void *ctx)
{
zap_handler_generic (ctx, zap_ok, large_identity);
}
static void zap_handler_wrong_version (void *ctx)
{
zap_handler_generic (ctx, zap_wrong_version);
@ -412,7 +428,8 @@ void setup_context_and_server_side (void **ctx,
void **server,
void **server_mon,
char *my_endpoint,
zmq_thread_fn zap_handler_ = &zap_handler)
zmq_thread_fn zap_handler_ = &zap_handler,
const char *identity = "IDENT")
{
*ctx = zmq_ctx_new ();
assert (*ctx);
@ -444,7 +461,7 @@ void setup_context_and_server_side (void **ctx,
rc = zmq_setsockopt (*server, ZMQ_CURVE_SECRETKEY, valid_server_secret, 41);
assert (rc == 0);
rc = zmq_setsockopt (*server, ZMQ_IDENTITY, "IDENT", 6);
rc = zmq_setsockopt (*server, ZMQ_IDENTITY, identity, strlen(identity));
assert (rc == 0);
rc = zmq_bind (*server, "tcp://127.0.0.1:*");
@ -1232,6 +1249,15 @@ int main (void)
shutdown_context_and_server_side (ctx, zap_thread, server, server_mon,
handler);
// test with a large identity (resulting in large metadata)
fprintf (stderr, "test_curve_security_with_valid_credentials (large identity)\n");
setup_context_and_server_side (&ctx, &handler, &zap_thread, &server,
&server_mon, my_endpoint, &zap_handler_large_identity, large_identity);
test_curve_security_with_valid_credentials (ctx, my_endpoint, server,
server_mon, timeout);
shutdown_context_and_server_side (ctx, zap_thread, server, server_mon,
handler);
ctx = zmq_ctx_new ();
test_curve_security_invalid_keysize (ctx);
rc = zmq_ctx_term (ctx);