From: Daniel Lenski <dlenski@gmail.com>
Date: Mon, 8 Jan 2018 01:54:38 +0000 (-0800)
Subject: Save latest ESP sequence number even if replay protection isn't in use
X-Git-Tag: v8.00~124
X-Git-Url: https://www.infradead.org/git/?a=commitdiff_plain;h=4daac3fa2e2351178c921921f49d9c14ae45bb94;p=users%2Fdwmw2%2Fopenconnect.git

Save latest ESP sequence number even if replay protection isn't in use

In the current source, incoming ESP sequence numbers
(vpninfo->esp_in[vpinfo->current_esp_in].seq) are not actually tracked at
all unless replay protection is in use.

At the time of a rekey, old_esp_maxseq is *set based on the current value of
the incoming seq* at the time of the switchover:

    if (new_keys) {
        vpninfo->old_esp_maxseq = vpninfo->esp_in[vpninfo->current_esp_in].seq + 32;

And then esp.c rejects packets with the old incoming SPI, unless seqp < old_esp_maxseq:

    } else if (pkt->esp.spi == old_esp->spi &&
               ntohl(pkt->esp.seq) + esp->seq < vpninfo->old_esp_maxseq) {
            vpn_progress(vpninfo, PRG_TRACE,
                         _("Consider SPI 0x%x, seq %u against outgoing ESP setup\n"),
                         (unsigned)ntohl(old_esp->spi), (unsigned)ntohl(pkt->esp.seq));
            if (decrypt_esp_packet(vpninfo, old_esp, pkt))
                    continue;

This code is supposed to allow a smooth handover from the old incoming SPI
to the new one after a rekey, so that in-flight packets from the old SPI
aren't totally dropped, but also aren't allowed to continue forever.

This patch tracks the latest sequence number even if ESP replay protection
isn't in use -- however inadvisable that may be -- allowing the handover to
work correctly.

This patch also improves the confusing trace message shown when a packet
from the old SPI is received.

[dwmw2: Just call verify_packet_seqno() every time, and let it return an
        artificial 'success' when replay protection is turned off. Also
        add changelog entry.]

Signed-off-by: Daniel Lenski <dlenski@gmail.com>
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
---

diff --git a/esp-seqno.c b/esp-seqno.c
index 6f7451da..d88dd3db 100644
--- a/esp-seqno.c
+++ b/esp-seqno.c
@@ -32,6 +32,14 @@
 int verify_packet_seqno(struct openconnect_info *vpninfo,
 			struct esp *esp, uint32_t seq)
 {
+	int err_val = -EINVAL;
+	const char *discard_verb = "Discarding";
+
+	if (!vpninfo->esp_replay_protect) {
+		err_val = 0;
+		discard_verb = "Tolerating";
+	}
+
 	/*
 	 * For incoming, esp->seq is the next *expected* packet, being
 	 * the sequence number *after* the latest we have received.
@@ -100,16 +108,16 @@ int verify_packet_seqno(struct openconnect_info *vpninfo,
 		if (delta > 65 || delta == 0) {
 			/* Too old. We can't know if it's a replay. */
 			vpn_progress(vpninfo, PRG_DEBUG,
-				     _("Discarding ancient ESP packet with seq %u (expected %" PRIu64 ")\n"),
-				     seq, esp->seq);
-			return -EINVAL;
+				     _("%s ancient ESP packet with seq %u (expected %" PRIu64 ")\n"),
+				     discard_verb, seq, esp->seq);
+			return err_val;
 		} else if (delta == 1) {
 			/* Not in the bitmask since it is by definition already received. */
 		replayed:
 			vpn_progress(vpninfo, PRG_DEBUG,
-				     _("Discarding replayed ESP packet with seq %u\n"),
-				     seq);
-			return -EINVAL;
+				     _("%s replayed ESP packet with seq %u\n"),
+				     discard_verb, seq);
+			return err_val;
 		} else {
 			/* Within the backlog window, so we remember whether we've seen it or not. */
 			uint64_t mask = 1ULL << (delta - 2);
diff --git a/esp.c b/esp.c
index f705aa3e..f487580e 100644
--- a/esp.c
+++ b/esp.c
@@ -195,7 +195,7 @@ int esp_mainloop(struct openconnect_info *vpninfo, int *timeout)
 		} else if (pkt->esp.spi == old_esp->spi &&
 			   ntohl(pkt->esp.seq) + esp->seq < vpninfo->old_esp_maxseq) {
 			vpn_progress(vpninfo, PRG_TRACE,
-				     _("Consider SPI 0x%x, seq %u against outgoing ESP setup\n"),
+				     _("Received ESP packet from old SPI 0x%x, seq %u\n"),
 				     (unsigned)ntohl(old_esp->spi), (unsigned)ntohl(pkt->esp.seq));
 			if (decrypt_esp_packet(vpninfo, old_esp, pkt))
 				continue;
diff --git a/gnutls-esp.c b/gnutls-esp.c
index 916cbc7c..a565f1c7 100644
--- a/gnutls-esp.c
+++ b/gnutls-esp.c
@@ -160,11 +160,7 @@ int decrypt_esp_packet(struct openconnect_info *vpninfo, struct esp *esp, struct
 		return -EINVAL;
 	}
 
-	/* Why in $DEITY's name would you ever *not* set this? Perhaps we
-	 * should do th check anyway, but only warn instead of discarding
-	 * the packet? */
-	if (vpninfo->esp_replay_protect &&
-	    verify_packet_seqno(vpninfo, esp, ntohl(pkt->esp.seq)))
+	if (verify_packet_seqno(vpninfo, esp, ntohl(pkt->esp.seq)))
 		return -EINVAL;
 
 	gnutls_cipher_set_iv(esp->cipher, pkt->esp.iv, sizeof(pkt->esp.iv));
diff --git a/openssl-esp.c b/openssl-esp.c
index c3dff510..c4fc94c7 100644
--- a/openssl-esp.c
+++ b/openssl-esp.c
@@ -198,14 +198,9 @@ int decrypt_esp_packet(struct openconnect_info *vpninfo, struct esp *esp, struct
 		return -EINVAL;
 	}
 
-	/* Why in $DEITY's name would you ever *not* set this? Perhaps we
-	 * should do th check anyway, but only warn instead of discarding
-	 * the packet? */
-	if (vpninfo->esp_replay_protect &&
-	    verify_packet_seqno(vpninfo, esp, ntohl(pkt->esp.seq)))
+	if (verify_packet_seqno(vpninfo, esp, ntohl(pkt->esp.seq)))
 		return -EINVAL;
 
-
 	if (!EVP_DecryptInit_ex(esp->cipher, NULL, NULL, NULL,
 				pkt->esp.iv)) {
 		vpn_progress(vpninfo, PRG_ERR,
diff --git a/tests/seqtest.c b/tests/seqtest.c
index 5f9a5df3..42760162 100644
--- a/tests/seqtest.c
+++ b/tests/seqtest.c
@@ -25,7 +25,9 @@
 #define vpn_progress(v, d, ...) printf(__VA_ARGS__)
 #define _(x) x
 
-struct openconnect_info;
+struct openconnect_info {
+	int esp_replay_protect;
+};
 
 struct esp {
 	uint64_t seq_backlog;
@@ -38,48 +40,49 @@ struct esp {
 int main(void)
 {
 	struct esp esptest = { 0, 0 };
+	struct openconnect_info vpninfo = { 1};
 
-	if (verify_packet_seqno(NULL, &esptest, 0) ||
-	    verify_packet_seqno(NULL, &esptest, 2) ||
-	    verify_packet_seqno(NULL, &esptest, 1) ||
-	    !verify_packet_seqno(NULL, &esptest, 0) ||
-	    verify_packet_seqno(NULL, &esptest, 64) ||
-	    verify_packet_seqno(NULL, &esptest, 65) ||
-	    !verify_packet_seqno(NULL, &esptest, 65) ||
-	    verify_packet_seqno(NULL, &esptest, 66) ||
-	    verify_packet_seqno(NULL, &esptest, 67) ||
-	    verify_packet_seqno(NULL, &esptest, 68) ||
-	    !verify_packet_seqno(NULL, &esptest, 68) ||
-	    !verify_packet_seqno(NULL, &esptest, 2) ||
-	    !verify_packet_seqno(NULL, &esptest, 3) ||
-	    verify_packet_seqno(NULL, &esptest, 4) ||
-	    verify_packet_seqno(NULL, &esptest, 164) ||
-	    !verify_packet_seqno(NULL, &esptest, 99) ||
-	    verify_packet_seqno(NULL, &esptest, 100) ||
-	    verify_packet_seqno(NULL, &esptest, 200) ||
-	    verify_packet_seqno(NULL, &esptest, 264) ||
-	    !verify_packet_seqno(NULL, &esptest, 199) ||
-	    !verify_packet_seqno(NULL, &esptest, 200) ||
-	    verify_packet_seqno(NULL, &esptest, 265) ||
-	    verify_packet_seqno(NULL, &esptest, 210) ||
-	    verify_packet_seqno(NULL, &esptest, 201) ||
-	    verify_packet_seqno(NULL, &esptest, 270) ||
-	    verify_packet_seqno(NULL, &esptest, 206) ||
-	    !verify_packet_seqno(NULL, &esptest, 210) ||
-	    verify_packet_seqno(NULL, &esptest, 333) ||
-	    !verify_packet_seqno(NULL, &esptest, 268) ||
-	    verify_packet_seqno(NULL, &esptest, 269) ||
-	    !verify_packet_seqno(NULL, &esptest, 270) ||
-	    verify_packet_seqno(NULL, &esptest, 0xfffffffd) ||
-	    !verify_packet_seqno(NULL, &esptest, 1) ||
-	    verify_packet_seqno(NULL, &esptest, 0xffffffc1) ||
-	    verify_packet_seqno(NULL, &esptest, 0xfffffffc) ||
-	    verify_packet_seqno(NULL, &esptest, 0xffffffff) ||
-	    !verify_packet_seqno(NULL, &esptest, 0) ||
-	    !verify_packet_seqno(NULL, &esptest, 0xffffffbe) ||
-	    verify_packet_seqno(NULL, &esptest, 0xffffffbf) ||
-	    !verify_packet_seqno(NULL, &esptest, 0xffffffc1) ||
-	    verify_packet_seqno(NULL, &esptest, 0xffffffc0))
+	if ( verify_packet_seqno(&vpninfo, &esptest, 0) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 2) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 1) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 0) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 64) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 65) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 65) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 66) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 67) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 68) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 68) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 2) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 3) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 4) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 164) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 99) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 100) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 200) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 264) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 199) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 200) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 265) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 210) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 201) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 270) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 206) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 210) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 333) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 268) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 269) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 270) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 0xfffffffd) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 1) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 0xffffffc1) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 0xfffffffc) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 0xffffffff) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 0) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 0xffffffbe) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 0xffffffbf) ||
+	    !verify_packet_seqno(&vpninfo, &esptest, 0xffffffc1) ||
+	     verify_packet_seqno(&vpninfo, &esptest, 0xffffffc0))
 		return 1;
 
 	return 0;
diff --git a/www/changelog.xml b/www/changelog.xml
index 300ea2c8..be4ce60a 100644
--- a/www/changelog.xml
+++ b/www/changelog.xml
@@ -15,6 +15,7 @@
 <ul>
    <li><b>OpenConnect HEAD</b>
      <ul>
+       <li>Fix ESP rekey when replay protection is disabled.</li>
        <li>Drop support for GnuTLS older than 3.2.10.</li>
        <li>Fix <tt>--passwd-on-stdin</tt> for Windows to not forcibly open console.</li>
        <li>Fix portability of shell scripts in test suite.</li>