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 6f7451da73832e0cbb5ce6715819d0a95ba366de..d88dd3dba1d6da96dbb6f7ea00ecf97f59a6d994 100644 (file)
--- 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 f705aa3eea271448d7f8249caed389d5f80716b6..f487580ea3cecf2f0b3cc8b43d772d1e0650d5c7 100644 (file)
--- 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 916cbc7c93327d40b727b0f5a8ed99a5b86d2c34..a565f1c70be32bf8d520066eb008fb8317defa76 100644 (file)
--- 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 c3dff510e2338ccaafc12bf1ef199f500eaa3c79..c4fc94c7d3d0c61a9b4157714b427fb0635a7ea6 100644 (file)
--- 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 5f9a5df3ff8dce31fda1a46fc6e776fff66494b4..4276016215037ccf385a72398bb0e36782e8ca91 100644 (file)
--- 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 300ea2c8d01652b5669a65a995a649bc18de2566..be4ce60a3b5612c22bb07e4e6ca6bec2d91c8282 100644 (file)
--- 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>