From 3f488f14527df0f3da6616bcc26c2cf69f627df9 Mon Sep 17 00:00:00 2001 From: Mark Wooding Date: Sat, 29 Apr 2017 13:55:40 +0100 Subject: [PATCH] site.c: Pass the length of the actual shared secret to the transform. The `set_new_transform' function used to grow its `sharedsecret' buffer to accommodate the chosen transform's desired key length, and then tells the transform that this is the size of its secret. Unfortunately this is pretty much a lie. In particular, the traditional DH closure doesn't actually do anything to fill the rest of the buffer with random stuff. Probably there ought to be a KDF here, but: * we can't introduce a KDF globally without breaking compatibility with old clients; and * the new EAX-based transform has its own cheap-and-cheerful (but effective) SHA512-based KDF baked into it. Anyway, the result is that, if the DH group produces short shared secrets, and the transform has an explicit key size it wants, then everything will seem to work right up until the transform tries to use uninitialized memory as key material. Then the good news is that the two sites likely end up using different keys and can't talk to each other. The /bad/ news is that their keys don't have enough entropy, and an adversary may be able to impersonate them to each other. We're probably not in this situation yet. We have two transforms and one DH group type. One transform has its own KDF, so is unaffected by this. The other, the old `serpent256-cbc (or is it `serpent-cbc256'?) transform, wants 608 bits (76 bytes) of key. It gets these directly from the big-endian base-256 encoded DH shared secret, so we OK unless the DH field is smaller than 608 bits. But if it is then you have other problems. Surprisingly, the fix is for the site code to ignore the transform's reported key size entirely. It tells the transform the size of the shared secret, and if the transform is unhappy then it can fail or apply a KDF by itself. Of course, now we're doing this, there's no need for the transform to advertise a desired key length, so remove this. Also, this means that the shared secret buffer isn't going to change size any more, so we can remove all of the machinery for that, too. Signed-off-by: Mark Wooding --- secnet.h | 1 - site.c | 20 ++++---------------- transform-cbcmac.c | 3 --- transform-eax.c | 1 - 4 files changed, 4 insertions(+), 21 deletions(-) diff --git a/secnet.h b/secnet.h index 731687e..ee6af9c 100644 --- a/secnet.h +++ b/secnet.h @@ -576,7 +576,6 @@ struct transform_inst_if { struct transform_if { void *st; int capab_bit; - int32_t keylen; /* <<< INT_MAX */ transform_createinstance_fn *create; }; diff --git a/site.c b/site.c index 2ca333b..0b2e2d0 100644 --- a/site.c +++ b/site.c @@ -369,7 +369,6 @@ struct site { uint64_t timeout; /* Timeout for current state */ uint8_t *dhsecret; uint8_t *sharedsecret; - uint32_t sharedsecretlen, sharedsecretallocd; struct transform_inst_if *new_transform; /* For key setup/verify */ }; @@ -550,26 +549,16 @@ static _Bool set_new_transform(struct site *st, char *pk) { _Bool ok; - /* Make room for the shared key */ - st->sharedsecretlen=st->chosen_transform->keylen?:st->dh->shared_len; - assert(st->sharedsecretlen); - if (st->sharedsecretlen > st->sharedsecretallocd) { - st->sharedsecretallocd=st->sharedsecretlen; - st->sharedsecret=safe_realloc_ary(st->sharedsecret,1, - st->sharedsecretallocd, - "site:sharedsecret"); - } - /* Generate the shared key */ if (!st->dh->makeshared(st->dh->st,st->dhsecret,st->dh->secret_len, - pk, st->sharedsecret,st->sharedsecretlen)) + pk, st->sharedsecret,st->dh->shared_len)) return False; /* Set up the transform */ struct transform_if *generator=st->chosen_transform; struct transform_inst_if *generated=generator->create(generator->st); ok = generated->setkey(generated->st,st->sharedsecret, - st->sharedsecretlen,st->our_name_later); + st->dh->shared_len,st->our_name_later); dispose_transform(&st->new_transform); if (!ok) return False; @@ -1503,7 +1492,7 @@ static void enter_state_run(struct site *st) FILLZERO(st->remoteN); dispose_transform(&st->new_transform); memset(st->dhsecret,0,st->dh->secret_len); - if (st->sharedsecret) memset(st->sharedsecret,0,st->sharedsecretlen); + memset(st->sharedsecret,0,st->dh->shared_len); set_link_quality(st); if (st->keepalive && !current_valid(st)) @@ -2242,8 +2231,7 @@ 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->secret_len,"site:dhsecret"); - st->sharedsecretlen=st->sharedsecretallocd=0; - st->sharedsecret=0; + st->sharedsecret=safe_malloc(st->dh->shared_len, "site:sharedsecret"); #define SET_CAPBIT(bit) do { \ uint32_t capflag = 1UL << (bit); \ diff --git a/transform-cbcmac.c b/transform-cbcmac.c index b481346..e7a3ee5 100644 --- a/transform-cbcmac.c +++ b/transform-cbcmac.c @@ -284,9 +284,6 @@ static list_t *transform_apply(closure_t *self, struct cloc loc, update_max_start_pad(&transform_max_start_pad, 28); /* 4byte seqnum, 16byte pad, 4byte MACIV, 4byte IV */ - /* We need 256*2 bits for serpent keys, 32 bits for CBC-IV and 32 bits - for CBCMAC-IV, and 32 bits for init sequence number */ - st->ops.keylen=REQUIRED_KEYLEN; st->ops.create=transform_create; /* First parameter must be a dict */ diff --git a/transform-eax.c b/transform-eax.c index 04cd0e6..9bd4380 100644 --- a/transform-eax.c +++ b/transform-eax.c @@ -312,7 +312,6 @@ static list_t *transform_apply(closure_t *self, struct cloc loc, update_max_start_pad(&transform_max_start_pad, 0); - st->ops.keylen=0; st->ops.create=transform_create; return new_closure(&st->cl); -- 2.11.0