Mailing List Archive

[PATCH] Implement seq_id checking.
I just implemented Juniper SSL VPN support in OpenConnect, and its UDP
transport is ESP. So I figured you might as well have a copy of this...
utterly untested (within vpnc):

diff --git a/tunip.c b/tunip.c
index 4f3b6ec..e43258d 100644
--- a/tunip.c
+++ b/tunip.c
@@ -112,7 +112,7 @@ struct encap_method {

int (*recv) (struct sa_block *s, unsigned char *buf, unsigned int bufsize);
void (*send_peer) (struct sa_block *s, unsigned char *buf, unsigned int bufsize);
- int (*recv_peer) (struct sa_block *s);
+ int (*recv_peer) (struct sa_block *s, uint32_t seq_id);
};

/* Yuck! Global variables... */
@@ -483,7 +483,87 @@ static void encap_udp_send_peer(struct sa_block *s, unsigned char *buf, unsigned
(long long)sent, s->ipsec.tx.buflen);
}

-static int encap_esp_recv_peer(struct sa_block *s)
+
+static int encap_esp_validate_seqid(struct sa_block *s, uint32_t seq_id)
+{
+ /*
+ * For incoming, s->ipsec.rx.seq_id is the next *expected* packet,
+ * being the sequence number *after* the latest we have received.
+ *
+ * Since it must always be true that packet s->ipsec.rx.seq_id-1
+ * has been received, there's no need to explicitly record that.
+ *
+ * So the backlog bitmap covers the 32 packets prior to that, with
+ * the LSB representing packet (s->ipsec.rx.seq_id - 2), and the
+ * MSB representing (s->ipsec.rx.seq_id - 33). A received packet
+ * is represented by a zero bit, and a missing packet is
+ * represented by a one.
+ *
+ * Thus we can allow out-of-order reception of packets that are
+ * within a reasonable interval of the latest packet received.
+ */
+
+ if (seq_id == s->ipsec.rx.seq_id) {
+ /* The common case. This is the packet we expected next. */
+ s->ipsec.rx.seq_backlog <<= 1;
+ s->ipsec.rx.seq_id++;
+ logmsg(LOG_DEBUG, "Accepting expected ESP packet with seq %u\n",
+ seq_id);
+ return 0;
+ } else if (seq_id + 33 < s->ipsec.rx.seq_id) {
+ /* Too old. We can't know if it's a replay. */
+ logmsg(LOG_NOTICE,
+ "Discarding ancient ESP packet with seq %u (expected %u)\n",
+ seq_id, s->ipsec.rx.seq_id);
+ return -EINVAL;
+ } else if (seq_id < s->ipsec.rx.seq_id) {
+ /* Within the backlog window, so we remember whether we've seen it or not. */
+ uint32_t mask = 1 << (s->ipsec.rx.seq_id - seq_id - 2);
+
+ if (s->ipsec.rx.seq_backlog & mask) {
+ logmsg(LOG_DEBUG,
+ "Accepting out-of-order ESP packet with seq %u (expected %u)\n",
+ seq_id, s->ipsec.rx.seq_id);
+ s->ipsec.rx.seq_backlog &= ~mask;
+ return 0;
+ }
+ logmsg(LOG_NOTICE,
+ "Discarding replayed ESP packet with seq %u\n", seq_id);
+ return -EINVAL;
+ } else {
+ /* The packet we were expecting has gone missing; this one is newer. */
+ int delta = seq_id - s->ipsec.rx.seq_id;
+
+ if (delta >= 32) {
+ /* We jumped a long way into the future. We have not seen
+ * any of the previous 32 packets so set the backlog bitmap
+ * to all ones. */
+ s->ipsec.rx.seq_backlog = 0xffffffff;
+ } else if (delta == 31) {
+ /* Avoid undefined behaviour that shifting by 32 would incur.
+ * The (clear) top bit represents the packet which is currently
+ * s->ipsec.rx.seq_id - 1, which we know was already received. */
+ s->ipsec.rx.seq_backlog = 0x7fffffff;
+ } else {
+ /* We have missed (delta) packets. Shift the backlog by that
+ * amount *plus* the one we would have shifted it anyway if
+ * we'd received the packet we were expecting. The zero bit
+ * representing the packet which is currently s->ipsec.rx.seq_id - 1,
+ * which we know has been received, ends up at bit position
+ * (1<<delta). Then we set all the bits lower than that, which
+ * represent the missing packets. */
+ s->ipsec.rx.seq_backlog <<= delta + 1;
+ s->ipsec.rx.seq_backlog |= (1<<delta) - 1;
+ }
+ logmsg(LOG_DEBUG,
+ "Accepting later-than-expected ESP packet with seq %u (expected %u)\n",
+ seq_id, s->ipsec.rx.seq_id);
+ s->ipsec.rx.seq_id = seq_id + 1;
+ return 0;
+ }
+}
+
+static int encap_esp_recv_peer(struct sa_block *s, uint32_t seq_id)
{
int len, i;
size_t blksz;
@@ -518,6 +598,9 @@ static int encap_esp_recv_peer(struct sa_block *s)
}
}

+ if (encap_esp_validate_seqid(s, seq_id))
+ return -1;
+
blksz = s->ipsec.blk_len;
if (s->ipsec.cry_algo && ((len % blksz) != 0)) {
logmsg(LOG_ALERT,
@@ -733,7 +816,7 @@ static void process_socket(struct sa_block *s)
}

/* Check auth digest and/or decrypt */
- if (s->ipsec.em->recv_peer(s) != 0)
+ if (s->ipsec.em->recv_peer(s, ntohl(eh->seq_id)) != 0)
return;

if (encap_any_decap(s) == 0) {
diff --git a/tunip.h b/tunip.h
index 216fdf0..fb86792 100644
--- a/tunip.h
+++ b/tunip.h
@@ -36,7 +36,8 @@ struct lifetime {

struct ike_sa {
uint32_t spi;
- uint32_t seq_id; /* for replay protection (not implemented) */
+ uint32_t seq_id; /* for replay protection */
+ uint32_t seq_backlog;

uint8_t *key;
uint8_t *key_cry;

--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
Re: [PATCH] Implement seq_id checking. [ In reply to ]
On Tue, 2015-01-27 at 12:36 +0000, David Woodhouse wrote:
> I just implemented Juniper SSL VPN support in OpenConnect, and its UDP
> transport is ESP. So I figured you might as well have a copy of this...
> utterly untested (within vpnc):

Seems to work for me, though I don't see any "discarding" messages that
would indicate hitting some of the sequence problem codepaths.

Dan

> diff --git a/tunip.c b/tunip.c
> index 4f3b6ec..e43258d 100644
> --- a/tunip.c
> +++ b/tunip.c
> @@ -112,7 +112,7 @@ struct encap_method {
>
> int (*recv) (struct sa_block *s, unsigned char *buf, unsigned int bufsize);
> void (*send_peer) (struct sa_block *s, unsigned char *buf, unsigned int bufsize);
> - int (*recv_peer) (struct sa_block *s);
> + int (*recv_peer) (struct sa_block *s, uint32_t seq_id);
> };
>
> /* Yuck! Global variables... */
> @@ -483,7 +483,87 @@ static void encap_udp_send_peer(struct sa_block *s, unsigned char *buf, unsigned
> (long long)sent, s->ipsec.tx.buflen);
> }
>
> -static int encap_esp_recv_peer(struct sa_block *s)
> +
> +static int encap_esp_validate_seqid(struct sa_block *s, uint32_t seq_id)
> +{
> + /*
> + * For incoming, s->ipsec.rx.seq_id is the next *expected* packet,
> + * being the sequence number *after* the latest we have received.
> + *
> + * Since it must always be true that packet s->ipsec.rx.seq_id-1
> + * has been received, there's no need to explicitly record that.
> + *
> + * So the backlog bitmap covers the 32 packets prior to that, with
> + * the LSB representing packet (s->ipsec.rx.seq_id - 2), and the
> + * MSB representing (s->ipsec.rx.seq_id - 33). A received packet
> + * is represented by a zero bit, and a missing packet is
> + * represented by a one.
> + *
> + * Thus we can allow out-of-order reception of packets that are
> + * within a reasonable interval of the latest packet received.
> + */
> +
> + if (seq_id == s->ipsec.rx.seq_id) {
> + /* The common case. This is the packet we expected next. */
> + s->ipsec.rx.seq_backlog <<= 1;
> + s->ipsec.rx.seq_id++;
> + logmsg(LOG_DEBUG, "Accepting expected ESP packet with seq %u\n",
> + seq_id);
> + return 0;
> + } else if (seq_id + 33 < s->ipsec.rx.seq_id) {
> + /* Too old. We can't know if it's a replay. */
> + logmsg(LOG_NOTICE,
> + "Discarding ancient ESP packet with seq %u (expected %u)\n",
> + seq_id, s->ipsec.rx.seq_id);
> + return -EINVAL;
> + } else if (seq_id < s->ipsec.rx.seq_id) {
> + /* Within the backlog window, so we remember whether we've seen it or not. */
> + uint32_t mask = 1 << (s->ipsec.rx.seq_id - seq_id - 2);
> +
> + if (s->ipsec.rx.seq_backlog & mask) {
> + logmsg(LOG_DEBUG,
> + "Accepting out-of-order ESP packet with seq %u (expected %u)\n",
> + seq_id, s->ipsec.rx.seq_id);
> + s->ipsec.rx.seq_backlog &= ~mask;
> + return 0;
> + }
> + logmsg(LOG_NOTICE,
> + "Discarding replayed ESP packet with seq %u\n", seq_id);
> + return -EINVAL;
> + } else {
> + /* The packet we were expecting has gone missing; this one is newer. */
> + int delta = seq_id - s->ipsec.rx.seq_id;
> +
> + if (delta >= 32) {
> + /* We jumped a long way into the future. We have not seen
> + * any of the previous 32 packets so set the backlog bitmap
> + * to all ones. */
> + s->ipsec.rx.seq_backlog = 0xffffffff;
> + } else if (delta == 31) {
> + /* Avoid undefined behaviour that shifting by 32 would incur.
> + * The (clear) top bit represents the packet which is currently
> + * s->ipsec.rx.seq_id - 1, which we know was already received. */
> + s->ipsec.rx.seq_backlog = 0x7fffffff;
> + } else {
> + /* We have missed (delta) packets. Shift the backlog by that
> + * amount *plus* the one we would have shifted it anyway if
> + * we'd received the packet we were expecting. The zero bit
> + * representing the packet which is currently s->ipsec.rx.seq_id - 1,
> + * which we know has been received, ends up at bit position
> + * (1<<delta). Then we set all the bits lower than that, which
> + * represent the missing packets. */
> + s->ipsec.rx.seq_backlog <<= delta + 1;
> + s->ipsec.rx.seq_backlog |= (1<<delta) - 1;
> + }
> + logmsg(LOG_DEBUG,
> + "Accepting later-than-expected ESP packet with seq %u (expected %u)\n",
> + seq_id, s->ipsec.rx.seq_id);
> + s->ipsec.rx.seq_id = seq_id + 1;
> + return 0;
> + }
> +}
> +
> +static int encap_esp_recv_peer(struct sa_block *s, uint32_t seq_id)
> {
> int len, i;
> size_t blksz;
> @@ -518,6 +598,9 @@ static int encap_esp_recv_peer(struct sa_block *s)
> }
> }
>
> + if (encap_esp_validate_seqid(s, seq_id))
> + return -1;
> +
> blksz = s->ipsec.blk_len;
> if (s->ipsec.cry_algo && ((len % blksz) != 0)) {
> logmsg(LOG_ALERT,
> @@ -733,7 +816,7 @@ static void process_socket(struct sa_block *s)
> }
>
> /* Check auth digest and/or decrypt */
> - if (s->ipsec.em->recv_peer(s) != 0)
> + if (s->ipsec.em->recv_peer(s, ntohl(eh->seq_id)) != 0)
> return;
>
> if (encap_any_decap(s) == 0) {
> diff --git a/tunip.h b/tunip.h
> index 216fdf0..fb86792 100644
> --- a/tunip.h
> +++ b/tunip.h
> @@ -36,7 +36,8 @@ struct lifetime {
>
> struct ike_sa {
> uint32_t spi;
> - uint32_t seq_id; /* for replay protection (not implemented) */
> + uint32_t seq_id; /* for replay protection */
> + uint32_t seq_backlog;
>
> uint8_t *key;
> uint8_t *key_cry;
>
> _______________________________________________
> vpnc-devel mailing list
> vpnc-devel@unix-ag.uni-kl.de
> https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
> http://www.unix-ag.uni-kl.de/~massar/vpnc/


_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/
Re: [PATCH] Implement seq_id checking. [ In reply to ]
On Fri, 2015-02-27 at 10:04 -0600, Dan Williams wrote:
> On Tue, 2015-01-27 at 12:36 +0000, David Woodhouse wrote:
> > I just implemented Juniper SSL VPN support in OpenConnect, and its UDP
> > transport is ESP. So I figured you might as well have a copy of this...
> > utterly untested (within vpnc):
>
> Seems to work for me, though I don't see any "discarding" messages that
> would indicate hitting some of the sequence problem codepaths.

Thanks for testing. You *shouldn't* see those messages, ideally. You
might occasionally see an out-of-order packet, but never the discarded
ones. You should be able to trigger that by capturing an ESP packet from
the wire and replaying it much later.

--
dwmw2
Re: [PATCH] Implement seq_id checking. [ In reply to ]
On Fri, 2015-02-27 at 10:04 -0600, Dan Williams wrote:
> On Tue, 2015-01-27 at 12:36 +0000, David Woodhouse wrote:
> > I just implemented Juniper SSL VPN support in OpenConnect, and its UDP
> > transport is ESP. So I figured you might as well have a copy of this...
> > utterly untested (within vpnc):
>
> Seems to work for me, though I don't see any "discarding" messages that
> would indicate hitting some of the sequence problem codepaths.

Note that I have updated the corresponding code in OpenConnect and
fixed a few bugs. Since the patch didn't get merged, even into the
Fedora packages of vpnc, I haven't bothered to update this version.

Someone probably should, though. Because vpnc accepting replayed ESP
packets is probably CVE-worthy.

--
dwmw2