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 00a58e61292b1e07542c660f73e044bc55614ac1..903185e2c4be65c0ea99815ff348473846597f97 100644 (file)
--- a/secnet.h
+++ b/secnet.h
@@ -723,7 +723,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 1102dcde72c186232fa43d73051325076dac78c3..af147472d08bba1316653c7ba92cf014972ae616 100644 (file)
--- a/site.c
+++ b/site.c
@@ -381,7 +381,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 */
 };
 
@@ -584,26 +583,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;
@@ -1725,7 +1714,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))
@@ -2535,8 +2524,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 b481346068833c12eefdc1463b20cf489a97286d..e7a3ee541c20230c1458935c76e2a7f75f31e9e0 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 04cd0e65d7648b28c4f0afd29437c53d139156de..9bd4380066b966d6036e488746537a120478920c 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);