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 <mdw@distorted.org.uk>

diff --git a/secnet.h b/secnet.h
index 731687e..ee6af9c 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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);