From 1e89a63f1638e43dee79afbb18d5f733b27d830b Mon Sep 17 00:00:00 2001 From: Etienne Dechamps Date: Sun, 17 May 2015 17:09:56 +0100 Subject: [PATCH 1/6] Prevent SPTPS key regeneration packets from entering an UDP relay path. Commit 10c1f60c643607d9dafd79271c3475cddf81e903 introduced a mechanism by which a packet received by REQ_KEY could continue its journey over UDP. This was based on the assumption that REQ_KEY messages would never be used for handshake packets (which should never be sent over UDP, because SPTPS currently doesn't handle lost handshake packets very well). Unfortunately, there is one case where handshake packets are sent using REQ_KEY: when regenerating the SPTPS key for a pre-established channel. With the current code, such packets risk getting relayed over UDP. When processing a REQ_KEY message, it is impossible for the receiving end to distinguish between a data SPTPS packet and a handshake packet, because this information is stored in the type field which is encrypted with the end-to-end key. This commit fixes the issue by making tinc use ANS_KEY for all SPTPS handshake messages. This works because ANS_KEY messages are never forwarded using the SPTPS relay mechanisms, therefore they are guaranteed to stick to TCP. --- src/net_packet.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/net_packet.c b/src/net_packet.c index f734af29..92a3eb53 100644 --- a/src/net_packet.c +++ b/src/net_packet.c @@ -735,7 +735,7 @@ bool send_sptps_data(node_t *to, node_t *from, int type, const void *data, size_ /* Send it via TCP if it is a handshake packet, TCPOnly is in use, this is a relay packet that the other node cannot understand, or this packet is larger than the MTU. */ if(type == SPTPS_HANDSHAKE || tcponly || (!direct && !relay_supported) || (type != PKT_PROBE && (len - SPTPS_DATAGRAM_OVERHEAD) > relay->minmtu)) { - if((from != myself || to->status.validkey) && (to->nexthop->connection->options >> 24) >= 7) { + if(type != SPTPS_HANDSHAKE && (to->nexthop->connection->options >> 24) >= 7) { char buf[len + sizeof to->id + sizeof from->id]; char* buf_ptr = buf; memcpy(buf_ptr, &to->id, sizeof to->id); buf_ptr += sizeof to->id; memcpy(buf_ptr, &from->id, sizeof from->id); buf_ptr += sizeof from->id; @@ -746,9 +746,10 @@ bool send_sptps_data(node_t *to, node_t *from, int type, const void *data, size_ char buf[len * 4 / 3 + 5]; b64encode(data, buf, len); - /* If no valid key is known yet, send the packets using ANS_KEY requests, - to ensure we get to learn the reflexive UDP address. */ - if(from == myself && !to->status.validkey) { + /* If this is a handshake packet, use ANS_KEY instead of REQ_KEY, for two reasons: + - We don't want intermediate nodes to switch to UDP to relay these packets; + - ANS_KEY allows us to learn the reflexive UDP address. */ + if(type == SPTPS_HANDSHAKE) { to->incompression = myself->incompression; return send_request(to->nexthop->connection, "%d %s %s %s -1 -1 -1 %d", ANS_KEY, from->name, to->name, buf, to->incompression); } else { From 23fda4db6d1bb400a97f6d2a07d9b700f9546129 Mon Sep 17 00:00:00 2001 From: Sven-Haegar Koch Date: Sun, 17 May 2015 05:29:21 +0200 Subject: [PATCH 2/6] Let sockaddr2hostname() handle AF_UNSPEC addresses. --- src/netutl.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/netutl.c b/src/netutl.c index 701a4309..2eebb644 100644 --- a/src/netutl.c +++ b/src/netutl.c @@ -121,7 +121,10 @@ char *sockaddr2hostname(const sockaddr_t *sa) { char port[NI_MAXSERV] = "unknown"; int err; - if(sa->sa.sa_family == AF_UNKNOWN) { + if(sa->sa.sa_family == AF_UNSPEC) { + xasprintf(&str, "unspec port unspec"); + return str; + } else if(sa->sa.sa_family == AF_UNKNOWN) { xasprintf(&str, "%s port %s", sa->unknown.address, sa->unknown.port); return str; } From 30e839b0a1810b9cb0a2de2595cef2f8ebb06357 Mon Sep 17 00:00:00 2001 From: Guus Sliepen Date: Sun, 17 May 2015 18:44:09 +0200 Subject: [PATCH 3/6] Don't send local_address in ADD_EDGE messages if it's AF_UNSPEC. --- src/protocol_edge.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/protocol_edge.c b/src/protocol_edge.c index 76e6eb48..7478aaeb 100644 --- a/src/protocol_edge.c +++ b/src/protocol_edge.c @@ -37,19 +37,26 @@ bool send_add_edge(connection_t *c, const edge_t *e) { bool x; char *address, *port; - char *local_address, *local_port; sockaddr2str(&e->address, &address, &port); - sockaddr2str(&e->local_address, &local_address, &local_port); - x = send_request(c, "%d %x %s %s %s %s %x %d %s %s", ADD_EDGE, rand(), - e->from->name, e->to->name, address, port, - e->options, e->weight, local_address, local_port); + if(e->local_address.sa.sa_family) { + char *local_address, *local_port; + sockaddr2str(&e->local_address, &local_address, &local_port); + + x = send_request(c, "%d %x %s %s %s %s %x %d %s %s", ADD_EDGE, rand(), + e->from->name, e->to->name, address, port, + e->options, e->weight, local_address, local_port); + free(local_address); + free(local_port); + } else { + x = send_request(c, "%d %x %s %s %s %s %x %d", ADD_EDGE, rand(), + e->from->name, e->to->name, address, port, + e->options, e->weight); + } free(address); free(port); - free(local_address); - free(local_port); return x; } From aa52300b2b6e9d923d6d5b8c95fa500f549620d0 Mon Sep 17 00:00:00 2001 From: Etienne Dechamps Date: Sun, 17 May 2015 17:51:05 +0100 Subject: [PATCH 4/6] Trivial: make sptps_receive_data_datagram() a little more readable. The new code updates variables as stuff is being consumed, so that the reader doesn't have to do that in his head. --- src/sptps.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/sptps.c b/src/sptps.c index e5946ed6..d6e6d417 100644 --- a/src/sptps.c +++ b/src/sptps.c @@ -447,6 +447,7 @@ static bool sptps_receive_data_datagram(sptps_t *s, const char *data, size_t len uint32_t seqno; memcpy(&seqno, data, 4); seqno = ntohl(seqno); + data += 4; len -= 4; if(!s->instate) { if(seqno != s->inseqno) @@ -454,38 +455,39 @@ static bool sptps_receive_data_datagram(sptps_t *s, const char *data, size_t len s->inseqno = seqno + 1; - uint8_t type = data[4]; + uint8_t type = *(data++); len--; if(type != SPTPS_HANDSHAKE) return error(s, EIO, "Application record received before handshake finished"); - return receive_handshake(s, data + 5, len - 5); + return receive_handshake(s, data, len); } // Decrypt char buffer[len]; - size_t outlen; - - if(!chacha_poly1305_decrypt(s->incipher, seqno, data + 4, len - 4, buffer, &outlen)) + if(!chacha_poly1305_decrypt(s->incipher, seqno, data, len, buffer, &outlen)) return error(s, EIO, "Failed to decrypt and verify packet"); if(!sptps_check_seqno(s, seqno, true)) return false; // Append a NULL byte for safety. - buffer[len - 20] = 0; + buffer[outlen] = 0; - uint8_t type = buffer[0]; + data = buffer; + len = outlen; + + uint8_t type = *(data++); len--; if(type < SPTPS_HANDSHAKE) { if(!s->instate) return error(s, EIO, "Application record received before handshake finished"); - if(!s->receive_record(s->handle, type, buffer + 1, len - 21)) + if(!s->receive_record(s->handle, type, data, len)) return false; } else if(type == SPTPS_HANDSHAKE) { - if(!receive_handshake(s, buffer + 1, len - 21)) + if(!receive_handshake(s, data, len)) return false; } else { return error(s, EIO, "Invalid record type %d", type); From 1a7a9078c093f77950192c32be009bbe463fe372 Mon Sep 17 00:00:00 2001 From: Etienne Dechamps Date: Sun, 17 May 2015 18:50:11 +0100 Subject: [PATCH 5/6] Proactively restart the SPTPS tunnel if we get receive errors. There are a number of ways a SPTPS tunnel can get into a corrupt state. For example, during key regeneration, the KEX and SIG messages from other nodes might arrive out of order, which confuses the hell out of the SPTPS code. Another possible scenario is not noticing another node crashed and restarted because there was no point in time where the node was seen completely disconnected from *all* nodes; this could result in using the wrong (old) key. There are probably other scenarios which have not even been considered yet. Distributed systems are hard. When SPTPS got confused by a packet, it used to crash the entire process; fortunately that was fixed by commit 2e7f68ad2b51648b89c4b5c61aeb4cec67c2fbbb. However, the error handling (or lack thereof) leaves a lot to be desired. Currently, when SPTPS encounters an error when receiving a packet, it just shrugs it off and continues as if nothing happened. The problem is, sometimes getting receive errors mean the tunnel is completely stuck and will not recover on its own. In that case, the node will become unreachable - possibly indefinitely. The goal of this commit is to improve SPTPS error handling by taking proactive action when an incoming packet triggers a failure, which is often an indicator that the tunnel is stuck in some way. When that happens, we simply restart SPTPS entirely, which should make the tunnel recover quickly. To prevent "storms" where two buggy nodes flood each other with invalid packets and therefore spend all their time negotiating new tunnels, we limit the frequency at which tunnel restarts happen to ten seconds. It is likely this commit will solve the "Invalid KEX record length during key regeneration" issue that has been seen in the wild. It is difficult to be sure though because we do not have a full understanding of all the possible conditions that can trigger this problem. --- src/net_packet.c | 21 +++++++++++++++++---- src/protocol_key.c | 29 ++++++++++++++++++++--------- 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/src/net_packet.c b/src/net_packet.c index 92a3eb53..8313a54f 100644 --- a/src/net_packet.c +++ b/src/net_packet.c @@ -288,7 +288,14 @@ static bool receive_udppacket(node_t *n, vpn_packet_t *inpkt) { n->status.udppacket = false; if(!result) { - logger(DEBUG_TRAFFIC, LOG_ERR, "Got bad packet from %s (%s)", n->name, n->hostname); + /* Uh-oh. It might be that the tunnel is stuck in some corrupted state, + so let's restart SPTPS in case that helps. But don't do that too often + to prevent storms, and because that would make life a little too easy + for external attackers trying to DoS us. */ + if(n->last_req_key < now.tv_sec - 10) { + logger(DEBUG_PROTOCOL, LOG_ERR, "Failed to decode raw TCP packet from %s (%s), restarting SPTPS", n->name, n->hostname); + send_req_key(n); + } return false; } return true; @@ -464,11 +471,17 @@ bool receive_tcppacket_sptps(connection_t *c, const char *data, int len) { /* The packet is for us */ - if(!from->status.validkey) { - logger(DEBUG_PROTOCOL, LOG_ERR, "Got SPTPS packet from %s (%s) but we don't have a valid key yet", from->name, from->hostname); + if(!sptps_receive_data(&from->sptps, data, len)) { + /* Uh-oh. It might be that the tunnel is stuck in some corrupted state, + so let's restart SPTPS in case that helps. But don't do that too often + to prevent storms. */ + if(from->last_req_key < now.tv_sec - 10) { + logger(DEBUG_PROTOCOL, LOG_ERR, "Failed to decode raw TCP packet from %s (%s), restarting SPTPS", from->name, from->hostname); + send_req_key(from); + } return true; } - sptps_receive_data(&from->sptps, data, len); + send_mtu_info(myself, from, MTU); return true; } diff --git a/src/protocol_key.c b/src/protocol_key.c index c183ac45..83851f23 100644 --- a/src/protocol_key.c +++ b/src/protocol_key.c @@ -107,9 +107,6 @@ bool send_req_key(node_t *to) { return true; } - if(to->sptps.label) - logger(DEBUG_ALWAYS, LOG_DEBUG, "send_req_key(%s) called while sptps->label != NULL!", to->name); - char label[25 + strlen(myself->name) + strlen(to->name)]; snprintf(label, sizeof label, "tinc UDP key expansion %s %s", myself->name, to->name); sptps_stop(&to->sptps); @@ -148,11 +145,16 @@ static bool req_key_ext_h(connection_t *c, const char *request, node_t *from, no try_tx(to, true); } else { /* The packet is for us */ - if(!from->status.validkey) { - logger(DEBUG_PROTOCOL, LOG_ERR, "Got SPTPS_PACKET from %s (%s) but we don't have a valid key yet", from->name, from->hostname); + if(!sptps_receive_data(&from->sptps, buf, len)) { + /* Uh-oh. It might be that the tunnel is stuck in some corrupted state, + so let's restart SPTPS in case that helps. But don't do that too often + to prevent storms. */ + if(from->last_req_key < now.tv_sec - 10) { + logger(DEBUG_PROTOCOL, LOG_ERR, "Failed to decode TCP packet from %s (%s), restarting SPTPS", from->name, from->hostname); + send_req_key(from); + } return true; } - sptps_receive_data(&from->sptps, buf, len); send_mtu_info(myself, from, MTU); } @@ -428,9 +430,18 @@ bool ans_key_h(connection_t *c, const char *request) { if(from->status.sptps) { char buf[strlen(key)]; int len = b64decode(key, buf, strlen(key)); - - if(!len || !sptps_receive_data(&from->sptps, buf, len)) - logger(DEBUG_ALWAYS, LOG_ERR, "Error processing SPTPS data from %s (%s)", from->name, from->hostname); + if(!len || !sptps_receive_data(&from->sptps, buf, len)) { + /* Uh-oh. It might be that the tunnel is stuck in some corrupted state, + so let's restart SPTPS in case that helps. But don't do that too often + to prevent storms. + Note that simply relying on handshake timeout is not enough, because + that doesn't apply to key regeneration. */ + if(from->last_req_key < now.tv_sec - 10) { + logger(DEBUG_PROTOCOL, LOG_ERR, "Failed to decode handshake TCP packet from %s (%s), restarting SPTPS", from->name, from->hostname); + send_req_key(from); + } + return true; + } if(from->status.validkey) { if(*address && *port) { From 2cb216d83d825fcca2fa2b66c756b253f8f0828b Mon Sep 17 00:00:00 2001 From: Etienne Dechamps Date: Sun, 17 May 2015 19:23:12 +0100 Subject: [PATCH 6/6] Don't send KEY_CHANGED messages if we don't support the legacy protocol. KEY_CHANGED messages are only useful to invalidate keys for non-SPTPS nodes; SPTPS nodes use a different internal mechanism (forced KEX) for that purpose. Therefore, if we know we can't talk to legacy nodes, there's no point in sending them these messages. --- src/protocol_key.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/protocol_key.c b/src/protocol_key.c index c183ac45..6721aa44 100644 --- a/src/protocol_key.c +++ b/src/protocol_key.c @@ -36,6 +36,7 @@ static bool mykeyused = false; void send_key_changed(void) { +#ifndef DISABLE_LEGACY send_request(everyone, "%d %x %s", KEY_CHANGED, rand(), myself->name); /* Immediately send new keys to directly connected nodes to keep UDP mappings alive */ @@ -43,6 +44,7 @@ void send_key_changed(void) { for list_each(connection_t, c, connection_list) if(c->edge && c->node && c->node->status.reachable && !c->node->status.sptps) send_ans_key(c->node); +#endif /* Force key exchange for connections using SPTPS */