From 48b97423bbbb3d89b49ba29bd86dfa835ec2b928 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:50 +0100 Subject: [PATCH] site: Send NAKs for undecryptable data packets (msg0) Packets which are not understood are supposed in general to produce NAKs, to let the peer know that we don't have the key they were using. However, previously this would only happen if the incoming packet had a local site index which was not in use. But it can happen that a particular index value is reused by a recently restarted secnet. Previously, in this case, data packets would simply be thrown away undecryptable. With this change, undecryptable data packets always generate a NAK. (But we don't generate a NAK if the packet was decryptable, but suffered from sequence number skew - if we did, then network glitches would trigger spurious key exchanges.) This is particularly relevant for mobile sites, as it can happen that the fixed site doesn't have an address for the mobile site - so the association will remain stuck in a broken state until the mobile site is also restarted. There is still a potential problem when a site is restarted mid key exchange. The peer will refuse to start a new key exchange (because of the retry timeout) and the restarted site may not know it's necessary. This will be dealt with in a later patch. Signed-off-by: Ian Jackson --- site.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/site.c b/site.c index a6fa9da..8fa4c11 100644 --- a/site.c +++ b/site.c @@ -731,7 +731,8 @@ static bool_t process_msg6(struct site *st, struct buffer_if *msg6, return True; } -static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0) +static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0, + const struct comm_addr *src) { cstring_t transform_err, auxkey_err, newkey_err="n/a"; struct msg0 m; @@ -750,11 +751,8 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0) "peer has used new key","auxiliary key",LOG_SEC); return True; } - - if (problem==2) { - slog(st,LOG_DROP,"transform: %s (merely skew)",transform_err); - return False; - } + if (problem==2) + goto skew; buffer_copy(msg0, &st->scratch); problem = st->auxiliary_key.transform->reverse @@ -778,11 +776,14 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0) } return True; } + if (problem==2) + goto skew; if (st->state==SITE_SENTMSG5) { buffer_copy(msg0, &st->scratch); - if (!st->new_transform->reverse(st->new_transform->st, - msg0,&newkey_err)) { + problem = st->new_transform->reverse(st->new_transform->st, + msg0,&newkey_err); + if (!problem) { /* 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)"); @@ -791,11 +792,18 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0) activate_new_key(st); return True; /* do process the data in this packet */ } + if (problem==2) + goto skew; } slog(st,LOG_SEC,"transform: %s (aux: %s, new: %s)", transform_err,auxkey_err,newkey_err); initiate_key_setup(st,"incoming message would not decrypt"); + send_nak(src,m.dest,m.source,m.type,msg0,"message would not decrypt"); + return False; + + skew: + slog(st,LOG_DROP,"transform: %s (merely skew)",transform_err); return False; } @@ -804,7 +812,7 @@ static bool_t process_msg0(struct site *st, struct buffer_if *msg0, { uint32_t type; - if (!decrypt_msg0(st,msg0)) + if (!decrypt_msg0(st,msg0,src)) return False; CHECK_AVAIL(msg0,4); -- 2.30.2