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

diff --git a/dh.c b/dh.c
index bb79b24709d7baf1acf927ec9b83e6374852e776..bf416073d0e8108a126ec4774d6135bd853aac4b 100644 (file)
--- 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,11 +55,13 @@ 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 void write_mpbin_anomalous(MP_INT *a, uint8_t *buffer,
@@ -76,8 +78,8 @@ static void write_mpbin_anomalous(MP_INT *a, uint8_t *buffer,
 
 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;
@@ -87,7 +89,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);
 
@@ -182,6 +184,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 5166918e05e58d5192ace01522120de43e973b9f..2a6761d12c7876279efb1fc69e0915e633462fd1 100644 (file)
--- 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 4110b4cace00be2ac365fddea8de109e5182f5a2..ecf9d7eafb40ad35aa0ff71b15205ba9cd787b11 100644 (file)
--- a/secnet.h
+++ b/secnet.h
@@ -758,26 +758,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 093b76a01478e30611ad21d4c868552020e766cd..65f543adea0b07d3f4cd0a466921a5dab6baeee3 100644 (file)
--- a/site.c
+++ b/site.c
@@ -562,7 +562,7 @@ struct msg {
     uint8_t *nR;
     uint8_t *nL;
     int32_t pklen;
-    char *pk;
+    uint8_t *pk;
     int32_t hashlen;
     struct alg_msg_data sig;
     int n_pubkeys_accepted_nom; /* may be > MAX_SIG_KEYS ! */
@@ -582,7 +582,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;
 
@@ -590,9 +590,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;
@@ -648,7 +650,9 @@ static bool_t generate_msg(struct site *st, uint32_t type, cstring_t what,
                           const struct msg *prompt
                           /* may be 0 for MSG1 */)
 {
-    string_t dhpub;
+    uint8_t *pklen_addr;
+    int32_t pklen;
+    void *pk;
     unsigned minor;
     int ki;
 
@@ -730,10 +734,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);
 
     bool_t ok=privkey->sign(privkey->st,
                            st->buffer.start,
@@ -1190,7 +1197,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;
 }
@@ -1221,7 +1228,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;
 }