- tor-commits - lists.torproject.org

[stegotorus/master] symbolic names for opcodes, better presentation of bad packet headers
by zwol＠torproject.org 20 Jul '12

20 Jul '12

commit 05efb662f00e77a4333fce9dd5fada425b62b0b3 Author: Zack Weinberg <zackw(a)cmu.edu> Date: Wed Jun 20 10:50:48 2012 -0700 symbolic names for opcodes, better presentation of bad packet headers --- src/protocol/chop.cc | 56 ++++++++++++++++++++++++++++++++----------------- 1 files changed, 36 insertions(+), 20 deletions(-) diff --git a/src/protocol/chop.cc b/src/protocol/chop.cc index 8b79167..0d914d8 100644 --- a/src/protocol/chop.cc +++ b/src/protocol/chop.cc @@ -77,14 +77,29 @@ enum opcode_t op_DAT = 0, // Pass data section along to upstream op_FIN = 1, // No further transmissions (pass data along if any) op_RST = 2, // Protocol error, close circuit now - op_RK1 = 3, // Commence rekeying - op_RK2 = 4, // Continue rekeying - op_RK3 = 5, // Conclude rekeying - op_RESERVED0 = 6, // 6 -- 127 reserved for future definition + op_RESERVED0 = 3, // 3 -- 127 reserved for future definition op_STEG0 = 128, // 128 -- 255 reserved for steganography modules op_LAST = 255 }; +static const char * +opname(opcode_t o, char fallbackbuf[4]) +{ + switch (o) { + case op_DAT: return "DAT"; + case op_FIN: return "FIN"; + case op_RST: return "RST"; + default: { + unsigned int x = o; + if (x < op_STEG0) + xsnprintf(fallbackbuf, sizeof fallbackbuf, "R%02x", x); + else + xsnprintf(fallbackbuf, sizeof fallbackbuf, "S%02x", x - op_STEG0); + return fallbackbuf; + } + } +} + class block_header { uint8_t clear[16]; @@ -857,9 +872,10 @@ chop_circuit_t::send_targeted(chop_conn_t *conn, size_t d, size_t p, opcode_t f, return -1; } - log_debug(conn, "transmitting block %u <d=%lu p=%lu f=%02x>", + char fallbackbuf[3]; + log_debug(conn, "transmitting block %u <d=%lu p=%lu f=%s>", hdr.seqno(), (unsigned long)hdr.dlen(), (unsigned long)hdr.plen(), - (uint8_t)hdr.opcode()); + opname(hdr.opcode(), fallbackbuf)); if (conn->send(block)) { evbuffer_free(block); @@ -1016,16 +1032,10 @@ chop_circuit_t::process_queue() pending_error = true; break; - case op_RK1: - case op_RK2: - case op_RK3: - log_warn(this, "rekeying not yet implemented"); - pending_error = true; - break; - default: - log_warn(this, "protocol error: unknown block opcode %x", - (unsigned int)blk.op); + char fallbackbuf[4]; + log_warn(this, "protocol error: unsupported block opcode %s", + opname(blk.op, fallbackbuf)); pending_error = true; break; } @@ -1260,9 +1270,14 @@ chop_conn_t::recv() block_header hdr(recv_pending, *upstream->recv_hdr_crypt); if (!hdr.valid(upstream->recv_queue.window())) { const uint8_t *c = hdr.cleartext(); - log_info(this, "invalid block header: %02x%02x%02x%02x|%02x%02x|%02x%02x|%02x|%02x%02x%02x%02x%02x%02x%02x", - c[0], c[1], c[2], c[3], c[4], c[5], c[6], c[7], - c[8], c[9], c[10], c[11], c[12], c[13], c[14], c[15]); + char fallbackbuf[4]; + log_info(this, "invalid block header: " + "%lu|%lu|%lu|%s|%02x%02x%02x%02x%02x%02x%02x", + (unsigned long)hdr.seqno(), + (unsigned long)hdr.dlen(), + (unsigned long)hdr.plen(), + opname(hdr.opcode(), fallbackbuf), + c[9], c[10], c[11], c[12], c[13], c[14], c[15]); return -1; } if (avail < hdr.total_len()) { @@ -1285,9 +1300,10 @@ chop_conn_t::recv() return -1; } - log_debug(this, "receiving block %u <d=%lu p=%lu f=%02x>", + char fallbackbuf[4]; + log_debug(this, "receiving block %u <d=%lu p=%lu f=%s>", hdr.seqno(), (unsigned long)hdr.dlen(), (unsigned long)hdr.plen(), - (unsigned int)hdr.opcode()); + opname(hdr.opcode(), fallbackbuf)); evbuffer *data = evbuffer_new(); if (!data || (hdr.dlen() && evbuffer_add(data, decodebuf, hdr.dlen()))) {

1 0

[stegotorus/master] fixed another error on 32-bit by casting types
by zwol＠torproject.org 20 Jul '12

20 Jul '12

commit e651f0b72d987d6d15ebef6b6b504b6dc2fa85c3 Author: Linda Briesemeister <linda.briesemeister(a)sri.com> Date: Mon Jun 25 13:24:41 2012 -0500 fixed another error on 32-bit by casting types --- src/steg/http.cc | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/steg/http.cc b/src/steg/http.cc index fef3724..7caf2e7 100644 --- a/src/steg/http.cc +++ b/src/steg/http.cc @@ -310,7 +310,7 @@ http_client_cookie_transmit (http_steg_t *s, struct evbuffer *source, log_debug(conn, "cookie input %lu encoded %lu final %lu/%lu", (unsigned long)sbuflen, (unsigned long)len, - (unsigned long)cookie_len, strlen(cookiebuf)); + (unsigned long)cookie_len, (unsigned long)strlen(cookiebuf)); log_debug(conn, "cookie encoded: %s", data2); log_debug(conn, "cookie final: %s", cookiebuf);

1 0

[stegotorus/master] Add another testing option to turn off all encryption in the chopper.
by zwol＠torproject.org 20 Jul '12

20 Jul '12

commit 4575a9637b902dbf835e96b5d245a6d53f33a868 Author: Zack Weinberg <zackw(a)cmu.edu> Date: Wed Jun 20 22:49:56 2012 -0700 Add another testing option to turn off all encryption in the chopper. --- src/crypt.cc | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/crypt.h | 20 +++++++++++ src/protocol/chop.cc | 68 +++++++++++++++++++++++++------------ 3 files changed, 157 insertions(+), 22 deletions(-) diff --git a/src/crypt.cc b/src/crypt.cc index 1110d0b..6d7e0d4 100644 --- a/src/crypt.cc +++ b/src/crypt.cc @@ -156,6 +156,13 @@ namespace { virtual void encrypt(uint8_t *out, const uint8_t *in); }; + struct ecb_encryptor_noop_impl : ecb_encryptor + { + ecb_encryptor_noop_impl() {} + virtual ~ecb_encryptor_noop_impl(); + virtual void encrypt(uint8_t *out, const uint8_t *in); + }; + struct ecb_decryptor_impl : ecb_decryptor { EVP_CIPHER_CTX ctx; @@ -163,6 +170,13 @@ namespace { virtual ~ecb_decryptor_impl(); virtual void decrypt(uint8_t *out, const uint8_t *in); }; + + struct ecb_decryptor_noop_impl : ecb_decryptor + { + ecb_decryptor_noop_impl() {} + virtual ~ecb_decryptor_noop_impl(); + virtual void decrypt(uint8_t *out, const uint8_t *in); + }; } ecb_encryptor * @@ -197,6 +211,12 @@ ecb_encryptor::create(key_generator *gen, size_t keylen) return enc; } +ecb_encryptor * +ecb_encryptor::create_noop() +{ + return new ecb_encryptor_noop_impl; +} + ecb_decryptor * ecb_decryptor::create(const uint8_t *key, size_t keylen) { @@ -229,13 +249,23 @@ ecb_decryptor::create(key_generator *gen, size_t keylen) return dec; } +ecb_decryptor * +ecb_decryptor::create_noop() +{ + return new ecb_decryptor_noop_impl; +} + ecb_encryptor::~ecb_encryptor() {} ecb_encryptor_impl::~ecb_encryptor_impl() { EVP_CIPHER_CTX_cleanup(&ctx); } +ecb_encryptor_noop_impl::~ecb_encryptor_noop_impl() +{} ecb_decryptor::~ecb_decryptor() {} ecb_decryptor_impl::~ecb_decryptor_impl() { EVP_CIPHER_CTX_cleanup(&ctx); } +ecb_decryptor_noop_impl::~ecb_decryptor_noop_impl() +{} void ecb_encryptor_impl::encrypt(uint8_t *out, const uint8_t *in) @@ -247,6 +277,12 @@ ecb_encryptor_impl::encrypt(uint8_t *out, const uint8_t *in) } void +ecb_encryptor_noop_impl::encrypt(uint8_t *out, const uint8_t *in) +{ + memcpy(out, in, AES_BLOCK_LEN); +} + +void ecb_decryptor_impl::decrypt(uint8_t *out, const uint8_t *in) { int olen; @@ -255,6 +291,12 @@ ecb_decryptor_impl::decrypt(uint8_t *out, const uint8_t *in) log_crypto_abort("ecb_decryptor::decrypt"); } +void +ecb_decryptor_noop_impl::decrypt(uint8_t *out, const uint8_t *in) +{ + memcpy(out, in, AES_BLOCK_LEN); +} + namespace { struct gcm_encryptor_impl : gcm_encryptor { @@ -265,6 +307,14 @@ namespace { const uint8_t *nonce, size_t nlen); }; + struct gcm_encryptor_noop_impl : gcm_encryptor + { + gcm_encryptor_noop_impl() {} + virtual ~gcm_encryptor_noop_impl(); + virtual void encrypt(uint8_t *out, const uint8_t *in, size_t inlen, + const uint8_t *nonce, size_t nlen); + }; + struct gcm_decryptor_impl : gcm_decryptor { EVP_CIPHER_CTX ctx; @@ -273,6 +323,14 @@ namespace { virtual int decrypt(uint8_t *out, const uint8_t *in, size_t inlen, const uint8_t *nonce, size_t nlen); }; + + struct gcm_decryptor_noop_impl : gcm_decryptor + { + gcm_decryptor_noop_impl() {} + virtual ~gcm_decryptor_noop_impl(); + virtual int decrypt(uint8_t *out, const uint8_t *in, size_t inlen, + const uint8_t *nonce, size_t nlen); + }; } // It *appears* (from inspecting the guts of libcrypto, *not* from the @@ -320,6 +378,12 @@ gcm_encryptor::create(key_generator *gen, size_t keylen) return enc; } +gcm_encryptor * +gcm_encryptor::create_noop() +{ + return new gcm_encryptor_noop_impl; +} + gcm_decryptor * gcm_decryptor::create(const uint8_t *key, size_t keylen) { @@ -348,12 +412,23 @@ gcm_decryptor::create(key_generator *gen, size_t keylen) return dec; } +gcm_decryptor * +gcm_decryptor::create_noop() +{ + return new gcm_decryptor_noop_impl; +} + gcm_encryptor::~gcm_encryptor() {} gcm_encryptor_impl::~gcm_encryptor_impl() { EVP_CIPHER_CTX_cleanup(&ctx); } +gcm_encryptor_noop_impl::~gcm_encryptor_noop_impl() +{} + gcm_decryptor::~gcm_decryptor() {} gcm_decryptor_impl::~gcm_decryptor_impl() { EVP_CIPHER_CTX_cleanup(&ctx); } +gcm_decryptor_noop_impl::~gcm_decryptor_noop_impl() +{} void gcm_encryptor_impl::encrypt(uint8_t *out, const uint8_t *in, size_t inlen, @@ -382,6 +457,14 @@ gcm_encryptor_impl::encrypt(uint8_t *out, const uint8_t *in, size_t inlen, log_crypto_abort("gcm_encryptor::write tag"); } +void +gcm_encryptor_noop_impl::encrypt(uint8_t *out, const uint8_t *in, size_t inlen, + const uint8_t *, size_t) +{ + memcpy(out, in, inlen); + memset(out + inlen, 0, 16); +} + int gcm_decryptor_impl::decrypt(uint8_t *out, const uint8_t *in, size_t inlen, const uint8_t *nonce, size_t nlen) @@ -417,6 +500,14 @@ gcm_decryptor_impl::decrypt(uint8_t *out, const uint8_t *in, size_t inlen, return 0; } +int +gcm_decryptor_noop_impl::decrypt(uint8_t *out, const uint8_t *in, size_t inlen, + const uint8_t *, size_t) +{ + memcpy(out, in, inlen - 16); + return 0; +} + // We use the slightly lower-level EC_* / ECDH_* routines for // ecdh_message, instead of the EVP_PKEY_* routines, because we don't // need algorithmic agility, and it means we only have to puzzle out diff --git a/src/crypt.h b/src/crypt.h index 869107a..bd6087d 100644 --- a/src/crypt.h +++ b/src/crypt.h @@ -51,6 +51,11 @@ struct ecb_encryptor 16, 24, or 32 bytes. */ static ecb_encryptor *create(key_generator *gen, size_t keylen); + /** Return a new AES/ECB encryption state that doesn't actually + encrypt anything -- it just copies its input to its output. + For testing purposes only. */ + static ecb_encryptor *create_noop(); + /** Encrypt exactly AES_BLOCK_LEN bytes of data in the buffer 'in' and write the result to 'out'. */ virtual void encrypt(uint8_t *out, const uint8_t *in) = 0; @@ -74,6 +79,11 @@ struct ecb_decryptor 16, 24, or 32 bytes. */ static ecb_decryptor *create(key_generator *gen, size_t keylen); + /** Return a new AES/ECB decryption state that doesn't actually + decrypt anything -- it just copies its input to its output. + For testing purposes only. */ + static ecb_decryptor *create_noop(); + /** Decrypt exactly AES_BLOCK_LEN bytes of data in the buffer 'in' and write the result to 'out'. */ virtual void decrypt(uint8_t *out, const uint8_t *in) = 0; @@ -98,6 +108,11 @@ struct gcm_encryptor 16, 24, or 32 bytes. */ static gcm_encryptor *create(key_generator *gen, size_t keylen); + /** Return a new AES/GCM encryption state that doesn't actually + encrypt anything -- it just copies its input to its output. + For testing purposes only. */ + static gcm_encryptor *create_noop(); + /** Encrypt 'inlen' bytes of data in the buffer 'in', writing the result plus an authentication tag to the buffer 'out', whose length must be at least 'inlen'+16 bytes. Use 'nonce' @@ -125,6 +140,11 @@ struct gcm_decryptor 16, 24, or 32 bytes. */ static gcm_decryptor *create(key_generator *gen, size_t keylen); + /** Return a new AES/GCM decryption state that doesn't actually + decrypt anything -- it just copies its input to its output. + For testing purposes only. */ + static gcm_decryptor *create_noop(); + /** Decrypt 'inlen' bytes of data in the buffer 'in'; the last 16 bytes of this buffer are assumed to be the authentication tag. Write the result to the buffer 'out', whose length must be at diff --git a/src/protocol/chop.cc b/src/protocol/chop.cc index c865c9b..bb8e906 100644 --- a/src/protocol/chop.cc +++ b/src/protocol/chop.cc @@ -388,6 +388,7 @@ struct chop_config_t : config_t vector<steg_config_t *> steg_targets; chop_circuit_table circuits; bool trace_packets; + bool encryption; CONFIG_DECLARE_METHODS(chop); }; @@ -398,6 +399,7 @@ chop_config_t::chop_config_t() { ignore_socks_destination = true; trace_packets = false; + encryption = true; } chop_config_t::~chop_config_t() @@ -445,18 +447,24 @@ chop_config_t::init(int n_options, const char *const *options) } else goto usage; - // if in client mode, accept and ignore --server-key= - if (mode != LSN_SIMPLE_SERVER && - !strncmp(options[1], "--server-key=", 13)) { - options++; - n_options--; - } - - if (!strcmp(options[1], "--trace-packets")) { + while (options[1][0] == '-') { + if (!strncmp(options[1], "--server-key=", 13)) { + // accept and ignore (for now) client only + if (mode == LSN_SIMPLE_SERVER) { + log_warn("chop: --server-key option is not valid in server mode"); + goto usage; + } + } else if (!strcmp(options[1], "--trace-packets")) { + trace_packets = true; + log_enable_timestamps(); + } else if (!strcmp(options[1], "--disable-encryption")) { + encryption = false; + } else { + log_warn("chop: unrecognized option '%s'", options[1]); + goto usage; + } options++; n_options--; - trace_packets = true; - log_enable_timestamps(); } up_address = resolve_address_port(options[1], 1, listen_up, defport); @@ -550,21 +558,37 @@ chop_config_t::circuit_create(size_t) chop_circuit_t *ckt = new chop_circuit_t; ckt->config = this; - key_generator *kgen = - key_generator::from_passphrase((const uint8_t *)passphrase, - sizeof(passphrase) - 1, - 0, 0, 0, 0); + key_generator *kgen = 0; + + if (encryption) + kgen = key_generator::from_passphrase((const uint8_t *)passphrase, + sizeof(passphrase) - 1, + 0, 0, 0, 0); if (mode == LSN_SIMPLE_SERVER) { - ckt->send_crypt = gcm_encryptor::create(kgen, 16); - ckt->send_hdr_crypt = ecb_encryptor::create(kgen, 16); - ckt->recv_crypt = gcm_decryptor::create(kgen, 16); - ckt->recv_hdr_crypt = ecb_decryptor::create(kgen, 16); + if (encryption) { + ckt->send_crypt = gcm_encryptor::create(kgen, 16); + ckt->send_hdr_crypt = ecb_encryptor::create(kgen, 16); + ckt->recv_crypt = gcm_decryptor::create(kgen, 16); + ckt->recv_hdr_crypt = ecb_decryptor::create(kgen, 16); + } else { + ckt->send_crypt = gcm_encryptor::create_noop(); + ckt->send_hdr_crypt = ecb_encryptor::create_noop(); + ckt->recv_crypt = gcm_decryptor::create_noop(); + ckt->recv_hdr_crypt = ecb_decryptor::create_noop(); + } } else { - ckt->recv_crypt = gcm_decryptor::create(kgen, 16); - ckt->recv_hdr_crypt = ecb_decryptor::create(kgen, 16); - ckt->send_crypt = gcm_encryptor::create(kgen, 16); - ckt->send_hdr_crypt = ecb_encryptor::create(kgen, 16); + if (encryption) { + ckt->recv_crypt = gcm_decryptor::create(kgen, 16); + ckt->recv_hdr_crypt = ecb_decryptor::create(kgen, 16); + ckt->send_crypt = gcm_encryptor::create(kgen, 16); + ckt->send_hdr_crypt = ecb_encryptor::create(kgen, 16); + } else { + ckt->recv_crypt = gcm_decryptor::create_noop(); + ckt->recv_hdr_crypt = ecb_decryptor::create_noop(); + ckt->send_crypt = gcm_encryptor::create_noop(); + ckt->send_hdr_crypt = ecb_encryptor::create_noop(); + } std::pair<chop_circuit_table::iterator, bool> out; do {

1 0

[stegotorus/master] Merge branch 'master' of https://git.safdef.isc.org/git/stegotorus
by zwol＠torproject.org 20 Jul '12

20 Jul '12

commit 86f261ebaa45966dea850845b1b09fa36278a469 Merge: f4baab1 e651f0b Author: Zack Weinberg <zackw(a)cmu.edu> Date: Mon Jun 25 17:24:32 2012 -0700 Merge branch 'master' of https://git.safdef.isc.org/git/stegotorus src/steg/http.cc | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

1 0

[stegotorus/master] Fix bug causing connections to remain open forever.
by zwol＠torproject.org 20 Jul '12

20 Jul '12

commit d1f6f3fc8ad3ad9fcc79e239e10ba7f0fe49e37d Author: Zack Weinberg <zackw(a)cmu.edu> Date: Wed Jun 27 18:48:06 2012 -0700 Fix bug causing connections to remain open forever. We failed to record when connections reached 'read EOF' state, so close() never got called. The analogous case for circuits is handled by the protocol. Also fix how the close_cleanup_cb gets activated per advice from libevent maintainer. --- src/connections.cc | 24 ++++++++++-------------- src/network.cc | 8 +++++--- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/src/connections.cc b/src/connections.cc index 99957fe..8f8a8c6 100644 --- a/src/connections.cc +++ b/src/connections.cc @@ -89,6 +89,10 @@ close_cleanup_cb(evutil_socket_t, short, void *arg) { conn_global_state *cgs = (conn_global_state *)arg; + log_debug("cleaning up %lu circuits and %lu connections", + (unsigned long)cgs->closed_circuits.size(), + (unsigned long)cgs->closed_connections.size()); + if (!cgs->closed_circuits.empty()) { unordered_set<circuit_t *> v; v.swap(cgs->closed_circuits); @@ -197,18 +201,14 @@ conn_t::close() if (this->buffer) bufferevent_disable(this->buffer, EV_READ|EV_WRITE); - bool need_event_add = + bool need_event = cgs->closed_connections.empty() && cgs->closed_circuits.empty(); cgs->connections.erase(this); cgs->closed_connections.insert(this); - if (need_event_add) { - struct timeval tv; - tv.tv_sec = 0; - tv.tv_usec = 0; - event_add(cgs->close_cleanup, &tv); - } + if (need_event) + event_active(cgs->close_cleanup, 0, 0); } /** Potentially called during connection construction or destruction. */ @@ -308,18 +308,14 @@ circuit_t::close() if (this->axe_timer) event_del(this->axe_timer); - bool need_event_add = + bool need_event = cgs->closed_connections.empty() && cgs->closed_circuits.empty(); cgs->circuits.erase(this); cgs->closed_circuits.insert(this); - if (need_event_add) { - struct timeval tv; - tv.tv_sec = 0; - tv.tv_usec = 0; - event_add(cgs->close_cleanup, &tv); - } + if (need_event) + event_active(cgs->close_cleanup, 0, 0); } config_t * diff --git a/src/network.cc b/src/network.cc index 7a8f4ce..945fcbf 100644 --- a/src/network.cc +++ b/src/network.cc @@ -389,6 +389,7 @@ downstream_event_cb(struct bufferevent *bev, short what, void *arg) if (what == (BEV_EVENT_EOF|BEV_EVENT_READING)) { /* Peer is done sending us data. */ conn->recv_eof(); + conn->read_eof = true; if (conn->read_eof && conn->write_eof) conn->close(); } else { @@ -435,11 +436,12 @@ downstream_flush_cb(struct bufferevent *bev, void *arg) { conn_t *conn = (conn_t *)arg; size_t remain = evbuffer_get_length(bufferevent_get_output(bev)); - log_debug(conn, "%lu bytes still to transmit%s%s%s%s%s", + log_debug(conn, "%lu bytes still to transmit%s%s%s%s%s%s", (unsigned long)remain, conn->connected ? "" : " (not connected)", - conn->pending_write_eof ? " (received EOF)" : "", - conn->write_eof ? " (at EOF)" : "", + conn->pending_write_eof ? " (reached EOF)" : "", + conn->write_eof ? " (sent EOF)" : "", + conn->read_eof ? " (received EOF)" : "", conn->circuit() ? "" : " (no circuit)", conn->ever_received ? "" : " (never received)");

1 0

[stegotorus/master] Fix hang on shutdown.
by zwol＠torproject.org 20 Jul '12

20 Jul '12

commit 1109030065086668970d05ce15b5d78d4ebb52bb Author: Zack Weinberg <zackw(a)cmu.edu> Date: Sun Jul 8 17:39:20 2012 +0200 Fix hang on shutdown. If all connections and circuits were already closed when conn_start_shutdown was called, close_cleanup_cb would never be called in the proper state to terminate the event loop. Activate the cleanup event explicitly from conn_start_shutdown to prevent this. --- src/connections.cc | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/src/connections.cc b/src/connections.cc index 8f8a8c6..91d9c7d 100644 --- a/src/connections.cc +++ b/src/connections.cc @@ -148,6 +148,11 @@ conn_start_shutdown(int barbaric) (*i)->close(); } } + + /* Make sure close_cleanup_cb is called at least once after this + point; if there were no connections to tear down it might not + otherwise happen. */ + event_active(cgs->close_cleanup, 0, 0); } size_t

1 0

[stegotorus/master] Defer connection and circuit deallocation to a stable point.
by zwol＠torproject.org 20 Jul '12

20 Jul '12

commit f4baab1f5bbd3ae63b457ba82b7d2bea7b54c3cd Author: Zack Weinberg <zackw(a)cmu.edu> Date: Mon Jun 25 17:24:03 2012 -0700 Defer connection and circuit deallocation to a stable point. We were crashing because libevent had internally queued up connection and/or circuit events and would go ahead and fire them even though the associated objects had been deallocated. If we instead disable all events on to-be-deallocated connections/circuits, wait for all of the current batch of events to be processed, and _then_ deallocate, the crash doesn't happen. As a consequence we can no longer test protocol code from the unit test harness; this is no great loss as we were only testing the null protocol that way, and the integration tests cover it adequately. Also fixes a bug in the integration test suite, where we could fail to wait for a subprocess. --- Makefile.am | 5 +- src/audit-globals.sh | 7 +- src/connections.cc | 222 ++++++++++++++++++++++++++++++----------- src/connections.h | 42 ++++++-- src/main.cc | 21 ++-- src/main.h | 9 -- src/network.cc | 35 ++++--- src/protocol.h | 2 + src/protocol/chop.cc | 79 ++++++++------ src/protocol/null.cc | 41 +++----- src/test/itestlib.py | 5 +- src/test/unittest.cc | 99 +------------------ src/test/unittest.h | 40 -------- src/test/unittest_config.cc | 87 ---------------- src/test/unittest_transfer.cc | 114 --------------------- 15 files changed, 292 insertions(+), 516 deletions(-) diff --git a/Makefile.am b/Makefile.am index 513e848..174e09c 100644 --- a/Makefile.am +++ b/Makefile.am @@ -56,11 +56,9 @@ stegotorus_SOURCES = \ UTGROUPS = \ src/test/unittest_base64.cc \ src/test/unittest_compression.cc \ - src/test/unittest_config.cc \ src/test/unittest_crypt.cc \ src/test/unittest_pdfsteg.cc \ - src/test/unittest_socks.cc \ - src/test/unittest_transfer.cc + src/test/unittest_socks.cc unittests_SOURCES = \ src/test/tinytest.cc \ @@ -75,7 +73,6 @@ noinst_HEADERS = \ src/connections.h \ src/crypt.h \ src/listener.h \ - src/main.h \ src/protocol.h \ src/rng.h \ src/socks.h \ diff --git a/src/audit-globals.sh b/src/audit-globals.sh index 7c28f21..2512bb6 100644 --- a/src/audit-globals.sh +++ b/src/audit-globals.sh @@ -29,12 +29,7 @@ sed ' /^compression ZLIB_CEILING$/d /^compression ZLIB_UINT_MAX$/d - /^connections circuits$/d - /^connections closing_all_connections$/d - /^connections connections$/d - /^connections last_ckt_serial$/d - /^connections last_conn_serial$/d - /^connections shutting_down$/d + /^connections cgs$/d /^crypt bctx$/d /^crypt crypto_initialized$/d /^crypt crypto_errs_initialized$/d diff --git a/src/connections.cc b/src/connections.cc index 505299a..99957fe 100644 --- a/src/connections.cc +++ b/src/connections.cc @@ -5,7 +5,6 @@ #include "util.h" #include "connections.h" -#include "main.h" #include "protocol.h" #include "socks.h" @@ -16,75 +15,147 @@ using std::tr1::unordered_set; -/** All active connections. */ -static unordered_set<conn_t *> connections; +static void close_cleanup_cb(evutil_socket_t, short, void *); -/** All active circuits. */ -static unordered_set<circuit_t *> circuits; +namespace { +struct conn_global_state +{ + /** All active connections. */ + unordered_set<conn_t *> connections; + + /** Connections which are to be deallocated after we return to the + event loop. */ + unordered_set<conn_t *> closed_connections; + + /** All active circuits. */ + unordered_set<circuit_t *> circuits; + + /** Circuits which are to be deallocated after we return to the + event loop. */ + unordered_set<circuit_t *> closed_circuits; + + /** The one and only event base used by this program. + Not owned by this object. */ + struct event_base *the_event_base; + + /** Low-priority event which fires when there are connections or + circuits waiting to be deallocated, and all other pending events + have been processed. This ensures that we don't deallocate + connections that have pending events. */ + struct event *close_cleanup; + + /** Most recently assigned serial numbers for connections and circuits. + Note that serial number 0 is never used. These are only used for + debugging messages, so we don't worry about them wrapping around. */ + unsigned int last_conn_serial; + unsigned int last_ckt_serial; + + /** True when stegotorus is shutting down: no further connections or + circuits may be created, and we break out of the event loop when + the last one (of either) is closed. */ + bool shutting_down; + + conn_global_state(struct event_base *evbase); + ~conn_global_state(); +}; + +conn_global_state::conn_global_state(struct event_base *evbase) + : the_event_base(evbase), + close_cleanup(0), + last_conn_serial(0), last_ckt_serial(0), + shutting_down(false) +{ + close_cleanup = evtimer_new(evbase, close_cleanup_cb, this); + log_assert(close_cleanup); + if (event_priority_set(close_cleanup, 1)) + log_abort("failed to demote priority of close-cleanup event"); +} -/** Most recently assigned serial numbers for connections and circuits. - Note that serial number 0 is never used. These are only used for - debugging messages, so we don't worry about them wrapping around. */ -static unsigned int last_conn_serial = 0; -static unsigned int last_ckt_serial = 0; +conn_global_state::~conn_global_state() +{ + log_assert(shutting_down); + log_assert(connections.empty()); + log_assert(closed_connections.empty()); + log_assert(circuits.empty()); + log_assert(closed_circuits.empty()); -/** True when stegotorus is shutting down: no further connections or - circuits may be created, and we break out of the event loop when - the last one (of either) is closed. */ -static bool shutting_down; + event_free(close_cleanup); +} -/** True in the middle of a barbaric connection shutdown; prevents - maybe_finish_shutdown from shutting down too early. */ -static bool closing_all_connections; +} // anonymous namespace static void -maybe_finish_shutdown(void) +close_cleanup_cb(evutil_socket_t, short, void *arg) { - if (!shutting_down || closing_all_connections || - !circuits.empty() || !connections.empty()) + conn_global_state *cgs = (conn_global_state *)arg; + + if (!cgs->closed_circuits.empty()) { + unordered_set<circuit_t *> v; + v.swap(cgs->closed_circuits); + for (unordered_set<circuit_t *>::iterator i = v.begin(); + i != v.end(); i++) + delete *i; + } + if (!cgs->closed_connections.empty()) { + unordered_set<conn_t *> v; + v.swap(cgs->closed_connections); + for (unordered_set<conn_t *>::iterator i = v.begin(); + i != v.end(); i++) + delete *i; + } + + if (!cgs->shutting_down || + !cgs->circuits.empty() || + !cgs->connections.empty()) return; - finish_shutdown(); + log_debug("finishing shutdown"); + event_base_loopexit(cgs->the_event_base, NULL); + delete cgs; + cgs = 0; +} + +static conn_global_state *cgs; + +void +conn_global_init(struct event_base *evbase) +{ + cgs = new conn_global_state(evbase); } void conn_start_shutdown(int barbaric) { - shutting_down = true; + cgs->shutting_down = true; if (barbaric) { - closing_all_connections = true; - - if (!circuits.empty()) { + if (!cgs->circuits.empty()) { unordered_set<circuit_t *> v; - v.swap(circuits); + v.swap(cgs->circuits); for (unordered_set<circuit_t *>::iterator i = v.begin(); i != v.end(); i++) - delete *i; + (*i)->close(); } - if (!connections.empty()) { + if (!cgs->connections.empty()) { unordered_set<conn_t *> v; - v.swap(connections); + v.swap(cgs->connections); for (unordered_set<conn_t *>::iterator i = v.begin(); i != v.end(); i++) - delete *i; + (*i)->close(); } - closing_all_connections = false; } - - maybe_finish_shutdown(); } size_t conn_count(void) { - return connections.size(); + return cgs->connections.size(); } size_t circuit_count(void) { - return circuits.size(); + return cgs->circuits.size(); } /** @@ -96,13 +167,13 @@ conn_create(config_t *cfg, size_t index, { conn_t *conn; - log_assert(!shutting_down); + log_assert(!cgs->shutting_down); conn = cfg->conn_create(index); conn->buffer = buf; conn->peername = peername; - conn->serial = ++last_conn_serial; - connections.insert(conn); + conn->serial = ++cgs->last_conn_serial; + cgs->connections.insert(conn); log_debug(conn, "new connection"); return conn; } @@ -112,16 +183,32 @@ conn_create(config_t *cfg, size_t index, */ conn_t::~conn_t() { - connections.erase(this); - log_debug(this, "closing connection; %lu remaining", - (unsigned long) connections.size()); - if (this->peername) free((void *)this->peername); if (this->buffer) bufferevent_free(this->buffer); +} - maybe_finish_shutdown(); +void +conn_t::close() +{ + log_debug(this, "closing connection; %lu remaining", + (unsigned long) cgs->connections.size()); + if (this->buffer) + bufferevent_disable(this->buffer, EV_READ|EV_WRITE); + + bool need_event_add = + cgs->closed_connections.empty() && cgs->closed_circuits.empty(); + + cgs->connections.erase(this); + cgs->closed_connections.insert(this); + + if (need_event_add) { + struct timeval tv; + tv.tv_sec = 0; + tv.tv_usec = 0; + event_add(cgs->close_cleanup, &tv); + } } /** Potentially called during connection construction or destruction. */ @@ -173,11 +260,7 @@ axe_timer_cb(evutil_socket_t, short, void *arg) circuit_t *ckt = (circuit_t *)arg; log_warn(ckt, "timeout waiting for new connections"); - if (ckt->connected && - evbuffer_get_length(bufferevent_get_output(ckt->up_buffer)) > 0) - circuit_do_flush(ckt); - else - delete ckt; + circuit_do_flush(ckt); } circuit_t * @@ -185,25 +268,21 @@ circuit_create(config_t *cfg, size_t index) { circuit_t *ckt; - log_assert(!shutting_down); + log_assert(!cgs->shutting_down); ckt = cfg->circuit_create(index); - ckt->serial = ++last_ckt_serial; + ckt->serial = ++cgs->last_ckt_serial; if (cfg->mode == LSN_SOCKS_CLIENT) ckt->socks_state = socks_state_new(); - circuits.insert(ckt); + cgs->circuits.insert(ckt); log_debug(ckt, "new circuit"); return ckt; } circuit_t::~circuit_t() { - circuits.erase(this); - log_debug(this, "closing circuit; %lu remaining", - (unsigned long)circuits.size()); - if (this->up_buffer) bufferevent_free(this->up_buffer); if (this->up_peer) @@ -214,8 +293,33 @@ circuit_t::~circuit_t() event_free(this->flush_timer); if (this->axe_timer) event_free(this->axe_timer); +} + +void +circuit_t::close() +{ + log_debug(this, "closing circuit; %lu remaining", + (unsigned long)cgs->circuits.size()); + + if (this->up_buffer) + bufferevent_disable(this->up_buffer, EV_READ|EV_WRITE); + if (this->flush_timer) + event_del(this->flush_timer); + if (this->axe_timer) + event_del(this->axe_timer); + + bool need_event_add = + cgs->closed_connections.empty() && cgs->closed_circuits.empty(); - maybe_finish_shutdown(); + cgs->circuits.erase(this); + cgs->closed_circuits.insert(this); + + if (need_event_add) { + struct timeval tv; + tv.tv_sec = 0; + tv.tv_usec = 0; + event_add(cgs->close_cleanup, &tv); + } } config_t * @@ -241,7 +345,7 @@ circuit_send(circuit_t *ckt) { if (ckt->send()) { log_info(ckt, "error during transmit"); - delete ckt; + ckt->close(); } } @@ -256,10 +360,10 @@ circuit_send_eof(circuit_t *ckt) ckt->pending_read_eof = true; if (ckt->socks_state) { log_debug(ckt, "EOF during SOCKS phase"); - delete ckt; + ckt->close(); } else if (ckt->send_eof()) { log_info(ckt, "error during transmit"); - delete ckt; + ckt->close(); } } diff --git a/src/connections.h b/src/connections.h index ef51c63..39f9984 100644 --- a/src/connections.h +++ b/src/connections.h @@ -35,11 +35,15 @@ struct conn_t { , pending_write_eof(false) {} - /** Close and deallocate a connection. If the connection is part of a - circuit, disconnect it from the circuit; this may cause the circuit - to close as well. */ + /** Deallocate a connection. Normally should not be invoked directly, + use close() instead. */ virtual ~conn_t(); + /** Close a connection and schedule it for deallocation. If the + connection is part of a circuit, disconnect it from the circuit; + this may cause the circuit to close as well. */ + virtual void close(); + /** Return the upstream circuit for this connection, if there is one. NOTE: this is *not* a pure virtual method because it can be called legitimately after the subclass destructor has run. */ @@ -53,15 +57,23 @@ struct conn_t { struct evbuffer *outbound() { return this->buffer ? bufferevent_get_output(this->buffer) : 0; } - /** Create an upstream circuit for this connection, if it is - possible to do so without receiving data from the downstream - peer. If data must be received first, this method should do - nothing (but return success), and the |recv| method is - responsible for creating the upstream circuit when appropriate. - Must return 0 on success, -1 on failure. */ + /** Called immediately after the TCP handshake completes, for + incoming connections to server mode. + + If it is possible to do so without receiving data from the + downstream peer, create an upstream circuit for this connection + here. If data must be received first, this method should do + nothing (but return success), and the |recv| method should + create the upstream circuit when appropriate. */ virtual int maybe_open_upstream() = 0; - /** Perform a connection handshake. Not all protocols have a handshake. */ + /** Called immediately after the TCP handshake completes, for + outgoing connections from client mode. + + If it is necessary to transmit something immediately on new + connections, do so from this method. (It may be more + appropriate to wait until the first time the associated circuit + wishes to transmit data on this connection.) */ virtual int handshake() = 0; /** Receive data from 'source' and pass it upstream (to the circuit). */ @@ -92,6 +104,9 @@ struct conn_t { virtual void transmit_soon(unsigned long timeout) = 0; }; +/** Prepare global connection-related state. Succeeds or crashes. */ +void conn_global_init(struct event_base *); + /** When all currently-open connections and circuits are closed, stop the main event loop and exit the program. If 'barbaric' is true, forcibly close them all now, then stop the event loop. @@ -149,8 +164,15 @@ struct circuit_t { , pending_read_eof(false) , pending_write_eof(false) {} + + /** Deallocate a circuit. Normally should not be invoked directly, + use close() instead. */ virtual ~circuit_t(); + /** Close a circuit and schedule it for deallocation. Will also + disconnect and close all connections that belong to this circuit. */ + virtual void close(); + /** Return the configuration that this circuit belongs to. */ virtual config_t *cfg() const; diff --git a/src/main.cc b/src/main.cc index 42687e1..56e3d81 100644 --- a/src/main.cc +++ b/src/main.cc @@ -4,7 +4,6 @@ */ #include "util.h" -#include "main.h" #include "connections.h" #include "crypt.h" @@ -35,7 +34,6 @@ using std::vector; using std::string; -static struct event_base *the_event_base; static bool allow_kq = false; static bool daemon_mode = false; static string pidfile_name; @@ -65,15 +63,6 @@ start_shutdown(int barbaric, const char *label) conn_start_shutdown(barbaric); /* possibly break existing connections */ } -/** Stop stegotorus's event loop. Final cleanup happens in main(). - Called by conn_start_shutdown and/or conn_free (see connections.c). */ -void -finish_shutdown(void) -{ - log_debug("finishing shutdown"); - event_base_loopexit(the_event_base, NULL); -} - /** This is called when we receive an asynchronous signal. It figures out the signal type and acts accordingly. @@ -440,10 +429,18 @@ main(int, const char *const *argv) /* Possibly worth doing in the future: activating Windows IOCP and telling it how many CPUs to use. */ - the_event_base = event_base_new_with_config(evcfg); + struct event_base *the_event_base = event_base_new_with_config(evcfg); if (!the_event_base) log_abort("failed to initialize networking (evbase)"); + /* Most events are processed at the default priority (0), but + connection cleanup events are processed at low priority (1) + to ensure that all pending I/O is handled first. */ + if (event_base_priority_init(the_event_base, 2)) + log_abort("failed to initialize networking (priority queues)"); + + conn_global_init(the_event_base); + /* ASN should this happen only when SOCKS is enabled? */ if (init_evdns_base(the_event_base)) log_abort("failed to initialize DNS resolver"); diff --git a/src/main.h b/src/main.h deleted file mode 100644 index 5786569..0000000 --- a/src/main.h +++ /dev/null @@ -1,9 +0,0 @@ -/* Copyright 2011 Nick Mathewson, George Kadianakis - * See LICENSE for other credits and copying information - */ -#ifndef MAIN_H -#define MAIN_H - -void finish_shutdown(void); - -#endif diff --git a/src/network.cc b/src/network.cc index 71d5b16..7a8f4ce 100644 --- a/src/network.cc +++ b/src/network.cc @@ -220,14 +220,14 @@ server_listener_cb(struct evconnlistener *, evutil_socket_t fd, /* If appropriate at this point, connect to upstream. */ if (conn->maybe_open_upstream() < 0) { log_debug(conn, "error opening upstream circuit"); - delete conn; + conn->close(); return; } /* Queue handshake, if any. */ if (conn->handshake() < 0) { log_debug(conn, "error during handshake"); - delete conn; + conn->close(); return; } @@ -270,7 +270,7 @@ socks_read_cb(struct bufferevent *bev, void *arg) if (socks_ret == SOCKS_INCOMPLETE) return; /* need to read more data. */ else if (socks_ret == SOCKS_BROKEN) - delete ckt; /* XXXX send socks reply */ + ckt->close(); /* XXXX send socks reply */ else if (socks_ret == SOCKS_CMD_NOT_CONNECT) { bufferevent_enable(bev, EV_WRITE); bufferevent_disable(bev, EV_READ); @@ -314,7 +314,7 @@ downstream_read_cb(struct bufferevent *bev, void *arg) if (down->recv()) { log_debug(down, "error during receive"); - delete down; + down->close(); } } @@ -346,9 +346,9 @@ upstream_event_cb(struct bufferevent *, short what, void *arg) /* Upstream is done sending us data. */ circuit_send_eof(ckt); if (ckt->read_eof && ckt->write_eof) - delete ckt; + ckt->close(); } else { - delete ckt; + ckt->close(); } } else { /* We should never get BEV_EVENT_CONNECTED here. @@ -390,9 +390,9 @@ downstream_event_cb(struct bufferevent *bev, short what, void *arg) /* Peer is done sending us data. */ conn->recv_eof(); if (conn->read_eof && conn->write_eof) - delete conn; + conn->close(); } else { - delete conn; + conn->close(); } } else { /* We should never get BEV_EVENT_CONNECTED here. @@ -423,7 +423,7 @@ upstream_flush_cb(struct bufferevent *bev, void *arg) ckt->write_eof = true; } if (ckt->read_eof && ckt->write_eof) - delete ckt; + ckt->close(); } } @@ -451,7 +451,7 @@ downstream_flush_cb(struct bufferevent *bev, void *arg) conn->write_eof = true; } if (conn->read_eof && conn->write_eof) - delete conn; + conn->close(); } } @@ -515,7 +515,7 @@ downstream_connect_cb(struct bufferevent *bev, short what, void *arg) /* Queue handshake, if any. */ if (conn->handshake() < 0) { log_debug(conn, "error during handshake"); - delete conn; + conn->close(); return; } @@ -567,7 +567,7 @@ downstream_socks_connect_cb(struct bufferevent *bev, short what, void *arg) socks_send_reply(socks, bufferevent_get_output(ckt->up_buffer), err); circuit_do_flush(ckt); } else { - delete ckt; + ckt->close(); } return; } @@ -604,7 +604,7 @@ downstream_socks_connect_cb(struct bufferevent *bev, short what, void *arg) /* Queue handshake, if any. */ if (conn->handshake()) { log_debug(conn, "error during handshake"); - delete conn; + conn->close(); return; } @@ -726,11 +726,11 @@ create_outbound_connections(circuit_t *ckt, bool is_socks) if (n == 0) { log_warn(ckt, "no target addresses available"); - delete ckt; + ckt->close(); } if (any_successes == 0) { log_warn(ckt, "no outbound connections were successful"); - delete ckt; + ckt->close(); } } @@ -785,7 +785,7 @@ create_outbound_connections_socks(circuit_t *ckt) failure: /* XXXX send socks reply */ - delete ckt; + ckt->close(); if (buf) bufferevent_free(buf); } @@ -815,6 +815,7 @@ conn_do_flush(conn_t *conn) if (remain == 0) downstream_flush_cb(conn->buffer, conn); else - log_debug(conn, "flushing %lu bytes to peer [enabled=%x]", (unsigned long)remain, + log_debug(conn, "flushing %lu bytes to peer [enabled=%x]", + (unsigned long)remain, bufferevent_get_enabled(conn->buffer)); } diff --git a/src/protocol.h b/src/protocol.h index a2e467d..874c7ac 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -128,6 +128,7 @@ extern const proto_module *const supported_protos[]; #define CONN_DECLARE_METHODS(mod) \ mod##_conn_t(); \ virtual ~mod##_conn_t(); \ + virtual void close(); \ virtual circuit_t *circuit() const; \ virtual int maybe_open_upstream(); \ virtual int handshake(); \ @@ -149,6 +150,7 @@ extern const proto_module *const supported_protos[]; #define CIRCUIT_DECLARE_METHODS(mod) \ mod##_circuit_t(); \ virtual ~mod##_circuit_t(); \ + virtual void close(); \ virtual config_t *cfg() const; \ virtual void add_downstream(conn_t *); \ virtual void drop_downstream(conn_t *); \ diff --git a/src/protocol/chop.cc b/src/protocol/chop.cc index bb8e906..9109295 100644 --- a/src/protocol/chop.cc +++ b/src/protocol/chop.cc @@ -612,15 +612,15 @@ chop_circuit_t::chop_circuit_t() chop_circuit_t::~chop_circuit_t() { - // Attempt to prevent events from firing on partially or completely - // torn down circuits. (This shouldn't happen, but it seems to.) - if (this->up_buffer) - bufferevent_disable(this->up_buffer, EV_READ|EV_WRITE); - if (this->flush_timer) - event_del(this->flush_timer); - if (this->axe_timer) - event_del(this->axe_timer); + delete send_crypt; + delete send_hdr_crypt; + delete recv_crypt; + delete recv_hdr_crypt; +} +void +chop_circuit_t::close() +{ if (!sent_fin || !received_fin || !upstream_eof) { log_warn(this, "destroying active circuit: fin%c%c eof%c ds=%lu", sent_fin ? '+' : '-', received_fin ? '+' : '-', @@ -632,16 +632,9 @@ chop_circuit_t::~chop_circuit_t() i != downstreams.end(); i++) { chop_conn_t *conn = *i; conn->upstream = NULL; - if (evbuffer_get_length(conn->outbound()) > 0) - conn_do_flush(conn); - else - delete conn; + conn_do_flush(conn); } - - delete send_crypt; - delete send_hdr_crypt; - delete recv_crypt; - delete recv_hdr_crypt; + downstreams.clear(); // The IDs for old circuits are preserved for a while (at present, // indefinitely; FIXME: purge them on a timer) against the @@ -657,6 +650,8 @@ chop_circuit_t::~chop_circuit_t() log_assert(out != config->circuits.end()); log_assert(out->second == this); out->second = NULL; + + circuit_t::close(); } config_t * @@ -707,12 +702,7 @@ chop_circuit_t::drop_downstream(chop_conn_t *conn) // to enable further transmissions from the server. if (downstreams.empty()) { if (sent_fin && received_fin) { - if (evbuffer_get_length(bufferevent_get_output(up_buffer)) > 0) - // this may already have happened, but there's no harm in - // doing it again - circuit_do_flush(this); - else - delete this; + circuit_do_flush(this); } else if (config->mode == LSN_SIMPLE_SERVER) { circuit_arm_axe_timer(this, axe_interval()); } else { @@ -1126,6 +1116,7 @@ chop_circuit_t::check_for_eof() // If we're at EOF both ways, close all connections, sending first // if necessary. if (sent_fin && received_fin) { + log_debug(this, "sent and received FIN"); circuit_disarm_flush_timer(this); for (unordered_set<chop_conn_t *>::iterator i = downstreams.begin(); i != downstreams.end(); i++) { @@ -1138,8 +1129,12 @@ chop_circuit_t::check_for_eof() // If we're the client we have to keep trying to talk as long as we // haven't both sent and received a FIN, or we might deadlock. - else if (config->mode != LSN_SIMPLE_SERVER) + else if (config->mode != LSN_SIMPLE_SERVER) { + log_debug(this, "client arming flush timer%s%s", + sent_fin ? " (sent FIN)" : "", + received_fin ? " (received FIN)": ""); circuit_arm_flush_timer(this, flush_interval()); + } return 0; } @@ -1167,20 +1162,25 @@ chop_conn_t::chop_conn_t() chop_conn_t::~chop_conn_t() { - // Attempt to prevent events from firing on partially or completely - // torn down connections. (This shouldn't happen, but it seems to.) - if (this->buffer) - bufferevent_disable(this->buffer, EV_READ|EV_WRITE); - if (this->must_send_timer) event_free(this->must_send_timer); - if (upstream) - upstream->drop_downstream(this); if (steg) delete steg; evbuffer_free(recv_pending); } +void +chop_conn_t::close() +{ + if (this->must_send_timer) + event_del(this->must_send_timer); + + if (upstream) + upstream->drop_downstream(this); + + conn_t::close(); +} + circuit_t * chop_conn_t::circuit() const { @@ -1266,7 +1266,7 @@ chop_conn_t::recv_handshake() } if (circuit_open_upstream(ck)) { log_warn(this, "failed to begin upstream connection"); - delete ck; + ck->close(); return -1; } log_debug(this, "created new circuit to %s", ck->up_peer); @@ -1290,7 +1290,19 @@ chop_conn_t::recv() return 0; if (!upstream) { - // Try to receive a handshake. + if (config->mode != LSN_SIMPLE_SERVER) { + // We're the client. Client connections start out attached to a + // circuit; therefore this is a server-to-client message that + // crossed with the teardown of the circuit it belonged to, and + // we don't have the decryption keys for it anymore. + // By construction it must be chaff, so just throw it away. + log_debug(this, "discarding chaff after circuit closed"); + log_assert(!must_send_p()); + conn_do_flush(this); + return 0; + } + + // We're the server. Try to receive a handshake. if (recv_handshake()) return -1; @@ -1304,7 +1316,6 @@ chop_conn_t::recv() // connection, possibly sending a response if the cover protocol // requires one. if (!upstream) { - evbuffer_drain(recv_pending, evbuffer_get_length(recv_pending)); if (must_send_p()) send(); conn_do_flush(this); diff --git a/src/protocol/null.cc b/src/protocol/null.cc index e195cc9..461fffe 100644 --- a/src/protocol/null.cc +++ b/src/protocol/null.cc @@ -135,24 +135,20 @@ null_circuit_t::null_circuit_t() null_circuit_t::~null_circuit_t() { - // Attempt to prevent events from firing on partially or completely - // torn down circuits. (This shouldn't happen, but it seems to.) - if (this->up_buffer) - bufferevent_disable(this->up_buffer, EV_READ|EV_WRITE); - if (this->flush_timer) - event_del(this->flush_timer); - if (this->axe_timer) - event_del(this->axe_timer); +} +void +null_circuit_t::close() +{ if (downstream) { /* break the circular reference before deallocating the downstream connection */ downstream->upstream = NULL; - if (evbuffer_get_length(downstream->outbound()) > 0) - conn_do_flush(downstream); - else - delete downstream; + conn_do_flush(downstream); + downstream = NULL; } + + circuit_t::close(); } config_t * @@ -192,13 +188,7 @@ null_circuit_t::drop_downstream(conn_t *cn) this->serial, conn->serial, conn->peername); this->downstream = NULL; conn->upstream = NULL; - - if (evbuffer_get_length(bufferevent_get_output(this->up_buffer)) > 0) - /* this may already have happened, but there's no harm in - doing it again */ - circuit_do_flush(this); - else - delete this; + circuit_do_flush(this); } /* Send data from the upstream buffer. */ @@ -206,7 +196,8 @@ int null_circuit_t::send() { log_debug(this, "sending %lu bytes", - (unsigned long)evbuffer_get_length(bufferevent_get_input(this->up_buffer))); + (unsigned long) + evbuffer_get_length(bufferevent_get_input(this->up_buffer))); return evbuffer_add_buffer(this->downstream->outbound(), bufferevent_get_input(this->up_buffer)); @@ -242,13 +233,15 @@ null_conn_t::null_conn_t() null_conn_t::~null_conn_t() { - // Attempt to prevent events from firing on partially or completely - // torn down connections. (This shouldn't happen, but it seems to.) - if (this->buffer) - bufferevent_disable(this->buffer, EV_READ|EV_WRITE); +} +void +null_conn_t::close() +{ if (this->upstream) this->upstream->drop_downstream(this); + + conn_t::close(); } /* Only used by connection callbacks */ diff --git a/src/test/itestlib.py b/src/test/itestlib.py index 7378c3d..6906e5c 100644 --- a/src/test/itestlib.py +++ b/src/test/itestlib.py @@ -97,9 +97,10 @@ class Stegotorus(subprocess.Popen): def check_completion(self, label, force_stderr=False): self.stdin.close() self.communicator.join() - self.timeout.cancel() + if self.poll() is not None: + self.timeout.cancel() self.timeout.join() - self.poll() + self.wait() report = "" diff --git a/src/test/unittest.cc b/src/test/unittest.cc index 958c82c..08383dc 100644 --- a/src/test/unittest.cc +++ b/src/test/unittest.cc @@ -4,95 +4,8 @@ */ #include "util.h" -#include "unittest.h" -#include "main.h" - #include "crypt.h" -#include "connections.h" -#include "protocol.h" - -#include <event2/event.h> -#include <event2/bufferevent.h> - -/* Generic test fixture for protocol tests (currently used by transfer). */ - -static void * -setup_proto_test_state(const struct testcase_t *tcase) -{ - struct proto_test_state *s = - (struct proto_test_state *)xzalloc(sizeof(struct proto_test_state)); - const struct proto_test_args *args = - (struct proto_test_args *)tcase->setup_data; - struct bufferevent *pairs[3][2]; - int i; - - s->args = args; - s->base = event_base_new(); - - for (i = 0; i < 3; i++) { - bufferevent_pair_new(s->base, 0, pairs[i]); - bufferevent_enable(pairs[i][0], EV_READ|EV_WRITE); - bufferevent_enable(pairs[i][1], EV_READ|EV_WRITE); - } - - s->cfg_client = config_create(args->nopts_client, args->opts_client); - s->cfg_server = config_create(args->nopts_server, args->opts_server); - s->cfg_client->base = s->base; - s->cfg_server->base = s->base; - - s->conn_client = conn_create(s->cfg_client, 0, pairs[0][0], - xstrdup("to-server")); - s->conn_server = conn_create(s->cfg_server, 0, pairs[0][1], - xstrdup("to-client")); - - s->buf_client = pairs[1][0]; - s->buf_server = pairs[2][0]; - - s->ckt_client = circuit_create(s->cfg_client, 0); - s->ckt_server = circuit_create(s->cfg_server, 0); - - circuit_add_upstream(s->ckt_client, pairs[1][1], - xstrdup("to-harness-client")); - circuit_add_upstream(s->ckt_server, pairs[2][1], - xstrdup("to-harness-server")); - - s->ckt_client->add_downstream(s->conn_client); - s->ckt_server->add_downstream(s->conn_server); - - return s; -} - -static int -cleanup_proto_test_state(const struct testcase_t *, void *state) -{ - struct proto_test_state *s = (struct proto_test_state *)state; - - /* We don't want to trigger circuit_*_shutdown, so dissociate the circuits - from their connections and close each separately. */ - s->ckt_client->drop_downstream(s->conn_client); - s->ckt_server->drop_downstream(s->conn_server); - - delete s->conn_client; - delete s->conn_server; - - delete s->cfg_client; - delete s->cfg_server; - - bufferevent_free(s->buf_client); - bufferevent_free(s->buf_server); - event_base_free(s->base); - - free(state); - return 1; -} - -const struct testcase_setup_t proto_test_fixture = - { setup_proto_test_state, cleanup_proto_test_state }; - -void -finish_shutdown(void) -{ -} +#include "unittest.h" int main(int argc, const char **argv) @@ -110,18 +23,8 @@ main(int argc, const char **argv) init_crypto(); - /* Ugly method to fix a Windows problem: - http://archives.seul.org/libevent/users/Oct-2010/msg00049.html */ -#ifdef _WIN32 - { - WSADATA wsaData; - WSAStartup(0x101, &wsaData); - } -#endif - rv = tinytest_main(argc, argv, unittest_groups); - conn_start_shutdown(1); free_crypto(); return rv; diff --git a/src/test/unittest.h b/src/test/unittest.h index 81c873a..b767d0b 100644 --- a/src/test/unittest.h +++ b/src/test/unittest.h @@ -8,46 +8,6 @@ #include "tinytest_macros.h" -/* Test fixture shared by most protocol tests. */ - -struct proto_test_state -{ - struct event_base *base; - struct bufferevent *buf_client; - struct bufferevent *buf_server; - - config_t *cfg_client; - config_t *cfg_server; - - circuit_t *ckt_client; - circuit_t *ckt_server; - - conn_t *conn_client; - conn_t *conn_server; - - const struct proto_test_args *args; -}; - -extern const struct testcase_setup_t proto_test_fixture; - -/* Any test case that uses the above fixture must provide one of these - as its setup_data. */ -struct proto_test_args -{ - /* These fields are mandatory. */ - size_t nopts_client; - size_t nopts_server; - const char *const *opts_client; - const char *const *opts_server; - - /* These fields are only used by "transfer" test cases and may be 0/NULL - otherwise. */ - size_t len_c2s_on_wire; - size_t len_s2c_on_wire; - const char *c2s_on_wire; - const char *s2c_on_wire; -}; - /* Master group list - defined in unitgrplist.c (which is generated). */ extern const struct testgroup_t unittest_groups[]; diff --git a/src/test/unittest_config.cc b/src/test/unittest_config.cc deleted file mode 100644 index 72b39bd..0000000 --- a/src/test/unittest_config.cc +++ /dev/null @@ -1,87 +0,0 @@ -/* Copyright 2011 Nick Mathewson, George Kadianakis - * Copyright 2011 SRI International - * See LICENSE for other credits and copying information - */ - -#include "util.h" -#include "unittest.h" - -#include "protocol.h" - -struct option_parsing_case { - config_t *result; - short should_succeed; - short n_opts; - const char *const opts[6]; -}; - -static void -test_config(void *cases) -{ - struct option_parsing_case *c; - for (c = (struct option_parsing_case *)cases; c->n_opts; c++) { - c->result = config_create(c->n_opts, c->opts); - if (c->should_succeed) - tt_ptr_op(c->result, !=, NULL); - else - tt_ptr_op(c->result, ==, NULL); - } - end:; -} - -static void * -setup_test_config(const struct testcase_t *tc) -{ - /* Suppress logs for the duration of this test. */ - log_set_method(LOG_METHOD_NULL, NULL); - - /* Forward the test data to the actual test function. */ - return tc->setup_data; -} - -static int -cleanup_test_config(const struct testcase_t *, void *state) -{ - struct option_parsing_case *c; - for (c = (struct option_parsing_case *)state; c->n_opts; c++) - if (c->result) - delete c->result; - - /* Reactivate logging */ - log_set_method(LOG_METHOD_STDERR, NULL); - return 1; -} - -static const struct testcase_setup_t config_fixture = - { setup_test_config, cleanup_test_config }; - -static struct option_parsing_case oc_null[] = { - /* wrong number of options */ - { 0, 0, 1, {"null"} }, - { 0, 0, 2, {"null", "client"} }, - { 0, 0, 3, {"null", "client", "127.0.0.1:5552"} }, - { 0, 0, 3, {"null", "server", "127.0.0.1:5552"} }, - { 0, 0, 4, {"null", "socks", "127.0.0.1:5552", "192.168.1.99:11253"} }, - /* unrecognized mode */ - { 0, 0, 3, {"null", "floodcontrol", "127.0.0.1:5552" } }, - { 0, 0, 4, {"null", "--frobozz", "client", "127.0.0.1:5552"} }, - { 0, 0, 4, {"null", "client", "--frobozz", "127.0.0.1:5552"} }, - /* bad address */ - { 0, 0, 3, {"null", "socks", "@:5552"} }, - { 0, 0, 3, {"null", "socks", "127.0.0.1:notanumber"} }, - /* should succeed */ - { 0, 1, 4, {"null", "client", "127.0.0.1:5552", "192.168.1.99:11253" } }, - { 0, 1, 4, {"null", "client", "127.0.0.1", "192.168.1.99:11253" } }, - { 0, 1, 4, {"null", "server", "127.0.0.1:5552", "192.168.1.99:11253" } }, - { 0, 1, 3, {"null", "socks", "127.0.0.1:5552" } }, - - { 0, 0, 0, {0} } -}; - -#define T(name) \ - { #name, test_config, 0, &config_fixture, oc_##name } - -struct testcase_t config_tests[] = { - T(null), - END_OF_TESTCASES -}; diff --git a/src/test/unittest_transfer.cc b/src/test/unittest_transfer.cc deleted file mode 100644 index de17ed0..0000000 --- a/src/test/unittest_transfer.cc +++ /dev/null @@ -1,114 +0,0 @@ -/* Copyright 2011 Nick Mathewson, George Kadianakis - * Copyright 2011 SRI International - * See LICENSE for other credits and copying information - */ - -#include "util.h" -#include "unittest.h" -#include "connections.h" - -#include <event2/buffer.h> - -static const char msg1[] = - "this is a 54-byte message passed from client to server"; -static const char msg2[] = - "this is a 55-byte message passed from server to client!"; - -#define SLEN(s) (sizeof(s)-1) - -static void -test_transfer(void *state) -{ - struct proto_test_state *s = (struct proto_test_state *)state; - const struct proto_test_args *a = s->args; - - /* Handshake */ - tt_int_op(0, ==, s->conn_client->handshake()); - tt_int_op(0, ==, s->conn_server->recv()); - tt_int_op(0, ==, s->conn_server->handshake()); - tt_int_op(0, ==, s->conn_client->recv()); - /* End of Handshake */ - - /* client -> server */ - evbuffer_add(bufferevent_get_output(s->buf_client), msg1, SLEN(msg1)); - circuit_send(s->ckt_client); - tt_int_op(0, ==, evbuffer_get_length(bufferevent_get_output(s->buf_client))); - tt_int_op(a->len_c2s_on_wire, ==, - evbuffer_get_length(s->conn_server->inbound())); - tt_mem_op(a->c2s_on_wire, ==, - evbuffer_pullup(s->conn_server->inbound(), - a->len_c2s_on_wire), - a->len_c2s_on_wire); - - s->conn_server->recv(); - tt_int_op(0, ==, evbuffer_get_length(s->conn_server->inbound())); - tt_int_op(SLEN(msg1), ==, - evbuffer_get_length(bufferevent_get_input(s->buf_server))); - tt_mem_op(msg1, ==, - evbuffer_pullup(bufferevent_get_input(s->buf_server), SLEN(msg1)), - SLEN(msg1)); - - /* server -> client */ - evbuffer_add(bufferevent_get_output(s->buf_server), msg2, SLEN(msg2)); - circuit_send(s->ckt_server); - tt_int_op(0, ==, evbuffer_get_length(bufferevent_get_output(s->buf_server))); - tt_int_op(a->len_s2c_on_wire, ==, - evbuffer_get_length(s->conn_client->inbound())); - tt_mem_op(a->s2c_on_wire, ==, - evbuffer_pullup(s->conn_client->inbound(), - a->len_s2c_on_wire), - a->len_s2c_on_wire); - - s->conn_client->recv(); - tt_int_op(0, ==, evbuffer_get_length(s->conn_client->inbound())); - tt_int_op(SLEN(msg2), ==, - evbuffer_get_length(bufferevent_get_input(s->buf_client))); - tt_mem_op(msg2, ==, - evbuffer_pullup(bufferevent_get_input(s->buf_client), SLEN(msg2)), - SLEN(msg2)); - - end:; -} - -#define enc1_null msg1 -#define enc2_null msg2 - -#if 0 /* temporarily disabled - causes crashes */ -static const char enc1_s_x_http[] = - "GET /003600007468697320697320612035342d62797465206d6573736167652070617" - "37365642066726f6d20636c69656e7420746f2073657276657200== HTTP/1.1\r\n" - "Host: to-server\r\n" - "Connection: close\r\n\r\n"; -static const char enc2_s_x_http[] = - "HTTP/1.1 200 OK\r\n" - "Expires: Thu, 01 Jan 1970 00:00:00 GMT\r\n" - "Cache-Control: no-store\r\n" - "Connection: close\r\n" - "Content-Type: application/octet-stream\r\n" - "Content-Length: 60\r\n\r\n" - "\x00\x37\x00\x00" - "this is a 55-byte message passed from server to client!\x00"; -#endif - -static const char *const o_client_null[] = - {"null", "socks", "127.0.0.1:1800"}; - -static const char *const o_server_null[] = - {"null", "server", "127.0.0.1:1800", "127.0.0.1:1801"}; - -#define TA(name) \ - static const struct proto_test_args tr_##name##_args = \ - { ALEN(o_client_##name), ALEN(o_server_##name), \ - o_client_##name, o_server_##name, \ - SLEN(enc1_##name), SLEN(enc2_##name), \ - enc1_##name, enc2_##name } - -TA(null); - -#define T(name) \ - { #name, test_transfer, 0, &proto_test_fixture, (void *)&tr_##name##_args } - -struct testcase_t transfer_tests[] = { - T(null), - END_OF_TESTCASES -};

1 0

[stegotorus/master] Fix long-standing cut-and-paste error in name of one test.
by zwol＠torproject.org 20 Jul '12

20 Jul '12

commit f7ad87f180ea30777b4bc356dc444b53df3285a5 Author: Zack Weinberg <zackw(a)cmu.edu> Date: Sun Jul 8 18:36:32 2012 +0200 Fix long-standing cut-and-paste error in name of one test. --- src/test/test_tl.py | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/test/test_tl.py b/src/test/test_tl.py index 46e5d5a..903a727 100644 --- a/src/test/test_tl.py +++ b/src/test/test_tl.py @@ -57,7 +57,7 @@ class TimelineTest(object): "127.0.0.1:5010","nosteg", )) - def test_chop_null2(self): + def test_chop_nosteg2(self): self.doTest("chop", ("chop", "server", "127.0.0.1:5001", "127.0.0.1:5010","nosteg","127.0.0.1:5011","nosteg",

1 0

[stegotorus/master] Break up chop.cc in preparation for adding more control blocks.
by zwol＠torproject.org 20 Jul '12

20 Jul '12

commit 17a1e39d73c4767bdc4c6a89a30b5378a4f0a0b9 Author: Zack Weinberg <zackw(a)cmu.edu> Date: Sun Jul 8 18:39:39 2012 +0200 Break up chop.cc in preparation for adding more control blocks. All of the header formatting code and the reassembly queue move to their own file. Some of the low-level transmission and reception code may follow. This will make it easier to add new types of control blocks (for retransmit, in-band connection close, the long-awaited rekeying and handshake logic, etc) and will also make it possible to unit test the separated code. --- Makefile.am | 2 + src/protocol/chop.cc | 292 +--------------------------------------------- src/protocol/chop_blk.cc | 104 ++++++++++++++++ src/protocol/chop_blk.h | 247 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 358 insertions(+), 287 deletions(-) diff --git a/Makefile.am b/Makefile.am index 174e09c..b449619 100644 --- a/Makefile.am +++ b/Makefile.am @@ -14,6 +14,7 @@ bin_PROGRAMS = stegotorus PROTOCOLS = \ src/protocol/chop.cc \ + src/protocol/chop_blk.cc \ src/protocol/null.cc STEGANOGRAPHERS = \ @@ -78,6 +79,7 @@ noinst_HEADERS = \ src/socks.h \ src/steg.h \ src/util.h \ + src/protocol/chop_blk.h \ src/test/tinytest.h \ src/test/tinytest_macros.h \ src/test/unittest.h diff --git a/src/protocol/chop.cc b/src/protocol/chop.cc index 9109295..9e3a605 100644 --- a/src/protocol/chop.cc +++ b/src/protocol/chop.cc @@ -1,10 +1,10 @@ -/* Copyright 2011, SRI International +/* Copyright 2011, 2012 SRI International * See LICENSE for other credits and copying information */ #include "util.h" +#include "chop_blk.h" #include "connections.h" -#include "crypt.h" #include "protocol.h" #include "rng.h" #include "steg.h" @@ -14,7 +14,6 @@ #include <vector> #include <event2/event.h> -#include <event2/buffer.h> /* The chopper is the core StegoTorus protocol implementation. For its design, see doc/chopper.txt. Note that it is still @@ -25,286 +24,7 @@ using std::tr1::unordered_set; using std::vector; using std::make_pair; -namespace -{ - -/* Packets on the wire have a 16-byte header, consisting of a 32-bit - sequence number, two 16-bit length fields ("D" and "P"), an 8-bit - opcode ("F"), and a 56-bit check field. All numbers in this header - are serialized in network byte order. - - | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | A | B | C | D | E | F | - |Sequence Number| D | P | F | Check | - - The header is encrypted with AES in ECB mode: this is safe because - the header is exactly one AES block long, the sequence number is - never repeated, the header-encryption key is not used for anything - else, and the high 24 bits of the sequence number, plus the check - field, constitute an 80-bit MAC. The receiver maintains a - 256-element sliding window of acceptable sequence numbers, which - begins one after the highest sequence number so far _processed_ - (not received). If the sequence number is outside this window, or - the check field is not all-bits-zero, the packet is discarded. An - attacker's odds of being able to manipulate the D, P, or F fields - or the low bits of the sequence number are therefore less than one - in 2^80. Unlike TCP, our sequence numbers always start at zero on - a new (or freshly rekeyed) circuit, and increment by one per - _block_, not per byte of data. Furthermore, they do not wrap: a - rekeying cycle (which resets the sequence number) is required to - occur before the highest-received sequence number reaches 2^32. - - Following the header are two variable-length payload sections, - "data" and "padding", whose length in bytes are given by the D and - P fields, respectively. These sections are encrypted, using a - different key, with AES in GCM mode. The *encrypted* packet header - doubles as the GCM nonce. The semantics of the "data" section's - contents, if any, are defined by the opcode F. The "padding" - section SHOULD be filled with zeroes by the sender; regardless, its - contents MUST be ignored by the receiver. Following these sections - is a 16-byte GCM authentication tag, computed over the data and - padding sections only, NOT the message header. */ - -const size_t HEADER_LEN = 16; -const size_t TRAILER_LEN = 16; -const size_t SECTION_LEN = UINT16_MAX; -const size_t MIN_BLOCK_SIZE = HEADER_LEN + TRAILER_LEN; -const size_t MAX_BLOCK_SIZE = MIN_BLOCK_SIZE + SECTION_LEN*2; - -const size_t HANDSHAKE_LEN = sizeof(uint32_t); - -enum opcode_t -{ - op_DAT = 0, // Pass data section along to upstream - op_FIN = 1, // No further transmissions (pass data along if any) - op_RST = 2, // Protocol error, close circuit now - op_RESERVED0 = 3, // 3 -- 127 reserved for future definition - op_STEG0 = 128, // 128 -- 255 reserved for steganography modules - op_LAST = 255 -}; - -static const char * -opname(opcode_t o, char fallbackbuf[4]) -{ - switch (o) { - case op_DAT: return "DAT"; - case op_FIN: return "FIN"; - case op_RST: return "RST"; - default: { - unsigned int x = o; - if (x < op_STEG0) - xsnprintf(fallbackbuf, sizeof fallbackbuf, "R%02x", x); - else - xsnprintf(fallbackbuf, sizeof fallbackbuf, "S%02x", x - op_STEG0); - return fallbackbuf; - } - } -} - -class block_header -{ - uint8_t clear[16]; - uint8_t ciphr[16]; - -public: - block_header(uint32_t s, uint16_t d, uint16_t p, opcode_t f, - ecb_encryptor &ec) - { - if (f > op_LAST || (f >= op_RESERVED0 && f < op_STEG0)) { - memset(clear, 0xFF, sizeof clear); // invalid! - memset(ciphr, 0xFF, sizeof ciphr); - return; - } - - // sequence number - clear[0] = (s >> 24) & 0xFF; - clear[1] = (s >> 16) & 0xFF; - clear[2] = (s >> 8) & 0xFF; - clear[3] = (s ) & 0xFF; - - // D field - clear[4] = (d >> 8) & 0xFF; - clear[5] = (d ) & 0xFF; - - // P field - clear[6] = (p >> 8) & 0xFF; - clear[7] = (p ) & 0xFF; - - // F field - clear[8] = uint8_t(f); - - // Check field - memset(clear + 9, 0, 7); - - ec.encrypt(ciphr, clear); - } - - block_header(evbuffer *buf, ecb_decryptor &dc) - { - if (evbuffer_copyout(buf, ciphr, sizeof ciphr) != sizeof ciphr) { - memset(clear, 0xFF, sizeof clear); - memset(ciphr, 0xFF, sizeof ciphr); - return; - } - dc.decrypt(clear, ciphr); - } - - uint32_t seqno() const - { - return ((uint32_t(clear[0]) << 24) | - (uint32_t(clear[1]) << 16) | - (uint32_t(clear[2]) << 8) | - (uint32_t(clear[3]) )); - - } - - size_t dlen() const - { - return ((uint16_t(clear[4]) << 8) | - (uint16_t(clear[5]) )); - } - - size_t plen() const - { - return ((uint16_t(clear[6]) << 8) | - (uint16_t(clear[7]) )); - } - - size_t total_len() const - { - return HEADER_LEN + TRAILER_LEN + dlen() + plen(); - } - - opcode_t opcode() const - { - return opcode_t(clear[8]); - } - - bool valid(uint64_t window) const - { - // This check must run in constant time. - uint8_t ck = (clear[ 9] | clear[10] | clear[11] | clear[12] | - clear[13] | clear[14] | clear[15]); - uint32_t delta = seqno() - window; - ck |= !!(delta & ~uint32_t(0xFF)); - return !ck; - } - - const uint8_t *nonce() const - { - return ciphr; - } - - const uint8_t *cleartext() const - { - return clear; - } -}; - -/* Most of a block's header information is processed before it reaches - the reassembly queue; the only things the queue needs to record are - the sequence number (which is stored implictly), the opcode, and an - evbuffer holding the data section. Zero-data blocks still get an - evbuffer, for simplicity's sake: a reassembly queue element holds a - received block if and only if its data pointer is non-null. - - The reassembly queue is a 256-element circular buffer of - 'reassembly_elt' structs. This corresponds to the 256-element - sliding window of sequence numbers which may legitimately be - received at any time. */ - -struct reassembly_elt -{ - evbuffer *data; - opcode_t op; -}; - -class reassembly_queue -{ - reassembly_elt cbuf[256]; - uint32_t next_to_process; - - reassembly_queue(const reassembly_queue&) DELETE_METHOD; - reassembly_queue& operator=(const reassembly_queue&) DELETE_METHOD; - -public: - reassembly_queue() - : next_to_process(0) - { - memset(cbuf, 0, sizeof cbuf); - } - - ~reassembly_queue() - { - for (int i = 0; i < 256; i++) - if (cbuf[i].data) - evbuffer_free(cbuf[i].data); - } - - // Remove the next block to be processed from the reassembly queue - // and return it. If we are out of blocks or the next block to - // process has not yet arrived, return an empty reassembly_elt. - // Caller is responsible for freeing the evbuffer in the - // reassembly_elt, if any. - reassembly_elt - remove_next() - { - reassembly_elt rv = { 0, op_DAT }; - uint8_t front = next_to_process & 0xFF; - log_debug("next_to_process=%d data=%p op=%02x", - next_to_process, cbuf[front].data, cbuf[front].op); - if (cbuf[front].data) { - rv = cbuf[front]; - cbuf[front].data = 0; - cbuf[front].op = op_DAT; - next_to_process++; - } - return rv; - } - - // Insert a block into the reassembly queue at sequence number - // SEQNO, with opcode OP and data section DATA. Returns true if the - // block was successfully added to the queue, false if it is either - // outside the acceptable window or duplicates a block already on - // the queue (both of these cases indicate protocol errors). - // DATA is consumed no matter what the return value is. - bool - insert(uint32_t seqno, opcode_t op, evbuffer *data, conn_t *conn) - { - if (seqno - window() > 255) { - log_info(conn, "block outside receive window"); - evbuffer_free(data); - return false; - } - uint8_t front = next_to_process & 0xFF; - uint8_t pos = front + (seqno - window()); - if (cbuf[pos].data) { - log_info(conn, "duplicate block"); - evbuffer_free(data); - return false; - } - - cbuf[pos].data = data; - cbuf[pos].op = op; - return true; - } - - // Return the current lowest acceptable sequence number in the - // receive window. This is the value to be passed to - // block_header::valid(). - uint32_t window() const { return next_to_process; } - - // As the last step of a rekeying cycle, the expected next sequence number - // is reset to zero. - void reset() - { - for (int i = 0; i < 256; i++) { - log_assert(!cbuf[i].data); - } - next_to_process = 0; - } -}; - -// Protocol objects +using namespace chop_blk; struct chop_config_t; struct chop_circuit_t; @@ -883,7 +603,7 @@ chop_circuit_t::send_targeted(chop_conn_t *conn, size_t d, size_t p, opcode_t f, } v.iov_len = blocksize; - block_header hdr(send_seq, d, p, f, *send_hdr_crypt); + header hdr(send_seq, d, p, f, *send_hdr_crypt); log_assert(hdr.valid(send_seq)); memcpy(v.iov_base, hdr.nonce(), HEADER_LEN); @@ -1335,7 +1055,7 @@ chop_conn_t::recv() break; } - block_header hdr(recv_pending, *upstream->recv_hdr_crypt); + header hdr(recv_pending, *upstream->recv_hdr_crypt); if (!hdr.valid(upstream->recv_queue.window())) { const uint8_t *c = hdr.cleartext(); char fallbackbuf[4]; @@ -1543,8 +1263,6 @@ chop_conn_t::must_send_timeout(evutil_socket_t, short, void *arg) static_cast<chop_conn_t *>(arg)->send(); } -} // anonymous namespace - PROTO_DEFINE_MODULE(chop); // Local Variables: diff --git a/src/protocol/chop_blk.cc b/src/protocol/chop_blk.cc new file mode 100644 index 0000000..460097d --- /dev/null +++ b/src/protocol/chop_blk.cc @@ -0,0 +1,104 @@ +/* Copyright 2011, 2012 SRI International + * See LICENSE for other credits and copying information + */ + +#include "util.h" +#include "chop_blk.h" + +/* The chopper is the core StegoTorus protocol implementation. + For its design, see doc/chopper.txt. Note that it is still + being implemented, and may change incompatibly. */ + +namespace chop_blk +{ + +const char * +opname(opcode_t o, char fallbackbuf[4]) +{ + switch (o) { + case op_DAT: return "DAT"; + case op_FIN: return "FIN"; + case op_RST: return "RST"; + default: { + unsigned int x = o; + if (x < op_STEG0) + xsnprintf(fallbackbuf, sizeof fallbackbuf, "R%02x", x); + else + xsnprintf(fallbackbuf, sizeof fallbackbuf, "S%02x", x - op_STEG0); + return fallbackbuf; + } + } +} + +reassembly_queue::reassembly_queue() + : next_to_process(0) +{ + memset(cbuf, 0, sizeof cbuf); +} + +reassembly_queue::~reassembly_queue() +{ + for (int i = 0; i < 256; i++) + if (cbuf[i].data) + evbuffer_free(cbuf[i].data); +} + +reassembly_elt +reassembly_queue::remove_next() +{ + reassembly_elt rv = { 0, op_DAT }; + uint8_t front = next_to_process & 0xFF; + char fallbackbuf[4]; + + log_debug("next_to_process=%d data=%p op=%s", + next_to_process, cbuf[front].data, + opname(cbuf[front].op, fallbackbuf)); + + if (cbuf[front].data) { + rv = cbuf[front]; + cbuf[front].data = 0; + cbuf[front].op = op_DAT; + next_to_process++; + } + return rv; +} + +bool +reassembly_queue::insert(uint32_t seqno, opcode_t op, + evbuffer *data, conn_t *conn) +{ + if (seqno - window() > 255) { + log_info(conn, "block outside receive window"); + evbuffer_free(data); + return false; + } + uint8_t front = next_to_process & 0xFF; + uint8_t pos = front + (seqno - window()); + if (cbuf[pos].data) { + log_info(conn, "duplicate block"); + evbuffer_free(data); + return false; + } + + cbuf[pos].data = data; + cbuf[pos].op = op; + return true; +} + +void +reassembly_queue::reset() +{ + for (int i = 0; i < 256; i++) { + log_assert(!cbuf[i].data); + } + next_to_process = 0; +} + +} // namespace chop_blk + +// Local Variables: +// mode: c++ +// c-basic-offset: 2 +// c-file-style: "gnu" +// c-file-offsets: ((innamespace . 0) (brace-list-open . 0)) +// End: diff --git a/src/protocol/chop_blk.h b/src/protocol/chop_blk.h new file mode 100644 index 0000000..9776a70 --- /dev/null +++ b/src/protocol/chop_blk.h @@ -0,0 +1,247 @@ +/* Copyright 2011, 2012 SRI International + * See LICENSE for other credits and copying information + */ + +#ifndef CHOP_BLK_H +#define CHOP_BLK_H + +#include "crypt.h" +#include <event2/buffer.h> + +namespace chop_blk +{ + +/* Packets on the wire have a 16-byte header, consisting of a 32-bit + sequence number, two 16-bit length fields ("D" and "P"), an 8-bit + opcode ("F"), and a 56-bit check field. All numbers in this header + are serialized in network byte order. + + | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | A | B | C | D | E | F | + |Sequence Number| D | P | F | Check | + + The header is encrypted with AES in ECB mode: this is safe because + the header is exactly one AES block long, the sequence number is + never repeated, the header-encryption key is not used for anything + else, and the high 24 bits of the sequence number, plus the check + field, constitute an 80-bit MAC. The receiver maintains a + 256-element sliding window of acceptable sequence numbers, which + begins one after the highest sequence number so far _processed_ + (not received). If the sequence number is outside this window, or + the check field is not all-bits-zero, the packet is discarded. An + attacker's odds of being able to manipulate the D, P, or F fields + or the low bits of the sequence number are therefore less than one + in 2^80. Unlike TCP, our sequence numbers always start at zero on + a new (or freshly rekeyed) circuit, and increment by one per + _block_, not per byte of data. Furthermore, they do not wrap: a + rekeying cycle (which resets the sequence number) is required to + occur before the highest-received sequence number reaches 2^32. + + Following the header are two variable-length payload sections, + "data" and "padding", whose length in bytes are given by the D and + P fields, respectively. These sections are encrypted, using a + different key, with AES in GCM mode. The *encrypted* packet header + doubles as the GCM nonce. The semantics of the "data" section's + contents, if any, are defined by the opcode F. The "padding" + section SHOULD be filled with zeroes by the sender; regardless, its + contents MUST be ignored by the receiver. Following these sections + is a 16-byte GCM authentication tag, computed over the data and + padding sections only, NOT the message header. */ + +const size_t HEADER_LEN = 16; +const size_t TRAILER_LEN = 16; +const size_t SECTION_LEN = UINT16_MAX; +const size_t MIN_BLOCK_SIZE = HEADER_LEN + TRAILER_LEN; +const size_t MAX_BLOCK_SIZE = MIN_BLOCK_SIZE + SECTION_LEN*2; + +const size_t HANDSHAKE_LEN = sizeof(uint32_t); + +enum opcode_t +{ + op_DAT = 0, // Pass data section along to upstream + op_FIN = 1, // No further transmissions (pass data along if any) + op_RST = 2, // Protocol error, close circuit now + op_RESERVED0 = 3, // 3 -- 127 reserved for future definition + op_STEG0 = 128, // 128 -- 255 reserved for steganography modules + op_LAST = 255 +}; + +/** + * Produce a human-readable codename for opcode O. + * FALLBACKBUF is used for opcodes that have no official assignment. + * Will either return FALLBACKBUF or a pointer to a string constant. + */ +extern const char *opname(opcode_t o, char fallbackbuf[4]); + +class header +{ + uint8_t clear[16]; + uint8_t ciphr[16]; + +public: + header(uint32_t s, uint16_t d, uint16_t p, opcode_t f, ecb_encryptor &ec) + { + if (f > op_LAST || (f >= op_RESERVED0 && f < op_STEG0)) { + memset(clear, 0xFF, sizeof clear); // invalid! + memset(ciphr, 0xFF, sizeof ciphr); + return; + } + + // sequence number + clear[0] = (s >> 24) & 0xFF; + clear[1] = (s >> 16) & 0xFF; + clear[2] = (s >> 8) & 0xFF; + clear[3] = (s ) & 0xFF; + + // D field + clear[4] = (d >> 8) & 0xFF; + clear[5] = (d ) & 0xFF; + + // P field + clear[6] = (p >> 8) & 0xFF; + clear[7] = (p ) & 0xFF; + + // F field + clear[8] = uint8_t(f); + + // Check field + memset(clear + 9, 0, 7); + + ec.encrypt(ciphr, clear); + } + + header(evbuffer *buf, ecb_decryptor &dc) + { + if (evbuffer_copyout(buf, ciphr, sizeof ciphr) != sizeof ciphr) { + memset(clear, 0xFF, sizeof clear); + memset(ciphr, 0xFF, sizeof ciphr); + return; + } + dc.decrypt(clear, ciphr); + } + + uint32_t seqno() const + { + return ((uint32_t(clear[0]) << 24) | + (uint32_t(clear[1]) << 16) | + (uint32_t(clear[2]) << 8) | + (uint32_t(clear[3]) )); + + } + + size_t dlen() const + { + return ((uint16_t(clear[4]) << 8) | + (uint16_t(clear[5]) )); + } + + size_t plen() const + { + return ((uint16_t(clear[6]) << 8) | + (uint16_t(clear[7]) )); + } + + size_t total_len() const + { + return HEADER_LEN + TRAILER_LEN + dlen() + plen(); + } + + opcode_t opcode() const + { + return opcode_t(clear[8]); + } + + bool valid(uint64_t window) const + { + // This check must run in constant time. + uint8_t ck = (clear[ 9] | clear[10] | clear[11] | clear[12] | + clear[13] | clear[14] | clear[15]); + uint32_t delta = seqno() - window; + ck |= !!(delta & ~uint32_t(0xFF)); + return !ck; + } + + const uint8_t *nonce() const + { + return ciphr; + } + + const uint8_t *cleartext() const + { + return clear; + } +}; + + +/* Most of a block's header information is processed before it reaches + the reassembly queue; the only things the queue needs to record are + the sequence number (which is stored implictly), the opcode, and an + evbuffer holding the data section. Zero-data blocks still get an + evbuffer, for simplicity's sake: a reassembly queue element holds a + received block if and only if its data pointer is non-null. + + The reassembly queue is a 256-element circular buffer of + 'reassembly_elt' structs. This corresponds to the 256-element + sliding window of sequence numbers which may legitimately be + received at any time. */ + +struct reassembly_elt +{ + evbuffer *data; + opcode_t op; +}; + +class reassembly_queue +{ + reassembly_elt cbuf[256]; + uint32_t next_to_process; + + reassembly_queue(const reassembly_queue&) DELETE_METHOD; + reassembly_queue& operator=(const reassembly_queue&) DELETE_METHOD; + +public: + reassembly_queue(); + ~reassembly_queue(); + + /** + * Remove the next block to be processed from the reassembly queue + * and return it. If we are out of blocks or the next block to + * process has not yet arrived, return an empty reassembly_elt. + * Caller is responsible for freeing the evbuffer in the + * reassembly_elt, if any. + */ + reassembly_elt remove_next(); + + /** + * Insert a block into the reassembly queue at sequence number + * SEQNO, with opcode OP and data section DATA. Returns true if the + * block was successfully added to the queue, false if it is either + * outside the acceptable window or duplicates a block already on + * the queue (both of these cases indicate protocol errors). + * DATA is consumed no matter what the return value is. + */ + bool insert(uint32_t seqno, opcode_t op, evbuffer *data, conn_t *conn); + + /** + * Return the current lowest acceptable sequence number in the + * receive window. This is the value to be passed to + * block_header::valid(). + */ + uint32_t window() const { return next_to_process; } + + /** + * Reset the expected next sequence number to zero. The queue must + * be empty. This is done as the last step of a rekeying cycle. + */ + void reset(); +}; + +} // namespace chop_blk + +#endif /* chop_blk.h */ + +// Local Variables: +// mode: c++ +// c-basic-offset: 2 +// c-file-style: "gnu" +// c-file-offsets: ((innamespace . 0) (brace-list-open . 0)) +// End:

1 0

[stegotorus/master] Stop half-closing downstream connections.
by zwol＠torproject.org 20 Jul '12

20 Jul '12

commit 9d42a1ad9962307c53079a8e30574ade4998d950 Author: Zack Weinberg <zackw(a)cmu.edu> Date: Sun Jul 8 20:14:11 2012 +0200 Stop half-closing downstream connections. Up till now, when we were done writing to a downstream socket, we would use shutdown() to send a TCP FIN immediately, even if there might be data still to be _read_ from that socket. This turns out to be unreliable: on the far side, the FIN may cause libevent to signal read EOF before we are completely done reading data from the network, causing block loss. (This is arguably a bug in libevent, but shutdown() on socket bufferevents is not officially supported at present, so we need to fix it on our side.) Also, we have reason to believe none of our intended cover protocols normally leave sockets half-closed for any significant length of time, so this might be a 'tell.' And it has been observed to confuse middleboxes to the point where they won't pass our traffic at all. Instead, on the receive side, take note of a half-closed connection if the cover protocol provides an in-band indication of that fact (e.g. 'Connection: close' in HTTP) (i.e. conn_t::expect_close() now does something) but do not call shutdown() on the transmit side. This exposes a race condition in connection-to-circuit association which could cause a spurious fatal assertion, and allows us to simplify the 'should we open new downstream connections?' logic substantially. We still half-close our *upstream* sockets when we're done writing to them. That should probably change too, but that may require changes to tltester. --- src/network.cc | 16 ++++++++++------ src/protocol/chop.cc | 20 ++++++++------------ src/steg/nosteg_rr.cc | 7 ++++--- 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/src/network.cc b/src/network.cc index 945fcbf..e4f2657 100644 --- a/src/network.cc +++ b/src/network.cc @@ -447,11 +447,7 @@ downstream_flush_cb(struct bufferevent *bev, void *arg) if (remain == 0 && ((conn->pending_write_eof && conn->connected) || (!conn->circuit() && conn->ever_received))) { - if (!conn->write_eof) { - log_debug(conn, "sending EOF downstream"); - shutdown(bufferevent_getfd(bev), SHUT_WR); - conn->write_eof = true; - } + conn->write_eof = true; if (conn->read_eof && conn->write_eof) conn->close(); } @@ -501,7 +497,15 @@ downstream_connect_cb(struct bufferevent *bev, short what, void *arg) connection, and replace this callback with the regular event_cb */ if (what & BEV_EVENT_CONNECTED) { circuit_t *ckt = conn->circuit(); - log_assert(ckt); + if (!ckt) { + // This can happen if the 3-way handshake for a new connection + // began while the circuit was still active but ended after the + // circuit was closed. Just drop the connection. + log_debug(conn, "successful connection for stale circuit"); + conn->close(); + return; + } + log_assert(ckt->up_peer); log_assert(conn->buffer == bev); diff --git a/src/protocol/chop.cc b/src/protocol/chop.cc index 9e3a605..bb4a95c 100644 --- a/src/protocol/chop.cc +++ b/src/protocol/chop.cc @@ -445,9 +445,11 @@ chop_circuit_t::send() struct evbuffer *xmit_pending = bufferevent_get_input(up_buffer); size_t avail = evbuffer_get_length(xmit_pending); size_t avail0 = avail; + bool no_target_connection = false; if (downstreams.empty()) { log_debug(this, "no downstream connections"); + no_target_connection = true; } else { // Send at least one block, even if there is no real data to send. do { @@ -458,6 +460,7 @@ chop_circuit_t::send() // this is not an error; it can happen e.g. when the server has // something to send immediately and the client hasn't spoken yet log_debug(this, "no target connection available"); + no_target_connection = true; break; } @@ -472,16 +475,10 @@ chop_circuit_t::send() dead_cycles++; log_debug(this, "%u dead cycles", dead_cycles); - // If there was real data or an EOF to send, and we didn't make - // any progress on it, or if there are no downstream connections - // at all, and we're the client, try opening new connections. If - // we're the server, we have to just twiddle our thumbs and hope - // the client does that. Note that due to the sliding window of - // receive blocks, there is a hard upper limit of 64 outstanding - // connections (that is, half the receive window). - if (downstreams.empty() || - (downstreams.size() <= 64 && - (avail0 > 0 || (upstream_eof && !sent_fin)))) { + // If we're the client and we had no target connection, try + // reopening new connections. If we're the server, we have to + // just twiddle our thumbs and hope the client does that. + if (no_target_connection) { if (config->mode != LSN_SIMPLE_SERVER) circuit_reopen_downstreams(this); else @@ -1160,8 +1157,7 @@ chop_conn_t::recv_eof() void chop_conn_t::expect_close() { - // We currently don't need to do anything here. - // FIXME: figure out if this hook is _ever_ useful, and if not, remove it. + read_eof = true; } void diff --git a/src/steg/nosteg_rr.cc b/src/steg/nosteg_rr.cc index 6719584..70aef90 100644 --- a/src/steg/nosteg_rr.cc +++ b/src/steg/nosteg_rr.cc @@ -104,9 +104,10 @@ nosteg_rr_steg_t::receive(struct evbuffer *dest) return -1; } - if (config->cfg->mode != LSN_SIMPLE_SERVER) { - conn->expect_close(); - } else if (!did_transmit) { + // Note: we can't call expect_close here because we don't know whether + // there's more data coming. + + if (config->cfg->mode == LSN_SIMPLE_SERVER && !did_transmit) { can_transmit = true; conn->transmit_soon(100); }

1 0