From a736afc8650f7b478bc3033ae3fccab981b265e0 Mon Sep 17 00:00:00 2001 From: Mark Wooding Date: Fri, 28 Apr 2017 22:51:36 +0100 Subject: [PATCH] Adjust the DH closure protocol to handle public values as raw binary. Responsibility for hex-encoding the public value now lies with the individual DH group implementation, rather than the common site-level machinery. Signed-off-by: Mark Wooding --- dh.c | 18 +++++++++++------- secnet-wireshark.lua | 12 ++++++++---- secnet.h | 30 +++++++++++++++++------------- site.c | 26 +++++++++++++++++--------- 4 files changed, 53 insertions(+), 33 deletions(-) diff --git a/dh.c b/dh.c index 0b5e4d2..7a25ee1 100644 --- a/dh.c +++ b/dh.c @@ -42,10 +42,10 @@ struct dh { MP_INT p,g; /* prime modulus and generator */ }; -static string_t dh_makepublic(void *sst, uint8_t *secret, int32_t secretlen) +static int32_t dh_makepublic(void *sst, void *public, int32_t publiclen, + uint8_t *secret, int32_t secretlen) { struct dh *st=sst; - string_t r; MP_INT a, b; /* a is secret key, b is public key */ mpz_init(&a); @@ -55,17 +55,19 @@ static string_t dh_makepublic(void *sst, uint8_t *secret, int32_t secretlen) mpz_powm_sec(&b, &st->g, &a, &st->p); - r=write_mpstring(&b); + assert(mpz_sizeinbase(&b, 16) + 2 <= (size_t)publiclen); + mpz_get_str(public, 16, &b); mpz_clear(&a); mpz_clear(&b); - return r; + + return strlen(public); } static dh_makeshared_fn dh_makeshared; static bool_t dh_makeshared(void *sst, uint8_t *secret, int32_t secretlen, - cstring_t rempublic, uint8_t *sharedsecret, - int32_t buflen) + const void *public, int32_t publiclen, + uint8_t *sharedsecret, int32_t buflen) { struct dh *st=sst; MP_INT a, b, c; @@ -75,7 +77,7 @@ static bool_t dh_makeshared(void *sst, uint8_t *secret, int32_t secretlen, mpz_init(&c); read_mpbin(&a, secret, secretlen); - mpz_set_str(&b, rempublic, 16); + mpz_set_str(&b, public, 16); mpz_powm_sec(&c, &b, &a, &st->p); @@ -170,6 +172,8 @@ static list_t *dh_apply(closure_t *self, struct cloc loc, dict_t *context, /* According to the docs, mpz_sizeinbase(,256) is allowed to return * an answer which is 1 too large. But mpz_sizeinbase(,2) isn't. */ + st->ops.public_len=(mpz_sizeinbase(&st->p,16)+2); + if (!dict) st->ops.capab_bit = CAPAB_BIT_TRADZP; else diff --git a/secnet-wireshark.lua b/secnet-wireshark.lua index 5166918..2a6761d 100644 --- a/secnet-wireshark.lua +++ b/secnet-wireshark.lua @@ -476,7 +476,11 @@ end local function dissect_dhval(st, buf, tree, pos, sz) -- Dissect a Diffie--Hellman public value. - return dissect_lenstr(st, buf, tree, "secnet.kx.dhval", pos, sz) + local len = buf(pos, 2):uint() + local sub = tree:add(PF["secnet.kx.dhval"], buf(pos, len + 2)) + sub:add(PF["secnet.kx.dhval.len"], buf(pos, 2)); pos = pos + 2 + sub:add(PF["secnet.kx.dhval.bytes"], buf(pos, len)); pos = pos + len + return pos end local function dissect_sig(st, buf, tree, pos, sz) @@ -784,9 +788,9 @@ do name = "Sender's public Diffie--Hellman length", type = ftypes.UINT16, base = base.DEC }, - ["secnet.kx.dhval.text"] = { - name = "Sender's public Diffie--Hellman text", type = ftypes.STRING, - base = base.ASCII + ["secnet.kx.dhval.bytes"] = { + name = "Sender's public Diffie--Hellman value bytes", + type = ftypes.BYTES, base = base.SPACE }, ["secnet.kx.sig"] = { name = "Sender's signature", type = ftypes.NONE diff --git a/secnet.h b/secnet.h index 554df4b..cda7f1d 100644 --- a/secnet.h +++ b/secnet.h @@ -611,26 +611,30 @@ struct netlink_if { /* DH interface */ -/* Returns public key as a malloced hex string. The secretlen will be the - * secret_len reported by the closure. This operation is not expected to - * fail. +/* Write public value corresponding to the secret to the public buffer, and + * return its actual length. The size of the buffer is given in publiclen, + * which will be at least the public_len reported by the closure. The + * secretlen will be the secret_len reported by the closure. This operation + * is not expected to fail. */ -typedef string_t dh_makepublic_fn(void *st, uint8_t *secret, - int32_t secretlen); - -/* Fills buffer (up to buflen) with shared secret. The rempublic string - * comes from the remote site, and may not be acceptable, though it has been - * checked for memory-safety. The secretlen and buflen are the secret_len - * and shared_len reported by the closure, respectively. Return false on - * faliure (e.g., if the publiclen is unacceptable). +typedef int32_t dh_makepublic_fn(void *st, void *public, int32_t publiclen, + uint8_t *secret, int32_t secretlen); + +/* Fills buffer (up to buflen) with shared secret. The publiclen comes from + * the remote site, and may not be acceptable, though it has been checked for + * memory-safety. The secretlen and buflen are the secret_len and shared_len + * reported by the closure, respectively. Return false on faliure (e.g., if + * the publiclen is unacceptable). */ -typedef bool_t dh_makeshared_fn(void *st, uint8_t *secret, - int32_t secretlen, cstring_t rempublic, +typedef bool_t dh_makeshared_fn(void *st, + uint8_t *secret, int32_t secretlen, + const void *public, int32_t publiclen, uint8_t *sharedsecret, int32_t buflen); struct dh_if { void *st; int32_t secret_len; /* Size of random secret to generate */ + int32_t public_len; /* Maximum number of bytes needed for public value */ int32_t shared_len; /* Size of generated shared secret */ int capab_bit; dh_makepublic_fn *makepublic; diff --git a/site.c b/site.c index 4da99e3..3971e68 100644 --- a/site.c +++ b/site.c @@ -534,7 +534,7 @@ struct msg { uint8_t *nR; uint8_t *nL; int32_t pklen; - char *pk; + uint8_t *pk; int32_t hashlen; int32_t siglen; char *sig; @@ -550,7 +550,7 @@ static int32_t wait_timeout(struct site *st) { return t; } -static _Bool set_new_transform(struct site *st, char *pk) +static _Bool set_new_transform(struct site *st, uint8_t *pk, int32_t pklen) { _Bool ok; @@ -558,9 +558,11 @@ static _Bool set_new_transform(struct site *st, char *pk) assert(!st->sharedsecret); st->sharedsecret = safe_malloc(st->chosen_dh->shared_len, "site:sharedsecret"); + pk[pklen]=0; /* clobbers the following signature length, which we've + * already copied */ if (!st->chosen_dh->makeshared(st->chosen_dh->st, st->dhsecret,st->chosen_dh->secret_len, - pk, + pk,pklen, st->sharedsecret, st->chosen_dh->shared_len)) return False; @@ -616,7 +618,10 @@ static bool_t generate_msg(struct site *st, uint32_t type, cstring_t what) { void *hst; uint8_t *hash; - string_t dhpub, sig; + string_t sig; + uint8_t *pklen_addr; + int32_t pklen; + void *pk; unsigned minor; st->retries=st->setup_retries; @@ -654,10 +659,13 @@ static bool_t generate_msg(struct site *st, uint32_t type, cstring_t what) buf_append_uint8(&st->buffer,st->chosen_dh->capab_bit); } while (0); - dhpub=st->chosen_dh->makepublic(st->chosen_dh->st, + pklen_addr=buf_append(&st->buffer,2); + pk=buf_append(&st->buffer,st->chosen_dh->public_len); + pklen=st->chosen_dh->makepublic(st->chosen_dh->st, + pk,st->chosen_dh->public_len, st->dhsecret,st->chosen_dh->secret_len); - buf_append_string(&st->buffer,dhpub); - free(dhpub); + put_uint16(pklen_addr,pklen); + buf_unappend(&st->buffer,st->chosen_dh->public_len-pklen); hash=safe_malloc(st->hash->len, "generate_msg"); hst=st->hash->init(); st->hash->update(hst,st->buffer.start,st->buffer.size); @@ -988,7 +996,7 @@ kind##_found: \ generate_dhsecret(st); /* Generate the shared key and set up the transform */ - if (!set_new_transform(st,m.pk)) return False; + if (!set_new_transform(st,m.pk,m.pklen)) return False; return True; } @@ -1019,7 +1027,7 @@ static bool_t process_msg4(struct site *st, struct buffer_if *msg4, m.pk[m.pklen]=0; /* Generate the shared key and set up the transform */ - if (!set_new_transform(st,m.pk)) return False; + if (!set_new_transform(st,m.pk,m.pklen)) return False; return True; } -- 2.11.0