Commit 5d4e30eb authored by Simon Giesecke's avatar Simon Giesecke Committed by Luca Boccassi

Replace console output by monitoring events for curve security issues (#2645)

* Fixing #2002 one way of doing it

 * Mechanisms can implement a new method `error_detail()`
 * This error detail have three values for the moment: no_detail
 (default), protocol, encryption.
    + generic enough to make sense for all mechanisms.
    - low granularity level on information.

* Fixing #2002: implementation of the error details

The ZMQ_EVENT_HANDSHAKE_FAILED event carries the error details
as value.

* Removed Microsoft extenstion for enum member access

This was leading to compilation error under linux.

* Adaptation of CURVE test cases

* Monitoring event: changed API for detailed events

Removed ZMQ_EVENT_HANDSHAKE_FAILED and replaced it by:
- ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL,
- ZMQ_EVENT_HANDSHAKE_FAILED_PROTOCOL,
- ZMQ_EVENT_HANDSHAKE_FAILED_ENCRYPTION

Adaptation of text case `security_curve`

* Removed event value comparison

This was introduced for the previous API model adaptation

* Removed the prints in std output and added missing details

`current_error_detail` was not set in every protocol error cases

* Fixed initialization of current_error_detail

* Fixed error in greeting test case

The handshake failure due to mechanism mismatch in greeting is actually
a protocol error. The error handling method consider it like so and
send a protocol handshake failure monitoring event instead of no_detail.

Fixed the test_security_curve expectation as well.

* Upgraded tests of monitoring events

The tests check the number of monitoring events received

* Problem: does not build under Linux or without ZMQ_DRAFT_API

Solution:
- properly use ZMQ_DRAFT_API conditional compilation
- use receive timeouts instead of Sleep

* Problem: duplicate definition of variable 'timeout'

Solution: merged definitions

* Problem: inconsistent timing dependencies

Solution: reduce timing dependency by using timeouts at more places

* Problem: assertion failure under Linux due to unexpected monitor event

Solution: output event type to aid debugging

* Problem: erroneous assertion code

* Problem: assertion failure with a garbage server key due to an extra third event

Solution: changed assertion to expect three events (needs to be checked)

* Problem: extra include directive to non-existent file

Solution: removed include directive

* Problem: assertion failure on appveyor for unknown reason

Solution: improve debug output

* Problem: no build with libsodium and draft api

Solution: add build configurations with libsodium and draft api

* Problem: assertion failure on CI

Solution: change assertion to reflect actual behaviour on CI (at least temporarily)

* Problem: error in condition in assertion code

* Problem: assertion failure on CI

Solution: generalize assertion to match behavior on CI

* Problem: assertion failures on CI

Solution: removed inconsistent assertion on no monitor events before flushing
improved debuggability by converting function into macro

* Problem: diverging test code for three analogous test cases with garbage key

Solution: extract common code into function

* Problem: does not build without ZMQ_BUILD_DRAFT_API

Solution: introduce dummy variable

* Attempt to remove workaround regarding ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL again

* Problem: EAGAIN error after handshake complete if there is no more data in inbuffer

Solution: Skip tcp_read attempt in that case

* Problem: handshaking event emitted after handshaking failed

Solution: use stream_engine_t::handshaking instead of mechanism_t::status() to determine whether still handshaking

* Include error code in debug output

* Improve debugging output: output flushed events

* Split up ZMQ_EVENT_HANDSHAKE_FAILED_PROTOCOL into ZMQ_EVENT_HANDSHAKE_FAILED_ZMTP and ZMQ_EVENT_HANDSHAKE_FAILED_ZAP

* Fixed compilation without ZMQ_BUILD_DRAFT_API

* Renamed ZMQ_EVENT_HANDSHAKE_SUCCEED to ZMQ_EVENT_HANDSHAKE_SUCCEEDED for language consistency

* Renamed ZMQ_EVENT_HANDSHAKE_SUCCEED to ZMQ_EVENT_HANDSHAKE_SUCCEEDED for language consistency

* Renamed ZMQ_EVENT_HANDSHAKE_SUCCEED to ZMQ_EVENT_HANDSHAKE_SUCCEEDED for language consistency

* Fixed assert_monitor_event (require event instead of allowing no event)
Reverted erroneous change to handshaking condition
Renamed test_wrong_key to test_garbage_key
Generalized assumption in test_garbage_key to allow for ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL with error == EPIPE

* Better isolate test cases from each other by providing a fresh context & server for each

* Added diagnostic output

* Changed assertion to reflect actual behavior on CI

* Fixed formatting, observe maximum line length

* Fixed formatting, observe maximum line length

* Increase timeout to check if this fixes valgrind run

* Close server with close_zero_linger

* Increase timeout to check if this fixes valgrind run

* Increase timeout to check if this fixes valgrind run

* Generalize assertion to also work with valgrind

* Fixed formatting

* Add more diagnostic output

* Generalize assertion to also work with valgrind
parent fda9daa2
......@@ -55,6 +55,19 @@ matrix:
- xmlto
- env: BUILD_TYPE=default CURVE=libsodium
os: osx
- env: BUILD_TYPE=default CURVE=libsodium DRAFT=enabled
os: linux
addons:
apt:
sources:
- sourceline: 'deb http://download.opensuse.org/repositories/network:/messaging:/zeromq:/git-stable/xUbuntu_14.04/ ./'
key_url: 'http://download.opensuse.org/repositories/network:/messaging:/zeromq:/git-stable/xUbuntu_14.04/Release.key'
packages:
- libsodium-dev
- asciidoc
- xmlto
- env: BUILD_TYPE=default CURVE=libsodium DRAFT=enabled
os: osx
- env: BUILD_TYPE=default CURVE=tweetnacl DRAFT=enabled ADDRESS_SANITIZER=enabled
os: linux
dist: trusty
......
......@@ -565,8 +565,11 @@ ZMQ_EXPORT void zmq_threadclose (void* thread);
#define ZMQ_BINDTODEVICE 90
/* DRAFT 0MQ socket events and monitoring */
#define ZMQ_EVENT_HANDSHAKE_FAILED 0x0800
#define ZMQ_EVENT_HANDSHAKE_SUCCEED 0x1000
#define ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL 0x0800
#define ZMQ_EVENT_HANDSHAKE_SUCCEEDED 0x1000
#define ZMQ_EVENT_HANDSHAKE_FAILED_ENCRYPTION 0x2000
#define ZMQ_EVENT_HANDSHAKE_FAILED_ZMTP 0x4000
#define ZMQ_EVENT_HANDSHAKE_FAILED_ZAP 0x8000
/* DRAFT Context options */
#define ZMQ_MSG_T_SIZE 6
......
......@@ -45,6 +45,7 @@ zmq::curve_server_t::curve_server_t (session_base_t *session_,
session (session_),
peer_address (peer_address_),
state (expect_hello),
current_error_detail (no_detail),
cn_nonce (1),
cn_peer_nonce(1)
{
......@@ -101,8 +102,8 @@ int zmq::curve_server_t::process_handshake_command (msg_t *msg_)
rc = process_initiate (msg_);
break;
default:
// Temporary support for security debugging
puts ("CURVE I: invalid handshake command");
// CURVE I: invalid handshake command
current_error_detail = zmtp;
errno = EPROTO;
rc = -1;
break;
......@@ -173,16 +174,16 @@ int zmq::curve_server_t::decode (msg_t *msg_)
zmq_assert (state == connected);
if (msg_->size () < 33) {
// Temporary support for security debugging
puts ("CURVE I: invalid CURVE client, sent malformed command");
// CURVE I : invalid CURVE client, sent malformed command
current_error_detail = zmtp;
errno = EPROTO;
return -1;
}
const uint8_t *message = static_cast <uint8_t *> (msg_->data ());
if (memcmp (message, "\x07MESSAGE", 8)) {
// Temporary support for security debugging
puts ("CURVE I: invalid CURVE client, did not send MESSAGE");
// CURVE I: invalid CURVE client, did not send MESSAGE
current_error_detail = zmtp;
errno = EPROTO;
return -1;
}
......@@ -229,8 +230,8 @@ int zmq::curve_server_t::decode (msg_t *msg_)
msg_->size ());
}
else {
// Temporary support for security debugging
puts ("CURVE I: connection key used for MESSAGE is wrong");
// CURVE I : connection key used for MESSAGE is wrong
current_error_detail = encryption;
errno = EPROTO;
}
free (message_plaintext);
......@@ -264,19 +265,24 @@ zmq::mechanism_t::status_t zmq::curve_server_t::status () const
return mechanism_t::handshaking;
}
zmq::mechanism_t::error_detail_t zmq::curve_server_t::error_detail() const
{
return current_error_detail;
}
int zmq::curve_server_t::process_hello (msg_t *msg_)
{
if (msg_->size () != 200) {
// Temporary support for security debugging
puts ("CURVE I: client HELLO is not correct size");
// CURVE I: client HELLO is not correct size
current_error_detail = zmtp;
errno = EPROTO;
return -1;
}
const uint8_t * const hello = static_cast <uint8_t *> (msg_->data ());
if (memcmp (hello, "\x05HELLO", 6)) {
// Temporary support for security debugging
puts ("CURVE I: client HELLO has invalid command name");
// CURVE I: client HELLO has invalid command name
current_error_detail = zmtp;
errno = EPROTO;
return -1;
}
......@@ -285,8 +291,8 @@ int zmq::curve_server_t::process_hello (msg_t *msg_)
const uint8_t minor = hello [7];
if (major != 1 || minor != 0) {
// Temporary support for security debugging
puts ("CURVE I: client HELLO has unknown version number");
// CURVE I: client HELLO has unknown version number
current_error_detail = zmtp;
errno = EPROTO;
return -1;
}
......@@ -310,8 +316,8 @@ int zmq::curve_server_t::process_hello (msg_t *msg_)
sizeof hello_box,
hello_nonce, cn_client, secret_key);
if (rc != 0) {
// Temporary support for security debugging
puts ("CURVE I: cannot open client HELLO -- wrong server key?");
// CURVE I: cannot open client HELLO -- wrong server key?
current_error_detail = encryption;
errno = EPROTO;
return -1;
}
......@@ -384,16 +390,16 @@ int zmq::curve_server_t::produce_welcome (msg_t *msg_)
int zmq::curve_server_t::process_initiate (msg_t *msg_)
{
if (msg_->size () < 257) {
// Temporary support for security debugging
puts ("CURVE I: client INITIATE is not correct size");
// CURVE I: client INITIATE is not correct size
current_error_detail = zmtp;
errno = EPROTO;
return -1;
}
const uint8_t *initiate = static_cast <uint8_t *> (msg_->data ());
if (memcmp (initiate, "\x08INITIATE", 9)) {
// Temporary support for security debugging
puts ("CURVE I: client INITIATE has invalid command name");
// CURVE I: client INITIATE has invalid command name
current_error_detail = zmtp;
errno = EPROTO;
return -1;
}
......@@ -413,8 +419,8 @@ int zmq::curve_server_t::process_initiate (msg_t *msg_)
sizeof cookie_box,
cookie_nonce, cookie_key);
if (rc != 0) {
// Temporary support for security debugging
puts ("CURVE I: cannot open client INITIATE cookie");
// CURVE I: cannot open client INITIATE cookie
current_error_detail = encryption;
errno = EPROTO;
return -1;
}
......@@ -422,8 +428,8 @@ int zmq::curve_server_t::process_initiate (msg_t *msg_)
// Check cookie plain text is as expected [C' + s']
if (memcmp (cookie_plaintext + crypto_secretbox_ZEROBYTES, cn_client, 32)
|| memcmp (cookie_plaintext + crypto_secretbox_ZEROBYTES + 32, cn_secret, 32)) {
// Temporary support for security debugging
puts ("CURVE I: client INITIATE cookie is not valid");
// CURVE I: client INITIATE cookie is not valid
current_error_detail = encryption;
errno = EPROTO;
return -1;
}
......@@ -446,8 +452,8 @@ int zmq::curve_server_t::process_initiate (msg_t *msg_)
rc = crypto_box_open (initiate_plaintext, initiate_box,
clen, initiate_nonce, cn_client, cn_secret);
if (rc != 0) {
// Temporary support for security debugging
puts ("CURVE I: cannot open client INITIATE");
// CURVE I: cannot open client INITIATE
current_error_detail = encryption;
errno = EPROTO;
return -1;
}
......@@ -471,16 +477,16 @@ int zmq::curve_server_t::process_initiate (msg_t *msg_)
sizeof vouch_box,
vouch_nonce, client_key, cn_secret);
if (rc != 0) {
// Temporary support for security debugging
puts ("CURVE I: cannot open client INITIATE vouch");
// CURVE I: cannot open client INITIATE vouch
current_error_detail = encryption;
errno = EPROTO;
return -1;
}
// What we decrypted must be the client's short-term public key
if (memcmp (vouch_plaintext + crypto_box_ZEROBYTES, cn_client, 32)) {
// Temporary support for security debugging
puts ("CURVE I: invalid handshake from client (public key)");
// CURVE I: invalid handshake from client (public key)
current_error_detail = encryption;
errno = EPROTO;
return -1;
}
......@@ -668,8 +674,8 @@ int zmq::curve_server_t::receive_and_process_zap_reply ()
if (rc == -1)
return close_and_return (msg, -1);
if ((msg [i].flags () & msg_t::more) == (i < 6? 0: msg_t::more)) {
// Temporary support for security debugging
puts ("CURVE I: ZAP handler sent incomplete reply message");
// CURVE I : ZAP handler sent incomplete reply message
current_error_detail = zap;
errno = EPROTO;
return close_and_return (msg, -1);
}
......@@ -677,32 +683,32 @@ int zmq::curve_server_t::receive_and_process_zap_reply ()
// Address delimiter frame
if (msg [0].size () > 0) {
// Temporary support for security debugging
puts ("CURVE I: ZAP handler sent malformed reply message");
// CURVE I: ZAP handler sent malformed reply message
current_error_detail = zap;
errno = EPROTO;
return close_and_return (msg, -1);
}
// Version frame
if (msg [1].size () != 3 || memcmp (msg [1].data (), "1.0", 3)) {
// Temporary support for security debugging
puts ("CURVE I: ZAP handler sent bad version number");
// CURVE I: ZAP handler sent bad version number
current_error_detail = zap;
errno = EPROTO;
return close_and_return (msg, -1);
}
// Request id frame
if (msg [2].size () != 1 || memcmp (msg [2].data (), "1", 1)) {
// Temporary support for security debugging
puts ("CURVE I: ZAP handler sent bad request ID");
// CURVE I: ZAP handler sent bad request ID
current_error_detail = zap;
errno = EPROTO;
return close_and_return (msg, -1);
}
// Status code frame
if (msg [3].size () != 3) {
// Temporary support for security debugging
puts ("CURVE I: ZAP handler rejected client authentication");
// CURVE I: ZAP handler sent invalid status code
current_error_detail = zap;
errno = EACCES;
return close_and_return (msg, -1);
}
......
......@@ -74,6 +74,7 @@ namespace zmq
virtual int decode (msg_t *msg_);
virtual int zap_msg_available ();
virtual status_t status () const;
virtual error_detail_t error_detail () const;
private:
......@@ -98,6 +99,9 @@ namespace zmq
// Status code as received from ZAP handler
std::string status_code;
// Details about the current error state
error_detail_t current_error_detail;
uint64_t cn_nonce;
uint64_t cn_peer_nonce;
......
......@@ -53,6 +53,14 @@ namespace zmq
error
};
// Provides more details when in status_t::error
enum error_detail_t {
no_detail,
zmtp,
zap,
encryption
};
mechanism_t (const options_t &options_);
virtual ~mechanism_t ();
......@@ -73,6 +81,10 @@ namespace zmq
// Returns the status of this mechanism.
virtual status_t status () const = 0;
// Returns details about of the current error of the mechanism.
// Returned value does not makes sense if the current status is not error.
virtual error_detail_t error_detail () const { return no_detail; }
void set_peer_identity (const void *id_ptr, size_t id_size);
void peer_identity (msg_t *msg_);
......
......@@ -1686,14 +1686,34 @@ void zmq::socket_base_t::event_disconnected (const std::string &addr_, zmq::fd_t
event(addr_, fd_, ZMQ_EVENT_DISCONNECTED);
}
void zmq::socket_base_t::event_handshake_failed(const std::string &addr_, int err_)
void zmq::socket_base_t::event_handshake_failed_no_detail (
const std::string &addr_, int err_)
{
event(addr_, err_, ZMQ_EVENT_HANDSHAKE_FAILED);
event (addr_, err_, ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL);
}
void zmq::socket_base_t::event_handshake_succeed(const std::string &addr_, int err_)
void zmq::socket_base_t::event_handshake_failed_zmtp (const std::string &addr_,
int err_)
{
event(addr_, err_, ZMQ_EVENT_HANDSHAKE_SUCCEED);
event (addr_, err_, ZMQ_EVENT_HANDSHAKE_FAILED_ZMTP);
}
void zmq::socket_base_t::event_handshake_failed_zap (const std::string &addr_,
int err_)
{
event (addr_, err_, ZMQ_EVENT_HANDSHAKE_FAILED_ZAP);
}
void zmq::socket_base_t::event_handshake_failed_encryption (
const std::string &addr_, int err_)
{
event (addr_, err_, ZMQ_EVENT_HANDSHAKE_FAILED_ENCRYPTION);
}
void zmq::socket_base_t::event_handshake_succeeded (const std::string &addr_,
int err_)
{
event (addr_, err_, ZMQ_EVENT_HANDSHAKE_SUCCEEDED);
}
void zmq::socket_base_t::event(const std::string &addr_, intptr_t value_, int type_)
......
......@@ -133,8 +133,11 @@ namespace zmq
void event_closed (const std::string &addr_, zmq::fd_t fd_);
void event_close_failed (const std::string &addr_, int err_);
void event_disconnected (const std::string &addr_, zmq::fd_t fd_);
void event_handshake_failed(const std::string &addr_, int err_);
void event_handshake_succeed(const std::string &addr_, int err_);
void event_handshake_failed_no_detail(const std::string &addr_, int err_);
void event_handshake_failed_zmtp(const std::string &addr_, int err_);
void event_handshake_failed_zap(const std::string &addr_, int err_);
void event_handshake_failed_encryption(const std::string &addr_, int err_);
void event_handshake_succeeded(const std::string &addr_, int err_);
protected:
......
......@@ -303,7 +303,7 @@ void zmq::stream_engine_t::in_event ()
return;
}
// If there's no data to process in the buffer...
// If there's no data to process in the buffer...
if (!insize) {
// Retrieve the buffer and read as much data as possible.
......@@ -316,6 +316,8 @@ void zmq::stream_engine_t::in_event ()
const int rc = tcp_read (s, inpos, bufsize);
if (rc == 0) {
// connection closed by peer
errno = EPIPE;
error (connection_error);
return;
}
......@@ -495,6 +497,7 @@ bool zmq::stream_engine_t::handshake ()
const int n = tcp_read (s, greeting_recv + greeting_bytes_read,
greeting_size - greeting_bytes_read);
if (n == 0) {
errno = EPIPE;
error (connection_error);
return false;
}
......@@ -782,8 +785,17 @@ int zmq::stream_engine_t::next_handshake_command (msg_t *msg_)
if (rc == 0)
msg_->set_flags (msg_t::command);
#ifdef ZMQ_BUILD_DRAFT_API
if(mechanism->status() == mechanism_t::error)
socket->event_handshake_failed(endpoint, 0);
if (mechanism->status () == mechanism_t::error) {
int err = errno;
if (mechanism->error_detail () == mechanism_t::zmtp)
socket->event_handshake_failed_zmtp (endpoint, err);
else if (mechanism->error_detail () == mechanism_t::zap)
socket->event_handshake_failed_zap (endpoint, err);
else if (mechanism->error_detail () == mechanism_t::encryption)
socket->event_handshake_failed_encryption (endpoint, err);
else
socket->event_handshake_failed_no_detail (endpoint, err);
}
#endif
return rc;
......@@ -868,7 +880,7 @@ void zmq::stream_engine_t::mechanism_ready ()
}
#ifdef ZMQ_BUILD_DRAFT_API
socket->event_handshake_succeed(endpoint, 0);
socket->event_handshake_succeeded (endpoint, 0);
#endif
}
......@@ -975,8 +987,22 @@ void zmq::stream_engine_t::error (error_reason_t reason)
}
zmq_assert (session);
#ifdef ZMQ_BUILD_DRAFT_API
if(mechanism == NULL || mechanism->status() == mechanism_t::handshaking)
socket->event_handshake_failed(endpoint, (int) s);
int err = errno;
if (mechanism == NULL) {
if (reason == protocol_error)
socket->event_handshake_failed_zmtp (endpoint, err);
else
socket->event_handshake_failed_no_detail (endpoint, err);
} else if (mechanism->status () == mechanism_t::handshaking) {
if (mechanism->error_detail () == mechanism_t::zmtp)
socket->event_handshake_failed_zmtp (endpoint, err);
else if (mechanism->error_detail () == mechanism_t::zap)
socket->event_handshake_failed_zap (endpoint, err);
else if (mechanism->error_detail () == mechanism_t::encryption)
socket->event_handshake_failed_encryption (endpoint, err);
else
socket->event_handshake_failed_no_detail (endpoint, err);
}
#endif
socket->event_disconnected (endpoint, (int) s);
session->flush ();
......
......@@ -50,8 +50,11 @@
#define ZMQ_BINDTODEVICE 90
/* DRAFT 0MQ socket events and monitoring */
#define ZMQ_EVENT_HANDSHAKE_FAILED 0x0800
#define ZMQ_EVENT_HANDSHAKE_SUCCEED 0x1000
#define ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL 0x0800
#define ZMQ_EVENT_HANDSHAKE_SUCCEEDED 0x1000
#define ZMQ_EVENT_HANDSHAKE_FAILED_ENCRYPTION 0x2000
#define ZMQ_EVENT_HANDSHAKE_FAILED_ZMTP 0x4000
#define ZMQ_EVENT_HANDSHAKE_FAILED_ZAP 0x8000
/* DRAFT Context options */
#define ZMQ_MSG_T_SIZE 6
......
......@@ -122,7 +122,7 @@ int main (void)
assert (event == ZMQ_EVENT_CONNECTED);
#ifdef ZMQ_BUILD_DRAFT_API
event = get_monitor_event (client_mon, NULL, NULL);
assert (event == ZMQ_EVENT_HANDSHAKE_SUCCEED);
assert (event == ZMQ_EVENT_HANDSHAKE_SUCCEEDED);
#endif
event = get_monitor_event (client_mon, NULL, NULL);
assert (event == ZMQ_EVENT_MONITOR_STOPPED);
......@@ -134,7 +134,7 @@ int main (void)
assert (event == ZMQ_EVENT_ACCEPTED);
#ifdef ZMQ_BUILD_DRAFT_API
event = get_monitor_event (server_mon, NULL, NULL);
assert (event == ZMQ_EVENT_HANDSHAKE_SUCCEED);
assert (event == ZMQ_EVENT_HANDSHAKE_SUCCEEDED);
#endif
event = get_monitor_event (server_mon, NULL, NULL);
// Sometimes the server sees the client closing before it gets closed.
......
This diff is collapsed.
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment