From 92a7d254975db245c3320855515bffc1aebda9e4 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:49 +0100 Subject: [PATCH 01/16] transform: split out transform-common.h To avoid too much duplication, some boilerplate and helpful code from transport.c is now brought out into macros in transport-common.h. It will be reused in the later commits introducing the EAX transform. Also, rename transform.c to transform-cbcmac.c, etc. Signed-off-by: Ian Jackson --- Makefile.in | 3 +- README | 2 +- modules.c | 2 +- secnet.h | 2 +- transform.c => transform-cbcmac.c | 55 ++++++------------------------ transform-common.h | 56 +++++++++++++++++++++++++++++++ 6 files changed, 71 insertions(+), 49 deletions(-) rename transform.c => transform-cbcmac.c (89%) create mode 100644 transform-common.h diff --git a/Makefile.in b/Makefile.in index 3f15f01..401fbb8 100644 --- a/Makefile.in +++ b/Makefile.in @@ -53,7 +53,8 @@ mandir:=@mandir@ TARGETS:=secnet OBJECTS:=secnet.o util.o conffile.yy.o conffile.tab.o conffile.o modules.o \ - resolver.o random.o udp.o site.o transform.o netlink.o rsa.o dh.o \ + resolver.o random.o udp.o site.o transform-cbcmac.o \ + netlink.o rsa.o dh.o \ serpentbe.o md5.o version.o tun.o slip.o sha1.o ipaddr.o log.o \ process.o @LIBOBJS@ \ hackypar.o diff --git a/README b/README index 84bb392..93730e9 100644 --- a/README +++ b/README @@ -336,7 +336,7 @@ setup but more relaxed about using old keys. These are noted with "mobile:", above, and apply whether the mobile peer is local or remote. -** transform +** transform-cbcmac Defines: serpent256-cbc (closure => transform closure) diff --git a/modules.c b/modules.c index 9b94e25..0290cd4 100644 --- a/modules.c +++ b/modules.c @@ -7,7 +7,7 @@ void init_builtin_modules(dict_t *dict) udp_module(dict); util_module(dict); site_module(dict); - transform_module(dict); + transform_cbcmac_module(dict); netlink_module(dict); rsa_module(dict); dh_module(dict); diff --git a/secnet.h b/secnet.h index 037ef80..dbca664 100644 --- a/secnet.h +++ b/secnet.h @@ -217,7 +217,7 @@ extern init_module random_module; extern init_module udp_module; extern init_module util_module; extern init_module site_module; -extern init_module transform_module; +extern init_module transform_cbcmac_module; extern init_module netlink_module; extern init_module rsa_module; extern init_module dh_module; diff --git a/transform.c b/transform-cbcmac.c similarity index 89% rename from transform.c rename to transform-cbcmac.c index 281e667..1e8a5e9 100644 --- a/transform.c +++ b/transform-cbcmac.c @@ -36,6 +36,8 @@ struct transform_inst { bool_t keyed; }; +#include "transform-common.h" + #define PKCS5_MASK 15 static bool_t transform_setkey(void *sst, uint8_t *key, int32_t keylen) @@ -67,12 +69,7 @@ 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; -} +TRANSFORM_VALID; static void transform_delkey(void *sst) { @@ -95,10 +92,7 @@ static uint32_t transform_forward(void *sst, struct buffer_if *buf, uint8_t *p, *n; int i; - if (!ti->keyed) { - *errmsg="transform unkeyed"; - return 1; - } + KEYED_CHECK; /* Sequence number */ buf_prepend_uint32(buf,ti->sendseq); @@ -164,7 +158,7 @@ static uint32_t transform_reverse(void *sst, struct buffer_if *buf, uint8_t *padp; int padlen; int i; - uint32_t seqnum, skew; + uint32_t seqnum; uint8_t iv[16]; uint8_t pct[16]; uint8_t macplain[16]; @@ -172,10 +166,7 @@ static uint32_t transform_reverse(void *sst, struct buffer_if *buf, uint8_t *n; uint8_t *macexpected; - if (!ti->keyed) { - *errmsg="transform unkeyed"; - return 1; - } + KEYED_CHECK; if (buf->size < 4 + 16 + 16) { *errmsg="msg too short"; @@ -238,46 +229,20 @@ static uint32_t transform_reverse(void *sst, struct buffer_if *buf, /* Sequence number must be within max_skew of lastrecvseq; lastrecvseq is only allowed to increase. */ seqnum=buf_unprepend_uint32(buf); - skew=seqnum-ti->lastrecvseq; - if (skew<0x8fffffff) { - /* Ok */ - ti->lastrecvseq=seqnum; - } else if ((0-skew)max_skew) { - /* Ok */ - } else { - /* Too much skew */ - *errmsg="seqnum: too much skew"; - return 2; - } + SEQNUM_CHECK(seqnum, ti->max_skew); return 0; } -static void transform_destroy(void *sst) -{ - struct transform_inst *st=sst; - - FILLZERO(*st); /* Destroy key material */ - free(st); -} +TRANSFORM_DESTROY; static struct transform_inst_if *transform_create(void *sst) { - struct transform_inst *ti; struct transform *st=sst; - ti=safe_malloc(sizeof(*ti),"transform_create"); - /* mlock XXX */ + TRANSFORM_CREATE_CORE; - 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; - ti->ops.destroy=transform_destroy; ti->max_skew=st->max_seq_skew; - ti->keyed=False; return &ti->ops; } @@ -316,7 +281,7 @@ static list_t *transform_apply(closure_t *self, struct cloc loc, return new_closure(&st->cl); } -void transform_module(dict_t *dict) +void transform_cbcmac_module(dict_t *dict) { struct keyInstance k; uint8_t data[32]; diff --git a/transform-common.h b/transform-common.h new file mode 100644 index 0000000..b3c70a8 --- /dev/null +++ b/transform-common.h @@ -0,0 +1,56 @@ + +#ifndef TRANSFORM_COMMON_H +#define TRANSFORM_COMMON_H + +#define KEYED_CHECK do{ \ + if (!ti->keyed) { \ + *errmsg="transform unkeyed"; \ + return 1; \ + } \ + }while(0) + +#define SEQNUM_CHECK(seqnum, max_skew) do{ \ + uint32_t skew=seqnum-ti->lastrecvseq; \ + if (skew<0x8fffffff) { \ + /* Ok */ \ + ti->lastrecvseq=seqnum; \ + } else if ((0-skew)keyed; \ + } + +#define TRANSFORM_DESTROY \ + static void transform_destroy(void *sst) \ + { \ + struct transform_inst *st=sst; \ + \ + FILLZERO(*st); /* Destroy key material */ \ + free(st); \ + } + +#define TRANSFORM_CREATE_CORE \ + struct transform_inst *ti; \ + ti=safe_malloc(sizeof(*ti),"transform_create"); \ + /* mlock XXX */ \ + 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; \ + ti->ops.destroy=transform_destroy; \ + ti->keyed=False; + +#endif /*TRANSFORM_COMMON_H*/ -- 2.30.2 From 7c9ca4bd3cf857d45d931f224fe6415b36d1cffe Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:49 +0100 Subject: [PATCH 02/16] transform: Allow DH to set the key size It turns out that the current Serpent CBC-MAC transform takes the raw DH shared secret, and parcels it up in byte ranges for the various uses (some of which are published). Well, obviously this is not a good idea. But also it means the interface isn't set up to allow the size of the key data provided to the transform to be determined by the size of the DH modulus. Fix this latter interface problem. Now a transform can set its keylen to 0, meaning it will be provided with the whole of the DH private value. We don't use this new feature yet. We can't make the existing transform use it without breaking compatibility, but it will be used by the new EAX-based transform. Signed-off-by: Ian Jackson --- dh.c | 4 ++++ secnet.h | 3 ++- site.c | 14 ++++++++------ 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/dh.c b/dh.c index 2383192..c37b538 100644 --- a/dh.c +++ b/dh.c @@ -125,6 +125,10 @@ static list_t *dh_apply(closure_t *self, struct cloc loc, dict_t *context, st->ops.len=sz; + st->ops.ceil_len=(mpz_sizeinbase(&st->p,2)+7)/8; + /* According to the docs, mpz_sizeinbase(,256) is allowed to return + * an answer which is 1 too large. But mpz_sizeinbase(,2) isn't. */ + return new_closure(&st->cl); } diff --git a/secnet.h b/secnet.h index dbca664..7d7eb4f 100644 --- a/secnet.h +++ b/secnet.h @@ -397,7 +397,7 @@ struct transform_if { void *st; int32_t max_start_pad; /* these three are all <<< INT_MAX */ int32_t max_end_pad; - int32_t keylen; + int32_t keylen; /* 0 means give the transform exactly as much as there is */ transform_createinstance_fn *create; }; @@ -444,6 +444,7 @@ typedef void dh_makeshared_fn(void *st, uint8_t *secret, struct dh_if { void *st; int32_t len; /* Approximate size of modulus in bytes */ + int32_t ceil_len; /* Number of bytes just sufficient to contain modulus */ dh_makepublic_fn *makepublic; dh_makeshared_fn *makeshared; }; diff --git a/site.c b/site.c index b9b4d0d..566b215 100644 --- a/site.c +++ b/site.c @@ -288,6 +288,7 @@ struct site { uint64_t timeout; /* Timeout for current state */ uint8_t *dhsecret; uint8_t *sharedsecret; + uint32_t sharedsecretlen; struct transform_inst_if *new_transform; /* For key setup/verify */ }; @@ -561,11 +562,11 @@ static bool_t process_msg3(struct site *st, struct buffer_if *msg3, /* Generate the shared key */ st->dh->makeshared(st->dh->st,st->dhsecret,st->dh->len,m.pk, - st->sharedsecret,st->transform->keylen); + st->sharedsecret,st->sharedsecretlen); /* Set up the transform */ st->new_transform->setkey(st->new_transform->st,st->sharedsecret, - st->transform->keylen); + st->sharedsecretlen); return True; } @@ -609,10 +610,10 @@ static bool_t process_msg4(struct site *st, struct buffer_if *msg4, m.pk[m.pklen]=0; /* Generate the shared key */ st->dh->makeshared(st->dh->st,st->dhsecret,st->dh->len,m.pk, - st->sharedsecret,st->transform->keylen); + st->sharedsecret,st->sharedsecretlen); /* Set up the transform */ st->new_transform->setkey(st->new_transform->st,st->sharedsecret, - st->transform->keylen); + st->sharedsecretlen); return True; } @@ -1009,7 +1010,7 @@ static void enter_state_run(struct site *st) memset(st->remoteN,0,NONCELEN); st->new_transform->delkey(st->new_transform->st); memset(st->dhsecret,0,st->dh->len); - memset(st->sharedsecret,0,st->transform->keylen); + memset(st->sharedsecret,0,st->sharedsecretlen); set_link_quality(st); } @@ -1557,7 +1558,8 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context, transport_peers_clear(st,&st->setup_peers); /* XXX mlock these */ st->dhsecret=safe_malloc(st->dh->len,"site:dhsecret"); - st->sharedsecret=safe_malloc(st->transform->keylen,"site:sharedsecret"); + st->sharedsecretlen=st->transform->keylen?:st->dh->ceil_len; + st->sharedsecret=safe_malloc(st->sharedsecretlen,"site:sharedsecret"); /* We need to compute some properties of our comms */ #define COMPUTE_WORST(pad) \ -- 2.30.2 From 0118121ae6578c69527fb80a60294c48663033b7 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:49 +0100 Subject: [PATCH 03/16] transform: Pass a direction flag to the transform The same transform is used for inbound and outbound packets. The transform should know which direction these packets are flowing in; that (a) allows a transform to reject packets which are "looping back" so to speak, and (b) makes it easier for a transform to generate unique nonces. This will be used by the forthcoming EAX transform. It is combined with the sequence number (the same values of which are used by both ends) to make the nonce, which must be unique across the single shared key, ie unique across both flows. Signed-off-by: Ian Jackson --- secnet.h | 7 +++++-- site.c | 4 ++-- transform-cbcmac.c | 3 ++- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/secnet.h b/secnet.h index 7d7eb4f..5e66a17 100644 --- a/secnet.h +++ b/secnet.h @@ -368,10 +368,13 @@ struct site_if { also depend on internal factors (eg. time) and keep internal state. A struct transform_if only represents a particular type of transformation; instances of the transformation (eg. with - particular key material) have a different C type. */ + particular key material) have a different C type. The same + secret key will be used in opposite directions between a pair of + secnets; one of these pairs will get direction==False, the other True. */ 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_setkey_fn(void *st, uint8_t *key, int32_t keylen, + bool_t direction); 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); diff --git a/site.c b/site.c index 566b215..f1a0317 100644 --- a/site.c +++ b/site.c @@ -566,7 +566,7 @@ static bool_t process_msg3(struct site *st, struct buffer_if *msg3, /* Set up the transform */ st->new_transform->setkey(st->new_transform->st,st->sharedsecret, - st->sharedsecretlen); + st->sharedsecretlen,st->setup_priority); return True; } @@ -613,7 +613,7 @@ static bool_t process_msg4(struct site *st, struct buffer_if *msg4, st->sharedsecret,st->sharedsecretlen); /* Set up the transform */ st->new_transform->setkey(st->new_transform->st,st->sharedsecret, - st->sharedsecretlen); + st->sharedsecretlen,st->setup_priority); return True; } diff --git a/transform-cbcmac.c b/transform-cbcmac.c index 1e8a5e9..5fb66ba 100644 --- a/transform-cbcmac.c +++ b/transform-cbcmac.c @@ -40,7 +40,8 @@ struct transform_inst { #define PKCS5_MASK 15 -static bool_t transform_setkey(void *sst, uint8_t *key, int32_t keylen) +static bool_t transform_setkey(void *sst, uint8_t *key, int32_t keylen, + bool_t direction) { struct transform_inst *ti=sst; -- 2.30.2 From b02b720ac62afd3a45c44e7ced37c090e7b39da9 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:49 +0100 Subject: [PATCH 04/16] transform: Provide Serpent-EAX transform This provides an alternative to the (rather badly broken) serpent256-cbc transform. In this patch, there is not yet any algorith negotiation for a smooth upgrade, nor any changes to the example configurations. Signed-off-by: Ian Jackson --- Makefile.in | 6 +- README | 5 + modules.c | 1 + secnet.8 | 21 +++- secnet.h | 3 + transform-eax.c | 307 ++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 338 insertions(+), 5 deletions(-) create mode 100644 transform-eax.c diff --git a/Makefile.in b/Makefile.in index 401fbb8..5a140fb 100644 --- a/Makefile.in +++ b/Makefile.in @@ -53,9 +53,9 @@ mandir:=@mandir@ TARGETS:=secnet OBJECTS:=secnet.o util.o conffile.yy.o conffile.tab.o conffile.o modules.o \ - resolver.o random.o udp.o site.o transform-cbcmac.o \ - netlink.o rsa.o dh.o \ - serpentbe.o md5.o version.o tun.o slip.o sha1.o ipaddr.o log.o \ + resolver.o random.o udp.o site.o transform-cbcmac.o transform-eax.o \ + netlink.o rsa.o dh.o serpent.o serpentbe.o \ + md5.o sha512.o version.o tun.o slip.o sha1.o ipaddr.o log.o \ process.o @LIBOBJS@ \ hackypar.o diff --git a/README b/README index 93730e9..71a5a44 100644 --- a/README +++ b/README @@ -336,6 +336,11 @@ setup but more relaxed about using old keys. These are noted with "mobile:", above, and apply whether the mobile peer is local or remote. +** transform-eax + +Defines: + eax-serpent (closure => transform closure) + ** transform-cbcmac Defines: diff --git a/modules.c b/modules.c index 0290cd4..724ccbb 100644 --- a/modules.c +++ b/modules.c @@ -7,6 +7,7 @@ void init_builtin_modules(dict_t *dict) udp_module(dict); util_module(dict); site_module(dict); + transform_eax_module(dict); transform_cbcmac_module(dict); netlink_module(dict); rsa_module(dict); diff --git a/secnet.8 b/secnet.8 index ef07a76..2bf2250 100644 --- a/secnet.8 +++ b/secnet.8 @@ -415,8 +415,8 @@ A \fIrandomsource closure\fR is a source of random numbers. .PP Read the contents of the file \fIPATH\fR (a string) and return it as a string. -.SS serpent256-cbc -\fBserpent256-cbc(\fIDICT\fB)\fR => \fItransform closure\fR +.SS eax-serpent +\eax-fBserpent(\fIDICT\fB)\fR => \fItransform closure\fR .PP Valid keys in the \fIDICT\fR argument are: .TP @@ -425,10 +425,27 @@ The maximum acceptable difference between the sequence number in a received, decrypted message and the previous one. The default is 10. It may be necessary to increase this is if connectivity is poor. +.TP +.B tag-length-bytes +The length of the message authentication tag. The default is 16, +for a 128-bit tag length. It must be no longer than the Serpent +blocksize, 16. Must be have the same value at both ends. +.TP +.B padding-rounding +Messages are padded to a multiple of this many bytes. This +serves to obscure the exact length of messages. The default is 16, .PP A \fItransform closure\fR is a reversible means of transforming messages for transmission over a (presumably) insecure network. It is responsible for both confidentiality and integrity. + +.SS serpent256-cbc +\fBserpent256-cbc(\fIDICT\fB)\fR => \fItransform closure\fR +.PP +Valid keys in the \fIDICT\fR argument are: +.TP +.B max-sequence-skew +As above. .PP Note that this uses a big-endian variant of the Serpent block cipher (which is not compatible with most other Serpent implementations). diff --git a/secnet.h b/secnet.h index 5e66a17..0433c1e 100644 --- a/secnet.h +++ b/secnet.h @@ -7,6 +7,8 @@ #include #include #include +#include +#include #include #include #include @@ -217,6 +219,7 @@ extern init_module random_module; extern init_module udp_module; extern init_module util_module; extern init_module site_module; +extern init_module transform_eax_module; extern init_module transform_cbcmac_module; extern init_module netlink_module; extern init_module rsa_module; diff --git a/transform-eax.c b/transform-eax.c new file mode 100644 index 0000000..5c7a120 --- /dev/null +++ b/transform-eax.c @@ -0,0 +1,307 @@ +/* + * eax-transform.c: EAX-Serpent bulk data transformation + * + * We use EAX with the following parameters: + * + * Plaintext: + * Concatenation of: + * Data packet as supplied to us + * Zero or more zero bytes ignored by receiver } padding + * One byte padding length } + * This is a bit like PKCS#5. It helps disguise message lengths. + * It also provides a further room for future expansion. When + * transmitting we pad the message to the next multiple of + * a configurable rounding factor, 16 bytes by default. + * + * Transmitted message: + * Concatenation of: + * EAX ciphertext + * 32-bit sequence number (initially zero) + * The sequence number allows us to discard far-too-old + * packets. + * + * Nonce: + * Concatenation of: + * 32-bit sequence number (big endian) + * initial value comes from SHA-512 hash (see below) + * 1 byte: 0x01 if sender has setup priority, 0x00 if it doesn't + * (ie, the direction of data flow) + * + * Header: None + * + * Tag length: + * 16 bytes (128 bits) by default + * + * Key: + * The first 32 bytes of the SHA-512 hash of the shared secret + * from the DH key exchange (the latter being expressed as + * the shortest possible big-endian octet string). + * + * The bytes [32,40> of the hash of the shared secret are used for + * initial sequence numbers: [32,36> for those sent by the end without + * setup priority, [36,40> for those for the other end. + * + */ + +#include "secnet.h" +#include "unaligned.h" +#include "util.h" +#include "serpent.h" +#include "sha512.h" +#include "transform-common.h" +#include "hexdebug.h" + +#define BLOCK_SIZE 16 +#define SEQLEN 4 + +struct transform_params { + uint32_t max_seq_skew, tag_length, padding_mask; +}; + +struct transform { + closure_t cl; + struct transform_if ops; + struct transform_params p; +}; + +struct transform_inst { + struct transform_inst_if ops; + struct transform_params p; + unsigned keyed:1; + /* remaining valid iff keyed */ + unsigned direction:1; + uint32_t sendseq; + uint32_t lastrecvseq; + struct keyInstance key; + uint8_t info_b[BLOCK_SIZE], info_p[BLOCK_SIZE]; +}; + +static void block_encrypt(struct transform_inst *transform_inst, + uint8_t dst[BLOCK_SIZE], + const uint8_t src[BLOCK_SIZE]) +{ + serpent_encrypt(&transform_inst->key, src, dst); +} + +#define INFO struct transform_inst *transform_inst +#define I transform_inst +#define EAX_ENTRYPOINT_DECL static +#define BLOCK_ENCRYPT(dst,src) block_encrypt(transform_inst,dst,src) +#define INFO_B (transform_inst->info_b) +#define INFO_P (transform_inst->info_p) + +#include "eax.c" + +#if 0 + +#define TEAX_DEBUG(ary,sz) teax_debug(__func__,__LINE__,#ary,#sz,ary,sz) +static void teax_debug(const char *func, int line, + const char *aryp, const char *szp, + const void *ary, size_t sz) +{ + fprintf(stderr,"TEAX %s:%-3d %10s %15s : ", func,line,aryp,szp); + hexdebug(stderr,ary,sz); + fprintf(stderr,"\n"); +} + +#else + +#define TEAX_DEBUG(ary,sz) /* empty */ + +#endif + +static bool_t transform_setkey(void *sst, uint8_t *key, int32_t keylen, + bool_t direction) +{ + struct transform_inst *ti=sst; + struct sha512_ctx hash_ctx; + uint8_t hash_out[64]; + + TEAX_DEBUG(key,keylen); + + sha512_init_ctx(&hash_ctx); + sha512_process_bytes(key, keylen, &hash_ctx); + sha512_finish_ctx(&hash_ctx, hash_out); + + TEAX_DEBUG(hash_out,32); + TEAX_DEBUG(hash_out+32,8); + + ti->direction=direction; + ti->sendseq=get_uint32(hash_out+32+direction*4); + ti->lastrecvseq=get_uint32(hash_out+32+!direction*4); + serpent_makekey(&ti->key, 32*8, hash_out); + eax_setup(ti); + ti->keyed=True; + + return True; +} + +TRANSFORM_VALID; + +TRANSFORM_DESTROY; + +static void transform_delkey(void *sst) +{ + struct transform_inst *ti=sst; + + FILLZERO(ti->key); + FILLZERO(ti->info_b); + FILLZERO(ti->info_p); + ti->keyed=False; +} + +static uint32_t transform_forward(void *sst, struct buffer_if *buf, + const char **errmsg) +{ + struct transform_inst *ti=sst; + + KEYED_CHECK; + + size_t padlen = ti->p.padding_mask - buf->size; + padlen &= ti->p.padding_mask; + padlen++; + + uint8_t *pad = buf_append(buf,padlen); + memset(pad, 0, padlen-1); + pad[padlen-1] = padlen; + + uint8_t nonce[SEQLEN+1]; + put_uint32(nonce,ti->sendseq); + nonce[SEQLEN] = ti->direction; + + TEAX_DEBUG(nonce,sizeof(nonce)); + TEAX_DEBUG(buf->start,buf->size); + + assert(buf_append(buf,ti->p.tag_length)); + eax_encrypt(ti, nonce,sizeof(nonce), 0,0, + buf->start,buf->size-ti->p.tag_length, + ti->p.tag_length, buf->start); + + TEAX_DEBUG(buf->start,buf->size); + + memcpy(buf_append(buf,SEQLEN), nonce, SEQLEN); + + TEAX_DEBUG(nonce,SEQLEN); + + ti->sendseq++; + + return 0; +} + +static uint32_t transform_reverse(void *sst, struct buffer_if *buf, + const char **errmsg) +{ + struct transform_inst *ti=sst; + + KEYED_CHECK; + + TEAX_DEBUG(buf->start,buf->size); + + uint8_t nonce[SEQLEN+1]; + const uint8_t *seqp = buf_unappend(buf,SEQLEN); + if (!seqp) goto too_short; + + TEAX_DEBUG(seqp,SEQLEN); + + uint32_t seqnum = get_uint32(seqp); + + memcpy(nonce,seqp,SEQLEN); + nonce[4] = !ti->direction; + + TEAX_DEBUG(nonce,sizeof(nonce)); + TEAX_DEBUG(buf->start,buf->size); + + bool_t ok = eax_decrypt(ti, nonce,sizeof(nonce), 0,0, buf->start,buf->size, + ti->p.tag_length, buf->start); + if (!ok) { + TEAX_DEBUG(0,0); + *errmsg="EAX decryption failed"; + return 1; + } + assert(buf->size >= (int)ti->p.tag_length); + buf->size -= ti->p.tag_length; + + TEAX_DEBUG(buf->start,buf->size); + + const uint8_t *padp = buf_unappend(buf,1); + if (!padp) goto too_short; + + TEAX_DEBUG(padp,1); + + size_t padlen = *padp; + if (!buf_unappend(buf,padlen-1)) goto too_short; + + SEQNUM_CHECK(seqnum, ti->p.max_seq_skew); + + TEAX_DEBUG(buf->start,buf->size); + + return 0; + + too_short: + *errmsg="ciphertext or plaintext too short"; + return 1; +} + +static struct transform_inst_if *transform_create(void *sst) +{ + struct transform *st=sst; + + TRANSFORM_CREATE_CORE; + + ti->p=st->p; + + return &ti->ops; +} + +static list_t *transform_apply(closure_t *self, struct cloc loc, + dict_t *context, list_t *args) +{ + struct transform *st; + item_t *item; + dict_t *dict; + + st=safe_malloc(sizeof(*st),"eax-serpent"); + st->cl.description="eax-serpent"; + st->cl.type=CL_TRANSFORM; + st->cl.apply=NULL; + st->cl.interface=&st->ops; + st->ops.st=st; + + /* First parameter must be a dict */ + item=list_elem(args,0); + if (!item || item->type!=t_dict) + cfgfatal(loc,"eax-serpent","parameter must be a dictionary\n"); + dict=item->data.dict; + + st->p.max_seq_skew=dict_read_number(dict, "max-sequence-skew", + False, "eax-serpent", loc, 10); + + st->p.tag_length=dict_read_number(dict, "tag-length-bytes", + False, "eax-serpent", loc, 128/8); + if (st->p.tag_length<1 || st->p.tag_length>BLOCK_SIZE) + cfgfatal(loc,"eax-serpent","tag-length-bytes out of range 0..%d\n", + BLOCK_SIZE); + + uint32_t padding_round=dict_read_number(dict, "padding-rounding", + False, "eax-serpent", loc, 16); + if (padding_round & (padding_round-1)) + cfgfatal(loc,"eax-serpent","padding-round not a power of two\n"); + if (padding_round > 255) + cfgfatal(loc,"eax-serpent","padding-round must be 1..128\n"); + if (padding_round == 0) + padding_round = 1; + st->p.padding_mask = padding_round-1; + + st->ops.max_start_pad=0; + st->ops.max_end_pad= padding_round + st->p.tag_length + SEQLEN; + + st->ops.keylen=0; + st->ops.create=transform_create; + + return new_closure(&st->cl); +} + +void transform_eax_module(dict_t *dict) +{ + add_closure(dict,"eax-serpent",transform_apply); +} -- 2.30.2 From 136740e64e3fedcd725490c2f80d0906de515197 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:50 +0100 Subject: [PATCH 05/16] magic: Introduce LABEL_NAK Replace ad-hoc comments on the message number of NAK packets with an explicit #define. No functional change. Signed-off-by: Ian Jackson --- magic.h | 1 + site.c | 2 +- udp.c | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/magic.h b/magic.h index 5ac1e34..c2503a0 100644 --- a/magic.h +++ b/magic.h @@ -3,6 +3,7 @@ #ifndef magic_h #define magic_h +#define LABEL_NAK 0x00000000 #define LABEL_MSG0 0x00020200 #define LABEL_MSG1 0x01010101 #define LABEL_MSG2 0x02020202 diff --git a/site.c b/site.c index f1a0317..a6fa9da 100644 --- a/site.c +++ b/site.c @@ -1286,7 +1286,7 @@ static bool_t site_incoming(void *sst, struct buffer_if *buf, uint32_t msgtype=ntohl(get_uint32(buf->start+8)); if (msgtype!=LABEL_MSG0) dump_packet(st,buf,source,True); switch (msgtype) { - case 0: /* NAK */ + case LABEL_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->current.remote_session_id) { diff --git a/udp.c b/udp.c index bbf8c64..77be5b1 100644 --- a/udp.c +++ b/udp.c @@ -21,6 +21,7 @@ #include "util.h" #include "unaligned.h" #include "ipaddr.h" +#include "magic.h" static beforepoll_fn udp_beforepoll; static afterpoll_fn udp_afterpoll; @@ -140,7 +141,7 @@ static void udp_afterpoll(void *state, struct pollfd *fds, int nfds) buffer_init(st->rbuf,0); buf_append_uint32(st->rbuf,dest); buf_append_uint32(st->rbuf,source); - buf_append_uint32(st->rbuf,0); /* NAK is msg type 0 */ + buf_append_uint32(st->rbuf,LABEL_NAK); sendto(st->fd, st->rbuf->start, st->rbuf->size, 0, (struct sockaddr *)&from, sizeof(from)); BUF_FREE(st->rbuf); -- 2.30.2 From bf28fc736c33aa8b26b3c3267cf4c3b33e69c4a5 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:50 +0100 Subject: [PATCH 06/16] udp.c: Do not send NAKs in response to NAKs If an incoming packet isn't name-addressed and has an invalid destination site id, udp.c would send a NAK. This is not a good idea - if somehow the source site id was wrong too, it will result in a NAK storm. This is a security vulnerability as it can be used by an attacker to trigger an unending NAK storm. Also, improve the message printed when a NAK is sent by udp.c because no site wanted to handle it. Signed-off-by: Ian Jackson --- udp.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/udp.c b/udp.c index 77be5b1..c83e618 100644 --- a/udp.c +++ b/udp.c @@ -133,17 +133,26 @@ static void udp_afterpoll(void *state, struct pollfd *fds, int nfds) } } if (!done) { - uint32_t source,dest; - /* Manufacture and send NAK packet */ - source=get_uint32(st->rbuf->start); /* Us */ - dest=get_uint32(st->rbuf->start+4); /* Them */ - Message(M_INFO,"udp (port %d): sending NAK\n",st->port); - buffer_init(st->rbuf,0); - buf_append_uint32(st->rbuf,dest); - buf_append_uint32(st->rbuf,source); - buf_append_uint32(st->rbuf,LABEL_NAK); - sendto(st->fd, st->rbuf->start, st->rbuf->size, 0, - (struct sockaddr *)&from, sizeof(from)); + uint32_t msgtype; + if (st->rbuf->size>12 /* prevents traffic amplification */ + && ((msgtype=get_uint32(st->rbuf->start+8)) + != LABEL_NAK)) { + uint32_t source,dest; + /* Manufacture and send NAK packet */ + source=get_uint32(st->rbuf->start); /* Us */ + dest=get_uint32(st->rbuf->start+4); /* Them */ + Message(M_INFO,"udp (port %d, peer %s):" + " %08"PRIx32"<-%08"PRIx32": %08"PRIx32":" + " unwanted/incorrect, sending NAK\n", + st->port, saddr_to_string(&from), + dest, source, msgtype); + buffer_init(st->rbuf,0); + buf_append_uint32(st->rbuf,dest); + buf_append_uint32(st->rbuf,source); + buf_append_uint32(st->rbuf,LABEL_NAK); + sendto(st->fd, st->rbuf->start, st->rbuf->size, 0, + (struct sockaddr *)&from, sizeof(from)); + } BUF_FREE(st->rbuf); } BUF_ASSERT_FREE(st->rbuf); -- 2.30.2 From 8534d6023d74efac11d7e2b5d1903638f3c1eca3 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:50 +0100 Subject: [PATCH 07/16] udp, util: Break out send_nak function Move the code in udp.c which constructs NAKs into its own function in util.c. This will make it easier to reuse. Signed-off-by: Ian Jackson --- udp.c | 20 +++++--------------- util.c | 17 +++++++++++++++++ util.h | 4 ++++ 3 files changed, 26 insertions(+), 15 deletions(-) diff --git a/udp.c b/udp.c index c83e618..483ed37 100644 --- a/udp.c +++ b/udp.c @@ -121,12 +121,12 @@ static void udp_afterpoll(void *state, struct pollfd *fds, int nfds) buf_unprepend(st->rbuf,2); memcpy(&from.sin_port,buf_unprepend(st->rbuf,2),2); } + struct comm_addr ca; + FILLZERO(ca); + ca.comm=&st->ops; + ca.sin=from; done=False; for (n=st->notify; n; n=n->next) { - struct comm_addr ca; - FILLZERO(ca); - ca.comm=&st->ops; - ca.sin=from; if (n->fn(n->state, st->rbuf, &ca)) { done=True; break; @@ -141,17 +141,7 @@ static void udp_afterpoll(void *state, struct pollfd *fds, int nfds) /* Manufacture and send NAK packet */ source=get_uint32(st->rbuf->start); /* Us */ dest=get_uint32(st->rbuf->start+4); /* Them */ - Message(M_INFO,"udp (port %d, peer %s):" - " %08"PRIx32"<-%08"PRIx32": %08"PRIx32":" - " unwanted/incorrect, sending NAK\n", - st->port, saddr_to_string(&from), - dest, source, msgtype); - buffer_init(st->rbuf,0); - buf_append_uint32(st->rbuf,dest); - buf_append_uint32(st->rbuf,source); - buf_append_uint32(st->rbuf,LABEL_NAK); - sendto(st->fd, st->rbuf->start, st->rbuf->size, 0, - (struct sockaddr *)&from, sizeof(from)); + send_nak(&ca,source,dest,msgtype,st->rbuf,"unwanted"); } BUF_FREE(st->rbuf); } diff --git a/util.c b/util.c index 3d11987..1b46bc0 100644 --- a/util.c +++ b/util.c @@ -38,6 +38,7 @@ #include #include "util.h" #include "unaligned.h" +#include "magic.h" #define MIN_BUFFER_SIZE 64 #define DEFAULT_BUFFER_SIZE 4096 @@ -381,6 +382,22 @@ static list_t *buffer_apply(closure_t *self, struct cloc loc, dict_t *context, return new_closure(&st->cl); } +void send_nak(const struct comm_addr *dest, uint32_t our_index, + uint32_t their_index, uint32_t msgtype, + struct buffer_if *buf, const char *logwhy) +{ + buffer_init(buf,dest->comm->min_start_pad); + buf_append_uint32(buf,their_index); + buf_append_uint32(buf,our_index); + buf_append_uint32(buf,LABEL_NAK); + if (logwhy) + Message(M_INFO,"%s: %08"PRIx32"<-%08"PRIx32": %08"PRIx32":" + " %s; sending NAK\n", + dest->comm->addr_to_string(dest->comm->st,dest), + our_index, their_index, msgtype, logwhy); + dest->comm->sendmsg(dest->comm->st, buf, dest); +} + int consttime_memeq(const void *s1in, const void *s2in, size_t n) { const uint8_t *s1=s1in, *s2=s2in; diff --git a/util.h b/util.h index cbe9f52..816c056 100644 --- a/util.h +++ b/util.h @@ -45,6 +45,10 @@ extern int32_t write_mpbin(MP_INT *a, uint8_t *buffer, int32_t buflen); extern struct log_if *init_log(list_t *loglist); +extern void send_nak(const struct comm_addr *dest, uint32_t our_index, + uint32_t their_index, uint32_t msgtype, + struct buffer_if *buf, const char *logwhy); + extern int consttime_memeq(const void *s1, const void *s2, size_t n); #endif /* util_h */ -- 2.30.2 From 48b97423bbbb3d89b49ba29bd86dfa835ec2b928 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:50 +0100 Subject: [PATCH 08/16] site: Send NAKs for undecryptable data packets (msg0) Packets which are not understood are supposed in general to produce NAKs, to let the peer know that we don't have the key they were using. However, previously this would only happen if the incoming packet had a local site index which was not in use. But it can happen that a particular index value is reused by a recently restarted secnet. Previously, in this case, data packets would simply be thrown away undecryptable. With this change, undecryptable data packets always generate a NAK. (But we don't generate a NAK if the packet was decryptable, but suffered from sequence number skew - if we did, then network glitches would trigger spurious key exchanges.) This is particularly relevant for mobile sites, as it can happen that the fixed site doesn't have an address for the mobile site - so the association will remain stuck in a broken state until the mobile site is also restarted. There is still a potential problem when a site is restarted mid key exchange. The peer will refuse to start a new key exchange (because of the retry timeout) and the restarted site may not know it's necessary. This will be dealt with in a later patch. Signed-off-by: Ian Jackson --- site.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/site.c b/site.c index a6fa9da..8fa4c11 100644 --- a/site.c +++ b/site.c @@ -731,7 +731,8 @@ static bool_t process_msg6(struct site *st, struct buffer_if *msg6, return True; } -static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0) +static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0, + const struct comm_addr *src) { cstring_t transform_err, auxkey_err, newkey_err="n/a"; struct msg0 m; @@ -750,11 +751,8 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0) "peer has used new key","auxiliary key",LOG_SEC); return True; } - - if (problem==2) { - slog(st,LOG_DROP,"transform: %s (merely skew)",transform_err); - return False; - } + if (problem==2) + goto skew; buffer_copy(msg0, &st->scratch); problem = st->auxiliary_key.transform->reverse @@ -778,11 +776,14 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0) } return True; } + if (problem==2) + goto skew; if (st->state==SITE_SENTMSG5) { buffer_copy(msg0, &st->scratch); - if (!st->new_transform->reverse(st->new_transform->st, - msg0,&newkey_err)) { + problem = st->new_transform->reverse(st->new_transform->st, + msg0,&newkey_err); + if (!problem) { /* It looks like we didn't get the peer's MSG6 */ /* This is like a cut-down enter_new_state(SITE_RUN) */ slog(st,LOG_STATE,"will enter state RUN (MSG0 with new key)"); @@ -791,11 +792,18 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0) activate_new_key(st); return True; /* do process the data in this packet */ } + if (problem==2) + goto skew; } slog(st,LOG_SEC,"transform: %s (aux: %s, new: %s)", transform_err,auxkey_err,newkey_err); initiate_key_setup(st,"incoming message would not decrypt"); + send_nak(src,m.dest,m.source,m.type,msg0,"message would not decrypt"); + return False; + + skew: + slog(st,LOG_DROP,"transform: %s (merely skew)",transform_err); return False; } @@ -804,7 +812,7 @@ static bool_t process_msg0(struct site *st, struct buffer_if *msg0, { uint32_t type; - if (!decrypt_msg0(st,msg0)) + if (!decrypt_msg0(st,msg0,src)) return False; CHECK_AVAIL(msg0,4); -- 2.30.2 From 1e80c220a810380ae8b5a155e2bd6937c951c83c Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:50 +0100 Subject: [PATCH 09/16] NOTES: Improve documentation of NAKs. Improve the description of the function of NAK packets. Document an ancient form of NAK, MSG8. Add a missing heading "Other messages" and put the description of the NAK message and of the obsolete NAK in it. Signed-off-by: Ian Jackson --- NOTES | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/NOTES b/NOTES index 84453df..6a245ec 100644 --- a/NOTES +++ b/NOTES @@ -247,32 +247,33 @@ retransmit or confirm reception. It is suggested that this message be sent when a key times out, or the tunnel is forcibly terminated for some reason. -8) i?,i?,NAK (encoded as zero) +**** Protocol sub-goal 3: send a packet -If the link-layer can't work out what to do with a packet (session has -gone away, etc.) it can transmit a NAK back to the sender. The sender -can then try to verify whether the session is alive by sending ping -packets, and forget the key if it isn't. Potential denial-of-service -if the attacker can stop the ping/pong packets getting through (the -key will be forgotten and another key setup must take place), but if -they can delete packets then we've lost anyway... +8) i?,i?,msg0,(send-packet/msg9,packet)_k -The attacker can of course forge NAKs since they aren't protected. But -if they can only forge packets then they won't be able to stop the -ping/pong working. Trust in NAKs can be rate-limited... +Some messages may take a long time to prepare (software modexp on slow +machines); this is a "please wait" message to indicate that a message +is in preparation. -Alternative idea (which is actually implemented): if you receive a -packet you can't decode, because there's no key established, then -initiate key setup... +**** Other messages -Keepalives are probably a good idea. +9) i?,i?,NAK (NAK is encoded as zero) -**** Protocol sub-goal 3: send a packet +If the link-layer can't work out what to do with a packet (session has +gone away, etc.) it can transmit a NAK back to the sender. -9) i?,i?,msg0,(send-packet/msg9,packet)_k +This can alert the sender to the situation where the sender has a key +but the receiver doesn't (eg because it has been restarted). The +sender, on receiving the NAK, will try to initiate a key exchange. -Some messages may take a long time to prepare (software modexp on slow -machines); this is a "please wait" message to indicate that a message -is in preparation. +Forged (or overly delayed) NAKs can cause wasted resources due to +spurious key exchange initiation, but there is a limit on this because +of the key exchange retry timeout. 10) i?,i?,msg8,A,B,nA,nB,msg? + +This is an obsolete form of NAK packet which is not sent by any even +vaguely recent version of secnet. (In fact, there is no evidence in +the git history of it ever being sent.) + +This message number is reserved. -- 2.30.2 From 5c21ea51197108524fae54c3fdd6c8048816c08c Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:51 +0100 Subject: [PATCH 10/16] NOTES: Remove paragraph about slow-to-prepare messages NOTES contained this paragraph: Some messages may take a long time to prepare (software modexp on slow machines); this is a "please wait" message to indicate that a message is in preparation. This paragraph was immediately after the description of the msg0(msg9) packet - ie, the ordinary data packet. It is therefore at the very least misplaced. In the git history this paragraph was introduced in "Import release 0.1.14" (4f5e39ec). However, the diff for that commit (ie the diff between 0.1.13 and 0.1.4) does not show any code which might correspond to this comment. There is not currently any code in site.c which responds to a message of this kind. I have had a quick look for code which sends such a message and failed to find any. So I think this paragraph represents a never-implemented intent, and should be removed. It certainly predates the "hacky parallelism" by a long way. Signed-off-by: Ian Jackson --- NOTES | 4 ---- 1 file changed, 4 deletions(-) diff --git a/NOTES b/NOTES index 6a245ec..09c083e 100644 --- a/NOTES +++ b/NOTES @@ -251,10 +251,6 @@ some reason. 8) i?,i?,msg0,(send-packet/msg9,packet)_k -Some messages may take a long time to prepare (software modexp on slow -machines); this is a "please wait" message to indicate that a message -is in preparation. - **** Other messages 9) i?,i?,NAK (NAK is encoded as zero) -- 2.30.2 From ddc1d9e0ad064de11d8bd735f7c86ab56555330e Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:51 +0100 Subject: [PATCH 11/16] NOTES: Remove unimplemented protocol negotiation The protocol negotiation mechanism documented in NOTES is not implemented. Remove it from the document. Signed-off-by: Ian Jackson --- NOTES | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/NOTES b/NOTES index 09c083e..33c010e 100644 --- a/NOTES +++ b/NOTES @@ -193,21 +193,18 @@ i? is appropriate index for receiver Note that 'i' may be re-used from one session to the next, whereas 'n' is always fresh. -The protocol version selection stuff is not yet implemented: I'm not -yet convinced it's a good idea. Instead, the initiator could try -using its preferred protocol (which starts with a different magic -number) and fall back if there's no reply. +The protocol version selection stuff is not yet implemented. Messages: -1) A->B: *,iA,msg1,A,B,protorange-A,nA +1) A->B: *,iA,msg1,A,B,nA -2) B->A: iA,iB,msg2,B,A,chosen-protocol,nB,nA +2) B->A: iA,iB,msg2,B,A,nB,nA (The order of B and A reverses in alternate messages so that the same code can be used to construct them...) -3) A->B: {iB,iA,msg3,A,B,protorange-A,chosen-protocol,nA,nB,g^x mod m}_PK_A^-1 +3) A->B: {iB,iA,msg3,A,B,nA,nB,g^x mod m}_PK_A^-1 If message 1 was a replay then A will not generate message 3, because it doesn't recognise nA. @@ -215,18 +212,11 @@ it doesn't recognise nA. If message 2 was from an attacker then B will not generate message 4, because it doesn't recognise nB. -If an attacker is trying to manipulate the chosen protocol, B can spot -this when it sees A's message 3. - -4) B->A: {iA,iB,msg4,B,A,protorange-B,chosen-protocol,nB,nA,g^y mod m}_PK_B^-1 +4) B->A: {iA,iB,msg4,B,A,nB,nA,g^y mod m}_PK_B^-1 At this point, A and B share a key, k. B must keep retransmitting message 4 until it receives a packet encrypted using key k. -A can abandon the exchange if the chosen protocol is not the one that -it would have chosen knowing the acceptable protocol ranges of A and -B. - 5) A: iB,iA,msg5,(ping/msg5)_k 6) B: iA,iB,msg6,(pong/msg6)_k -- 2.30.2 From 1737eeef9bc4aec1b4d7baa220ce48238b498006 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:51 +0100 Subject: [PATCH 12/16] site: fix site name checking leaving room for expansion Previously, secnet would only check that the site names sent by the peer had the expected site names as prefixes. This would be a security bug on a vpn where one site name is the prefix of another site name. In fact, if the peer sends a short name, secnet will suffer a buffer read overrun and maybe crash - although this is exploitable only for DoS at worst, and not as an information leak. (With reasonable peers, it won't cause packets to be mishandled, because the next field after each name string is a 16-byte integer giving a name or public key byte count, which would have to contain a value greater than 8000 to look like printable characters.) Fix this bug. However, we use this as an opportunity to provide some space (in the signed part of MSG2..4) for future extensions. (At present there is nothing in the parts of MSG3 and MSG4 covered by the signature which is not supposedly entirely fixed - apart from the amount of zero-padding at the MS end of our DH public exponent. This is bad because it provides no way to do transport protocol upgrade/downgrade negotiation without leaving open a protocol version downgrade attack.) After this change the name fields in MSG2..4 are, following the 16-bit length, either just the characters of the name, or the name, followed by a nul byte, followed by zero or more bytes of additional data. Additional data beyond what the receiver expects (which currently means any additional data), is to be ignored. Let us work through some examples. Suppose the expected site name is "expected". Here are the behaviours in relation to various names being transmitted in the packet. (This applies both to the local name, and to the remote name; "rejected" means other instances of the protocol engine get a go, and perhaps the instance with the correct names will accept the message.) MSG Sent name Old secnet New secnet 1 expected ok ok 1 expected\0extra rejected rejected 1 expected/suffix rejected rejected 1 expect rejected rejected 2-4 expected ok ok 2-4 expected\0extra ok, extra ignored ok, extra data 2-4 expected/suffix wrongly accepted rejected 2-4 expect read overrun rejected We will do the same for MSG1 in a later commit. Signed-off-by: Ian Jackson --- NOTES | 11 ++++++----- site.c | 53 +++++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 45 insertions(+), 19 deletions(-) diff --git a/NOTES b/NOTES index 33c010e..ddd14a5 100644 --- a/NOTES +++ b/NOTES @@ -174,8 +174,9 @@ quite stable so the feature doesn't gain us much. Definitions: -A is the originating gateway machine -B is the destination gateway machine +A is the originating gateway machine name +B is the destination gateway machine name +A+ and B+ are the names with optional additional data, currently ignored PK_A is the public RSA key of A PK_B is the public RSA key of B PK_A^-1 is the private RSA key of A @@ -199,12 +200,12 @@ Messages: 1) A->B: *,iA,msg1,A,B,nA -2) B->A: iA,iB,msg2,B,A,nB,nA +2) B->A: iA,iB,msg2,B+,A+,nB,nA (The order of B and A reverses in alternate messages so that the same code can be used to construct them...) -3) A->B: {iB,iA,msg3,A,B,nA,nB,g^x mod m}_PK_A^-1 +3) A->B: {iB,iA,msg3,A+,B+,nA,nB,g^x mod m}_PK_A^-1 If message 1 was a replay then A will not generate message 3, because it doesn't recognise nA. @@ -212,7 +213,7 @@ it doesn't recognise nA. If message 2 was from an attacker then B will not generate message 4, because it doesn't recognise nB. -4) B->A: {iA,iB,msg4,B,A,nB,nA,g^y mod m}_PK_B^-1 +4) B->A: {iA,iB,msg4,B+,A+,nB,nA,g^y mod m}_PK_B^-1 At this point, A and B share a key, k. B must keep retransmitting message 4 until it receives a packet encrypted using key k. diff --git a/site.c b/site.c index 8fa4c11..2e8335a 100644 --- a/site.c +++ b/site.c @@ -347,14 +347,19 @@ static bool_t current_valid(struct site *st) type=buf_unprepend_uint32((b)); \ if (type!=(t)) return False; } while(0) +struct parsedname { + int32_t len; + uint8_t *name; + int32_t extrainfo_len; + uint8_t *extrainfo; +}; + struct msg { uint8_t *hashstart; uint32_t dest; uint32_t source; - int32_t remlen; - uint8_t *remote; - int32_t loclen; - uint8_t *local; + struct parsedname remote; + struct parsedname local; uint8_t *nR; uint8_t *nL; int32_t pklen; @@ -402,6 +407,24 @@ static bool_t generate_msg(struct site *st, uint32_t type, cstring_t what) return True; } +static bool_t unpick_name(struct buffer_if *msg, struct parsedname *nm) +{ + CHECK_AVAIL(msg,2); + nm->len=buf_unprepend_uint16(msg); + CHECK_AVAIL(msg,nm->len); + nm->name=buf_unprepend(msg,nm->len); + uint8_t *nul=memchr(nm->name,0,nm->len); + if (!nul) { + nm->extrainfo_len=0; + nm->extrainfo=0; + } else { + nm->extrainfo=nul+1; + nm->extrainfo_len=msg->start-nm->extrainfo; + nm->len=nul-nm->name; + } + return True; +} + static bool_t unpick_msg(struct site *st, uint32_t type, struct buffer_if *msg, struct msg *m) { @@ -411,14 +434,8 @@ static bool_t unpick_msg(struct site *st, uint32_t type, CHECK_AVAIL(msg,4); m->source=buf_unprepend_uint32(msg); CHECK_TYPE(msg,type); - CHECK_AVAIL(msg,2); - m->remlen=buf_unprepend_uint16(msg); - CHECK_AVAIL(msg,m->remlen); - m->remote=buf_unprepend(msg,m->remlen); - CHECK_AVAIL(msg,2); - m->loclen=buf_unprepend_uint16(msg); - CHECK_AVAIL(msg,m->loclen); - m->local=buf_unprepend(msg,m->loclen); + if (!unpick_name(msg,&m->remote)) return False; + if (!unpick_name(msg,&m->local)) return False; CHECK_AVAIL(msg,NONCELEN); m->nR=buf_unprepend(msg,NONCELEN); if (type==LABEL_MSG1) { @@ -444,6 +461,14 @@ static bool_t unpick_msg(struct site *st, uint32_t type, return True; } +static bool_t name_matches(const struct parsedname *nm, const char *expected) +{ + int expected_len=strlen(expected); + return + nm->len == expected_len && + !memcmp(nm->name, expected, expected_len); +} + static bool_t check_msg(struct site *st, uint32_t type, struct msg *m, cstring_t *error) { @@ -451,11 +476,11 @@ static bool_t check_msg(struct site *st, uint32_t type, struct msg *m, /* Check that the site names and our nonce have been sent back correctly, and then store our peer's nonce. */ - if (memcmp(m->remote,st->remotename,strlen(st->remotename)!=0)) { + if (!name_matches(&m->remote,st->remotename)) { *error="wrong remote site name"; return False; } - if (memcmp(m->local,st->localname,strlen(st->localname)!=0)) { + if (!name_matches(&m->local,st->localname)) { *error="wrong local site name"; return False; } -- 2.30.2 From 7e29719ec13da8db08c7aa4753d958141967bf0a Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:51 +0100 Subject: [PATCH 13/16] site: Extra info in name fields for MSG1, clearer processing We change the parsing of the names in MSG1 packets to align it with that used for MSG2..4, abolishing the simple memcmp-based "setupsig" arrangement. This means that MSG1 packets, like MSG2..4, can contain extra information in the name fields. This results in the following table of behaviours (where "old secnet" is the version before we introduced additional data at all): MSG Sent name Old secnet New secnet 1 expected ok ok 1 expected\0extra rejected ok, extra data 1 expected/suffix rejected rejected 1 expect rejected rejected 2-4 expected ok ok 2-4 expected\0extra ok, extra ignored ok, extra data 2-4 expected/suffix wrongly accepted rejected 2-4 expect read overrun rejected After the new secnet is widely deployed, we will be able to use the extra data field in MSG1 for public key algorithm agility and public key rollover. Also, intend to introduce a new packet which (like MSG1) is addressed by name rather than the site id; this change makes that easier too. In detail, we: * Make site_incoming extract the message type earlier, and use the message type to choose how to check the address. * Abolish st->setupsig and st->setupsiglen; * Break out a function named_for_us, which uses unpick_msg and then checks the names; * Arrange to keep the unpicked message from named_for_us and reuse it in process_msg1. * Eliminate checking dest==0 for name-addressed packets. This last point constitutes a change to the implemented protocol. The old code would treat a packet as name-addressed iff the destination "index" was zero. However, the requirement to send a zero in this field is not documented. After this patch, peers which send MSG1 with a non-zero destination index will have their messages honoured rather than discarded. But we do document the restriction. There is no other significant functional implication of this last point. Peers which send non-MSG1s with zero destination index will have their packets rejected both before and afterwards (although there may be a change in whether a NAK is sent and in logging) but anyway those peers are broken because all of our generated indexes are nonzero. A downside of this change is that we will now unpick an incoming MSG1 repeatedly until we find which site instance wants it. But this unpicking is not particularly arduous. Signed-off-by: Ian Jackson --- NOTES | 5 ++- site.c | 116 +++++++++++++++++++++++++++------------------------------ 2 files changed, 59 insertions(+), 62 deletions(-) diff --git a/NOTES b/NOTES index ddd14a5..3e9d71d 100644 --- a/NOTES +++ b/NOTES @@ -198,7 +198,10 @@ The protocol version selection stuff is not yet implemented. Messages: -1) A->B: *,iA,msg1,A,B,nA +1) A->B: *,iA,msg1,A+,B+,nA + +i* must be encoded as 0. (However, it is permitted for a site to use +zero as its "index" for another site.) 2) B->A: iA,iB,msg2,B+,A+,nB,nA diff --git a/site.c b/site.c index 2e8335a..98bd6b6 100644 --- a/site.c +++ b/site.c @@ -253,9 +253,6 @@ struct site { after this time, initiate a new key exchange */ - uint8_t *setupsig; /* Expected signature of incoming MSG1 packets */ - int32_t setupsiglen; /* Allows us to discard packets quickly if - they are not for us */ bool_t setup_priority; /* Do we have precedence if both sites emit message 1 simultaneously? */ uint32_t log_events; @@ -506,19 +503,15 @@ static bool_t generate_msg1(struct site *st) } static bool_t process_msg1(struct site *st, struct buffer_if *msg1, - const struct comm_addr *src) + const struct comm_addr *src, struct msg *m) { - struct msg m; - /* We've already determined we're in an appropriate state to process an incoming MSG1, and that the MSG1 has correct values of A and B. */ - if (!unpick_msg(st,LABEL_MSG1,msg1,&m)) return False; - transport_record_peer(st,&st->setup_peers,src,"msg1"); - st->setup_session_id=m.source; - memcpy(st->remoteN,m.nR,NONCELEN); + st->setup_session_id=m->source; + memcpy(st->remoteN,m->nR,NONCELEN); return True; } @@ -1254,6 +1247,18 @@ static void site_outgoing(void *sst, struct buffer_if *buf) initiate_key_setup(st,"outgoing packet"); } +static bool_t named_for_us(struct site *st, const struct buffer_if *buf_in, + uint32_t type, struct msg *m) + /* For packets which are identified by the local and remote names. + * If it has our name and our peer's name in it it's for us. */ +{ + struct buffer_if buf[1]; + buffer_readonly_clone(buf,buf_in); + return unpick_msg(st,type,buf,m) + && name_matches(&m->remote,st->remotename) + && name_matches(&m->local,st->localname); +} + /* This function is called by the communication device to deliver packets from our peers. */ static bool_t site_incoming(void *sst, struct buffer_if *buf, @@ -1263,60 +1268,57 @@ static bool_t site_incoming(void *sst, struct buffer_if *buf, if (buf->size < 12) return False; + uint32_t msgtype=ntohl(get_uint32(buf->start+8)); uint32_t dest=ntohl(*(uint32_t *)buf->start); - - if (dest==0) { - /* It could be for any site - it should have LABEL_MSG1 and - might have our name and our peer's name in it */ - if (buf->size<(st->setupsiglen+8+NONCELEN)) return False; - if (memcmp(buf->start+8,st->setupsig,st->setupsiglen)==0) { - /* It's addressed to us. Decide what to do about it. */ - dump_packet(st,buf,source,True); - if (st->state==SITE_RUN || st->state==SITE_RESOLVE || - st->state==SITE_WAIT) { - /* We should definitely process it */ - if (process_msg1(st,buf,source)) { - slog(st,LOG_SETUP_INIT,"key setup initiated by peer"); + struct msg named_msg; + + if (msgtype==LABEL_MSG1) { + if (!named_for_us(st,buf,msgtype,&named_msg)) + return False; + /* It's a MSG1 addressed to us. Decide what to do about it. */ + dump_packet(st,buf,source,True); + if (st->state==SITE_RUN || st->state==SITE_RESOLVE || + st->state==SITE_WAIT) { + /* We should definitely process it */ + if (process_msg1(st,buf,source,&named_msg)) { + slog(st,LOG_SETUP_INIT,"key setup initiated by peer"); + enter_new_state(st,SITE_SENTMSG2); + } else { + slog(st,LOG_ERROR,"failed to process incoming msg1"); + } + BUF_FREE(buf); + return True; + } else if (st->state==SITE_SENTMSG1) { + /* We've just sent a message 1! They may have crossed on + the wire. If we have priority then we ignore the + incoming one, otherwise we process it as usual. */ + if (st->setup_priority) { + BUF_FREE(buf); + slog(st,LOG_DUMP,"crossed msg1s; we are higher " + "priority => ignore incoming msg1"); + return True; + } else { + slog(st,LOG_DUMP,"crossed msg1s; we are lower " + "priority => use incoming msg1"); + if (process_msg1(st,buf,source,&named_msg)) { + BUF_FREE(&st->buffer); /* Free our old message 1 */ enter_new_state(st,SITE_SENTMSG2); } else { - slog(st,LOG_ERROR,"failed to process incoming msg1"); + slog(st,LOG_ERROR,"failed to process an incoming " + "crossed msg1 (we have low priority)"); } BUF_FREE(buf); return True; - } else if (st->state==SITE_SENTMSG1) { - /* We've just sent a message 1! They may have crossed on - the wire. If we have priority then we ignore the - incoming one, otherwise we process it as usual. */ - if (st->setup_priority) { - BUF_FREE(buf); - slog(st,LOG_DUMP,"crossed msg1s; we are higher " - "priority => ignore incoming msg1"); - return True; - } else { - slog(st,LOG_DUMP,"crossed msg1s; we are lower " - "priority => use incoming msg1"); - if (process_msg1(st,buf,source)) { - BUF_FREE(&st->buffer); /* Free our old message 1 */ - enter_new_state(st,SITE_SENTMSG2); - } else { - slog(st,LOG_ERROR,"failed to process an incoming " - "crossed msg1 (we have low priority)"); - } - BUF_FREE(buf); - return True; - } } - /* The message 1 was received at an unexpected stage of the - key setup. XXX POLICY - what do we do? */ - slog(st,LOG_UNEXPECTED,"unexpected incoming message 1"); - BUF_FREE(buf); - return True; } - return False; /* Not for us. */ + /* The message 1 was received at an unexpected stage of the + key setup. XXX POLICY - what do we do? */ + slog(st,LOG_UNEXPECTED,"unexpected incoming message 1"); + BUF_FREE(buf); + return True; } if (dest==st->index) { /* Explicitly addressed to us */ - uint32_t msgtype=ntohl(get_uint32(buf->start+8)); if (msgtype!=LABEL_MSG0) dump_packet(st,buf,source,True); switch (msgtype) { case LABEL_NAK: @@ -1565,14 +1567,6 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context, /* The information we expect to see in incoming messages of type 1 */ /* fixme: lots of unchecked overflows here, but the results are only corrupted packets rather than undefined behaviour */ - st->setupsiglen=strlen(st->remotename)+strlen(st->localname)+8; - st->setupsig=safe_malloc(st->setupsiglen,"site_apply"); - put_uint32(st->setupsig+0,LABEL_MSG1); - put_uint16(st->setupsig+4,strlen(st->remotename)); - memcpy(&st->setupsig[6],st->remotename,strlen(st->remotename)); - put_uint16(st->setupsig+(6+strlen(st->remotename)),strlen(st->localname)); - memcpy(&st->setupsig[8+strlen(st->remotename)],st->localname, - strlen(st->localname)); st->setup_priority=(strcmp(st->localname,st->remotename)>0); buffer_new(&st->buffer,SETUP_BUFFER_LEN); -- 2.30.2 From 0a6cbadea08d824e26838a18bb75745c78f27461 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:51 +0100 Subject: [PATCH 14/16] site: use unaligned.h's functions, not pointer cast and ntohl Switch site.c to using unaligned.h's functions for accessing multi-byte values inside messages. There were a few places where this construction was used: something = ntohl(*(uint32_t*)(buf->start + offset)); It is much clearer to use this equivalent construction: something = get_uint32(buf->start + offset); Also the packet's message type was extracted from the message using get_uint32 and then put through ntohl. This is, of course, wrong. Currently all the message type codes are palindromes, so it doesn't matter in practice. Fix it anyway. Signed-off-by: Ian Jackson --- site.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/site.c b/site.c index 98bd6b6..5b071b2 100644 --- a/site.c +++ b/site.c @@ -859,9 +859,9 @@ static bool_t process_msg0(struct site *st, struct buffer_if *msg0, static void dump_packet(struct site *st, struct buffer_if *buf, const struct comm_addr *addr, bool_t incoming) { - uint32_t dest=ntohl(*(uint32_t *)buf->start); - uint32_t source=ntohl(*(uint32_t *)(buf->start+4)); - uint32_t msgtype=ntohl(*(uint32_t *)(buf->start+8)); + uint32_t dest=get_uint32(buf->start); + uint32_t source=get_uint32(buf->start+4); + uint32_t msgtype=get_uint32(buf->start+8); if (st->log_events & LOG_DUMP) slilog(st->log,M_DEBUG,"%s: %s: %08x<-%08x: %08x:", @@ -1268,8 +1268,8 @@ static bool_t site_incoming(void *sst, struct buffer_if *buf, if (buf->size < 12) return False; - uint32_t msgtype=ntohl(get_uint32(buf->start+8)); - uint32_t dest=ntohl(*(uint32_t *)buf->start); + uint32_t dest=get_uint32(buf->start); + uint32_t msgtype=get_uint32(buf->start+8); struct msg named_msg; if (msgtype==LABEL_MSG1) { -- 2.30.2 From 09a385fbbb18ace5daedd473c91c5b584dab3b2a Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:52 +0100 Subject: [PATCH 15/16] site: interpret first 4 bytes of extrainfo as capabilities Define the first four bytes of the additional data (after the nul in site names in MSG1..4) in the sender's name field to be a capability bitmask. Currently no capability flags are defined. To support this, replace extrainfo and its _len in struct parsedname with a struct buffer_if. We record the capabilities from MSG1 or MSG3, and check that they haven't changed when processing MSG2..4, as applicable. (Because older secnets don't cope with flags in MSG1, we have to define two kinds of capability flag: ones expected to be in MSG1, and ones which can appear later in the exchange.) The support for "Early" capabilities is intended to be used in the future to support algorithm agility in key exchange. We also advertise our own capabilities (currently, all-bits-zero) in our own messages. Signed-off-by: Ian Jackson --- NOTES | 28 ++++++++++++++++++++-- magic.h | 4 ++++ site.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++------ 3 files changed, 96 insertions(+), 9 deletions(-) diff --git a/NOTES b/NOTES index 3e9d71d..8619ee5 100644 --- a/NOTES +++ b/NOTES @@ -176,7 +176,7 @@ Definitions: A is the originating gateway machine name B is the destination gateway machine name -A+ and B+ are the names with optional additional data, currently ignored +A+ and B+ are the names with optional additional data, see below PK_A is the public RSA key of A PK_B is the public RSA key of B PK_A^-1 is the private RSA key of A @@ -194,7 +194,31 @@ i? is appropriate index for receiver Note that 'i' may be re-used from one session to the next, whereas 'n' is always fresh. -The protocol version selection stuff is not yet implemented. +The optional additional data after the sender's name consists of some +initial subset of the following list of items: + * A 32-bit integer with a set of capability flags, representing the + abilities of the sender. + * More data which is yet to be defined and which must be ignored + by receivers. +The optional additional data after the receiver's name is not +currently used. If any is seen, it must be ignored. + +Capability flag bits must be in one the following two categories: + +1. Early capability flags must be advertised in MSG1 or MSG2, as + applicable. If MSG3 or MSG4 advertise any "early" capability bits, + MSG1 or MSG3 (as applicable) must have advertised them too. Sadly, + advertising an early capability flag will produce MSG1s which are + not understood by versions of secnet which predate the capability + mechanism. + +2. Late capability flags are advertised in MSG2 or MSG3, as + applicable. They may also appear in MSG1, but this is not + guaranteed. MSG4 must advertise the same set as MSG2. + +No capability flags are currently defined. Unknown capability flags +should be treated as late ones. + Messages: diff --git a/magic.h b/magic.h index c2503a0..3ae7af4 100644 --- a/magic.h +++ b/magic.h @@ -15,4 +15,8 @@ #define LABEL_MSG8 0x08080808 #define LABEL_MSG9 0x09090909 +/* uses of the 32-bit capability bitmap */ +/* no flags currently defined */ +#define CAPAB_EARLY 0x00000000 /* no Early flags defined (see NOTES) */ + #endif /* magic_h */ diff --git a/site.c b/site.c index 5b071b2..0a02785 100644 --- a/site.c +++ b/site.c @@ -244,6 +244,7 @@ struct site { struct hash_if *hash; uint32_t index; /* Index of this site */ + uint32_t local_capabilities; int32_t setup_retries; /* How many times to send setup packets */ int32_t setup_retry_interval; /* Initial timeout for setup packets */ int32_t wait_timeout; /* How long to wait if setup unsuccessful */ @@ -275,6 +276,7 @@ struct site { packet; we keep trying to continue the exchange, and have to timeout before we can listen for another setup packet); perhaps we should keep a list of 'bad' sources for setup packets. */ + uint32_t remote_capabilities; uint32_t setup_session_id; transport_peers setup_peers; uint8_t localN[NONCELEN]; /* Nonces for key exchange */ @@ -347,8 +349,7 @@ static bool_t current_valid(struct site *st) struct parsedname { int32_t len; uint8_t *name; - int32_t extrainfo_len; - uint8_t *extrainfo; + struct buffer_if extrainfo; }; struct msg { @@ -357,6 +358,7 @@ struct msg { uint32_t source; struct parsedname remote; struct parsedname local; + uint32_t remote_capabilities; uint8_t *nR; uint8_t *nL; int32_t pklen; @@ -366,6 +368,34 @@ struct msg { char *sig; }; +struct xinfoadd { + int32_t lenpos, afternul; +}; +static void append_string_xinfo_start(struct buffer_if *buf, + struct xinfoadd *xia, + const char *str) + /* Helps construct one of the names with additional info as found + * in MSG1..4. Call this function first, then append all the + * desired extra info (not including the nul byte) to the buffer, + * then call append_string_xinfo_done. */ +{ + xia->lenpos = buf->size; + buf_append_string(buf,str); + buf_append_uint8(buf,0); + xia->afternul = buf->size; +} +static void append_string_xinfo_done(struct buffer_if *buf, + struct xinfoadd *xia) +{ + /* we just need to adjust the string length */ + if (buf->size == xia->afternul) { + /* no extra info, strip the nul too */ + buf_unappend_uint8(buf); + } else { + put_uint16(buf->start+xia->lenpos, buf->size-(xia->lenpos+2)); + } +} + /* Build any of msg1 to msg4. msg5 and msg6 are built from the inside out using a transform of config data supplied by netlink */ static bool_t generate_msg(struct site *st, uint32_t type, cstring_t what) @@ -381,7 +411,14 @@ static bool_t generate_msg(struct site *st, uint32_t type, cstring_t what) (type==LABEL_MSG1?0:st->setup_session_id)); buf_append_uint32(&st->buffer,st->index); buf_append_uint32(&st->buffer,type); - buf_append_string(&st->buffer,st->localname); + + struct xinfoadd xia; + append_string_xinfo_start(&st->buffer,&xia,st->localname); + if ((st->local_capabilities & CAPAB_EARLY) || (type != LABEL_MSG1)) { + buf_append_uint32(&st->buffer,st->local_capabilities); + } + append_string_xinfo_done(&st->buffer,&xia); + buf_append_string(&st->buffer,st->remotename); memcpy(buf_append(&st->buffer,NONCELEN),st->localN,NONCELEN); if (type==LABEL_MSG1) return True; @@ -412,11 +449,9 @@ static bool_t unpick_name(struct buffer_if *msg, struct parsedname *nm) nm->name=buf_unprepend(msg,nm->len); uint8_t *nul=memchr(nm->name,0,nm->len); if (!nul) { - nm->extrainfo_len=0; - nm->extrainfo=0; + buffer_readonly_view(&nm->extrainfo,0,0); } else { - nm->extrainfo=nul+1; - nm->extrainfo_len=msg->start-nm->extrainfo; + buffer_readonly_view(&nm->extrainfo, nul+1, msg->start-(nul+1)); nm->len=nul-nm->name; } return True; @@ -432,6 +467,11 @@ static bool_t unpick_msg(struct site *st, uint32_t type, m->source=buf_unprepend_uint32(msg); CHECK_TYPE(msg,type); if (!unpick_name(msg,&m->remote)) return False; + m->remote_capabilities=0; + if (m->remote.extrainfo.size) { + CHECK_AVAIL(&m->remote.extrainfo,4); + m->remote_capabilities=buf_unprepend_uint32(&m->remote.extrainfo); + } if (!unpick_name(msg,&m->local)) return False; CHECK_AVAIL(msg,NONCELEN); m->nR=buf_unprepend(msg,NONCELEN); @@ -490,7 +530,13 @@ static bool_t check_msg(struct site *st, uint32_t type, struct msg *m, *error="wrong remotely-generated nonce"; return False; } + /* MSG3 has complicated rules about capabilities, which are + * handled in process_msg3. */ if (type==LABEL_MSG3) return True; + if (m->remote_capabilities!=st->remote_capabilities) { + *error="remote capabilities changed"; + return False; + } if (type==LABEL_MSG4) return True; *error="unknown message type"; return False; @@ -511,6 +557,7 @@ static bool_t process_msg1(struct site *st, struct buffer_if *msg1, transport_record_peer(st,&st->setup_peers,src,"msg1"); st->setup_session_id=m->source; + st->remote_capabilities=m->remote_capabilities; memcpy(st->remoteN,m->nR,NONCELEN); return True; } @@ -533,6 +580,7 @@ static bool_t process_msg2(struct site *st, struct buffer_if *msg2, return False; } st->setup_session_id=m.source; + st->remote_capabilities=m.remote_capabilities; memcpy(st->remoteN,m.nR,NONCELEN); return True; } @@ -558,6 +606,15 @@ static bool_t process_msg3(struct site *st, struct buffer_if *msg3, slog(st,LOG_SEC,"msg3: %s",err); return False; } + uint32_t capab_adv_late = m.remote_capabilities + & ~st->remote_capabilities & CAPAB_EARLY; + if (capab_adv_late) { + slog(st,LOG_SEC,"msg3 impermissibly adds early capability flag(s)" + " %#"PRIx32" (was %#"PRIx32", now %#"PRIx32")", + capab_adv_late, st->remote_capabilities, m.remote_capabilities); + return False; + } + st->remote_capabilities|=m.remote_capabilities; /* Check signature and store g^x mod m */ hash=safe_malloc(st->hash->len, "process_msg3"); @@ -1493,6 +1550,7 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context, assert(index_sequence < 0xffffffffUL); st->index = ++index_sequence; + st->local_capabilities = 0; st->netlink=find_cl_if(dict,"link",CL_NETLINK,True,"site",loc); list_t *comms_cfg=dict_lookup(dict,"comm"); @@ -1579,6 +1637,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->remote_capabilities=0; st->current.key_timeout=0; st->auxiliary_key.key_timeout=0; transport_peers_clear(st,&st->peers); -- 2.30.2 From 0afd257e3beecf259a24a315d370b6d43db9fb44 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:52 +0100 Subject: [PATCH 16/16] site: Check transform errors; factor out transform handling Make sure we always check the error return from transform->forwards and ->backwards. Otherwise logic errors in the site state machine might result in us sending out packets with unencrypted insider plaintext, or the like, due to the transform being unkeyed when we try to use it. Factor some repeated idioms for transform handling into a set of new functions. This will make the next patch much easier. We arrange to pass dispose_transform a pointer to the actual state variable where the transform is kept; this means that it can be changed to update that pointer. Signed-off-by: Ian Jackson --- site.c | 90 ++++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 63 insertions(+), 27 deletions(-) diff --git a/site.c b/site.c index 0a02785..9813de0 100644 --- a/site.c +++ b/site.c @@ -334,11 +334,35 @@ 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 is_transform_valid(struct transform_inst_if *transform) +{ + return transform->valid(transform->st); +} + static bool_t current_valid(struct site *st) { - return st->current.transform->valid(st->current.transform->st); + return is_transform_valid(st->current.transform); +} + +#define DEFINE_CALL_TRANSFORM(fwdrev) \ +static int call_transform_##fwdrev(struct site *st, \ + struct transform_inst_if *transform, \ + struct buffer_if *buf, \ + const char **errmsg) \ +{ \ + return transform->fwdrev(transform->st,buf,errmsg); \ } +DEFINE_CALL_TRANSFORM(forwards) +DEFINE_CALL_TRANSFORM(reverse) + +static void dispose_transform(struct transform_inst_if **transform_var) +{ + /* will become more sophisticated very shortly */ + struct transform_inst_if *transform=*transform_var; + transform->delkey(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; \ @@ -368,6 +392,12 @@ struct msg { char *sig; }; +static void set_new_transform(struct site *st) +{ + st->new_transform->setkey(st->new_transform->st,st->sharedsecret, + st->sharedsecretlen,st->setup_priority); +} + struct xinfoadd { int32_t lenpos, afternul; }; @@ -640,8 +670,7 @@ static bool_t process_msg3(struct site *st, struct buffer_if *msg3, st->sharedsecret,st->sharedsecretlen); /* Set up the transform */ - st->new_transform->setkey(st->new_transform->st,st->sharedsecret, - st->sharedsecretlen,st->setup_priority); + set_new_transform(st); return True; } @@ -687,8 +716,7 @@ static bool_t process_msg4(struct site *st, struct buffer_if *msg4, st->dh->makeshared(st->dh->st,st->dhsecret,st->dh->len,m.pk, st->sharedsecret,st->sharedsecretlen); /* Set up the transform */ - st->new_transform->setkey(st->new_transform->st,st->sharedsecret, - st->sharedsecretlen,st->setup_priority); + set_new_transform(st); return True; } @@ -722,8 +750,9 @@ static bool_t generate_msg5(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_MSG5); - st->new_transform->forwards(st->new_transform->st,&st->buffer, - &transform_err); + if (call_transform_forwards(st,st->new_transform, + &st->buffer,&transform_err)) + return False; buf_prepend_uint32(&st->buffer,LABEL_MSG5); buf_prepend_uint32(&st->buffer,st->index); buf_prepend_uint32(&st->buffer,st->setup_session_id); @@ -741,7 +770,7 @@ static bool_t process_msg5(struct site *st, struct buffer_if *msg5, if (!unpick_msg0(st,msg5,&m)) return False; - if (transform->reverse(transform->st,msg5,&transform_err)) { + if (call_transform_reverse(st,transform,msg5,&transform_err)) { /* There's a problem */ slog(st,LOG_SEC,"process_msg5: transform: %s",transform_err); return False; @@ -768,7 +797,9 @@ static void create_msg6(struct site *st, struct transform_inst_if *transform, /* Give the netlink code an opportunity to put its own stuff in the message (configuration information, etc.) */ buf_prepend_uint32(&st->buffer,LABEL_MSG6); - transform->forwards(transform->st,&st->buffer,&transform_err); + int problem = call_transform_forwards(st,transform, + &st->buffer,&transform_err); + assert(!problem); buf_prepend_uint32(&st->buffer,LABEL_MSG6); buf_prepend_uint32(&st->buffer,st->index); buf_prepend_uint32(&st->buffer,session_id); @@ -776,6 +807,8 @@ static void create_msg6(struct site *st, struct transform_inst_if *transform, static bool_t generate_msg6(struct site *st) { + if (!is_transform_valid(st->new_transform)) + return False; create_msg6(st,st->new_transform,st->setup_session_id); st->retries=1; /* Peer will retransmit MSG5 if this packet gets lost */ return True; @@ -789,8 +822,7 @@ static bool_t process_msg6(struct site *st, struct buffer_if *msg6, if (!unpick_msg0(st,msg6,&m)) return False; - if (st->new_transform->reverse(st->new_transform->st, - msg6,&transform_err)) { + if (call_transform_reverse(st,st->new_transform,msg6,&transform_err)) { /* There's a problem */ slog(st,LOG_SEC,"process_msg6: transform: %s",transform_err); return False; @@ -818,8 +850,8 @@ 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, - msg0,&transform_err); + problem = call_transform_reverse(st,st->current.transform, + msg0,&transform_err); if (!problem) { if (!st->auxiliary_is_new) delete_one_key(st,&st->auxiliary_key, @@ -830,8 +862,8 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0, goto skew; buffer_copy(msg0, &st->scratch); - problem = st->auxiliary_key.transform->reverse - (st->auxiliary_key.transform->st,msg0,&auxkey_err); + problem = call_transform_reverse + (st,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) { @@ -856,8 +888,8 @@ static bool_t decrypt_msg0(struct site *st, struct buffer_if *msg0, if (st->state==SITE_SENTMSG5) { buffer_copy(msg0, &st->scratch); - problem = st->new_transform->reverse(st->new_transform->st, - msg0,&newkey_err); + problem = call_transform_reverse(st,st->new_transform, + msg0,&newkey_err); if (!problem) { /* It looks like we didn't get the peer's MSG6 */ /* This is like a cut-down enter_new_state(SITE_RUN) */ @@ -947,8 +979,8 @@ static bool_t send_msg(struct site *st) t=st->auxiliary_key.transform; st->auxiliary_key.transform=st->new_transform; st->new_transform=t; + dispose_transform(&st->new_transform); - 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; @@ -1017,8 +1049,8 @@ static void activate_new_key(struct site *st) st->auxiliary_key.transform=st->current.transform; st->current.transform=st->new_transform; st->new_transform=t; + dispose_transform(&st->new_transform); - t->delkey(t->st); st->timeout=0; st->auxiliary_is_new=0; st->auxiliary_key.key_timeout=st->current.key_timeout; @@ -1034,9 +1066,9 @@ static void activate_new_key(struct site *st) 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 (!is_transform_valid(key->transform)) return; if (reason) slog(st,loglevel,"%s deleted (%s)",which,reason); - key->transform->delkey(key->transform->st); + dispose_transform(&key->transform); key->key_timeout=0; } @@ -1061,7 +1093,7 @@ static void enter_state_stop(struct site *st) st->state=SITE_STOP; st->timeout=0; delete_keys(st,"entering state STOP",LOG_TIMEOUT_KEY); - st->new_transform->delkey(st->new_transform->st); + dispose_transform(&st->new_transform); } static void set_link_quality(struct site *st) @@ -1091,7 +1123,7 @@ static void enter_state_run(struct site *st) transport_peers_clear(st,&st->setup_peers); memset(st->localN,0,NONCELEN); memset(st->remoteN,0,NONCELEN); - st->new_transform->delkey(st->new_transform->st); + dispose_transform(&st->new_transform); memset(st->dhsecret,0,st->dh->len); memset(st->sharedsecret,0,st->sharedsecretlen); set_link_quality(st); @@ -1185,13 +1217,15 @@ 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->buffer, &transform_err); + if (call_transform_forwards(st, st->current.transform, + &st->buffer, &transform_err)) + goto free_out; buf_prepend_uint32(&st->buffer,LABEL_MSG0); buf_prepend_uint32(&st->buffer,st->index); buf_prepend_uint32(&st->buffer,st->current.remote_session_id); transport_xmit(st,&st->peers,&st->buffer,True); BUF_FREE(&st->buffer); + free_out: return True; } return False; @@ -1288,13 +1322,15 @@ 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, - buf, &transform_err); + if (call_transform_forwards(st, st->current.transform, + buf, &transform_err)) + goto free_out; buf_prepend_uint32(buf,LABEL_MSG0); buf_prepend_uint32(buf,st->index); buf_prepend_uint32(buf,st->current.remote_session_id); transport_xmit(st,&st->peers,buf,False); } + free_out: BUF_FREE(buf); return; } -- 2.30.2