chiark / gitweb /
site: Deal with losing peer's MSG6 - go to RUN on MSG0 with new key
authorIan Jackson <ijackson@chiark.greenend.org.uk>
Thu, 14 Jun 2012 00:31:00 +0000 (01:31 +0100)
committerIan Jackson <ijackson@chiark.greenend.org.uk>
Thu, 12 Jul 2012 19:02:21 +0000 (20:02 +0100)
The original protocol, as implemented, has the following bug:

The responder sends MSG6 on its transition from SENTMSG4 to RUN, on
receipt of the initiator's MSG5.  Unlike all the other key protocol
messages, MSG6 is not retransmitted (for there is no retransmission in
RUN).  If the responder's MSG6 is lost, the initiator has no way of
knowing and won't switch to the new key.  The initator will keep
retransmitting MSG5, which the responder (in old versions of secnet)
ignores.  The responder will be sending data with the new key, which
the initiator (in old versions of secnet) ignores.  This broken
situation can persist until the initiator sends data, which it will do
with the old key, possibly causing a new key exchange attempt to be
initiated by the original responder (in old versions of secnet only),
which may or may not experience the same bug.

This bug needs to be fixed separately at both ends, ideally, so that
running a fixed version at either end avoids the bug.

Here we fix for the case where we are the initiator, as follows:

If are in SENTMSG5 and we see data encrypted with the new key,
conclude that we must have lost the MSG6 and simply switch to the new
key, and state RUN, right away.

Because the transform operates destructively (and changing that fact
about the transform API would be too intrusive), this means we need to
keep a copy of the original ciphertext in process_msg0.

For this purpose we introduce a new buffer `scratch' in the site state
structure, and a new buffer_copy utility function for making a copy of
a buffer (reallocating the destination buffer if necessary).

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
site.c
util.c
util.h

diff --git a/site.c b/site.c
index 4dbebaf..c0f4387 100644 (file)
--- a/site.c
+++ b/site.c
@@ -277,6 +277,7 @@ struct site {
     uint8_t localN[NONCELEN]; /* Nonces for key exchange */
     uint8_t remoteN[NONCELEN];
     struct buffer_if buffer; /* Current outgoing key exchange packet */
+    struct buffer_if scratch;
     int32_t retries; /* Number of retries remaining */
     uint64_t timeout; /* Timeout for current state */
     uint8_t *dhsecret;
@@ -321,6 +322,7 @@ static void enter_state_run(struct site *st);
 static bool_t enter_state_resolve(struct site *st);
 static bool_t enter_new_state(struct site *st,uint32_t next);
 static void enter_state_wait(struct site *st);
+static void activate_new_key(struct site *st);
 
 #define CHECK_AVAIL(b,l) do { if ((b)->size<(l)) return False; } while(0)
 #define CHECK_EMPTY(b) do { if ((b)->size!=0) return False; } while(0)
@@ -711,12 +713,15 @@ static bool_t process_msg6(struct site *st, struct buffer_if *msg6,
 
 static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0)
 {
-    cstring_t transform_err;
+    cstring_t transform_err, newkey_err="n/a";
     struct msg0 m;
     uint32_t problem;
 
     if (!unpick_msg0(st,msg0,&m)) return False;
 
+    /* Keep a copy so we can try decrypting it with multiple keys */
+    buffer_copy(&st->scratch, msg0);
+
     problem = st->current_transform->reverse(st->current_transform->st,
                                             msg0,&transform_err);
     if (!problem) return True;
@@ -726,7 +731,21 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0)
        return False;
     }
 
-    slog(st,LOG_SEC,"transform: %s",transform_err);
+    if (st->state==SITE_SENTMSG5) {
+       buffer_copy(msg0, &st->scratch);
+       if (!st->new_transform->reverse(st->new_transform->st,
+                                       msg0,&newkey_err)) {
+           /* It looks like we didn't get the peer's MSG6 */
+           /* This is like a cut-down enter_new_state(SITE_RUN) */
+           slog(st,LOG_STATE,"will enter state RUN (MSG0 with new key)");
+           BUF_FREE(&st->buffer);
+           st->timeout=0;
+           activate_new_key(st);
+           return True; /* do process the data in this packet */
+       }
+    }
+
+    slog(st,LOG_SEC,"transform: %s (new: %s)",transform_err,newkey_err);
     initiate_key_setup(st,"incoming message would not decrypt");
     return False;
 }
@@ -1236,9 +1255,9 @@ static bool_t site_incoming(void *sst, struct buffer_if *buf,
            /* Setup packet: expected only in state SENTMSG4 */
            /* (may turn up in state RUN if our return MSG6 was lost
               and the new key has already been activated. In that
-              case we should treat it as an ordinary PING packet. We
-              can't pass it to process_msg5() because the
-              new_transform will now be unkeyed. XXX) */
+              case we discard it. The peer will realise that we
+              are using the new key when they see our data packets.
+              Until then the peer's data packets to us get discarded. */
            if (st->state!=SITE_SENTMSG4) {
                slog(st,LOG_UNEXPECTED,"unexpected MSG5");
            } else if (process_msg5(st,buf,source)) {
@@ -1427,6 +1446,9 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context,
 
     buffer_new(&st->buffer,SETUP_BUFFER_LEN);
 
+    buffer_new(&st->scratch,0);
+    BUF_ALLOC(&st->scratch,"site:scratch");
+
     /* We are interested in poll(), but only for timeouts. We don't have
        any fds of our own. */
     register_for_poll(st, site_beforepoll, site_afterpoll, 0, "site");
diff --git a/util.c b/util.c
index 086c234..f5f3d75 100644 (file)
--- a/util.c
+++ b/util.c
@@ -301,6 +301,18 @@ void buffer_new(struct buffer_if *buf, int32_t len)
     buf->base=safe_malloc(len,"buffer_new");
 }
 
+void buffer_copy(struct buffer_if *dst, const struct buffer_if *src)
+{
+    if (dst->len < src->len) {
+       dst->base=realloc(dst->base,src->len);
+       if (!dst->base) fatal_perror("buffer_copy");
+       dst->len = src->len;
+    }
+    dst->start = dst->base + (src->start - src->base);
+    dst->size = src->size;
+    memcpy(dst->start, src->start, dst->size);
+}
+
 static list_t *buffer_apply(closure_t *self, struct cloc loc, dict_t *context,
                            list_t *args)
 {
diff --git a/util.h b/util.h
index e40fb54..af19363 100644 (file)
--- a/util.h
+++ b/util.h
@@ -23,6 +23,7 @@ extern void buffer_assert_used(struct buffer_if *buffer, cstring_t file,
                               int line);
 extern void buffer_new(struct buffer_if *buffer, int32_t len);
 extern void buffer_init(struct buffer_if *buffer, int32_t max_start_pad);
+extern void buffer_copy(struct buffer_if *dst, const struct buffer_if *src);
 extern void *buf_append(struct buffer_if *buf, int32_t amount);
 extern void *buf_prepend(struct buffer_if *buf, int32_t amount);
 extern void *buf_unappend(struct buffer_if *buf, int32_t amount);