From 3f266960564d3ade2714ae840f7e3eb42d974248 Mon Sep 17 00:00:00 2001 From: Starbeamrainbowlabs Date: Sat, 12 Feb 2022 01:53:31 +0000 Subject: [PATCH] Connection: Implement sequence numbering system to avoid replay attacks --- src/lib/transport/Connection.mjs | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/src/lib/transport/Connection.mjs b/src/lib/transport/Connection.mjs index 4a48bed..6bf542b 100644 --- a/src/lib/transport/Connection.mjs +++ b/src/lib/transport/Connection.mjs @@ -45,6 +45,9 @@ class Connection extends EventEmitter { this.rekey_interval = this.rekey_interval_base + crypto.randomInt(0, 15 * 60 * 1000); this.rekey_in_progress = false; + this.sequence_count_receive = 0; + this.sequence_count_send = 0; + this.session_key = Buffer.from(secret_join, "base64"); } @@ -93,8 +96,13 @@ class Connection extends EventEmitter { this.session_key = await rekey(this, this.session_key); this.rekey_interval = this.rekey_interval_base + crypto.randomInt(0, 15 * 60 * 1000); this.rekey_last = new Date(); + // Also reset the sequence counters. This will help avoid integer overflow issues - however unlikely they may be + this.sequence_count_send = 0; + this.sequence_count_receive = 0; /** * The session key has been re-exchanged. + * This event is fired with the rekeying process + * is complete. * @event Connection#rekey * @type {void} */ @@ -144,9 +152,21 @@ class Connection extends EventEmitter { } async handle_message(msg_text) { + // If this JSON.parse() call fails, we kill the connection 'cause the + // catch part of the above try..catch will trigger in this.handle_frame. + // This is very important, because if it didn't kill the connection + // we would no doubt be open to all sorts of unpleasant attacks + // given that we wouldn't have a chance to check the sequence number + // (see below). const msg = JSON.parse(msg_text); - l.debug(`RECEIVE:${msg.event}`, msg.message); + l.debug(`RECEIVE ${msg.sequence}:${msg.event}`, msg.message); + + if(msg.sequence !== this.sequence_count_receive) { + l.warn(`Killing connection due to invalid sequence number in received message: expected ${this.sequence_count_receive}, but got ${msg.sequence}.`); + this.destroy(); + } + this.sequence_count_receive++; if(msg.event == "rekey" && !this.rekey_in_progress) { // Set and forget here @@ -178,12 +198,12 @@ class Connection extends EventEmitter { /* TODO: Consider anonymous TLS, with jpake for mututal authentication TODO: Consider https://devdocs.io/node/crypto#crypto.createCipheriv() - which lets us use any openssl ciphers we like - e.g. ChaCha20-Poly1305 - TODO: We're currently vulnerable to a replay attack. We need to mitigate this somehow - probably by maintaining a sequence number. Instead of sending the sequence number though we should instead compute a MAC that also includes the message length and a bunch of other things etc. Of course, we will also need to make sure we don't fall afoul of mac-then-encrypt, encrypt-then-mac, etc issues... - Ref https://www.rfc-editor.org/rfc/rfc4346#appendix-F.2 for the sequence counter in TLS - We do *not* need another manual MAC, as tweetnacl's secretbox uses xsalsa20-poly1305, which is an *authenticated* encryption algorithm. Thus, all we need do is prepend the plaintext with a sequence number. + Note here that we do *not* need another manual MAC to make this + authenticated, as tweetnacl's secretbox uses xsalsa20-poly1305, + which is an *authenticated* encryption algorithm. */ - let payload = JSON.stringify({ event, message }); + let payload = JSON.stringify({ event, message, sequence: this.sequence_count_send++ }); payload = encrypt_bytes( this.session_key, Buffer.from(payload, "utf-8")