dh: Fix mpz padding bug in use of write_mpbin

If the BN needs less than buflen bytes, write_mpbin would write only
the first len bytes.  dh_makeshared wouldn't notice.  The remaining
bytes will be left uninitialised.

In current code this is only called from site.c, where it so happens
right now that this buffer is always zero on entry.  So the effect is
thst we pad the bignum with zeroes at the LS end, which is wrong.

We can't just change this because it's baked into the protocol.
So actually implement it properly.

We do this in the write_mpbin function, renaming it, because the old
API for write_mpbin invites precisely this error.

I don't think this is of an significant consequence
cryptographically.  Perhaps we should introduce a non-anomalous
version of DH over prime fields.  Or perhaps we should just leave it
as is and expect to switch to X448 or something.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

diff --git a/dh.c b/dh.c
index f94665c9b52637fd4fda9988da612214a2e539df..261209aafedcfaa7077143e43fa27b268a507d10 100644 (file)
--- a/dh.c
+++ b/dh.c
@@ -61,14 +61,16 @@ static string_t dh_makepublic(void *sst, uint8_t *secret, int32_t secretlen)
     return r;
 }
 
-static int32_t write_mpbin(MP_INT *a, uint8_t *buffer,
-                                    int32_t buflen)
+static void write_mpbin_anomalous(MP_INT *a, uint8_t *buffer,
+                                 int32_t buflen)
+    /* If the BN is smaller than buflen, pads it *at the wrong end* */
 {
     char *hb = write_mpstring(a);
     int32_t len;
     hex_decode(buffer, buflen, &len, hb, True);
+    if (len<buflen)
+       memset(buffer+len,0,buflen-len);
     free(hb);
-    return len;
 }
 
 static dh_makeshared_fn dh_makeshared;
@@ -88,7 +90,7 @@ static void dh_makeshared(void *sst, uint8_t *secret, int32_t secretlen,
 
     mpz_powm_sec(&c, &b, &a, &st->p);
 
-    write_mpbin(&c,sharedsecret,buflen);
+    write_mpbin_anomalous(&c,sharedsecret,buflen);
 
     mpz_clear(&a);
     mpz_clear(&b);