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>
struct transform_if {
void *st;
int capab_bit;
struct transform_if {
void *st;
int capab_bit;
- int32_t keylen; /* <<< INT_MAX */
transform_createinstance_fn *create;
};
transform_createinstance_fn *create;
};
uint64_t timeout; /* Timeout for current state */
uint8_t *dhsecret;
uint8_t *sharedsecret;
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 */
};
struct transform_inst_if *new_transform; /* For key setup/verify */
};
- /* 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,
/* 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,
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;
dispose_transform(&st->new_transform);
if (!ok) return False;
FILLZERO(st->remoteN);
dispose_transform(&st->new_transform);
memset(st->dhsecret,0,st->dh->secret_len);
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))
set_link_quality(st);
if (st->keepalive && !current_valid(st))
transport_peers_clear(st,&st->setup_peers);
/* XXX mlock these */
st->dhsecret=safe_malloc(st->dh->secret_len,"site:dhsecret");
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); \
#define SET_CAPBIT(bit) do { \
uint32_t capflag = 1UL << (bit); \
update_max_start_pad(&transform_max_start_pad, 28);
/* 4byte seqnum, 16byte pad, 4byte MACIV, 4byte IV */
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 */
st->ops.create=transform_create;
/* First parameter must be a dict */
update_max_start_pad(&transform_max_start_pad, 0);
update_max_start_pad(&transform_max_start_pad, 0);
st->ops.create=transform_create;
return new_closure(&st->cl);
st->ops.create=transform_create;
return new_closure(&st->cl);