site, transform: Do not initiate rekey when packets too much out of order

If packets arrive sufficiently out of order, we may unnecessarily
initiate a rekey simply because of the skew.  Solve this as follows:
 * transform->reverse gives a distinct error code for `too much skew'.
 * site does not initiate a rekey when it sees that error code.

The rekey-on-decrypt-failure feature is in principle unnecessary.
However, due to another bug it is possible for a key setup to be
regarded as successful at only one of the two ends, and the ability to
immediately do another key setup to try to fix things is useful.

In principle this new behaviour might prevent a necessary rekey: if
the sender has sent around 2^31 packets none of which were received,
the receiver would see the new packets as too old.  But: firstly, this
is very unlikely (it would have involved transmitting several Gbytes
into the void, in a period less than the maximum key lifetime).
Secondly, we ought to expire keys before then anyway so the sender
should know that the key had `worn out' (although currently this is
not enforced).

The benefit of this change is that the old behaviour would be likely
to initiate unnecessary rekeys when on unreliable links (eg, mobile
internet).  Unnecessary rekeys are a bad idea in these circumstances
not just because they clog up the link but also because they make the
association more fragile.

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

diff --git a/secnet.h b/secnet.h
index 5f8e43bc5545b5f2a3ba2e9daf4178cca5ffb283..b276798918e06284dccc95f592dfcb17b39136ac 100644 (file)
--- a/secnet.h
+++ b/secnet.h
@@ -389,7 +389,11 @@ typedef struct transform_inst_if *transform_createinstance_fn(void *st);
 typedef bool_t transform_setkey_fn(void *st, uint8_t *key, int32_t keylen);
 typedef void transform_delkey_fn(void *st);
 typedef void transform_destroyinstance_fn(void *st);
 typedef bool_t transform_setkey_fn(void *st, uint8_t *key, int32_t keylen);
 typedef void transform_delkey_fn(void *st);
 typedef void transform_destroyinstance_fn(void *st);

-/* Returns 0 for 'all is well', any other value for a problem */
+/* Returns:
+ *   0: all is well
+ *   1: for any other problem
+ *   2: message decrypted but sequence number was out of range
+ */

 typedef uint32_t transform_apply_fn(void *st, struct buffer_if *buf,
                                    const char **errmsg);
 
 typedef uint32_t transform_apply_fn(void *st, struct buffer_if *buf,
                                    const char **errmsg);
 

diff --git a/site.c b/site.c
index 4d3a6125bfed91a8956cbc3b3c3ebd608e8f95bc..4dbebaf3cfeefbf399d21129ee56c2f081a52e73 100644 (file)
--- a/site.c
+++ b/site.c
@@ -721,6 +721,11 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0)
                                             msg0,&transform_err);
     if (!problem) return True;
 
                                             msg0,&transform_err);
     if (!problem) return True;
 

+    if (problem==2) {
+       slog(st,LOG_DROP,"transform: %s (merely skew)",transform_err);
+       return False;
+    }
+

     slog(st,LOG_SEC,"transform: %s",transform_err);
     initiate_key_setup(st,"incoming message would not decrypt");
     return False;
     slog(st,LOG_SEC,"transform: %s",transform_err);
     initiate_key_setup(st,"incoming message would not decrypt");
     return False;

diff --git a/transform.c b/transform.c
index f55aa447dfd41c1c64c3dfc6b658010057b77d55..f1da5642099d0d1ec7f1546498985e60b06a5157 100644 (file)
--- a/transform.c
+++ b/transform.c
@@ -247,7 +247,7 @@ static uint32_t transform_reverse(void *sst, struct buffer_if *buf,
     } else {
        /* Too much skew */
        *errmsg="seqnum: too much skew";
     } else {
        /* Too much skew */
        *errmsg="seqnum: too much skew";

-       return 1;
+       return 2;

     }
     
     return 0;
     }
     
     return 0;