From ijackson at chiark.greenend.org.uk  Thu Jun 14 01:38:30 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 14 Jun 2012 01:38:30 +0100
Subject: [PATCH 0/7] Security and logging fixes
Message-ID: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>

I've been working on fixing some bugs related to poor handling of lost
packets near the end of key setup.  However, while I was trying to
sort out that rather complicated problem, I encountered some yaks:

 1/7 SECURITY: actually reject messages with improper lengths
 2/7 Makefile: honour EXTRA_CFLAGS, etc.
 3/7 log: Eliminate potential out-of-control recursion
 4/7 log: Print truncated messages
 5/7 messages: add some missing newlines
 6/7 netlink: report why a packet is bad
 7/7 netlink: abolish check_config and output_config

This has all been only very lightly tested.

These changes can be found, along with two work-in-progress commits to
the state machine re lost packets, in my REBASING branch
   login.chiark.greenend.org.uk:/u/ian/things/secnet#wip
aka
   http://www.chiark.greenend.org.uk/ucgi/~ian/git?p=secnet.git;a=shortlog;h=refs/heads/wip



From ijackson at chiark.greenend.org.uk  Thu Jun 14 01:38:31 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 14 Jun 2012 01:38:31 +0100
Subject: [PATCH 1/7] SECURITY: actually reject messages with improper lengths
In-Reply-To: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1339634317-16407-2-git-send-email-ijackson@chiark.greenend.org.uk>

transform_reverse checks to see if the incoming message is a multiple
of the block cipher block size.  If the message fails this check, it
sets *errmsg but it fails to return.  Instead, it proceeds to attempt
to decrypt it.  This will involve a buffer read/write overrun.

It transform_reverse fails to check to see if the incoming message is
long enough to contain the IV, MAC and padding.  This can easily be
exploited to cause secnet to segfault.

buffer_unprepend should check that the buffer has enough data in it to
unprepend.  With the other patches, this should be irrelevant, but it
turns various other potential read overrun bugs into crashes.

site_incoming should check that the incoming message is long enough.
Again, without this, this is an buffer read overrun or possibly a
segfault.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 site.c      |    3 +++
 transform.c |    5 +++++
 util.c      |    1 +
 3 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/site.c b/site.c
index 624752c..0cd364b 100644
--- a/site.c
+++ b/site.c
@@ -1121,6 +1121,9 @@ static bool_t site_incoming(void *sst, struct buffer_if *buf,
 			    const struct comm_addr *source)
 {
     struct site *st=sst;
+
+    if (buf->size < 12) return False;
+
     uint32_t dest=ntohl(*(uint32_t *)buf->start);
 
     if (dest==0) {
diff --git a/transform.c b/transform.c
index 8fdf9fd..f55aa44 100644
--- a/transform.c
+++ b/transform.c
@@ -171,6 +171,10 @@ static uint32_t transform_reverse(void *sst, struct buffer_if *buf,
 	return 1;
     }
 
+    if (buf->size < 4 + 16 + 16) {
+	*errmsg="msg too short";
+	return 1;
+    }
 
     /* CBC */
     memset(iv,0,16);
@@ -181,6 +185,7 @@ static uint32_t transform_reverse(void *sst, struct buffer_if *buf,
     /* Assert bufsize is multiple of blocksize */
     if (buf->size&0xf) {
 	*errmsg="msg not multiple of cipher blocksize";
+	return 1;
     }
     serpent_encrypt(&ti->cryptkey,iv,iv);
     for (n=buf->start; n<buf->start+buf->size; n+=16)
diff --git a/util.c b/util.c
index 63fe76f..086c234 100644
--- a/util.c
+++ b/util.c
@@ -269,6 +269,7 @@ void *buf_unappend(struct buffer_if *buf, int32_t amount) {
 
 void *buf_unprepend(struct buffer_if *buf, int32_t amount) {
     void *p;
+    if (buf->size < amount) return 0;
     p=buf->start;
     buf->start+=amount;
     buf->size-=amount;
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 14 01:38:32 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 14 Jun 2012 01:38:32 +0100
Subject: [PATCH 2/7] Makefile: honour EXTRA_CFLAGS, etc.
In-Reply-To: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1339634317-16407-3-git-send-email-ijackson@chiark.greenend.org.uk>

This makes it easier to add command-line options at build-time.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 Makefile.in |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile.in b/Makefile.in
index c9bb82b..182204b 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -38,10 +38,10 @@ CFLAGS:=-Wall @WRITESTRINGS@ @CFLAGS@ -Werror \
 	-Wpointer-arith -Wformat=2 -Winit-self \
 	-Wswitch-enum -Wunused-variable -Wbad-function-cast \
 	-Wno-strict-aliasing -fno-strict-aliasing
-ALL_CFLAGS:=@DEFS@ -I$(srcdir) -I. $(CFLAGS)
-CPPFLAGS:=@CPPFLAGS@
-LDFLAGS:=@LDFLAGS@
-LDLIBS:=@LIBS@
+ALL_CFLAGS:=@DEFS@ -I$(srcdir) -I. $(CFLAGS) $(EXTRA_CFLAGS)
+CPPFLAGS:=@CPPFLAGS@ $(EXTRA_CPPFLAGS)
+LDFLAGS:=@LDFLAGS@ $(EXTRA_LDFLAGS)
+LDLIBS:=@LIBS@ $(EXTRA_LDLIBS)
 
 prefix:=@prefix@
 exec_prefix:=@exec_prefix@
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 14 01:38:33 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 14 Jun 2012 01:38:33 +0100
Subject: [PATCH 3/7] log: Eliminate potential out-of-control recursion
In-Reply-To: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1339634317-16407-4-git-send-email-ijackson@chiark.greenend.org.uk>

vMessage looks for the system log instance.  If there is one, it uses
it; otherwise it just uses stderr or stdout.

The system log instance consists, eventually, of syslog_vlog or
logfile_vlog.  Both of these functions have a fallback: if they are
properly set up they do their thing; otherwise they call vMessage.
This is wrong because in this situation they have probably been called
by vMessage so it doesn't help.

At first sight this ought to produce unbounded recursion but for
complicated reasons another bug prevents this.  Instead, messages can
just vanish.

Break out the fallback mode of [v]Message into [v]MessageFallback so
that syslog_vlog and logfile_vlog can use it directly.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 log.c |   38 ++++++++++++++++++++++++++------------
 1 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/log.c b/log.c
index 3a50a27..4f1b651 100644
--- a/log.c
+++ b/log.c
@@ -14,9 +14,20 @@ bool_t secnet_is_daemon=False;
 uint32_t message_level=M_WARNING|M_ERR|M_SECURITY|M_FATAL;
 struct log_if *system_log=NULL;
 
-static void vMessage(uint32_t class, const char *message, va_list args)
+static void vMessageFallback(uint32_t class, const char *message, va_list args)
 {
     FILE *dest=stdout;
+    /* Messages go to stdout/stderr */
+    if (class & message_level) {
+	if (class&M_FATAL || class&M_ERR || class&M_WARNING) {
+	    dest=stderr;
+	}
+	vfprintf(dest,message,args);
+    }
+}
+
+static void vMessage(uint32_t class, const char *message, va_list args)
+{
 #define MESSAGE_BUFLEN 1023
     static char buff[MESSAGE_BUFLEN+1]={0,};
     size_t bp;
@@ -34,13 +45,7 @@ static void vMessage(uint32_t class, const char *message, va_list args)
 	    memmove(buff,nlp+1,strlen(nlp+1)+1);
 	}
     } else {
-	/* Messages go to stdout/stderr */
-	if (class & message_level) {
-	    if (class&M_FATAL || class&M_ERR || class&M_WARNING) {
-		dest=stderr;
-	    }
-	    vfprintf(dest,message,args);
-	}
+	vMessageFallback(class,message,args);
     }
 }  
 
@@ -53,6 +58,15 @@ void Message(uint32_t class, const char *message, ...)
     va_end(ap);
 }
 
+static void MessageFallback(uint32_t class, const char *message, ...)
+{
+    va_list ap;
+
+    va_start(ap,message);
+    vMessageFallback(class,message,ap);
+    va_end(ap);
+}
+
 static NORETURN(vfatal(int status, bool_t perror, const char *message,
 		       va_list args));
 
@@ -262,8 +276,8 @@ static void logfile_vlog(void *sst, int class, const char *message,
 	    fflush(st->f);
 	}
     } else {
-	vMessage(class,message,args);
-	Message(class,"\n");
+	vMessageFallback(class,message,args);
+	MessageFallback(class,"\n");
     }
 }
 
@@ -391,8 +405,8 @@ static void syslog_vlog(void *sst, int class, const char *message,
     if (st->open)
 	vsyslog(msgclass_to_syslogpriority(class),message,args);
     else {
-	vMessage(class,message,args);
-	Message(class,"\n");
+	vMessageFallback(class,message,args);
+	MessageFallback(class,"\n");
     }
 }
 
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 14 01:38:34 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 14 Jun 2012 01:38:34 +0100
Subject: [PATCH 4/7] log: Print truncated messages
In-Reply-To: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1339634317-16407-5-git-send-email-ijackson@chiark.greenend.org.uk>

If a message doesn't survive the vsnprintf in vMessage untruncated,
the result may be that its newline is lost.  After that, buff will
never manage to gain a newline and all further messages will be
discarded.

Fix this by always putting "\n\0" at the end of buff.  That way a
truncated message is printed and the buffer will be cleared out for
the next call.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 log.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/log.c b/log.c
index 4f1b651..9cd0572 100644
--- a/log.c
+++ b/log.c
@@ -38,6 +38,8 @@ static void vMessage(uint32_t class, const char *message, va_list args)
 	bp=strlen(buff);
 	assert(bp < MESSAGE_BUFLEN);
 	vsnprintf(buff+bp,MESSAGE_BUFLEN-bp,message,args);
+	buff[sizeof(buff)-2] = '\n';
+	buff[sizeof(buff)-1] = '\0';
 	/* Each line is sent separately */
 	while ((nlp=strchr(buff,'\n'))) {
 	    *nlp=0;
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 14 01:38:35 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 14 Jun 2012 01:38:35 +0100
Subject: [PATCH 5/7] messages: add some missing newlines
In-Reply-To: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1339634317-16407-6-git-send-email-ijackson@chiark.greenend.org.uk>

Message and cfgfatal must be called with a message containing a
newline (or, a newline sent in a later call).  Fix a few call sites.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 log.c     |    4 ++--
 process.c |    2 +-
 rsa.c     |    8 ++++----
 site.c    |    7 ++++---
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/log.c b/log.c
index 9cd0572..d940df5 100644
--- a/log.c
+++ b/log.c
@@ -170,11 +170,11 @@ void cfgfile_postreadcheck(struct cloc loc, FILE *f)
 {
     assert(loc.file);
     if (ferror(f)) {
-	Message(M_FATAL, "error reading config file (%s): %s",
+	Message(M_FATAL, "error reading config file (%s): %s\n",
 		loc.file, strerror(errno));
 	exit(current_phase);
     } else if (feof(f)) {
-	Message(M_FATAL, "unexpected end of config file (%s)", loc.file);
+	Message(M_FATAL, "unexpected end of config file (%s)\n", loc.file);
 	exit(current_phase);
     }
 }
diff --git a/process.c b/process.c
index 0a7a0f2..a9ff3d9 100644
--- a/process.c
+++ b/process.c
@@ -152,7 +152,7 @@ int sys_cmd(const char *path, const char *arg, ...)
 			path, arg, WTERMSIG(rv), strsignal(WTERMSIG(rv)),
 			WCOREDUMP(rv) ? " - core dumped" : "");
 	    else
-		Message(M_ERR, "sys_cmd(%s,%s,...) exited with wstat %#x",
+		Message(M_ERR, "sys_cmd(%s,%s,...) exited with wstat %#x\n",
 			path, arg, rv);
 	}
     } else if (c==0) {
diff --git a/rsa.c b/rsa.c
index caa030b..0bd106f 100644
--- a/rsa.c
+++ b/rsa.c
@@ -194,7 +194,7 @@ static list_t *rsapub_apply(closure_t *self, struct cloc loc, dict_t *context,
     i=list_elem(args,0);
     if (i) {
 	if (i->type!=t_string) {
-	    cfgfatal(i->loc,"rsa-public","first argument must be a string");
+	    cfgfatal(i->loc,"rsa-public","first argument must be a string\n");
 	}
 	e=i->data.string;
 	if (mpz_init_set_str(&st->e,e,10)!=0) {
@@ -208,7 +208,7 @@ static list_t *rsapub_apply(closure_t *self, struct cloc loc, dict_t *context,
     i=list_elem(args,1);
     if (i) {
 	if (i->type!=t_string) {
-	    cfgfatal(i->loc,"rsa-public","second argument must be a string");
+	    cfgfatal(i->loc,"rsa-public","second argument must be a string\n");
 	}
 	n=i->data.string;
 	if (mpz_init_set_str(&st->n,n,10)!=0) {
@@ -267,7 +267,7 @@ static list_t *rsapriv_apply(closure_t *self, struct cloc loc, dict_t *context,
     i=list_elem(args,0);
     if (i) {
 	if (i->type!=t_string) {
-	    cfgfatal(i->loc,"rsa-public","first argument must be a string");
+	    cfgfatal(i->loc,"rsa-public","first argument must be a string\n");
 	}
 	filename=i->data.string;
     } else {
@@ -313,7 +313,7 @@ static list_t *rsapriv_apply(closure_t *self, struct cloc loc, dict_t *context,
     }
     b=safe_malloc(length,"rsapriv_apply");
     if (fread(b,length,1,f) != 1) {
-	cfgfatal_maybefile(f,loc,"rsa-private","error reading modulus");
+	cfgfatal_maybefile(f,loc,"rsa-private","error reading modulus\n");
     }
     mpz_init(&st->n);
     read_mpbin(&st->n,b,length);
diff --git a/site.c b/site.c
index 0cd364b..b54b34e 100644
--- a/site.c
+++ b/site.c
@@ -1342,15 +1342,16 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context,
     st->netlink=find_cl_if(dict,"link",CL_NETLINK,True,"site",loc);
 
     list_t *comms_cfg=dict_lookup(dict,"comm");
-    if (!comms_cfg) cfgfatal(loc,"site","closure list \"comm\" not found");
+    if (!comms_cfg) cfgfatal(loc,"site","closure list \"comm\" not found\n");
     st->ncomms=list_length(comms_cfg);
     st->comms=safe_malloc_ary(sizeof(*st->comms),st->ncomms,"comms");
     assert(st->ncomms);
     for (i=0; i<st->ncomms; i++) {
 	item_t *item=list_elem(comms_cfg,i);
-	if (item->type!=t_closure) cfgfatal(loc,"site","comm is not a closure");
+	if (item->type!=t_closure)
+	    cfgfatal(loc,"site","comm is not a closure\n");
 	closure_t *cl=item->data.closure;
-	if (cl->type!=CL_COMM) cfgfatal(loc,"site","comm closure wrong type");
+	if (cl->type!=CL_COMM) cfgfatal(loc,"site","comm closure wrong type\n");
 	st->comms[i]=cl->interface;
     }
 
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 14 01:38:36 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 14 Jun 2012 01:38:36 +0100
Subject: [PATCH 6/7] netlink: report why a packet is bad
In-Reply-To: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1339634317-16407-7-git-send-email-ijackson@chiark.greenend.org.uk>

Replace
 <netlink>: bad IP packet from <source>
with
 <netlink>: bad IP packet from <source>: reason

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 netlink.c |   28 ++++++++++++++++++++--------
 1 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/netlink.c b/netlink.c
index 5226ad1..559821d 100644
--- a/netlink.c
+++ b/netlink.c
@@ -384,19 +384,29 @@ static void netlink_icmp_simple(struct netlink *st, struct buffer_if *buf,
  * 3. Checksums correctly.
  * 4. Doesn't have a bogus length
  */
-static bool_t netlink_check(struct netlink *st, struct buffer_if *buf)
+static bool_t netlink_check(struct netlink *st, struct buffer_if *buf,
+			    char *errmsgbuf, int errmsgbuflen)
 {
+#define BAD(...) do{					\
+	snprintf(errmsgbuf,errmsgbuflen,__VA_ARGS__);	\
+	return False;					\
+    }while(0)
+
     struct iphdr *iph=(struct iphdr *)buf->start;
     int32_t len;
 
-    if (iph->ihl < 5 || iph->version != 4) return False;
-    if (buf->size < iph->ihl*4) return False;
-    if (ip_fast_csum((uint8_t *)iph, iph->ihl)!=0) return False;
+    if (iph->ihl < 5) BAD("ihl %u",iph->ihl);
+    if (iph->version != 4) BAD("version %u",iph->version);
+    if (buf->size < iph->ihl*4) BAD("size %"PRId32"<%u*4",buf->size,iph->ihl);
+    if (ip_fast_csum((uint8_t *)iph, iph->ihl)!=0) BAD("csum");
     len=ntohs(iph->tot_len);
     /* There should be no padding */
-    if (buf->size!=len || len<(iph->ihl<<2)) return False;
+    if (buf->size!=len) BAD("len %"PRId32"!=%"PRId32,buf->size,len);
+    if (len<(iph->ihl<<2)) BAD("len %"PRId32"<(%u<<2)",len,iph->ihl);
     /* XXX check that there's no source route specified */
     return True;
+
+#undef BAD
 }
 
 /* Deliver a packet. "client" is the _origin_ of the packet, not its
@@ -595,11 +605,13 @@ static void netlink_incoming(struct netlink *st, struct netlink_client *client,
 {
     uint32_t source,dest;
     struct iphdr *iph;
+    char errmsgbuf[50];
 
     BUF_ASSERT_USED(buf);
-    if (!netlink_check(st,buf)) {
-	Message(M_WARNING,"%s: bad IP packet from %s\n",
-		st->name,client?client->name:"host");
+    if (!netlink_check(st,buf,errmsgbuf,sizeof(errmsgbuf))) {
+	Message(M_WARNING,"%s: bad IP packet from %s: %s\n",
+		st->name,client?client->name:"host",
+		errmsgbuf);
 	BUF_FREE(buf);
 	return;
     }
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 14 01:38:37 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 14 Jun 2012 01:38:37 +0100
Subject: [PATCH 7/7] netlink: abolish check_config and output_config
In-Reply-To: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1339634317-16407-8-git-send-email-ijackson@chiark.greenend.org.uk>

Apparently, a long time ago, MSG5 and MSG6 used to contain some
netlink configuration data, which the receiver of the MSG5 or MSG6
would check.

However, for a long time now the output_config function has been a
no-op and the check function has unconditionally eaten and discarded
anything extra in the message.

Furthermore, because the MSG6 is not retransmitted, this mechanism
couldn't be reliable without a protocol change.  So the existing
interface is defective.

So, abolish it the interface, the dummy implementation, and all the
call sites.  The check_config call sites in site.c now instead
directly discard any unexpected data at the end of MSG5 and MSG6.

This patch should cause no behavioural change in actual operation.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 netlink.c |   23 -----------------------
 secnet.h  |    2 --
 site.c    |   16 ++++------------
 3 files changed, 4 insertions(+), 37 deletions(-)

diff --git a/netlink.c b/netlink.c
index 559821d..fdad4ff 100644
--- a/netlink.c
+++ b/netlink.c
@@ -803,27 +803,6 @@ static void netlink_signal_handler(void *sst, int signum)
     netlink_dump_routes(st,True);
 }
 
-static void netlink_inst_output_config(void *sst, struct buffer_if *buf)
-{
-/*    struct netlink_client *c=sst; */
-/*    struct netlink *st=c->nst; */
-
-    /* For now we don't output anything */
-    BUF_ASSERT_USED(buf);
-}
-
-static bool_t netlink_inst_check_config(void *sst, struct buffer_if *buf)
-{
-/*    struct netlink_client *c=sst; */
-/*    struct netlink *st=c->nst; */
-
-    BUF_ASSERT_USED(buf);
-    /* We need to eat all of the configuration information from the buffer
-       for backward compatibility. */
-    buf->size=0;
-    return True;
-}
-
 static void netlink_inst_set_mtu(void *sst, int32_t new_mtu)
 {
     struct netlink_client *c=sst;
@@ -910,8 +889,6 @@ static closure_t *netlink_inst_create(struct netlink *st,
     c->ops.reg=netlink_inst_reg;
     c->ops.deliver=netlink_inst_incoming;
     c->ops.set_quality=netlink_set_quality;
-    c->ops.output_config=netlink_inst_output_config;
-    c->ops.check_config=netlink_inst_check_config;
     c->ops.set_mtu=netlink_inst_set_mtu;
     c->nst=st;
 
diff --git a/secnet.h b/secnet.h
index 06beb05..937ee9c 100644
--- a/secnet.h
+++ b/secnet.h
@@ -437,8 +437,6 @@ struct netlink_if {
     netlink_register_fn *reg;
     netlink_deliver_fn *deliver;
     netlink_link_quality_fn *set_quality;
-    netlink_output_config_fn *output_config;
-    netlink_check_config_fn *check_config;
     netlink_set_mtu_fn *set_mtu;
 };
 
diff --git a/site.c b/site.c
index b54b34e..233afe8 100644
--- a/site.c
+++ b/site.c
@@ -628,7 +628,6 @@ static bool_t generate_msg5(struct site *st)
     buffer_init(&st->buffer,st->transform->max_start_pad+(4*4));
     /* Give the netlink code an opportunity to put its own stuff in the
        message (configuration information, etc.) */
-    st->netlink->output_config(st->netlink->st,&st->buffer);
     buf_prepend_uint32(&st->buffer,LABEL_MSG5);
     st->new_transform->forwards(st->new_transform->st,&st->buffer,
 				&transform_err);
@@ -660,11 +659,8 @@ static bool_t process_msg5(struct site *st, struct buffer_if *msg5,
 	slog(st,LOG_SEC,"MSG5/PING packet contained wrong label");
 	return False;
     }
-    if (!st->netlink->check_config(st->netlink->st,msg5)) {
-	slog(st,LOG_SEC,"MSG5/PING packet contained bad netlink config");
-	return False;
-    }
-    CHECK_EMPTY(msg5);
+    /* Older versions of secnet used to write some config data here
+     * which we ignore.  So we don't CHECK_EMPTY */
     return True;
 }
 
@@ -677,7 +673,6 @@ static bool_t generate_msg6(struct site *st)
     buffer_init(&st->buffer,st->transform->max_start_pad+(4*4));
     /* Give the netlink code an opportunity to put its own stuff in the
        message (configuration information, etc.) */
-    st->netlink->output_config(st->netlink->st,&st->buffer);
     buf_prepend_uint32(&st->buffer,LABEL_MSG6);
     st->new_transform->forwards(st->new_transform->st,&st->buffer,
 				&transform_err);
@@ -709,11 +704,8 @@ static bool_t process_msg6(struct site *st, struct buffer_if *msg6,
 	slog(st,LOG_SEC,"MSG6/PONG packet contained invalid data");
 	return False;
     }
-    if (!st->netlink->check_config(st->netlink->st,msg6)) {
-	slog(st,LOG_SEC,"MSG6/PONG packet contained bad netlink config");
-	return False;
-    }
-    CHECK_EMPTY(msg6);
+    /* Older versions of secnet used to write some config data here
+     * which we ignore.  So we don't CHECK_EMPTY */
     return True;
 }
 
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:47 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:47 +0100
Subject: [PATCH 07/19] netlink: report why a packet is bad
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-8-git-send-email-ijackson@chiark.greenend.org.uk>

Replace
 <netlink>: bad IP packet from <source>
with
 <netlink>: bad IP packet from <source>: reason

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 netlink.c |   28 ++++++++++++++++++++--------
 1 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/netlink.c b/netlink.c
index 5226ad1..559821d 100644
--- a/netlink.c
+++ b/netlink.c
@@ -384,19 +384,29 @@ static void netlink_icmp_simple(struct netlink *st, struct buffer_if *buf,
  * 3. Checksums correctly.
  * 4. Doesn't have a bogus length
  */
-static bool_t netlink_check(struct netlink *st, struct buffer_if *buf)
+static bool_t netlink_check(struct netlink *st, struct buffer_if *buf,
+			    char *errmsgbuf, int errmsgbuflen)
 {
+#define BAD(...) do{					\
+	snprintf(errmsgbuf,errmsgbuflen,__VA_ARGS__);	\
+	return False;					\
+    }while(0)
+
     struct iphdr *iph=(struct iphdr *)buf->start;
     int32_t len;
 
-    if (iph->ihl < 5 || iph->version != 4) return False;
-    if (buf->size < iph->ihl*4) return False;
-    if (ip_fast_csum((uint8_t *)iph, iph->ihl)!=0) return False;
+    if (iph->ihl < 5) BAD("ihl %u",iph->ihl);
+    if (iph->version != 4) BAD("version %u",iph->version);
+    if (buf->size < iph->ihl*4) BAD("size %"PRId32"<%u*4",buf->size,iph->ihl);
+    if (ip_fast_csum((uint8_t *)iph, iph->ihl)!=0) BAD("csum");
     len=ntohs(iph->tot_len);
     /* There should be no padding */
-    if (buf->size!=len || len<(iph->ihl<<2)) return False;
+    if (buf->size!=len) BAD("len %"PRId32"!=%"PRId32,buf->size,len);
+    if (len<(iph->ihl<<2)) BAD("len %"PRId32"<(%u<<2)",len,iph->ihl);
     /* XXX check that there's no source route specified */
     return True;
+
+#undef BAD
 }
 
 /* Deliver a packet. "client" is the _origin_ of the packet, not its
@@ -595,11 +605,13 @@ static void netlink_incoming(struct netlink *st, struct netlink_client *client,
 {
     uint32_t source,dest;
     struct iphdr *iph;
+    char errmsgbuf[50];
 
     BUF_ASSERT_USED(buf);
-    if (!netlink_check(st,buf)) {
-	Message(M_WARNING,"%s: bad IP packet from %s\n",
-		st->name,client?client->name:"host");
+    if (!netlink_check(st,buf,errmsgbuf,sizeof(errmsgbuf))) {
+	Message(M_WARNING,"%s: bad IP packet from %s: %s\n",
+		st->name,client?client->name:"host",
+		errmsgbuf);
 	BUF_FREE(buf);
 	return;
     }
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:44 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:44 +0100
Subject: [PATCH 04/19] log: Print truncated messages
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-5-git-send-email-ijackson@chiark.greenend.org.uk>

If a message doesn't survive the vsnprintf in vMessage untruncated,
the result may be that its newline is lost.  After that, buff will
never manage to gain a newline and all further messages will be
discarded.

Fix this by always putting "\n\0" at the end of buff.  That way a
truncated message is printed and the buffer will be cleared out for
the next call.

This does mean that sometimes a message which is just slightly shorter
than the buffer will produce a spurious empty message sent to the
syslog.  However, the whole logging approach here will inevitably
occasionally insert spurious newlines, if only to split up long
messages; long messages may also be truncated.  The bad effect of the
change in this patch is simply to make the length at which
infelicities appear very slightly shorter.  The good effect is that it
turns a very nasty failure mode into a benign one.

So I think it is reasonable to err on the side of code which clearly
cannot go wrong in a bad way.  Other approaches which make the
infelicity threshold a couple of characters longer, or which slightly
reduce the different kinds of infelicity, are much more complicated
and therefore more likely to have bugs.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 log.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/log.c b/log.c
index 4f1b651..9cd0572 100644
--- a/log.c
+++ b/log.c
@@ -38,6 +38,8 @@ static void vMessage(uint32_t class, const char *message, va_list args)
 	bp=strlen(buff);
 	assert(bp < MESSAGE_BUFLEN);
 	vsnprintf(buff+bp,MESSAGE_BUFLEN-bp,message,args);
+	buff[sizeof(buff)-2] = '\n';
+	buff[sizeof(buff)-1] = '\0';
 	/* Each line is sent separately */
 	while ((nlp=strchr(buff,'\n'))) {
 	    *nlp=0;
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:56 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:56 +0100
Subject: [PATCH 16/19] site: Move current_transform,
	_key_timeout and remote_session_id into a struct
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-17-git-send-email-ijackson@chiark.greenend.org.uk>

We are going to introduce another set of these for the previous key,
and putting them together in this struct means we will be able to talk
about themm easily.

This patch is very mechanical: we move the three variables into the
new struct, and simply search-and-replace the references.

No functional change.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 site.c |   51 ++++++++++++++++++++++++++++-----------------------
 1 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/site.c b/site.c
index 5fa173b..1bf8dec 100644
--- a/site.c
+++ b/site.c
@@ -214,6 +214,12 @@ static void transport_xmit(struct site *st, transport_peers *peers,
  /***** END of transport peers declarations *****/
 
 
+struct data_key {
+    struct transform_inst_if *transform;
+    uint64_t key_timeout; /* End of life of current key */
+    uint32_t remote_session_id;
+};
+
 struct site {
     closure_t cl;
     struct site_if ops;
@@ -259,9 +265,7 @@ struct site {
     uint64_t now; /* Most recently seen time */
 
     /* The currently established session */
-    uint32_t remote_session_id;
-    struct transform_inst_if *current_transform;
-    uint64_t current_key_timeout; /* End of life of current key */
+    struct data_key current;
     uint64_t renegotiate_key_time; /* When we can negotiate a new key */
     transport_peers peers; /* Current address(es) of peer for data traffic */
 
@@ -325,7 +329,7 @@ static void activate_new_key(struct site *st);
 
 static bool_t current_valid(struct site *st)
 {
-    return st->current_transform->valid(st->current_transform->st);
+    return st->current.transform->valid(st->current.transform->st);
 }
 
 #define CHECK_AVAIL(b,l) do { if ((b)->size<(l)) return False; } while(0)
@@ -730,7 +734,7 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0)
     /* 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,
+    problem = st->current.transform->reverse(st->current.transform->st,
 					     msg0,&transform_err);
     if (!problem) return True;
 
@@ -871,16 +875,16 @@ static void activate_new_key(struct site *st)
 
     /* We have two transform instances, which we swap between active
        and setup */
-    t=st->current_transform;
-    st->current_transform=st->new_transform;
+    t=st->current.transform;
+    st->current.transform=st->new_transform;
     st->new_transform=t;
 
     t->delkey(t->st);
     st->timeout=0;
-    st->current_key_timeout=st->now+st->key_lifetime;
+    st->current.key_timeout=st->now+st->key_lifetime;
     st->renegotiate_key_time=st->now+st->key_renegotiate_time;
     transport_peers_copy(st,&st->peers,&st->setup_peers);
-    st->remote_session_id=st->setup_session_id;
+    st->current.remote_session_id=st->setup_session_id;
 
     slog(st,LOG_ACTIVATE_KEY,"new key activated");
     enter_state_run(st);
@@ -891,8 +895,8 @@ static void delete_key(struct site *st, cstring_t reason, uint32_t loglevel)
     if (current_valid(st)) {
 	slog(st,loglevel,"session closed (%s)",reason);
 
-	st->current_transform->delkey(st->current_transform->st);
-	st->current_key_timeout=0;
+	st->current.transform->delkey(st->current.transform->st);
+	st->current.key_timeout=0;
 	set_link_quality(st);
     }
 }
@@ -1031,11 +1035,11 @@ static bool_t send_msg7(struct site *st, cstring_t reason)
 	buffer_init(&st->buffer,st->transform->max_start_pad+(4*3));
 	buf_append_uint32(&st->buffer,LABEL_MSG7);
 	buf_append_string(&st->buffer,reason);
-	st->current_transform->forwards(st->current_transform->st,
+	st->current.transform->forwards(st->current.transform->st,
 					&st->buffer, &transform_err);
 	buf_prepend_uint32(&st->buffer,LABEL_MSG0);
 	buf_prepend_uint32(&st->buffer,st->index);
-	buf_prepend_uint32(&st->buffer,st->remote_session_id);
+	buf_prepend_uint32(&st->buffer,st->current.remote_session_id);
 	transport_xmit(st,&st->peers,&st->buffer,True);
 	BUF_FREE(&st->buffer);
 	return True;
@@ -1076,10 +1080,10 @@ static int site_beforepoll(void *sst, struct pollfd *fds, int *nfds_io,
     st->now=*now;
 
     /* Work out when our next timeout is. The earlier of 'timeout' or
-       'current_key_timeout'. A stored value of '0' indicates no timeout
+       'current.key_timeout'. A stored value of '0' indicates no timeout
        active. */
     site_settimeout(st->timeout, timeout_io);
-    site_settimeout(st->current_key_timeout, timeout_io);
+    site_settimeout(st->current.key_timeout, timeout_io);
 
     return 0; /* success */
 }
@@ -1102,7 +1106,7 @@ static void site_afterpoll(void *sst, struct pollfd *fds, int nfds)
 		 st->state);
 	}
     }
-    if (st->current_key_timeout && *now>st->current_key_timeout) {
+    if (st->current.key_timeout && *now>st->current.key_timeout) {
 	delete_key(st,"maximum key life exceeded",LOG_TIMEOUT_KEY);
     }
 }
@@ -1126,11 +1130,11 @@ static void site_outgoing(void *sst, struct buffer_if *buf)
 	/* Transform it and send it */
 	if (buf->size>0) {
 	    buf_prepend_uint32(buf,LABEL_MSG9);
-	    st->current_transform->forwards(st->current_transform->st,
+	    st->current.transform->forwards(st->current.transform->st,
 					    buf, &transform_err);
 	    buf_prepend_uint32(buf,LABEL_MSG0);
 	    buf_prepend_uint32(buf,st->index);
-	    buf_prepend_uint32(buf,st->remote_session_id);
+	    buf_prepend_uint32(buf,st->current.remote_session_id);
 	    transport_xmit(st,&st->peers,buf,False);
 	}
 	BUF_FREE(buf);
@@ -1210,7 +1214,7 @@ static bool_t site_incoming(void *sst, struct buffer_if *buf,
 	case 0: /* NAK */
 	    /* If the source is our current peer then initiate a key setup,
 	       because our peer's forgotten the key */
-	    if (get_uint32(buf->start+4)==st->remote_session_id) {
+	    if (get_uint32(buf->start+4)==st->current.remote_session_id) {
 		initiate_key_setup(st,"received a NAK");
 	    } else {
 		slog(st,LOG_SEC,"bad incoming NAK");
@@ -1272,10 +1276,11 @@ static bool_t site_incoming(void *sst, struct buffer_if *buf,
 		    slog(st,LOG_SEC,"invalid MSG5");
 		}
 	    } else if (st->state==SITE_RUN) {
-		if (process_msg5(st,buf,source,st->current_transform)) {
+		if (process_msg5(st,buf,source,st->current.transform)) {
 		    slog(st,LOG_DROP,"got MSG5, retransmitting MSG6");
 		    transport_setup_msgok(st,source);
-		    create_msg6(st,st->current_transform,st->remote_session_id);
+		    create_msg6(st,st->current.transform,
+				st->current.remote_session_id);
 		    transport_xmit(st,&st->peers,&st->buffer,True);
 		    BUF_FREE(&st->buffer);
 		} else {
@@ -1472,7 +1477,7 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context,
     register_for_poll(st, site_beforepoll, site_afterpoll, 0, "site");
     st->timeout=0;
 
-    st->current_key_timeout=0;
+    st->current.key_timeout=0;
     transport_peers_clear(st,&st->peers);
     transport_peers_clear(st,&st->setup_peers);
     /* XXX mlock these */
@@ -1499,7 +1504,7 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context,
     for (i=0; i<st->ncomms; i++)
 	st->comms[i]->request_notify(st->comms[i]->st, st, site_incoming);
 
-    st->current_transform=st->transform->create(st->transform->st);
+    st->current.transform=st->transform->create(st->transform->st);
     st->new_transform=st->transform->create(st->transform->st);
 
     enter_state_stop(st);
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:41 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:41 +0100
Subject: [PATCH 01/19] SECURITY: actually reject messages with improper lengths
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-2-git-send-email-ijackson@chiark.greenend.org.uk>

transform_reverse checks to see if the incoming message is a multiple
of the block cipher block size.  If the message fails this check, it
sets *errmsg but it fails to return.  Instead, it proceeds to attempt
to decrypt it.  This will involve a buffer read/write overrun.

It transform_reverse fails to check to see if the incoming message is
long enough to contain the IV, MAC and padding.  This can easily be
exploited to cause secnet to segfault.

buffer_unprepend should check that the buffer has enough data in it to
unprepend.  With the other patches, this should be irrelevant, but it
turns various other potential read overrun bugs into crashes.

site_incoming should check that the incoming message is long enough.
Again, without this, this is an buffer read overrun or possibly a
segfault.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 site.c      |    3 +++
 transform.c |    5 +++++
 util.c      |    1 +
 3 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/site.c b/site.c
index 624752c..0cd364b 100644
--- a/site.c
+++ b/site.c
@@ -1121,6 +1121,9 @@ static bool_t site_incoming(void *sst, struct buffer_if *buf,
 			    const struct comm_addr *source)
 {
     struct site *st=sst;
+
+    if (buf->size < 12) return False;
+
     uint32_t dest=ntohl(*(uint32_t *)buf->start);
 
     if (dest==0) {
diff --git a/transform.c b/transform.c
index 8fdf9fd..f55aa44 100644
--- a/transform.c
+++ b/transform.c
@@ -171,6 +171,10 @@ static uint32_t transform_reverse(void *sst, struct buffer_if *buf,
 	return 1;
     }
 
+    if (buf->size < 4 + 16 + 16) {
+	*errmsg="msg too short";
+	return 1;
+    }
 
     /* CBC */
     memset(iv,0,16);
@@ -181,6 +185,7 @@ static uint32_t transform_reverse(void *sst, struct buffer_if *buf,
     /* Assert bufsize is multiple of blocksize */
     if (buf->size&0xf) {
 	*errmsg="msg not multiple of cipher blocksize";
+	return 1;
     }
     serpent_encrypt(&ti->cryptkey,iv,iv);
     for (n=buf->start; n<buf->start+buf->size; n+=16)
diff --git a/util.c b/util.c
index 63fe76f..086c234 100644
--- a/util.c
+++ b/util.c
@@ -269,6 +269,7 @@ void *buf_unappend(struct buffer_if *buf, int32_t amount) {
 
 void *buf_unprepend(struct buffer_if *buf, int32_t amount) {
     void *p;
+    if (buf->size < amount) return 0;
     p=buf->start;
     buf->start+=amount;
     buf->size-=amount;
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:53 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:53 +0100
Subject: [PATCH 13/19] site: Deal with losing our MSG6 - retransmit MSG6 when
	we see MSG5 in RUN
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-14-git-send-email-ijackson@chiark.greenend.org.uk>

Fix the lost MSG6 bug in the case where we are the responder:

If we are in RUN and we see a SENTMSG5 encrypted with what we now
regard as the data transfer key, conclude that the peer must have lost
the MSG6.  Regenerate and transmit a new MSG6.

It is still possible, even after this patch, to get the two ends out
of sync: if, just after the responder has switched to the new key and
entered RUN, the network fails entirely for the key setup timeout, the
initiator will abandon the key setup and switch to WAIT, and continue
to use the new key.  However, the responder has already discarded the
old key.

To fix this is not trivial: it would require that at both ends be
willing to keep both old and new data keys available until the peer
has sent data with the new key (which might never happen).  If there
were also a key setup, it would then need three keys: the old data
key, the current data key which the peer hasn't started using yet, and
the fresh key it is trying to negotiate.  So we would have to a third
key, with its own lifetime expiry.

This requires a generalised version of generate_msg6 which allows the
new call site to specify which transform and session id to use, and
which doesn't set st->retries.  So break the meat of generate_msg6 out
into a new function which I have chosen to call create_msg6.  (The
fact that functions called `generate_msg*' set st->retries is slightly
unfortunate.)

We also have to add a transform argument to process_msg5 so that we
can try decrypting MSG5s with the data transfer key.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 site.c |   42 +++++++++++++++++++++++++++++-------------
 1 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/site.c b/site.c
index c0f4387..9f3ee85 100644
--- a/site.c
+++ b/site.c
@@ -642,15 +642,15 @@ static bool_t generate_msg5(struct site *st)
 }
 
 static bool_t process_msg5(struct site *st, struct buffer_if *msg5,
-			   const struct comm_addr *src)
+			   const struct comm_addr *src,
+			   struct transform_inst_if *transform)
 {
     struct msg0 m;
     cstring_t transform_err;
 
     if (!unpick_msg0(st,msg5,&m)) return False;
 
-    if (st->new_transform->reverse(st->new_transform->st,
-				   msg5,&transform_err)) {
+    if (transform->reverse(transform->st,msg5,&transform_err)) {
 	/* There's a problem */
 	slog(st,LOG_SEC,"process_msg5: transform: %s",transform_err);
 	return False;
@@ -666,7 +666,8 @@ static bool_t process_msg5(struct site *st, struct buffer_if *msg5,
     return True;
 }
 
-static bool_t generate_msg6(struct site *st)
+static void create_msg6(struct site *st, struct transform_inst_if *transform,
+			uint32_t session_id)
 {
     cstring_t transform_err;
 
@@ -676,12 +677,15 @@ static bool_t generate_msg6(struct site *st)
     /* Give the netlink code an opportunity to put its own stuff in the
        message (configuration information, etc.) */
     buf_prepend_uint32(&st->buffer,LABEL_MSG6);
-    st->new_transform->forwards(st->new_transform->st,&st->buffer,
-				&transform_err);
+    transform->forwards(transform->st,&st->buffer,&transform_err);
     buf_prepend_uint32(&st->buffer,LABEL_MSG6);
     buf_prepend_uint32(&st->buffer,st->index);
-    buf_prepend_uint32(&st->buffer,st->setup_session_id);
+    buf_prepend_uint32(&st->buffer,session_id);
+}
 
+static bool_t generate_msg6(struct site *st)
+{
+    create_msg6(st,st->new_transform,st->setup_session_id);
     st->retries=1; /* Peer will retransmit MSG5 if this packet gets lost */
     return True;
 }
@@ -1258,13 +1262,25 @@ static bool_t site_incoming(void *sst, struct buffer_if *buf,
 	       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)) {
-		transport_setup_msgok(st,source);
-		enter_new_state(st,SITE_RUN);
+	    if (st->state==SITE_SENTMSG4) {
+		if (process_msg5(st,buf,source,st->new_transform)) {
+		    transport_setup_msgok(st,source);
+		    enter_new_state(st,SITE_RUN);
+		} else {
+		    slog(st,LOG_SEC,"invalid MSG5");
+		}
+	    } else if (st->state==SITE_RUN) {
+		if (process_msg5(st,buf,source,st->current_transform)) {
+		    slog(st,LOG_DROP,"got MSG5, retransmitting MSG6");
+		    transport_setup_msgok(st,source);
+		    create_msg6(st,st->current_transform,st->remote_session_id);
+		    transport_xmit(st,&st->peers,&st->buffer,True);
+		    BUF_FREE(&st->buffer);
+		} else {
+		    slog(st,LOG_SEC,"invalid MSG5 (in state RUN)");
+		}
 	    } else {
-		slog(st,LOG_SEC,"invalid MSG5");
+		slog(st,LOG_UNEXPECTED,"unexpected MSG5");
 	    }
 	    break;
 	case LABEL_MSG6:
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:54 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:54 +0100
Subject: [PATCH 14/19] transform: add ->valid() function
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-15-git-send-email-ijackson@chiark.greenend.org.uk>

This allows outside callers to find out whether the transform has been
keyed, and can therefore relieve them of the need to track this
separately.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 secnet.h    |    2 ++
 transform.c |    8 ++++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/secnet.h b/secnet.h
index 855e099..44828ba 100644
--- a/secnet.h
+++ b/secnet.h
@@ -387,6 +387,7 @@ struct site_if {
 
 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 bool_t transform_valid_fn(void *st); /* 0: no key; 1: ok */
 typedef void transform_delkey_fn(void *st);
 typedef void transform_destroyinstance_fn(void *st);
 /* Returns:
@@ -400,6 +401,7 @@ typedef uint32_t transform_apply_fn(void *st, struct buffer_if *buf,
 struct transform_inst_if {
     void *st;
     transform_setkey_fn *setkey;
+    transform_valid_fn *valid;
     transform_delkey_fn *delkey;
     transform_apply_fn *forwards;
     transform_apply_fn *reverse;
diff --git a/transform.c b/transform.c
index f1da564..289b02e 100644
--- a/transform.c
+++ b/transform.c
@@ -68,6 +68,13 @@ static bool_t transform_setkey(void *sst, uint8_t *key, int32_t keylen)
     return True;
 }
 
+static bool_t transform_valid(void *sst)
+{
+    struct transform_inst *ti=sst;
+
+    return ti->keyed;
+}
+
 static void transform_delkey(void *sst)
 {
     struct transform_inst *ti=sst;
@@ -271,6 +278,7 @@ static struct transform_inst_if *transform_create(void *sst)
 
     ti->ops.st=ti;
     ti->ops.setkey=transform_setkey;
+    ti->ops.valid=transform_valid;
     ti->ops.delkey=transform_delkey;
     ti->ops.forwards=transform_forward;
     ti->ops.reverse=transform_reverse;
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:40 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:40 +0100
Subject: [PATCH v2 00/19] Security, logging and reliability fixes
Message-ID: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>

Important fixes, posted already (commit message for the log truncation
lockup fix has more justification now):

 01/19 SECURITY: actually reject messages with improper lengths
 02/19 Makefile: honour EXTRA_CFLAGS, etc.
 03/19 log: Eliminate potential out-of-control recursion
 04/19 log: Print truncated messages
 05/19 messages: add some missing newlines

One new bugfix:

 06/19 site: transport peers: fix incorrect stride when debug output enabled

Previously posted improvements:

 07/19 netlink: report why a packet is bad
 08/19 netlink: abolish check_config and output_config

If no-one objects, I intend to push the changes up to here to master
some time soon.

The remaining patches are new.  They fix a protocol design error which
could lead to two secnets disagreeing about which key they are trying
to use.  It comes in roughly two sub-series, and I have tried to break
it up into nicely reviewable pieces.  These need a lot more testing; I
intend to deploy them on xenophobe and zealot and see how they do.

 09/19 site: Break out separate function for decrypting msg0
 10/19 site: Remove pointless check from decrypt_msg0
 11/19 site, transform: Do not initiate rekey when packets too much out of
 12/19 site: Deal with losing peer's MSG6 - go to RUN on MSG0 with new key
 13/19 site: Deal with losing our MSG6 - retransmit MSG6 when we see MSG5 

 14/19 transform: add ->valid() function
 15/19 site: No longer track key validity ourselves
 16/19 site: Move current_transform, _key_timeout and remote_session_id in
 17/19 site: Generalise deletion and timeout of keys
 18/19 site: Keep old keys, and allow them to be used by peer
 19/19 site: When if our MSG5s (or peer's MSG6s) get lost, preserve the ke



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:46 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:46 +0100
Subject: [PATCH 06/19] site: transport peers: fix incorrect stride when debug
	output enabled
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-7-git-send-email-ijackson@chiark.greenend.org.uk>

When there are multiple peer addresses, attempts to copy them from one
table of peers addresses to another with transport_peers_copy will go
wrong because the stride argument to transport_peers_debug is wrong -
we take sizeof() the pointer rather than of the array element.

This will typically cause a segfault if it happens, but the bug can
only be triggered if LOG_PEER_ADDRS debugging is enabled.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 site.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/site.c b/site.c
index b54b34e..fcc36d8 100644
--- a/site.c
+++ b/site.c
@@ -1603,7 +1603,7 @@ static void transport_peers_copy(struct site *st, transport_peers *dst,
     dst->npeers=src->npeers;
     memcpy(dst->peers, src->peers, sizeof(*dst->peers) * dst->npeers);
     transport_peers_debug(st,dst,"copy",
-			  src->npeers, &src->peers->addr, sizeof(src->peers));
+			  src->npeers, &src->peers->addr, sizeof(*src->peers));
 }
 
 void transport_xmit(struct site *st, transport_peers *peers,
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:49 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:49 +0100
Subject: [PATCH 09/19] site: Break out separate function for decrypting msg0
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-10-git-send-email-ijackson@chiark.greenend.org.uk>

The control flow here is going to become more complicated, and this
will the next patches, and the resulting code, clearer.

Note that process_msg0's return value is never used; it is only
defined to return bool_t so that it can use the CHECK_AVAIL macro.
Knowing this will make it easier to see that the patch is correct.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 site.c |   33 ++++++++++++++++++++++-----------
 1 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/site.c b/site.c
index 05206f6..f65051f 100644
--- a/site.c
+++ b/site.c
@@ -709,26 +709,37 @@ static bool_t process_msg6(struct site *st, struct buffer_if *msg6,
     return True;
 }
 
-static bool_t process_msg0(struct site *st, struct buffer_if *msg0,
-			   const struct comm_addr *src)
+static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0)
 {
-    struct msg0 m;
     cstring_t transform_err;
-    uint32_t type;
+    struct msg0 m;
+    uint32_t problem;
 
     if (!st->current_valid) {
 	slog(st,LOG_DROP,"incoming message but no current key -> dropping");
-	return initiate_key_setup(st,"incoming message but no current key");
+	initiate_key_setup(st,"incoming message but no current key");
+	return False;
     }
 
     if (!unpick_msg0(st,msg0,&m)) return False;
 
-    if (st->current_transform->reverse(st->current_transform->st,
-				       msg0,&transform_err)) {
-	/* There's a problem */
-	slog(st,LOG_SEC,"transform: %s",transform_err);
-	return initiate_key_setup(st,"incoming message would not decrypt");
-    }
+    problem = st->current_transform->reverse(st->current_transform->st,
+					     msg0,&transform_err);
+    if (!problem) return True;
+
+    slog(st,LOG_SEC,"transform: %s",transform_err);
+    initiate_key_setup(st,"incoming message would not decrypt");
+    return False;
+}
+
+static bool_t process_msg0(struct site *st, struct buffer_if *msg0,
+			   const struct comm_addr *src)
+{
+    uint32_t type;
+
+    if (!decrypt_msg0(st,msg0))
+	return False;
+
     CHECK_AVAIL(msg0,4);
     type=buf_unprepend_uint32(msg0);
     switch(type) {
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:58 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:58 +0100
Subject: [PATCH 18/19] site: Keep old keys, and allow them to be used by peer
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-19-git-send-email-ijackson@chiark.greenend.org.uk>

After we have switched to a new key, keep the old key around until we
see packets using the new key.

This is part of the fix to the "connectivity loss during final key
setup" bug.  Fixing this requires that both ends be willing to keep
both old and new data keys available until the peer has sent data with
the new key (which might never happen).

If there were also a key setup, the site would then need three keys:
the old data key, the current data key which the peer hasn't started
using yet, and the fresh key it is trying to negotiate.  So we have to
a third key, with its own lifetime expiry.

In the code we call the old key the "auxiliary" key because we're
going to use it for an additional fixup in the next patch.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 site.c |   33 +++++++++++++++++++++++++++------
 1 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/site.c b/site.c
index a91c1be..4358cad 100644
--- a/site.c
+++ b/site.c
@@ -266,6 +266,7 @@ struct site {
 
     /* The currently established session */
     struct data_key current;
+    struct data_key auxiliary_key;
     uint64_t renegotiate_key_time; /* When we can negotiate a new key */
     transport_peers peers; /* Current address(es) of peer for data traffic */
 
@@ -729,7 +730,7 @@ 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, newkey_err="n/a";
+    cstring_t transform_err, auxkey_err, newkey_err="n/a";
     struct msg0 m;
     uint32_t problem;
 
@@ -740,13 +741,25 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0)
 
     problem = st->current.transform->reverse(st->current.transform->st,
 					     msg0,&transform_err);
-    if (!problem) return True;
+    if (!problem) {
+	delete_one_key(st,&st->auxiliary_key,
+		       "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;
     }
 
+    buffer_copy(msg0, &st->scratch);
+    problem = st->auxiliary_key.transform->reverse
+	(st->auxiliary_key.transform->st,msg0,&auxkey_err);
+    if (problem==0) {
+	slog(st,LOG_DROP,"processing packet which uses auxiliary key");
+	return True;
+    }
+
     if (st->state==SITE_SENTMSG5) {
 	buffer_copy(msg0, &st->scratch);
 	if (!st->new_transform->reverse(st->new_transform->st,
@@ -761,7 +774,8 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0)
 	}
     }
 
-    slog(st,LOG_SEC,"transform: %s (new: %s)",transform_err,newkey_err);
+    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");
     return False;
 }
@@ -877,14 +891,16 @@ static void activate_new_key(struct site *st)
 {
     struct transform_inst_if *t;
 
-    /* We have two transform instances, which we swap between active
-       and setup */
-    t=st->current.transform;
+    /* We have three transform instances, which we swap between old,
+       active and setup */
+    t=st->auxiliary_key.transform;
+    st->auxiliary_key.transform=st->current.transform;
     st->current.transform=st->new_transform;
     st->new_transform=t;
 
     t->delkey(t->st);
     st->timeout=0;
+    st->auxiliary_key.key_timeout=st->current.key_timeout;
     st->current.key_timeout=st->now+st->key_lifetime;
     st->renegotiate_key_time=st->now+st->key_renegotiate_time;
     transport_peers_copy(st,&st->peers,&st->setup_peers);
@@ -911,6 +927,7 @@ static void delete_keys(struct site *st, cstring_t reason, uint32_t loglevel)
 	delete_one_key(st,&st->current,0,0,0);
 	set_link_quality(st);
     }
+    delete_one_key(st,&st->auxiliary_key,0,0,0);
 }
 
 static void state_assert(struct site *st, bool_t ok)
@@ -1096,6 +1113,7 @@ static int site_beforepoll(void *sst, struct pollfd *fds, int *nfds_io,
        active. */
     site_settimeout(st->timeout, timeout_io);
     site_settimeout(st->current.key_timeout, timeout_io);
+    site_settimeout(st->auxiliary_key.key_timeout, timeout_io);
 
     return 0; /* success */
 }
@@ -1127,6 +1145,7 @@ static void site_afterpoll(void *sst, struct pollfd *fds, int nfds)
 	}
     }
     check_expiry(st,&st->current,"current key");
+    check_expiry(st,&st->auxiliary_key,"auxiliary key");
 }
 
 /* This function is called by the netlink device to deliver packets
@@ -1496,6 +1515,7 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context,
     st->timeout=0;
 
     st->current.key_timeout=0;
+    st->auxiliary_key.key_timeout=0;
     transport_peers_clear(st,&st->peers);
     transport_peers_clear(st,&st->setup_peers);
     /* XXX mlock these */
@@ -1523,6 +1543,7 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context,
 	st->comms[i]->request_notify(st->comms[i]->st, st, site_incoming);
 
     st->current.transform=st->transform->create(st->transform->st);
+    st->auxiliary_key.transform=st->transform->create(st->transform->st);
     st->new_transform=st->transform->create(st->transform->st);
 
     enter_state_stop(st);
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:57 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:57 +0100
Subject: [PATCH 17/19] site: Generalise deletion and timeout of keys
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-18-git-send-email-ijackson@chiark.greenend.org.uk>

Introduce delete_one_key, and rename delete_key to delete_keys.  We
distinguish, now, between deleting a single key, an deleting all the
keys for this site.

The expiry check calls delete_one_key rather than delete_keys, and is
likewise done with a helper function.

No functional change other than to the key expiry log message.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 site.c |   36 +++++++++++++++++++++++++++---------
 1 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/site.c b/site.c
index 1bf8dec..a91c1be 100644
--- a/site.c
+++ b/site.c
@@ -319,7 +319,11 @@ static void slog(struct site *st, uint32_t event, cstring_t msg, ...)
 }
 
 static void set_link_quality(struct site *st);
-static void delete_key(struct site *st, cstring_t reason, uint32_t loglevel);
+static void delete_keys(struct site *st, cstring_t reason, uint32_t loglevel);
+static void delete_one_key(struct site *st, struct data_key *key,
+			   const char *reason /* may be 0 meaning don't log*/,
+			   const char *which /* ignored if !reasonn */,
+			   uint32_t loglevel /* ignored if !reasonn */);
 static bool_t initiate_key_setup(struct site *st, cstring_t reason);
 static void enter_state_run(struct site *st);
 static bool_t enter_state_resolve(struct site *st);
@@ -775,7 +779,7 @@ static bool_t process_msg0(struct site *st, struct buffer_if *msg0,
     switch(type) {
     case LABEL_MSG7:
 	/* We must forget about the current session. */
-	delete_key(st,"request from peer",LOG_SEC);
+	delete_keys(st,"request from peer",LOG_SEC);
 	return True;
     case LABEL_MSG9:
 	/* Deliver to netlink layer */
@@ -890,13 +894,21 @@ static void activate_new_key(struct site *st)
     enter_state_run(st);
 }
 
-static void delete_key(struct site *st, cstring_t reason, uint32_t loglevel)
+static void delete_one_key(struct site *st, struct data_key *key,
+			   cstring_t reason, cstring_t which, uint32_t loglevel)
+{
+    if (!key->transform->valid(key->transform->st)) return;
+    if (reason) slog(st,loglevel,"%s deleted (%s)",which,reason);
+    key->transform->delkey(key->transform->st);
+    key->key_timeout=0;
+}
+
+static void delete_keys(struct site *st, cstring_t reason, uint32_t loglevel)
 {
     if (current_valid(st)) {
 	slog(st,loglevel,"session closed (%s)",reason);
 
-	st->current.transform->delkey(st->current.transform->st);
-	st->current.key_timeout=0;
+	delete_one_key(st,&st->current,0,0,0);
 	set_link_quality(st);
     }
 }
@@ -910,7 +922,7 @@ static void enter_state_stop(struct site *st)
 {
     st->state=SITE_STOP;
     st->timeout=0;
-    delete_key(st,"entering state STOP",LOG_TIMEOUT_KEY);
+    delete_keys(st,"entering state STOP",LOG_TIMEOUT_KEY);
     st->new_transform->delkey(st->new_transform->st);
 }
 
@@ -1088,6 +1100,14 @@ static int site_beforepoll(void *sst, struct pollfd *fds, int *nfds_io,
     return 0; /* success */
 }
 
+static void check_expiry(struct site *st, struct data_key *key,
+			 const char *which)
+{
+    if (key->key_timeout && *now>key->key_timeout) {
+	delete_one_key(st,key,"maximum life exceeded",which,LOG_TIMEOUT_KEY);
+    }
+}
+
 /* NB site_afterpoll will be called before site_beforepoll is ever called */
 static void site_afterpoll(void *sst, struct pollfd *fds, int nfds)
 {
@@ -1106,9 +1126,7 @@ static void site_afterpoll(void *sst, struct pollfd *fds, int nfds)
 		 st->state);
 	}
     }
-    if (st->current.key_timeout && *now>st->current.key_timeout) {
-	delete_key(st,"maximum key life exceeded",LOG_TIMEOUT_KEY);
-    }
+    check_expiry(st,&st->current,"current key");
 }
 
 /* This function is called by the netlink device to deliver packets
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:43 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:43 +0100
Subject: [PATCH 03/19] log: Eliminate potential out-of-control recursion
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-4-git-send-email-ijackson@chiark.greenend.org.uk>

vMessage looks for the system log instance.  If there is one, it uses
it; otherwise it just uses stderr or stdout.

The system log instance consists, eventually, of syslog_vlog or
logfile_vlog.  Both of these functions have a fallback: if they are
properly set up they do their thing; otherwise they call vMessage.
This is wrong because in this situation they have probably been called
by vMessage so it doesn't help.

At first sight this ought to produce unbounded recursion but for
complicated reasons another bug prevents this.  Instead, messages can
just vanish.

Break out the fallback mode of [v]Message into [v]MessageFallback so
that syslog_vlog and logfile_vlog can use it directly.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 log.c |   38 ++++++++++++++++++++++++++------------
 1 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/log.c b/log.c
index 3a50a27..4f1b651 100644
--- a/log.c
+++ b/log.c
@@ -14,9 +14,20 @@ bool_t secnet_is_daemon=False;
 uint32_t message_level=M_WARNING|M_ERR|M_SECURITY|M_FATAL;
 struct log_if *system_log=NULL;
 
-static void vMessage(uint32_t class, const char *message, va_list args)
+static void vMessageFallback(uint32_t class, const char *message, va_list args)
 {
     FILE *dest=stdout;
+    /* Messages go to stdout/stderr */
+    if (class & message_level) {
+	if (class&M_FATAL || class&M_ERR || class&M_WARNING) {
+	    dest=stderr;
+	}
+	vfprintf(dest,message,args);
+    }
+}
+
+static void vMessage(uint32_t class, const char *message, va_list args)
+{
 #define MESSAGE_BUFLEN 1023
     static char buff[MESSAGE_BUFLEN+1]={0,};
     size_t bp;
@@ -34,13 +45,7 @@ static void vMessage(uint32_t class, const char *message, va_list args)
 	    memmove(buff,nlp+1,strlen(nlp+1)+1);
 	}
     } else {
-	/* Messages go to stdout/stderr */
-	if (class & message_level) {
-	    if (class&M_FATAL || class&M_ERR || class&M_WARNING) {
-		dest=stderr;
-	    }
-	    vfprintf(dest,message,args);
-	}
+	vMessageFallback(class,message,args);
     }
 }  
 
@@ -53,6 +58,15 @@ void Message(uint32_t class, const char *message, ...)
     va_end(ap);
 }
 
+static void MessageFallback(uint32_t class, const char *message, ...)
+{
+    va_list ap;
+
+    va_start(ap,message);
+    vMessageFallback(class,message,ap);
+    va_end(ap);
+}
+
 static NORETURN(vfatal(int status, bool_t perror, const char *message,
 		       va_list args));
 
@@ -262,8 +276,8 @@ static void logfile_vlog(void *sst, int class, const char *message,
 	    fflush(st->f);
 	}
     } else {
-	vMessage(class,message,args);
-	Message(class,"\n");
+	vMessageFallback(class,message,args);
+	MessageFallback(class,"\n");
     }
 }
 
@@ -391,8 +405,8 @@ static void syslog_vlog(void *sst, int class, const char *message,
     if (st->open)
 	vsyslog(msgclass_to_syslogpriority(class),message,args);
     else {
-	vMessage(class,message,args);
-	Message(class,"\n");
+	vMessageFallback(class,message,args);
+	MessageFallback(class,"\n");
     }
 }
 
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:59 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:59 +0100
Subject: [PATCH 19/19] site: When if our MSG5s (or peer's MSG6s) get lost,
	preserve the key
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-20-git-send-email-ijackson@chiark.greenend.org.uk>

When we time out in state SENTMSG5, keep the key we negotiated.
SENTMSG5 gives the peer permission to start sending packets with it so
we need to be able to decrypt them.  If we see such packets, we switch
to using the new key at that point and throw the old key away.

This is the final fix to the "connectivity loss during final key
setup can cause locked-up session" bug.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 site.c |   42 ++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/site.c b/site.c
index 4358cad..db65bd8 100644
--- a/site.c
+++ b/site.c
@@ -267,7 +267,9 @@ struct site {
     /* The currently established session */
     struct data_key current;
     struct data_key auxiliary_key;
+    bool_t auxiliary_is_new;
     uint64_t renegotiate_key_time; /* When we can negotiate a new key */
+    uint64_t auxiliary_renegotiate_key_time;
     transport_peers peers; /* Current address(es) of peer for data traffic */
 
     /* The current key setup protocol exchange.  We can only be
@@ -742,8 +744,9 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0)
     problem = st->current.transform->reverse(st->current.transform->st,
 					     msg0,&transform_err);
     if (!problem) {
-	delete_one_key(st,&st->auxiliary_key,
-		       "peer has used new key","auxiliary key",LOG_SEC);
+	if (!st->auxiliary_is_new)
+	    delete_one_key(st,&st->auxiliary_key,
+			   "peer has used new key","auxiliary key",LOG_SEC);
 	return True;
     }
 
@@ -757,6 +760,21 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0)
 	(st->auxiliary_key.transform->st,msg0,&auxkey_err);
     if (problem==0) {
 	slog(st,LOG_DROP,"processing packet which uses auxiliary key");
+	if (st->auxiliary_is_new) {
+	    /* We previously timed out in state SENTMSG5 but it turns
+	     * out that our peer did in fact get our MSG5 and is
+	     * using the new key.  So we should switch to it too. */
+	    /* This is a bit like activate_new_key. */
+	    struct data_key t;
+	    t=st->current;
+	    st->current=st->auxiliary_key;
+	    st->auxiliary_key=t;
+
+	    delete_one_key(st,&st->auxiliary_key,"peer has used new key",
+			   "previous key",LOG_SEC);
+	    st->auxiliary_is_new=0;
+	    st->renegotiate_key_time=st->auxiliary_renegotiate_key_time;
+	}
 	return True;
     }
 
@@ -836,6 +854,24 @@ static bool_t send_msg(struct site *st)
 	st->timeout=st->now+st->setup_retry_interval;
 	st->retries--;
 	return True;
+    } else if (st->state==SITE_SENTMSG5) {
+	slog(st,LOG_SETUP_TIMEOUT,"timed out sending MSG5, stashing new key");
+	/* We stash the key we have produced, in case it turns out that
+	 * our peer did see our MSG5 after all and starts using it. */
+	/* This is a bit like some of activate_new_key */
+	struct transform_inst_if *t;
+	t=st->auxiliary_key.transform;
+	st->auxiliary_key.transform=st->new_transform;
+	st->new_transform=t;
+
+	t->delkey(t->st);
+	st->auxiliary_is_new=1;
+	st->auxiliary_key.key_timeout=st->now+st->key_lifetime;
+	st->auxiliary_renegotiate_key_time=st->now+st->key_renegotiate_time;
+	st->auxiliary_key.remote_session_id=st->setup_session_id;
+
+	enter_state_wait(st);
+	return False;
     } else {
 	slog(st,LOG_SETUP_TIMEOUT,"timed out sending key setup packet "
 	    "(in state %s)",state_name(st->state));
@@ -900,6 +936,7 @@ static void activate_new_key(struct site *st)
 
     t->delkey(t->st);
     st->timeout=0;
+    st->auxiliary_is_new=0;
     st->auxiliary_key.key_timeout=st->current.key_timeout;
     st->current.key_timeout=st->now+st->key_lifetime;
     st->renegotiate_key_time=st->now+st->key_renegotiate_time;
@@ -1545,6 +1582,7 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context,
     st->current.transform=st->transform->create(st->transform->st);
     st->auxiliary_key.transform=st->transform->create(st->transform->st);
     st->new_transform=st->transform->create(st->transform->st);
+    st->auxiliary_is_new=0;
 
     enter_state_stop(st);
 
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:42 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:42 +0100
Subject: [PATCH 02/19] Makefile: honour EXTRA_CFLAGS, etc.
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-3-git-send-email-ijackson@chiark.greenend.org.uk>

This makes it easier to add command-line options at build-time.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 Makefile.in |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile.in b/Makefile.in
index c9bb82b..182204b 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -38,10 +38,10 @@ CFLAGS:=-Wall @WRITESTRINGS@ @CFLAGS@ -Werror \
 	-Wpointer-arith -Wformat=2 -Winit-self \
 	-Wswitch-enum -Wunused-variable -Wbad-function-cast \
 	-Wno-strict-aliasing -fno-strict-aliasing
-ALL_CFLAGS:=@DEFS@ -I$(srcdir) -I. $(CFLAGS)
-CPPFLAGS:=@CPPFLAGS@
-LDFLAGS:=@LDFLAGS@
-LDLIBS:=@LIBS@
+ALL_CFLAGS:=@DEFS@ -I$(srcdir) -I. $(CFLAGS) $(EXTRA_CFLAGS)
+CPPFLAGS:=@CPPFLAGS@ $(EXTRA_CPPFLAGS)
+LDFLAGS:=@LDFLAGS@ $(EXTRA_LDFLAGS)
+LDLIBS:=@LIBS@ $(EXTRA_LDLIBS)
 
 prefix:=@prefix@
 exec_prefix:=@exec_prefix@
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:55 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:55 +0100
Subject: [PATCH 15/19] site: No longer track key validity ourselves
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-16-git-send-email-ijackson@chiark.greenend.org.uk>

We used to have a variable st->current_valid, which we would always
set to True when assigning a key to current_transform, and to False
when calling delkey (or on initialisation).

Now that we have transform->valid(), this is no longer needed.

We do introduce a trivial helper function current_valid(st) to help us
call it, so the places where st->current_valid was checked don't get
too ugly.

There is no intentional change to the externally-visible behaviour.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 site.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/site.c b/site.c
index 9f3ee85..5fa173b 100644
--- a/site.c
+++ b/site.c
@@ -261,7 +261,6 @@ struct site {
     /* The currently established session */
     uint32_t remote_session_id;
     struct transform_inst_if *current_transform;
-    bool_t current_valid;
     uint64_t current_key_timeout; /* End of life of current key */
     uint64_t renegotiate_key_time; /* When we can negotiate a new key */
     transport_peers peers; /* Current address(es) of peer for data traffic */
@@ -324,6 +323,11 @@ 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);
 
+static bool_t current_valid(struct site *st)
+{
+    return st->current_transform->valid(st->current_transform->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)
 #define CHECK_TYPE(b,t) do { uint32_t type; \
@@ -873,7 +877,6 @@ static void activate_new_key(struct site *st)
 
     t->delkey(t->st);
     st->timeout=0;
-    st->current_valid=True;
     st->current_key_timeout=st->now+st->key_lifetime;
     st->renegotiate_key_time=st->now+st->key_renegotiate_time;
     transport_peers_copy(st,&st->peers,&st->setup_peers);
@@ -885,10 +888,9 @@ static void activate_new_key(struct site *st)
 
 static void delete_key(struct site *st, cstring_t reason, uint32_t loglevel)
 {
-    if (st->current_valid) {
+    if (current_valid(st)) {
 	slog(st,loglevel,"session closed (%s)",reason);
 
-	st->current_valid=False;
 	st->current_transform->delkey(st->current_transform->st);
 	st->current_key_timeout=0;
 	set_link_quality(st);
@@ -911,7 +913,7 @@ static void enter_state_stop(struct site *st)
 static void set_link_quality(struct site *st)
 {
     uint32_t quality;
-    if (st->current_valid)
+    if (current_valid(st))
 	quality=LINK_QUALITY_UP;
     else if (st->state==SITE_WAIT || st->state==SITE_STOP)
 	quality=LINK_QUALITY_DOWN;
@@ -1023,7 +1025,7 @@ static bool_t send_msg7(struct site *st, cstring_t reason)
 {
     cstring_t transform_err;
 
-    if (st->current_valid && st->buffer.free
+    if (current_valid(st) && st->buffer.free
 	&& transport_peers_valid(&st->peers)) {
 	BUF_ALLOC(&st->buffer,"site:MSG7");
 	buffer_init(&st->buffer,st->transform->max_start_pad+(4*3));
@@ -1120,7 +1122,7 @@ static void site_outgoing(void *sst, struct buffer_if *buf)
 
     /* In all other states we consider delivering the packet if we have
        a valid key and a valid address to send it to. */
-    if (st->current_valid && transport_peers_valid(&st->peers)) {
+    if (current_valid(st) && transport_peers_valid(&st->peers)) {
 	/* Transform it and send it */
 	if (buf->size>0) {
 	    buf_prepend_uint32(buf,LABEL_MSG9);
@@ -1470,7 +1472,6 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context,
     register_for_poll(st, site_beforepoll, site_afterpoll, 0, "site");
     st->timeout=0;
 
-    st->current_valid=False;
     st->current_key_timeout=0;
     transport_peers_clear(st,&st->peers);
     transport_peers_clear(st,&st->setup_peers);
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:51 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:51 +0100
Subject: [PATCH 11/19] site,
	transform: Do not initiate rekey when packets too much out of order
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-12-git-send-email-ijackson@chiark.greenend.org.uk>

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 at chiark.greenend.org.uk>
---
 secnet.h    |    6 +++++-
 site.c      |    5 +++++
 transform.c |    2 +-
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/secnet.h b/secnet.h
index 937ee9c..855e099 100644
--- 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);
-/* 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);
 
diff --git a/site.c b/site.c
index 4d3a612..4dbebaf 100644
--- 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;
 
+    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;
diff --git a/transform.c b/transform.c
index f55aa44..f1da564 100644
--- 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";
-	return 1;
+	return 2;
     }
     
     return 0;
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:48 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:48 +0100
Subject: [PATCH 08/19] netlink: abolish check_config and output_config
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-9-git-send-email-ijackson@chiark.greenend.org.uk>

Apparently, a long time ago, MSG5 and MSG6 used to contain some
netlink configuration data, which the receiver of the MSG5 or MSG6
would check.

However, for a long time now the output_config function has been a
no-op and the check function has unconditionally eaten and discarded
anything extra in the message.

Furthermore, because the MSG6 is not retransmitted, this mechanism
couldn't be reliable without a protocol change.  So the existing
interface is defective.

So, abolish it the interface, the dummy implementation, and all the
call sites.  The check_config call sites in site.c now instead
directly discard any unexpected data at the end of MSG5 and MSG6.

This patch should cause no behavioural change in actual operation.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 netlink.c |   23 -----------------------
 secnet.h  |    2 --
 site.c    |   16 ++++------------
 3 files changed, 4 insertions(+), 37 deletions(-)

diff --git a/netlink.c b/netlink.c
index 559821d..fdad4ff 100644
--- a/netlink.c
+++ b/netlink.c
@@ -803,27 +803,6 @@ static void netlink_signal_handler(void *sst, int signum)
     netlink_dump_routes(st,True);
 }
 
-static void netlink_inst_output_config(void *sst, struct buffer_if *buf)
-{
-/*    struct netlink_client *c=sst; */
-/*    struct netlink *st=c->nst; */
-
-    /* For now we don't output anything */
-    BUF_ASSERT_USED(buf);
-}
-
-static bool_t netlink_inst_check_config(void *sst, struct buffer_if *buf)
-{
-/*    struct netlink_client *c=sst; */
-/*    struct netlink *st=c->nst; */
-
-    BUF_ASSERT_USED(buf);
-    /* We need to eat all of the configuration information from the buffer
-       for backward compatibility. */
-    buf->size=0;
-    return True;
-}
-
 static void netlink_inst_set_mtu(void *sst, int32_t new_mtu)
 {
     struct netlink_client *c=sst;
@@ -910,8 +889,6 @@ static closure_t *netlink_inst_create(struct netlink *st,
     c->ops.reg=netlink_inst_reg;
     c->ops.deliver=netlink_inst_incoming;
     c->ops.set_quality=netlink_set_quality;
-    c->ops.output_config=netlink_inst_output_config;
-    c->ops.check_config=netlink_inst_check_config;
     c->ops.set_mtu=netlink_inst_set_mtu;
     c->nst=st;
 
diff --git a/secnet.h b/secnet.h
index 06beb05..937ee9c 100644
--- a/secnet.h
+++ b/secnet.h
@@ -437,8 +437,6 @@ struct netlink_if {
     netlink_register_fn *reg;
     netlink_deliver_fn *deliver;
     netlink_link_quality_fn *set_quality;
-    netlink_output_config_fn *output_config;
-    netlink_check_config_fn *check_config;
     netlink_set_mtu_fn *set_mtu;
 };
 
diff --git a/site.c b/site.c
index fcc36d8..05206f6 100644
--- a/site.c
+++ b/site.c
@@ -628,7 +628,6 @@ static bool_t generate_msg5(struct site *st)
     buffer_init(&st->buffer,st->transform->max_start_pad+(4*4));
     /* Give the netlink code an opportunity to put its own stuff in the
        message (configuration information, etc.) */
-    st->netlink->output_config(st->netlink->st,&st->buffer);
     buf_prepend_uint32(&st->buffer,LABEL_MSG5);
     st->new_transform->forwards(st->new_transform->st,&st->buffer,
 				&transform_err);
@@ -660,11 +659,8 @@ static bool_t process_msg5(struct site *st, struct buffer_if *msg5,
 	slog(st,LOG_SEC,"MSG5/PING packet contained wrong label");
 	return False;
     }
-    if (!st->netlink->check_config(st->netlink->st,msg5)) {
-	slog(st,LOG_SEC,"MSG5/PING packet contained bad netlink config");
-	return False;
-    }
-    CHECK_EMPTY(msg5);
+    /* Older versions of secnet used to write some config data here
+     * which we ignore.  So we don't CHECK_EMPTY */
     return True;
 }
 
@@ -677,7 +673,6 @@ static bool_t generate_msg6(struct site *st)
     buffer_init(&st->buffer,st->transform->max_start_pad+(4*4));
     /* Give the netlink code an opportunity to put its own stuff in the
        message (configuration information, etc.) */
-    st->netlink->output_config(st->netlink->st,&st->buffer);
     buf_prepend_uint32(&st->buffer,LABEL_MSG6);
     st->new_transform->forwards(st->new_transform->st,&st->buffer,
 				&transform_err);
@@ -709,11 +704,8 @@ static bool_t process_msg6(struct site *st, struct buffer_if *msg6,
 	slog(st,LOG_SEC,"MSG6/PONG packet contained invalid data");
 	return False;
     }
-    if (!st->netlink->check_config(st->netlink->st,msg6)) {
-	slog(st,LOG_SEC,"MSG6/PONG packet contained bad netlink config");
-	return False;
-    }
-    CHECK_EMPTY(msg6);
+    /* Older versions of secnet used to write some config data here
+     * which we ignore.  So we don't CHECK_EMPTY */
     return True;
 }
 
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:45 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:45 +0100
Subject: [PATCH 05/19] messages: add some missing newlines
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-6-git-send-email-ijackson@chiark.greenend.org.uk>

Message and cfgfatal must be called with a message containing a
newline (or, a newline sent in a later call).  Fix a few call sites.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 log.c     |    4 ++--
 process.c |    2 +-
 rsa.c     |    8 ++++----
 site.c    |    7 ++++---
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/log.c b/log.c
index 9cd0572..d940df5 100644
--- a/log.c
+++ b/log.c
@@ -170,11 +170,11 @@ void cfgfile_postreadcheck(struct cloc loc, FILE *f)
 {
     assert(loc.file);
     if (ferror(f)) {
-	Message(M_FATAL, "error reading config file (%s): %s",
+	Message(M_FATAL, "error reading config file (%s): %s\n",
 		loc.file, strerror(errno));
 	exit(current_phase);
     } else if (feof(f)) {
-	Message(M_FATAL, "unexpected end of config file (%s)", loc.file);
+	Message(M_FATAL, "unexpected end of config file (%s)\n", loc.file);
 	exit(current_phase);
     }
 }
diff --git a/process.c b/process.c
index 0a7a0f2..a9ff3d9 100644
--- a/process.c
+++ b/process.c
@@ -152,7 +152,7 @@ int sys_cmd(const char *path, const char *arg, ...)
 			path, arg, WTERMSIG(rv), strsignal(WTERMSIG(rv)),
 			WCOREDUMP(rv) ? " - core dumped" : "");
 	    else
-		Message(M_ERR, "sys_cmd(%s,%s,...) exited with wstat %#x",
+		Message(M_ERR, "sys_cmd(%s,%s,...) exited with wstat %#x\n",
 			path, arg, rv);
 	}
     } else if (c==0) {
diff --git a/rsa.c b/rsa.c
index caa030b..0bd106f 100644
--- a/rsa.c
+++ b/rsa.c
@@ -194,7 +194,7 @@ static list_t *rsapub_apply(closure_t *self, struct cloc loc, dict_t *context,
     i=list_elem(args,0);
     if (i) {
 	if (i->type!=t_string) {
-	    cfgfatal(i->loc,"rsa-public","first argument must be a string");
+	    cfgfatal(i->loc,"rsa-public","first argument must be a string\n");
 	}
 	e=i->data.string;
 	if (mpz_init_set_str(&st->e,e,10)!=0) {
@@ -208,7 +208,7 @@ static list_t *rsapub_apply(closure_t *self, struct cloc loc, dict_t *context,
     i=list_elem(args,1);
     if (i) {
 	if (i->type!=t_string) {
-	    cfgfatal(i->loc,"rsa-public","second argument must be a string");
+	    cfgfatal(i->loc,"rsa-public","second argument must be a string\n");
 	}
 	n=i->data.string;
 	if (mpz_init_set_str(&st->n,n,10)!=0) {
@@ -267,7 +267,7 @@ static list_t *rsapriv_apply(closure_t *self, struct cloc loc, dict_t *context,
     i=list_elem(args,0);
     if (i) {
 	if (i->type!=t_string) {
-	    cfgfatal(i->loc,"rsa-public","first argument must be a string");
+	    cfgfatal(i->loc,"rsa-public","first argument must be a string\n");
 	}
 	filename=i->data.string;
     } else {
@@ -313,7 +313,7 @@ static list_t *rsapriv_apply(closure_t *self, struct cloc loc, dict_t *context,
     }
     b=safe_malloc(length,"rsapriv_apply");
     if (fread(b,length,1,f) != 1) {
-	cfgfatal_maybefile(f,loc,"rsa-private","error reading modulus");
+	cfgfatal_maybefile(f,loc,"rsa-private","error reading modulus\n");
     }
     mpz_init(&st->n);
     read_mpbin(&st->n,b,length);
diff --git a/site.c b/site.c
index 0cd364b..b54b34e 100644
--- a/site.c
+++ b/site.c
@@ -1342,15 +1342,16 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context,
     st->netlink=find_cl_if(dict,"link",CL_NETLINK,True,"site",loc);
 
     list_t *comms_cfg=dict_lookup(dict,"comm");
-    if (!comms_cfg) cfgfatal(loc,"site","closure list \"comm\" not found");
+    if (!comms_cfg) cfgfatal(loc,"site","closure list \"comm\" not found\n");
     st->ncomms=list_length(comms_cfg);
     st->comms=safe_malloc_ary(sizeof(*st->comms),st->ncomms,"comms");
     assert(st->ncomms);
     for (i=0; i<st->ncomms; i++) {
 	item_t *item=list_elem(comms_cfg,i);
-	if (item->type!=t_closure) cfgfatal(loc,"site","comm is not a closure");
+	if (item->type!=t_closure)
+	    cfgfatal(loc,"site","comm is not a closure\n");
 	closure_t *cl=item->data.closure;
-	if (cl->type!=CL_COMM) cfgfatal(loc,"site","comm closure wrong type");
+	if (cl->type!=CL_COMM) cfgfatal(loc,"site","comm closure wrong type\n");
 	st->comms[i]=cl->interface;
     }
 
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:52 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:52 +0100
Subject: [PATCH 12/19] site: Deal with losing peer's MSG6 - go to RUN on MSG0
	with new key
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-13-git-send-email-ijackson@chiark.greenend.org.uk>

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 at chiark.greenend.org.uk>
---
 site.c |   32 +++++++++++++++++++++++++++-----
 util.c |   12 ++++++++++++
 util.h |    1 +
 3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/site.c b/site.c
index 4dbebaf..c0f4387 100644
--- 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
--- 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
--- 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);
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 04:22:50 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 04:22:50 +0100
Subject: [PATCH 10/19] site: Remove pointless check from decrypt_msg0
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1340248979-32459-11-git-send-email-ijackson@chiark.greenend.org.uk>

It is not necessary to check whether we have a current key before
attempting to unpick and decrypt a MSG0.  If we don't,
transform->reverse will simply fail due to lacking a key.

This check needs to be removed, because in forthcoming patches we will
want to be able to try to decrypt the packet with other keys, at which
point the lack of a currently valid data transfer key will not be
relevant and this check will be harmful.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 site.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/site.c b/site.c
index f65051f..4d3a612 100644
--- a/site.c
+++ b/site.c
@@ -715,12 +715,6 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0)
     struct msg0 m;
     uint32_t problem;
 
-    if (!st->current_valid) {
-	slog(st,LOG_DROP,"incoming message but no current key -> dropping");
-	initiate_key_setup(st,"incoming message but no current key");
-	return False;
-    }
-
     if (!unpick_msg0(st,msg0,&m)) return False;
 
     problem = st->current_transform->reverse(st->current_transform->st,
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Thu Jun 21 18:04:32 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 18:04:32 +0100
Subject: [PATCH 04/19] log: Print truncated messages
In-Reply-To: <1340248979-32459-5-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
	<1340248979-32459-5-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <20451.21536.971513.130992@chiark.greenend.org.uk>

Ian Jackson writes ("[PATCH 04/19] log: Print truncated messages"):
> If a message doesn't survive the vsnprintf in vMessage untruncated,
...
>  	vsnprintf(buff+bp,MESSAGE_BUFLEN-bp,message,args);
> +	buff[sizeof(buff)-2] = '\n';
> +	buff[sizeof(buff)-1] = '\0';
>  	/* Each line is sent separately */

I should have changed these sizeofs to MESSAGE_BUFLENs.

Ian.


From ijackson at chiark.greenend.org.uk  Thu Jun 21 19:16:36 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 21 Jun 2012 19:16:36 +0100
Subject: [PATCH 4/7] log: Print truncated messages [and 1 more messages]
In-Reply-To: <E1Sf57S-0003i8-7W@atreus.tartarus.org>,
	<20451.21536.971513.130992@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
	<1340248979-32459-5-git-send-email-ijackson@chiark.greenend.org.uk>
	<20451.21536.971513.130992@chiark.greenend.org.uk>
	<1339634317-16407-1-git-send-email-ijackson@chiark.greenend.org.uk>
	<1339634317-16407-5-git-send-email-ijackson@chiark.greenend.org.uk>
	<E1Sf57S-0003i8-7W@atreus.tartarus.org>
Message-ID: <20451.25860.843732.918532@chiark.greenend.org.uk>

Simon Tatham writes ("Re: [PATCH 4/7] log: Print truncated messages"):
> [I tried to send this to sgo-software-discuss, but Mailman told me I
> wasn't allowed to post to it.]

It's subscriber postings only I'm afraid, to reduce spam.  Please
subscribe; if you read via the newsgroup, just turn off your mail
delivery.

> Ian Jackson  <ijackson at chiark.greenend.org.uk> wrote:
> > Fix this by always putting "\n\0" at the end of buff.  That way a
> > truncated message is printed and the buffer will be cleared out for
> > the next call.
> [...]
> >  	assert(bp < MESSAGE_BUFLEN);
> >  	vsnprintf(buff+bp,MESSAGE_BUFLEN-bp,message,args);
> > +	buff[sizeof(buff)-2] = '\n';
> > +	buff[sizeof(buff)-1] = '\0';
> 
> In the case where the line was _almost_ too long, so that vsnprintf
> wrote its own \0 to buff[sizeof(buff)-2], will this not have the
> unfortunate effect of adding a second newline?

See my updated commit message.

> (Also, if it were me I'd stick with one of MESSAGE_BUFLEN and
> sizeof(buff), rather than switching from one to the other half way
> through. I presume they're equivalent anyway, but it's more
> confusing like this.)

Because of this comment I wrote:

Ian Jackson writes ("Re: [PATCH 04/19] log: Print truncated messages"):
> I should have changed these sizeofs to MESSAGE_BUFLENs.

But in fact:

      static char buff[MESSAGE_BUFLEN+1]={0,};

So using MESSAGE_BUFLEN would be wrong.  And using sizeof() makes it
completely clear that at least that part of the code is correct
(modulo minor infelicities).

Ian.


From rjk at terraraq.org.uk  Sun Jul  1 17:12:31 2012
From: rjk at terraraq.org.uk (Richard Kettlewell)
Date: Sun, 01 Jul 2012 17:12:31 +0100
Subject: [PATCH 2/5] make-secnet-sites: Fix userv invocation after
	pfilepath
In-Reply-To: <1324159397-28439-3-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1324159397-28439-1-git-send-email-ijackson@chiark.greenend.org.uk>
	<1324159397-28439-3-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <4FF076EF.6000206@terraraq.org.uk>

I don't seem to have time to do the full set of outstanding patches 
justice, but I did notice a couple of trivial things:

On 17/12/2011 22:03, Ian Jackson wrote:
> The commit 9b8369e07aeba5ed2c69fb4a7f74d07c8cebe015
>   make-secnet-sites: refactor to break out new function "pfilepath"
> broke the userv service invocation, because it turned out that later
> code depended on the "headerlines" variable whose assignment had been
> removed and replaced by a call to pfilepath.
>
> Make pfilepath return the lines read from the file and assign the
> result to headerlines.
>
> Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
[...]
 > +		headerinput=pfilepath(header,allow_include=True)

The description seems to be slightly inaccurate - 'headerinput' rather 
than 'headerlines'.

On 17/12/2011 22:03, Ian Jackson wrote:
 > -#define LINK_QUALITY_DOWN 0   /* No chance of a packet being 
delivered */
 > -#define LINK_QUALITY_DOWN_STALE_ADDRESS 1 /* Link down, old address 
information */
 > -#define LINK_QUALITY_DOWN_CURRENT_ADDRESS 2 /* Link down, current 
address information */
 > -#define LINK_QUALITY_UP 3     /* Link active */
 > +#define LINK_QUALITY_UNUSED 0   /* This link is unused, do not make 
this netlink */
 > +#define LINK_QUALITY_DOWN 1   /* No chance of a packet being 
delivered right away*/
 > +#define LINK_QUALITY_DOWN_STALE_ADDRESS 2 /* Link down, old address 
information */
 > +#define LINK_QUALITY_DOWN_CURRENT_ADDRESS 3 /* Link down, current 
address information */
 > +#define LINK_QUALITY_UP 4     /* Link active */

Perhaps this should be an enum.

ttfn/rjk


From ijackson at chiark.greenend.org.uk  Wed Jul 11 01:09:57 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Wed, 11 Jul 2012 01:09:57 +0100
Subject: [PATCH 1/9] chiark live tree fixes including proposed sites bugfix
Message-ID: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>

These patches are in ~secnet/secnet-live.git on chiark:
 1/9 netlink: Fix up link down behaviour
 2/9 make-secnet-sites: Fix userv invocation after pfilepath
 3/9 make-secnet-sites: Allow sites with no address
 4/9 make-secnet-sites: New -P <prefix> option
 5/9 make-secnet-sites: If definition found in wrong place, bomb out
 6/9 make-secnet-sites: Actually include addresses in sites.conf

I'm proposing these additional patches to sort out some problems with
the way I was using `include':
 7/9 make-secnet-sites: Do newline-trimming in pline()
 8/9 make-secnet-sites: In -u mode, output file "dereferences" include directives
 9/9] make-secnet-sites: Do not permit "include" in simple sites files

In particular, Steve was complaining that the generated sites file
contained an "include" directive which (a) means you can't process it
with any released version of secnet and (b) anyway a normal site admin
running make-secnet-sites should not have to trust the sites file
(to the extent of having to scrutinise it for includes).

I don't have time right now but in a test copy of ~secnet/sgo-vpn I
ran this
  USERV_USER=ian USERV_GROUP=ian-rela ~/things/Fvpn/secnet.git/make-secnet-sites -u header groupfiles newsites ian-rela </dev/null
and it seemed to do the right thing.

The file "newsites" is below.

Thanks,
Ian.



# sites file autogenerated by make-secnet-sites
# generated Wed Jul 11 01:07:13 2012, invoked by ian
# use make-secnet-sites to turn this file into a
# valid /etc/secnet/sites.conf file

vpn sgo
contact steve at greenend.org.uk
dh 8db5f2c15ac96d9f3382d1ef4688fba14dc7908ae7dfd71a9cfe7f479a75d506dc53f159aeaf488bde073fe544bc91c099f101fcf60074f30c06e36263c03ca9e07931ce3fc235fe1171dc6d9316fb097bd4362891e2c36e234e7c16b038fd97b1f165c710e90537de66ee4f54001f5712b050d4e07de3fba07607b19b64f6c3 2
hash sha1
key-lifetime 72000000
restrict-nets 192.168.0.0/16 10.0.0.0/8 172.16.0.0/12
setup-timeout 2000
setup-retries 5

location greenend Mgend
restrict-nets 192.168.73.0/24 172.19.71.0/24

location chiark secnet
restrict-nets 172.18.113.208/28 172.18.45.0/24 172.21.115.0/24 172.21.29.0/24 172.27.25.80/28 172.31.80.16/32 172.31.80.6/32 172.31.80.7/32 172.31.80.8/32 192.168.37.0/24 192.168.76.0/24 192.168.93.0/24 172.31.80.9/32 172.31.80.16/31 172.31.80.32/29

location perfection jmat-ns
restrict-nets 192.168.5.0/24

location stardust sgta-sd
restrict-nets 172.31.66.0/24

location empire Tempire
restrict-nets 172.16.22.0/24

location ecstacy dunc-xtc
restrict-nets 192.168.76.0/24

location lilac dame-lil
restrict-nets 192.168.93.0/24

location relativity ian-rela
restrict-nets 172.18.45.0/24

location anjou rich-avn
restrict-nets 172.17.207.0/24 172.31.59.0/25

location easel matthewv
restrict-nets 172.27.25.80/28

location burrow vcla-brw
restrict-nets 172.20.45.0/24

location rapun Tempire
restrict-nets 172.31.80.10/32 172.31.80.11/32

location ipltd stev-pub
restrict-nets 172.30.52.0/22

location badgers sion-net
restrict-nets 172.19.244.0/24

location panacea owen-dns
restrict-nets 192.168.1.0/24

location jmusa jmatthew
restrict-nets 192.168.131.0/24

location gallery gtaylor
restrict-nets 172.17.217.0/24

location ixion sgta-ixn
restrict-nets 172.31.80.2/31

location lemoncurd bjha-dd
restrict-nets 172.17.11.0/24

location virtue matthewv
restrict-nets 172.29.176.192/28

location pelham cjwa-pel
restrict-nets 172.20.153.0/24

location greysky sbleas
restrict-nets 172.16.249.0/24

location thyme jdamery
restrict-nets 172.27.21.0/24

location atreus sgta-ixn
restrict-nets 172.31.80.14/31

location excommunication ijac-exv
restrict-nets 172.18.218.0/24

end-definitions
# Section submitted by user stevee, Mon Jun 11 16:59:18 2012
# Checked by make-secnet-sites version 0.1.18

vpn sgo
location greenend
contact steve at greenend.org.uk
site sinister
  networks 192.168.73.0/24
  peer 192.168.73.73
  address sinister.dynamic.greenend.org.uk 410
  pubkey 1024 35 142982503868096218874861713282365059678627873984481457880430507120010850400504702632632743735995919032493811078069101443728064938518604011871397141029747105689310082778680675110366272191956450327164919190408487263157267830267376075547419528532762755363767018559182902601280415801934613813608200657376498960611

# Section submitted by user ijackson, Thu Apr 24 23:47:46 2008
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location excommunication
contact ijackson+excommunication-vpn at chiark.greenend.org.uk
site emerald
  networks 172.18.218.0/24
  peer 172.18.218.11
  address excommunication.dynamic.greenend.org.uk 410
  pubkey 1024 35 140140330328542611800768776564048756920532922679702433211185267618732339317309494277940847589770057285554469957412734720126249719061010541225270308485820564277786300219744090145710268363118782640220667516224329767745989052149026271633687684932195329469377592430156297863238196394532882568848856901877761448803


# Section submitted by user sbleas, Sun Jul 24 16:59:32 2005
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location greysky
contact sbleas at chiark.greenend.org.uk
site greysky
  networks 172.16.249.0/24
  peer 172.16.249.105
  address greysky.dynamic.greenend.org.uk 410
  pubkey 1024 35 127127562973458072013956521838509993176018348974866364590732071675333791729011602234570373309275738257663428277820156230049276461703651797721756193107918513366292430228959882513559077151595253974711791540876336402604533818479298306474727826719514368996354838075431106628205540730847897510174714028472046246947

# Section submitted by user sgtatham, Mon Oct 15 09:51:55 2001
# Checked by make-secnet-sites.py version 0.1.5

vpn sgo
location stardust
contact anakin at pobox.com
site judicator
  networks 172.31.66.0/24
  peer 172.31.66.254
  address stardust.dynamic.greenend.org.uk 410
  pubkey 1024 35 172103696521215386942519828460041746050840885934062292617904380279615471736075326145760322489758313543838103638966973721653333512954992808645100024939775711379889481486529488596638415567978529720299164391410297002238407954683095686146831539990746931773177959852388886583892047733901855723271374635031351724959

# Section submitted by user bjharris, Wed Oct  6 15:26:15 2004
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location lemoncurd
contact bjh21 at bjh21.me.uk
site rockall
  networks 172.17.11.0/24
  peer 172.17.11.60
  address lemoncurd.org.uk 410
  pubkey 1024 35 132151396510268534149505401089289240055627968580164854163798489423833024690387147792312076862053262724542284758806695232878898860150602283376136732357319353987641558111936289770024863287626090483891903304466468644900615672053832505822843597812177969878369217341333537807173313144275666943731678917609083577319

# Section submitted by user jmatthew, Wed Dec 31 17:33:42 2003
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location jmusa
contact john at matthews-net.org.uk
site usa-gw
	networks 192.168.131.0/24
	peer 192.168.131.11
	address jmusa.dynamic.greenend.org.uk 410
	pubkey 1024 35 142643431555554550472916261193128717981199292292504257783723748005140530544381817872610877087250995842428377481330886182161397412580727774142766085672635544039342810887416467355989319776669626403825020242609567013182851677637217123437741414362169489217937381371543282324472416822842829850640409774156852415619

# Section submitted by user cjwatson, Wed Jun 15 15:00:27 2005
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location pelham
contact cjwatson at chiark.greenend.org.uk
site riva
  networks 172.20.153.0/24
  peer 172.20.153.10
  address riva.dynamic.greenend.org.uk 410
  pubkey 1024 35 150012366428468594867083352773615114169539174921839743006410352678375470281904372418104180435366929816985289769210263068474248655047132242107403943281003608757467341308161831412815384316950515725027446003610950608601521981103370621812314178156457469223198123248433229369422402448995630381856566588155949836283

# Section submitted by user jdamery, Tue Aug  9 16:08:14 2005
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location thyme
contact jdamery at chiark.greenend.org.uk
site florimel
  networks 172.27.21.0/24
  peer 172.27.21.254
  address thyme.dynamic.greenend.org.uk 410
  pubkey 1024 35 160318659654793190913085833127499777734906806235496225999055026087085856348129353956290177650179664266447942757478049564033841320525562038598209224283761728290596323923257745190494231997864281274778223682956454764112695255284837266675307042166597326016979631610595618841042691817940562212521211242693199315489

# Section submitted by user gtaylor, Fri Jul 16 22:40:07 2004
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location gallery
contact gareth at tartarus.org
site hmm
  networks 172.17.217.0/24
  peer 172.17.217.60
  address gallery.dynamic.greenend.org.uk 410
  pubkey 1024 35 130958098339862227905167401600446158622074102462153051991122465660362552335477855627008108211455129080374721948440023448655511420869273792195458617450663877274526194847651505517926597314157802020132090442712003025595251851296479161499610157612158694554090010411921662616601901875331775155727155191398156304127

# Section submitted by user jmatthew, Wed Mar  6 00:52:29 2002
# Checked by make-secnet-sites.py version 0.1.10

vpn sgo
location perfection
contact john at matthews-net.org.uk
site nijinsky
  address nijinsky.dynamic.greenend.org.uk 410
  networks 192.168.5.0/24
  peer 192.168.5.6
  pubkey 1024 35 128487881270944914639894387086741587847238577932503703584182757008429388448547881228220638514218369598813290574301895104614010538549221379761288229500242233913704725858930469014371823580095468883518123786280810635743808658136214470344616744051666823002144177711188605565894144473112763128933101121830747767789

# Section submitted by user owend, Mon Feb 24 21:31:24 2003
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location panacea
contact owend at chiark.greenend.org.uk
site titus
	networks 192.168.1.0/24
	address titus.dynamic.greenend.org.uk 410
	pubkey 1024 35 146225772119682547715902565190190643312756178641782659251896268987727101468304162035884567488029655760768307646333249877570668813143061197445149530457158125674840720104202993965202972760680655663057991630631396830006751250015740456156769595500824214983186619770455615107386517341802957624394513827742184347819 root at titus
	peer 192.168.1.2

# Section submitted by user sgtatham, Wed May 14 10:53:13 2008
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location ixion
contact vpn-coordinator at tartarus.org
site ixion
  address ixion.tartarus.org 410
  peer 172.31.80.3
  networks 172.31.80.2/31
  pubkey 1024 35 135333718531590228378209512514588704905145163880290136458715219505633627524404094513841678384216092020068922682779012031026257466322004228051099195436852327776120623133459099558984695286392569159942323721178965470705931187002028531553053165265063371892878856840073984710117038421304936395808048725039337338333

vpn sgo
location atreus
contact vpn-coordinator at tartarus.org
site atreus
  address atreus.tartarus.org 410
  peer 172.31.80.15
  networks 172.31.80.14/31
  pubkey 2048 35 26386319295514409858007579671143345210262550781321846615285747099130268279668952404846339284499169134555099847799357608277227591276037359293661370358841589462282266129573226685051041853504652540984673273422688457397485784634165164842116489758018052457208727505964119927584194998773050611697605744817532136486200727234200187716841076605226473700556924701371915248196610024942422536365779412790936877712158535393641150340226374015237441125818584988914821922138222990391410104618536467064458838238178850491574569031670286794518285548203153812853983482745909664546888732246331952837152245416907880131592399740624837625679 vpn at atreus

# Section submitted by user siona, Tue Mar  6 15:26:24 2007
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location badgers
contact siona at chiark.greenend.org.uk
site fire
  networks 172.19.244.0/24
  address badgers.dynamic.greenend.org.uk 410
  pubkey 1024 35 155731326790044739106090711115198879520777017954119343461361714460734416771081319121759446476640504789659086902138186665340305195172945901909900310053117781470765984941859407464659608317407019642408971547435723285606385928404216184987310688337736368714229386819003389465769973671939200231250948373943667618783
  peer 172.19.244.99

# Section submitted by user matthewv, Tue Jun 14 23:58:30 2005
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location virtue
contact matthewv at chiark.greenend.org.uk
site favour
  networks 172.29.176.192/28
  peer 172.29.176.201
  address virtue.dynamic.greenend.org.uk 410
  pubkey 1024 35 112503312958985565044601916565591039668261902008656306950653005868251944371825751502535170935146665126841334356339197447356763943357119328264016284509802774866518883912993850804943785987907536554834455749153832655364691483663746573743535195732001171608892541084605865287636495507774192680506896016961914459489 root at favour


# Section submitted by user ryounger, Wed Aug 18 10:17:34 2004
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location burrow
contact secnet at impropriety.org.uk
site halfwit
  networks 172.20.45.0/24
  peer 172.20.45.5
  address gratuitous.impropriety.org.uk 410
  pubkey 1024 35 127352641627380578435435660403054326335938240273516567392207537544193320376266588030801778470343899472769835230025259387501364174492688866745690203025753623277631602353630366424977412655178660161681619060738333874667820132248434451494335570682903964699211302382298568449859032089815519068092320169359184327869

# Section submitted by user matthewv, Sun Sep 28 18:34:03 2003
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location empire
contact matthew at sel.cam.ac.uk
site ming
  networks 172.16.22.0/24
  peer 172.16.22.13
  address principate.dynamic.greenend.org.uk 410
  pubkey 1024 35 165071423440701853363893560927297586228481194931512496542471628738409407818304132718956517427644298614855393881984527589004690615381645469433292776002869535128726273661260726105285810230324344055069927329237731699020935099041474392961091014739925575121643707013549314129040478488901732049671622188637347370207 root at ming

location rapun
contact matthew at sel.cam.ac.uk
site rapun
  networks 172.31.80.10/32 172.31.80.11/32
  peer 172.31.80.11
  address rapun.sel.cam.ac.uk 410
  pubkey 1024 35 110270723853336412341894163820357195263465313079948732046687371982689615145227226613429024584939303872015256345044383540183538141602818556714430216405752101698535466750482451177171172758971229602031319939166399398348347460470456866963642044943707321634031462625903404176360162676176173043391403157175328752941 root at rapun

# Section submitted by user richard, Sun Nov  7 19:04:00 2004
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location anjou
contact rjk at greenend.org.uk
site curator
  networks 172.17.207.0/24 172.31.59.0/25
  peer 172.17.207.5
  address anjou.dynamic.greenend.org.uk 410
  pubkey 1024 35 126679595520726979962622623759738370463752210336129637076258076196879436564166661212563050995716520919538643180560139625301713248103546058087333945717388003337678713651877892744210348471418393000853911928130371606672166647819375096598657243179753326942738703579072996316084163949055081621109198792950913566211

# Section submitted by user secnet, Sun Jan 25 01:02:34 2009
# Checked by make-secnet-sites version 0.1.16

vpn sgo
location chiark
contact secnet at chiark.greenend.org.uk
site chiark
  address chiark.greenend.org.uk 410
  peer 172.31.80.9
  networks 172.18.113.208/28 172.18.45.0/24 172.21.115.0/24 172.21.29.0/24 172.27.25.80/28 172.31.80.16/32 172.31.80.6/32 172.31.80.7/32 172.31.80.8/32 192.168.37.0/24 192.168.76.0/24 192.168.93.0/24 172.31.80.9/32 172.31.80.16/31 172.31.80.32/29
  pubkey 1024 35 142727529003925298656414420811632378296524497304179619105191015695357742460158797298827122878793501199537822285205911063864424908425497400229865955603317464754223907562647708278695773351548155481372989733141442122782882941206619661017696209688431601952580912499262706702512738421030425164121104288841795045999

# Section submitted by user stevee, Mon Jun 11 16:55:59 2012
# Checked by make-secnet-sites version 0.1.18

vpn sgo
location ipltd
contact sde at individualpubs.co.uk
site milton
  networks 172.30.52.0/26
  peer 172.30.52.6
  address secnet.milton.individualpubs.co.uk 410
  pubkey 1024 35 103819683786502279159773330747756109037427621165612669180913116723484974798517151020219076696223565051950604312958723900086139489285136070607005094488463441604213796768394056033874614632549577685465750180452079996642629208303123736605483212526511140263433658278815863739598311601242327016449392967024651067677
site coalheavers
  networks 172.30.52.64/26
  peer 172.30.52.70
  address secnet.coalheavers.individualpubs.co.uk 410
  pubkey 1024 35 135676368852737967437118969944190656621895450779572854299434593799576233093728022502592982033078082239800524997353214277607157638686306335983729691307532604146507728225690090305603227497792215610967466079745015364320300371659179811698561112006015921077424715318938054911612614715186892195550649032922742357509
site pembury
  networks 172.30.52.128/26
  peer 172.30.52.134
  address secnet.pembury.individualpubs.co.uk 410
  pubkey 1024 35 135525422562501888354767840299070718142012758186740632284460383406541465758525767102354019639464074754127240487452770551812776120512972462336217880604213678326093583625722118836188663760477198688887962650569041800520651662368580343843125044148901520945763806762233073800091138914859139497232003400493786138701
site whitelion
  networks 172.30.52.192/26
  peer 172.30.52.195
  address secnet.whitelion.individualpubs.co.uk 410
  pubkey 2048 35 24124139906411201483430278998300459346209181648588447838634557473142617326423094637990065615759268365532640262961154697003807788851304489778519423758311870419548451871707772625498983141615243082831954873299173351953696658284454997879609771415443088351129606246814125151516261656288221577912973789619169557736317647851013036620799821434567147815969576152166310449831742853072340641498081658794736455209830298796160236847261150100692846950792810183472852288888570587958978145794567121577380059457530519109467738632952725917803462199473750997853636055980188980138986295315951838415412566846884835372246283753195840399233
site naisby
  networks 172.30.53.0/26
  peer 172.30.53.3
  address naisby.dyndns.org 410
  pubkey 2048 35 23496932390846350006988903273689995529605442595208449496301215214523257298821956734082626831127389971279867860463844649591040499106920038765516079670943890056365922077683038785706450274752934070275306274191551501736910294338834719075780904166624029759273201802692626111603586293804720140640964242329171023597889018301623638755140852348589391468473507370043341996406876751101754876344216323889569939599454387958504841498748817044562980800841577617550578480358187232109759349388659009098602911245715834839842658839182515701957178993870897460382235435783478519228534063317662378085748664828760472728036858318519344109081
site devonshire
  networks 172.30.53.64/26
  peer 172.30.53.68
  address secnet.devonshire.individualpubs.co.uk 410
  pubkey 2048 35 26902315040175593876256072038553120405966707330340750045690511223604564174210548351097919706705196291563374405464944675832933762514612014430798579204912629788133854670914760043124561374006860857753208885380137932505841804168687042795885528925329290637064880998723072594595620494154932978564225048482665706091738179497298576789227621740933384149568004124773711339763311587023507505687967197728606354605301193857428240189843122718523671591383502512529001928606700076149721477047380958711196992296863152608864037365881801156117520321893534754079819993485727497061246464085753706365619156594774785738135258589182774229261
site ur
  networks 172.30.53.128/26
  peer 172.30.53.131
  address secnet.individualpubs.co.uk 410
  pubkey 2048 35 20355897268737979684293048877574689021779035982345049009043820324200673860803727671821805138175619321066286905115257692769641390216302303558362771479303888730136308693168090233787044931146610841460852125505817950606485287552708042330205630231876476137513837506224118220485195333892166862957470534693004159419697888019261430352441090651910400489629389805959091845431611665235399586623269467846243675164591334264439954249654404693615749690231508561586018669034812321971389933439432195577966285485733406158092316156495578419065480152327544896885831898721783706047292395415649760584201984481444598038744208243190513273603 root at ur
site waterbeach
  networks 172.30.53.192/26
  peer 172.30.53.194
  address milton.individualpubs.co.uk 410
  pubkey 2048 65537 22562158633121890206224570825426894540624603287907174899301268915349732481472165595198818947985200977756775390893863893888581373092922323295580736109937512156762797804505458433699834203987969541987844291075715312951629005274714558611767348648628228408305639290929676149407305052065714142854475924384362457569426063603805301794551497061718847913631785376232441553705533999150961829592246931361481449559498692064213954939958902797961047065924888489078058430227403503544022488904082958500903518314590365158800680918050913620772661052981864887185846946527361166335491313276846416237504316473788961715718243151128748257731 sde at vulcan

# Section submitted by user ian, Wed Jul 11 01:07:13 2012
# Checked by make-secnet-sites version 0.1.18


# end of sites file


From ijackson at chiark.greenend.org.uk  Wed Jul 11 01:09:58 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Wed, 11 Jul 2012 01:09:58 +0100
Subject: [PATCH 1/9] netlink: Fix up link down behaviour
In-Reply-To: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1341965406-1735-2-git-send-email-ijackson@chiark.greenend.org.uk>

Partially reverts 04f92904ea6c41517ff7154910c16ef4c3bc646b
 "userv-ipif: Always request routes from userv, regardless of link quality"

It turns out that this check is necessary to avoid bringing up a route
for a "netlink" stanza in the configuration file which is never used.

In particular, this avoids bringing up a netlink for (a) sites which
are not mentioned in the config file (b) the site on which secnet is
running.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 netlink.c |    2 +-
 secnet.h  |    9 +++++----
 slip.c    |   12 +++++++-----
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/netlink.c b/netlink.c
index 5226ad1..df51d9c 100644
--- a/netlink.c
+++ b/netlink.c
@@ -909,7 +909,7 @@ static closure_t *netlink_inst_create(struct netlink *st,
     c->deliver=NULL;
     c->dst=NULL;
     c->name=name;
-    c->link_quality=LINK_QUALITY_DOWN;
+    c->link_quality=LINK_QUALITY_UNUSED;
     c->mtu=mtu?mtu:st->mtu;
     c->options=options;
     c->outcount=0;
diff --git a/secnet.h b/secnet.h
index 06beb05..ecbb995 100644
--- a/secnet.h
+++ b/secnet.h
@@ -420,10 +420,11 @@ struct transform_if {
 typedef void netlink_deliver_fn(void *st, struct buffer_if *buf);
 /* site code can tell netlink when outgoing packets will be dropped,
    so netlink can generate appropriate ICMP and make routing decisions */
-#define LINK_QUALITY_DOWN 0   /* No chance of a packet being delivered */
-#define LINK_QUALITY_DOWN_STALE_ADDRESS 1 /* Link down, old address information */
-#define LINK_QUALITY_DOWN_CURRENT_ADDRESS 2 /* Link down, current address information */
-#define LINK_QUALITY_UP 3     /* Link active */
+#define LINK_QUALITY_UNUSED 0   /* This link is unused, do not make this netlink */
+#define LINK_QUALITY_DOWN 1   /* No chance of a packet being delivered right away*/
+#define LINK_QUALITY_DOWN_STALE_ADDRESS 2 /* Link down, old address information */
+#define LINK_QUALITY_DOWN_CURRENT_ADDRESS 3 /* Link down, current address information */
+#define LINK_QUALITY_UP 4     /* Link active */
 #define MAXIMUM_LINK_QUALITY 3
 typedef void netlink_link_quality_fn(void *st, uint32_t quality);
 typedef void netlink_register_fn(void *st, netlink_deliver_fn *deliver,
diff --git a/slip.c b/slip.c
index 7c138d1..db89a27 100644
--- a/slip.c
+++ b/slip.c
@@ -257,11 +257,13 @@ static void userv_invoke_userv(struct userv *st)
 
     allnets=ipset_new();
     for (r=st->slip.nl.clients; r; r=r->next) {
-	struct ipset *nan;
-	r->kup=True;
-	nan=ipset_union(allnets,r->networks);
-	ipset_free(allnets);
-	allnets=nan;
+	if (r->link_quality > LINK_QUALITY_UNUSED) {
+	    struct ipset *nan;
+	    r->kup=True;
+	    nan=ipset_union(allnets,r->networks);
+	    ipset_free(allnets);
+	    allnets=nan;
+	}
     }
     snets=ipset_to_subnet_list(allnets);
     ipset_free(allnets);
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Wed Jul 11 01:09:59 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Wed, 11 Jul 2012 01:09:59 +0100
Subject: [PATCH 2/9] make-secnet-sites: Fix userv invocation after pfilepath
In-Reply-To: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1341965406-1735-3-git-send-email-ijackson@chiark.greenend.org.uk>

The commit 9b8369e07aeba5ed2c69fb4a7f74d07c8cebe015
 make-secnet-sites: refactor to break out new function "pfilepath"
broke the userv service invocation, because it turned out that later
code depended on the "headerlines" variable whose assignment had been
removed and replaced by a call to pfilepath.

Make pfilepath return the lines read from the file and assign the
result to headerlines.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 make-secnet-sites |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/make-secnet-sites b/make-secnet-sites
index 547f572..5911a39 100755
--- a/make-secnet-sites
+++ b/make-secnet-sites
@@ -391,8 +391,10 @@ def pline(i,allow_include=False):
 
 def pfilepath(pathname,allow_include=False):
 	f=open(pathname)
-	pfile(pathname,f.readlines(),allow_include=allow_include)
+	lines=f.readlines()
+	pfile(pathname,lines,allow_include=allow_include)
 	f.close()
+	return lines
 
 def pfile(name,lines,allow_include=False):
 	"Process a file"
@@ -465,7 +467,7 @@ else:
 		if not ok:
 			print "caller not in group %s"%group
 			sys.exit(1)
-		pfilepath(header,allow_include=True)
+		headerinput=pfilepath(header,allow_include=True)
 		userinput=sys.stdin.readlines()
 		pfile("user input",userinput)
 	else:
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Wed Jul 11 01:10:00 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Wed, 11 Jul 2012 01:10:00 +0100
Subject: [PATCH 3/9] make-secnet-sites: Allow sites with no address
In-Reply-To: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1341965406-1735-4-git-send-email-ijackson@chiark.greenend.org.uk>

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 make-secnet-sites |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/make-secnet-sites b/make-secnet-sites
index 5911a39..9d0c6a6 100755
--- a/make-secnet-sites
+++ b/make-secnet-sites
@@ -267,12 +267,12 @@ class sitelevel(level):
 	 'networks':None,
 	 'peer':None,
 	 'pubkey':(lambda n,v:"key %s;\n"%v),
+	 'address':None,
 	 'mobile':sp,
 	})
 	require_properties={
 	 'dh':"Diffie-Hellman group",
 	 'contact':"Site admin contact address",
-	 'address':"Site external access address",
 	 'networks':"Networks claimed by the site",
 	 'hash':"hash function",
 	 'peer':"Gateway address of the site",
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Wed Jul 11 01:10:01 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Wed, 11 Jul 2012 01:10:01 +0100
Subject: [PATCH 4/9] make-secnet-sites: New -P <prefix> option
In-Reply-To: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1341965406-1735-5-git-send-email-ijackson@chiark.greenend.org.uk>

make-secnet-sites generates a config file which defines the keys
"vpn-data", "vpn", and "all-sites".

To make it possible to usefully include the output of more than one
different piece of output from make-secnet-sites, support the option
-P <prefix>, which generates a config file which defines
"<prefix>vpn-data", "<prefix>vpn", and "<prefix>all-sites".

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 make-secnet-sites |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/make-secnet-sites b/make-secnet-sites
index 9d0c6a6..c49467a 100755
--- a/make-secnet-sites
+++ b/make-secnet-sites
@@ -320,6 +320,7 @@ def moan(msg):
 root=level(['root','root'])   # All vpns are children of this node
 obstack=[root]
 allow_defs=0   # Level above which new definitions are permitted
+prefix=''
 
 def set_property(obj,w):
 	"Set a property on a configuration node"
@@ -415,20 +416,21 @@ def outputsites(w):
 	w.write("# Command line: %s\n\n"%string.join(sys.argv))
 
 	# Raw VPN data section of file
-	w.write("vpn-data {\n")
+	w.write(prefix+"vpn-data {\n")
 	for i in root.children.values():
 		i.output_data(w,2,"")
 	w.write("};\n")
 
 	# Per-VPN flattened lists
-	w.write("vpn {\n")
+	w.write(prefix+"vpn {\n")
 	for i in root.children.values():
-		i.output_vpnflat(w,2,"vpn-data")
+		i.output_vpnflat(w,2,prefix+"vpn-data")
 	w.write("};\n")
 
 	# Flattened list of sites
-	w.write("all-sites %s;\n"%string.join(map(lambda x:"vpn/%s/all-sites"%
-		x,root.children.keys()),","))
+	w.write(prefix+"all-sites %s;\n"%string.join(
+		map(lambda x:"%svpn/%s/all-sites"%(prefix,x),
+			root.children.keys()),","))
 
 # Are we being invoked from userv?
 service=0
@@ -471,6 +473,9 @@ else:
 		userinput=sys.stdin.readlines()
 		pfile("user input",userinput)
 	else:
+		if sys.argv[1]=='-P':
+			prefix=sys.argv[2]
+			sys.argv[1:3]=[]
 		if len(sys.argv)>3:
 			print "Too many arguments"
 			sys.exit(1)
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Wed Jul 11 01:10:02 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Wed, 11 Jul 2012 01:10:02 +0100
Subject: [PATCH 5/9] make-secnet-sites: If definition found in wrong place,
	bomb out
In-Reply-To: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1341965406-1735-6-git-send-email-ijackson@chiark.greenend.org.uk>

If it doesn't exit here, make-secnet-sites might subsequently crash
with Python stack backtrace.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 make-secnet-sites |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/make-secnet-sites b/make-secnet-sites
index c49467a..9e00720 100755
--- a/make-secnet-sites
+++ b/make-secnet-sites
@@ -376,6 +376,8 @@ def pline(i,allow_include=False):
 			if nl.depth<allow_defs:
 				complain("New definitions not allowed at "
 					"level %d"%nl.depth)
+				# we risk crashing if we continue
+				sys.exit(1)
 			current.children[w[1]]=nl
 			current=nl
 		obstack.append(current)
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Wed Jul 11 01:10:03 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Wed, 11 Jul 2012 01:10:03 +0100
Subject: [PATCH 6/9] make-secnet-sites: Actually include addresses in
	sites.conf
In-Reply-To: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1341965406-1735-7-git-send-email-ijackson@chiark.greenend.org.uk>

This was broken in

   commit c9d84e31f48a04eb0af525324707f22c738cc8b7
   Author: Ian Jackson <ijackson at chiark.greenend.org.uk>
   Date:   Sat Dec 17 21:21:47 2011 +0000

       make-secnet-sites: Allow sites with no address

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 make-secnet-sites |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/make-secnet-sites b/make-secnet-sites
index 9e00720..dc6ec78 100755
--- a/make-secnet-sites
+++ b/make-secnet-sites
@@ -267,7 +267,7 @@ class sitelevel(level):
 	 'networks':None,
 	 'peer':None,
 	 'pubkey':(lambda n,v:"key %s;\n"%v),
-	 'address':None,
+	 'address':(lambda n,v:"address %s;\n"%v),
 	 'mobile':sp,
 	})
 	require_properties={
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Wed Jul 11 01:10:04 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Wed, 11 Jul 2012 01:10:04 +0100
Subject: [PATCH 7/9] make-secnet-sites: Do newline-trimming in pline()
In-Reply-To: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1341965406-1735-8-git-send-email-ijackson@chiark.greenend.org.uk>

This will make life a little easier in a future patch.  While we're at
it, use rstrip rather than open-coding it.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 make-secnet-sites |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/make-secnet-sites b/make-secnet-sites
index dc6ec78..0c19a78 100755
--- a/make-secnet-sites
+++ b/make-secnet-sites
@@ -333,7 +333,7 @@ def set_property(obj,w):
 def pline(i,allow_include=False):
 	"Process a configuration file line"
 	global allow_defs, obstack, root
-	w=string.split(i)
+	w=string.split(i.rstrip('\n'))
 	if len(w)==0: return
 	keyword=w[0]
 	current=obstack[len(obstack)-1]
@@ -407,7 +407,6 @@ def pfile(name,lines,allow_include=False):
 	for i in lines:
 		line=line+1
 		if (i[0]=='#'): continue
-		if (i[len(i)-1]=='\n'):	i=i[:len(i)-1] # strip trailing LF
 		pline(i,allow_include=allow_include)
 
 def outputsites(w):
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Wed Jul 11 01:10:05 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Wed, 11 Jul 2012 01:10:05 +0100
Subject: [PATCH 8/9] make-secnet-sites: In -u mode,
	output file "dereferences" include directives
In-Reply-To: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1341965406-1735-9-git-send-email-ijackson@chiark.greenend.org.uk>

Whenm make-secnet-sites is writing the "output" sites file in -u
(groupfile update) mode, it includes the effective contents of files
referenced in "include" directives, rather than the "include"
directive itself.

So the "output" sites file does not any longer depend on any files
included by the header.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 make-secnet-sites |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/make-secnet-sites b/make-secnet-sites
index 0c19a78..aa50344 100755
--- a/make-secnet-sites
+++ b/make-secnet-sites
@@ -334,23 +334,22 @@ def pline(i,allow_include=False):
 	"Process a configuration file line"
 	global allow_defs, obstack, root
 	w=string.split(i.rstrip('\n'))
-	if len(w)==0: return
+	if len(w)==0: return [i]
 	keyword=w[0]
 	current=obstack[len(obstack)-1]
 	if keyword=='end-definitions':
 		allow_defs=sitelevel.depth
 		obstack=[root]
-		return
+		return [i]
 	if keyword=='include':
 		if not allow_include:
 			complain("include not permitted here")
-			return
+			return []
 		if len(w) != 2:
 			complain("include requires one argument")
-			return
+			return []
 		newfile=os.path.join(os.path.dirname(file),w[1])
-		pfilepath(newfile,allow_include=allow_include)
-		return
+		return pfilepath(newfile,allow_include=allow_include)
 	if levels.has_key(keyword):
 		# We may go up any number of levels, but only down by one
 		newdepth=levels[keyword].depth
@@ -381,33 +380,34 @@ def pline(i,allow_include=False):
 			current.children[w[1]]=nl
 			current=nl
 		obstack.append(current)
-		return
+		return [i]
 	if current.allow_properties.has_key(keyword):
 		set_property(current,w)
-		return
+		return [i]
 	else:
 		complain("Property %s not allowed at %s level"%
 			(keyword,current.type))
-		return
+		return []
 
 	complain("unknown keyword '%s'"%(keyword))
 
 def pfilepath(pathname,allow_include=False):
 	f=open(pathname)
-	lines=f.readlines()
-	pfile(pathname,lines,allow_include=allow_include)
+	outlines=pfile(pathname,f.readlines(),allow_include=allow_include)
 	f.close()
-	return lines
+	return outlines
 
 def pfile(name,lines,allow_include=False):
 	"Process a file"
 	global file,line
 	file=name
 	line=0
+	outlines=[]
 	for i in lines:
 		line=line+1
 		if (i[0]=='#'): continue
-		pline(i,allow_include=allow_include)
+		outlines += pline(i,allow_include=allow_include)
+	return outlines
 
 def outputsites(w):
 	"Output include file for secnet configuration"
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Wed Jul 11 01:10:06 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Wed, 11 Jul 2012 01:10:06 +0100
Subject: [PATCH 9/9] make-secnet-sites: Do not permit "include" in simple
	sites files
In-Reply-To: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <1341965406-1735-10-git-send-email-ijackson@chiark.greenend.org.uk>

Restrict the "include" directive to the "header" of -u (groupfile
update) mode.  Callers who are simply using make-secnet-sites to
transform a (possibly untrusted) sites file into a (to be trusted)
sites.conf file should not have to worry about includes.

"include" directives are already forbidden in group files.

Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
 make-secnet-sites |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/make-secnet-sites b/make-secnet-sites
index aa50344..966bb77 100755
--- a/make-secnet-sites
+++ b/make-secnet-sites
@@ -480,7 +480,7 @@ else:
 		if len(sys.argv)>3:
 			print "Too many arguments"
 			sys.exit(1)
-		pfilepath(sys.argv[1],allow_include=True)
+		pfilepath(sys.argv[1])
 		of=sys.stdout
 		if len(sys.argv)>2:
 			of=open(sys.argv[2],'w')
-- 
1.7.2.5



From ijackson at chiark.greenend.org.uk  Wed Jul 11 01:14:41 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Wed, 11 Jul 2012 01:14:41 +0100
Subject: [PATCH 1/9] chiark live tree fixes including proposed sites bugfix
In-Reply-To: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <20476.50545.300560.879921@chiark.greenend.org.uk>

Ian Jackson writes ("[PATCH 1/9] chiark live tree fixes including proposed sites bugfix"):
> These patches are in ~secnet/secnet-live.git on chiark:
>  1/9 netlink: Fix up link down behaviour
>  2/9 make-secnet-sites: Fix userv invocation after pfilepath
>  3/9 make-secnet-sites: Allow sites with no address
>  4/9 make-secnet-sites: New -P <prefix> option
>  5/9 make-secnet-sites: If definition found in wrong place, bomb out
>  6/9 make-secnet-sites: Actually include addresses in sites.conf
> 
> I'm proposing these additional patches to sort out some problems with
> the way I was using `include':
>  7/9 make-secnet-sites: Do newline-trimming in pline()
>  8/9 make-secnet-sites: In -u mode, output file "dereferences" include directives
>  9/9] make-secnet-sites: Do not permit "include" in simple sites files
> 
> In particular, Steve was complaining that the generated sites file
> contained an "include" directive which (a) means you can't process it
> with any released version of secnet and (b) anyway a normal site admin
> running make-secnet-sites should not have to trust the sites file
> (to the extent of having to scrutinise it for includes).

Steve, if this looks like a plausible approach to you I'll push those
changes to the live tree on chiark and test them there.  Alternatively
feel free to do that yourself; they are here right now

 login.chiark.greenend.org.uk:/u/ian/things/secnet wip-sites-include-2

 http://www.chiark.greenend.org.uk/ucgi/~ian/git?p=secnet.git;a=shortlog;h=refs/heads/wip-sites-include-2

NB that's a potentially rebasing branch.

Ian.


From matthewv at chiark.greenend.org.uk  Thu Jul 12 17:28:05 2012
From: matthewv at chiark.greenend.org.uk (Matthew Vernon)
Date: Thu, 12 Jul 2012 17:28:05 +0100
Subject: No subject
Message-ID: <4FFEFB15.90203@chiark.greenend.org.uk>

A non-text attachment was scrubbed...
Name: 0001-Update-build-dependency-documentation.patch
Type: text/x-patch
Size: 1133 bytes
Desc: not available
URL: <http://www.chiark.greenend.org.uk/pipermail/sgo-software-discuss/attachments/20120712/73cb20df/attachment.bin>

From ijackson at chiark.greenend.org.uk  Thu Jul 12 19:47:39 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 12 Jul 2012 19:47:39 +0100
Subject: [missing subject]
In-Reply-To: <4FFEFB15.90203@chiark.greenend.org.uk>
References: <4FFEFB15.90203@chiark.greenend.org.uk>
Message-ID: <20479.7115.925158.169842@chiark.greenend.org.uk>

Matthew Vernon writes ("[missing subject]"):
> Subject: [PATCH] Update build-dependency documentation
> To: sgo-software-discuss at greenend.org.uk

Thanks.  I have incorporated this into my tree and will ensure that it
makes it into the master in due course.

You didn't include a Signed-off-by.  Since I'm confident that you did
in fact write this patch and intend for it to be distributed and
licensed like the rest of secnet, I added your Signed-off-by myself.

I trust this is OK.  If not please let me know ASAP.

Ian.

See eg
 http://wiki.xen.org/wiki/SubmittingXenPatches#Signing_off_a_patch


From ijackson at chiark.greenend.org.uk  Thu Jul 12 19:56:09 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 12 Jul 2012 19:56:09 +0100
Subject: [PATCH 2/5] make-secnet-sites: Fix userv invocation after
	pfilepath
In-Reply-To: <4FF076EF.6000206@terraraq.org.uk>
References: <1324159397-28439-1-git-send-email-ijackson@chiark.greenend.org.uk>
	<1324159397-28439-3-git-send-email-ijackson@chiark.greenend.org.uk>
	<4FF076EF.6000206@terraraq.org.uk>
Message-ID: <20479.7625.840913.739309@chiark.greenend.org.uk>

Richard Kettlewell writes ("Re: [PATCH 2/5] make-secnet-sites: Fix userv invocation after pfilepath"):
> I don't seem to have time to do the full set of outstanding patches 
> justice, but I did notice a couple of trivial things:

Thanks.

> On 17/12/2011 22:03, Ian Jackson wrote:
>  > +		headerinput=pfilepath(header,allow_include=True)
> 
> The description seems to be slightly inaccurate - 'headerinput' rather 
> than 'headerlines'.

I have fixed this.  (NB this means that the "live" branch in chiark's
/home/secnet/secnet-live.git is going to be rebasing.)

> On 17/12/2011 22:03, Ian Jackson wrote:
>  > -#define LINK_QUALITY_DOWN 0   /* No chance of a packet being 
> delivered */
>  > -#define LINK_QUALITY_DOWN_STALE_ADDRESS 1 /* Link down, old address 
> information */
>  > -#define LINK_QUALITY_DOWN_CURRENT_ADDRESS 2 /* Link down, current 
> address information */
>  > -#define LINK_QUALITY_UP 3     /* Link active */
>  > +#define LINK_QUALITY_UNUSED 0   /* This link is unused, do not make 
> this netlink */
>  > +#define LINK_QUALITY_DOWN 1   /* No chance of a packet being 
> delivered right away*/
>  > +#define LINK_QUALITY_DOWN_STALE_ADDRESS 2 /* Link down, old address 
> information */
>  > +#define LINK_QUALITY_DOWN_CURRENT_ADDRESS 3 /* Link down, current 
> address information */
>  > +#define LINK_QUALITY_UP 4     /* Link active */
> 
> Perhaps this should be an enum.

Maybe.  I don't think that's critical now though.

Ian.


From ijackson at chiark.greenend.org.uk  Thu Jul 12 20:32:50 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 12 Jul 2012 20:32:50 +0100
Subject: [PATCH 0/5] Fixes for recent breakage, make mobile work
In-Reply-To: <1324159397-28439-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1324159397-28439-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <20479.9826.788938.950710@chiark.greenend.org.uk>

Ian Jackson writes ("[PATCH 0/5] Fixes for recent breakage, make mobile work"):
> Some of my recent changes turned out not to work very well.  
> 
> Also, some other changes were needed to get the mobile sites
> functionality working properly, and to be able to split out on chiark
> configuration for "unusual" sites.
> 
> Signed-off-by: Ian Jackson <ijackson at chiark.greenend.org.uk>

This series (with the message fix as mentioned) is going to be in
0.3.0~beta1.

Ian.


From ijackson at chiark.greenend.org.uk  Thu Jul 12 20:34:32 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 12 Jul 2012 20:34:32 +0100
Subject: [PATCH v2 00/19] Security, logging and reliability fixes
In-Reply-To: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1340248979-32459-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <20479.9928.832828.837451@chiark.greenend.org.uk>

Ian Jackson writes ("[PATCH v2 00/19] Security, logging and reliability fixes"):
> Important fixes, posted already (commit message for the log truncation
> lockup fix has more justification now):

This series is going to be in 0.3.0~beta1.

Ian.


From ijackson at chiark.greenend.org.uk  Thu Jul 12 20:40:20 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 12 Jul 2012 20:40:20 +0100
Subject: [PATCH 1/9] chiark live tree fixes including proposed sites bugfix
In-Reply-To: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
References: <1341965406-1735-1-git-send-email-ijackson@chiark.greenend.org.uk>
Message-ID: <20479.10276.516950.214751@chiark.greenend.org.uk>

Ian Jackson writes ("[PATCH 1/9] chiark live tree fixes including proposed sites bugfix"):
> These patches are in ~secnet/secnet-live.git on chiark:
...
> I'm proposing these additional patches to sort out some problems with
> the way I was using `include':
>  7/9 make-secnet-sites: Do newline-trimming in pline()
>  8/9 make-secnet-sites: In -u mode, output file "dereferences" include directives
>  9/9] make-secnet-sites: Do not permit "include" in simple sites files

These patches are going to be in 0.3.0~beta1.

> In particular, Steve was complaining that the generated sites file
> contained an "include" directive which (a) means you can't process it
> with any released version of secnet and (b) anyway a normal site admin
> running make-secnet-sites should not have to trust the sites file
> (to the extent of having to scrutinise it for includes).

This seems to work.

> I don't have time right now but in a test copy of ~secnet/sgo-vpn I
> ran this
>   USERV_USER=ian USERV_GROUP=ian-rela ~/things/Fvpn/secnet.git/make-secnet-sites -u header groupfiles newsites ian-rela </dev/null
> and it seemed to do the right thing.
> 
> The file "newsites" is below.

I ran
  userv secnet vpnsites ian-rela </dev/null
and
  userv secnet chiarkvpnsites ian-rela </u/ian/vpn-site-zealot
to get it to regenerate the sites files and it seems to have done the
right thing.  In particular the sgo-vpn/sites file now contains the
expected contents, no include directive, and can be processed by
make-secnet-sites into something that looks about right.

Steve: it would be good if you could manage to take a look at this
before we actually want to release 0.3.0, which ought to be fairly
soon given the security bugs in 0.2.x.

Ian.


From ijackson at chiark.greenend.org.uk  Thu Jul 12 20:49:25 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 12 Jul 2012 20:49:25 +0100
Subject: secnet 0.3.0~beta1
Message-ID: <20479.10821.484822.201999@chiark.greenend.org.uk>

I have decided that we should actually incorporate outstanding patches
and make a release.  Therefore I have pushed and tagged 0.3.0~beta1
and also made a tarball and .deb of it.

This (well, something almost exactly like it) is now running on chiark
and I am also testing it on my netbook.  I'd encourage anyone else who
cares about it not being broken to test it.

secnet (0.3.0~beta1) unstable; urgency=low

  * New upstream version.
   - SECURITY FIX: avoid crashes (or buffer overrun) on short packets.
   - Bugfixes relating to packet loss during key exchange.
   - Bugfixes relating to link up/down status.
   - Bugfixes relating to logging.
   - make-secnet-sites made more sophisticated to support two vpns on chiark.
   - Documentation improvements.
   - Build system improvements.
  * Debian packaging improvements:
   - Native package.
   - Maintainer / uploaders.
   - init script requires $remove_fs since we're in /usr.

 -- Ian Jackson <ijackson at chiark.greenend.org.uk>  Thu, 12 Jul 2012 20:18:16 +0100

http://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git/secnet.git/
http://www.chiark.greenend.org.uk/~secnet/release/0.3.0~beta1/

chiark:/home/secnet:

3df156009b64e69149dc7dbb0867b85e8b1913b28db920f6ab09d04e92c05eed  secnet_0.3.0~beta1.dsc
17bcae3a25b3ccd4141c75da87f785e1b714d482ae2071c494c84d2e0f3eeb79  secnet_0.3.0~beta1.tar.gz
11de3985e9697bdc59bf8319db3948719dacd21310d045e715a5291ae99fb1b1  secnet_0.3.0~beta1_i386.build
e6795cc3c39682552ac83b6260d6967c2b9bfe844ee459a6e4583065b34d8da1  secnet_0.3.0~beta1_i386.changes
4ef2473f6ba2b05c09f1b51949489511d32d4a87c6582d7636fcd8f4a31e5a2a  secnet_0.3.0~beta1_i386.deb

Ian.


From ijackson at chiark.greenend.org.uk  Thu Jul 12 21:42:09 2012
From: ijackson at chiark.greenend.org.uk (Ian Jackson)
Date: Thu, 12 Jul 2012 21:42:09 +0100
Subject: secnet 0.3.0~beta1
In-Reply-To: <4FFF2CEA.9000801@greenend.org.uk>
References: <20479.10821.484822.201999@chiark.greenend.org.uk>
	<4FFF2CEA.9000801@greenend.org.uk>
Message-ID: <20479.13985.256940.2204@chiark.greenend.org.uk>

(sgo-software discuss added back into the CC)

Stephen Early writes ("Re: secnet 0.3.0~beta1"):
> Works for me with the current sites file from chiark (where the previous 
> version did not).

Good, thanks.

> Notes:
> 
> version.c still says 0.2.1

Gah.

> make-secnet-sites still says 0.1.18

Gah.

Do we have a release checklist ?

> The directory name in the .tar.gz is secnet.git - is this deliberate?

I used git-buildpackage and that was the name of the directory on my
workstation.  It would be nicer if it were called secnet/ or
secnet-<version>/ but I can't see how to get git-buildpackage to do
that.

Ian.


From matthewv at chiark.greenend.org.uk  Fri Jul 13 14:00:44 2012
From: matthewv at chiark.greenend.org.uk (Matthew Vernon)
Date: Fri, 13 Jul 2012 14:00:44 +0100
Subject: secnet 0.3.0~beta1
In-Reply-To: <20479.10821.484822.201999@chiark.greenend.org.uk>
References: <20479.10821.484822.201999@chiark.greenend.org.uk>
Message-ID: <50001BFC.5080207@chiark.greenend.org.uk>

Hi,

On 12/07/12 20:49, Ian Jackson wrote:
> I have decided that we should actually incorporate outstanding patches
> and make a release.  Therefore I have pushed and tagged 0.3.0~beta1
> and also made a tarball and .deb of it.

This seems to work for me.

I think the postinst should run
adduser --system --no-create-home secnet
?

Thanks,

Matthew


From anakin at pobox.com  Tue Nov 27 19:10:17 2012
From: anakin at pobox.com (Simon Tatham)
Date: Tue, 27 Nov 2012 19:10:17 +0000
Subject: secnet bug: tun and mobile sites
Message-ID: <E1TdQXx-0003S5-Vv@atreus.tartarus.org>

secnet's 'tun' netlink will add and remove kernel routing table
entries during PHASE_RUN if the OPT_SOFTROUTE option is set.
However, at startup during PHASE_GETRESOURCES it will set up routes
for a site only if that site lists an address. So if you have a site
in your sites file with no address but also haven't enabled
OPT_SOFTROUTE (e.g. because you run secnet so that it drops privs),
then no route for that site will _ever_ be set up.

These two policies don't match. We should bring a site's route(s) up
at startup in any situation where we will not be prepared to do so
dynamically during run time. In other words, routes should be added
at startup time not only if they have a fixed address parameter, but
also if they do not have OPT_SOFTROUTE set.

(See also bdd4351ff2fc6dc8b1dad689f751ac46347636cf, which seems to
be fixing the analogous bug for userv-ipif.)

The attached patch implements this fix, and causes my home secnet
implementation to be able to route to my laptop successfully
(sgo/resolution/resolution). In order to do this I've had to move
the definition of OPT_SOFTROUTE out of netlink.c into netlink.h so
that tun_set_route can see it, which suggests a possible layering
violation, but on the other hand since the netlink_client structure
is visible outside netlink.c it seems only reasonable that the bit
flags used in its 'options' field should be visible too.

Cheers,
Simon
-- 
Simon Tatham         "_shin_, n. An ingenious device for
<anakin at pobox.com>    finding tables and chairs in the dark."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-tun-add-hard-routes-even-if-they-are-currently-down.patch
Type: text/x-diff
Size: 3835 bytes
Desc: not available
URL: <http://www.chiark.greenend.org.uk/pipermail/sgo-software-discuss/attachments/20121127/ad0fbaca/attachment.patch>

From anakin at pobox.com  Tue Nov 27 20:28:22 2012
From: anakin at pobox.com (Simon Tatham)
Date: Tue, 27 Nov 2012 20:28:22 +0000
Subject: secnet bug: tun and mobile sites
In-Reply-To: <E1TdQXx-0003S5-Vv@atreus.tartarus.org>
Message-ID: <E1TdRlW-0006Fp-Pm@atreus.tartarus.org>

Simon Tatham <anakin at pobox.com> wrote:

> The attached patch implements this fix,

Oh, I've just noticed a previous mailing list message muttering
about Signed-off-by lines. If my patch is acceptable, please feel
free to add to my commit message the line

Signed-off-by: Simon Tatham <anakin at pobox.com>
-- 
Simon Tatham         "I thought I'd put my foot so far into my mouth I
<anakin at pobox.com>    wouldn't be able to sit down without standing up."